Skip to content

fix(auth): stabilize oauth redirect/session handling and api origin #131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
LucasSantana-Dev merged 6 commits into from
Mar 10, 2026
Merged

fix(auth): stabilize oauth redirect/session handling and api origin #131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
743aba6
fix(auth): stabilize oauth redirect uri and e2e selectors
LucasSantana-Dev Mar 10, 2026
576de71
fix(auth): adapt redis sessions and enforce same-origin api base
LucasSantana-Dev Mar 10, 2026
7953206
fix(frontend): reduce sonar duplication and harden browser redirect c…
LucasSantana-Dev Mar 10, 2026
22e7867
fix(auth): harden callback canonicalization and proxy/session middleware
LucasSantana-Dev Mar 10, 2026
09d6a4d
fix(ci): address sonar hotspot and duplication in auth/session changes
LucasSantana-Dev Mar 10, 2026
74f9336
test(auth): increase coverage for session fallback and api client
LucasSantana-Dev Mar 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Backend CORS now accepts configured origins plus `*.lucassantana.tech` and `*.luk-homeserver.com.br` hosts for dashboard/API split-domain setups
- Backend auth/Last.fm redirect targets now use the primary frontend origin when `WEBAPP_FRONTEND_URL` contains multiple comma-separated domains
- Frontend API client now auto-resolves hosted API base to `lucky-api.lucassantana.tech` or `api.luk-homeserver.com.br` when `VITE_API_BASE_URL` is not set
- Backend OAuth session persistence now uses a connect-redis v9 compatibility adapter for ioredis clients, preventing callback save failures
- Frontend API inference now uses same-origin `/api` for `*.lucassantana.tech` to keep OAuth/session requests on one browser origin
- Deploy smoke check now falls back to `/api/health` when `/api/health/auth-config` is unavailable
- Vercel routing no longer rewrites `/api/*` back to the same Lucky host, preventing `508 INFINITE_LOOP` on OAuth login
- Frontend API base URL now supports `VITE_API_BASE_URL` for hosted deployments that use a separate backend origin
Expand All @@ -38,6 +40,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Frontend theming now maps legacy `lucky-*` classes to the Lucky purple/gold palette
- Frontend typography now uses Lucky type tokens (`Sora`, `Manrope`, `JetBrains Mono`) instead of the old default stack
- Vercel build now generates Prisma client before shared/frontend builds to prevent missing generated client errors
- OAuth callback now reuses the same redirect URI across auth start/callback token exchange, with forwarded-host fallback for proxied HTTPS deployments
- E2E stability improvements: dashboard/servers/track-history tests now use deterministic locators and route-delay handling

### Changed

Expand Down
10 changes: 8 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ packages/
### Bot
- Multi-platform music (YouTube, Spotify) with queue, shuffle, repeat, lyrics, autoplay
- Dynamic Discord presence rotation with live guild/member/session stats and command CTA
- Autoplay recommendations use anti-repeat filtering with queue buffering so shuffle stays useful during autoplay
- Now-playing card updates in place to avoid channel spam on track changes
- Video/audio downloads with format selection and progress tracking
- Moderation: warn, mute, kick, ban with case tracking
Expand Down Expand Up @@ -130,11 +131,15 @@ Triggers the GitHub `Deploy to Homelab` workflow, waits for completion, and show
Vercel note: `vercel.json` runs `npm run db:generate` before `build:shared` and `build:frontend` to ensure Prisma generated client files are present during cloud builds.
For hosted frontend deployments, set `VITE_API_BASE_URL` to your backend API origin
(example: `https://api.yourdomain.com/api`) to avoid auth/API loop misrouting.
Without `VITE_API_BASE_URL`, frontend now auto-targets `lucky-api.lucassantana.tech` for
`*.lucassantana.tech` hosts and `api.luk-homeserver.com.br` for `*.luk-homeserver.com.br`.
Without `VITE_API_BASE_URL`, frontend uses same-origin `/api` for
`*.lucassantana.tech` hosts and `api.luk-homeserver.com.br` for
`*.luk-homeserver.com.br`.
When `WEBAPP_FRONTEND_URL` includes multiple origins, use comma-separated values
(example: `https://lucky.lucassantana.tech,https://lukbot.vercel.app`); backend CORS
accepts all configured entries while OAuth/Last.fm redirects use the first origin.
Set `WEBAPP_REDIRECT_URI` to the exact Discord OAuth callback URL registered in the
Discord Developer Portal (example:
`https://lucky-api.lucassantana.tech/api/auth/callback`).

## Environment Variables

Expand All @@ -148,6 +153,7 @@ See `.env.example` for all available options. Key variables:
| `REDIS_HOST` | No | Redis host (default: localhost) |
| `WEBAPP_ENABLED` | No | Enable web dashboard (default: false) |
| `WEBAPP_SESSION_SECRET` | No | Session encryption key |
| `WEBAPP_REDIRECT_URI` | No | Explicit Discord OAuth callback URL (for split domains use API host, e.g. `https://lucky-api.lucassantana.tech/api/auth/callback`) |
| `CLIENT_SECRET` | No | Discord OAuth secret (for dashboard) |
| `SENTRY_DSN` | No | Error tracking |

Expand Down
11 changes: 8 additions & 3 deletions nginx/nginx.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
resolver 127.0.0.11 valid=10s ipv6=off;

map $http_x_forwarded_proto $proxy_x_forwarded_proto {
default $http_x_forwarded_proto;
'' $scheme;
}
Comment on lines +3 to +6

@coderabbitai coderabbitai Bot Mar 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🌐 Web query:

nginx best practices X-Forwarded-Proto security trusted proxy

💡 Result:

Core rule

Treat X-Forwarded-Proto (and all X-Forwarded-*) as untrusted client input unless the request arrived from a proxy/load balancer you explicitly trust. X-Forwarded-Proto is only a de-facto convention indicating the client↔proxy scheme. [1]

NGINX best practices (secure “trusted proxy” setup)

  1. Only trust known proxy IPs (never 0.0.0.0/0)

    • Use the Real IP module to declare which upstream hops are trusted to assert “real” client info. [2]
    • Configure only your LB/CDN subnets in set_real_ip_from, select the header, and enable recursive parsing so NGINX finds the last non-trusted address in the chain. [2]

  2. Overwrite (don’t forward) spoofable headers to your upstream

    • When NGINX proxies to an app, explicitly set headers with proxy_set_header (don’t rely on what the client sent). [3]
    • For X-Forwarded-Proto specifically:
      • If NGINX terminates TLS: set it from NGINX’s own view: $scheme.
      • If TLS terminates at the load balancer: have the LB set X-Forwarded-Proto, and ensure NGINX only accepts traffic from the LB (network policy / firewall / private listener), otherwise clients can spoof it.

  3. Prefer the standardized Forwarded header when possible

    • RFC 7239 defines Forwarded: ...; proto=https and is explicit that these fields cannot be relied upon unless proxies are trusted, and that intermediaries should handle/replace existing forwarded info carefully. [4]

  4. Assume attackers will try header-based bypasses

    • OWASP testing guidance explicitly calls out X-Forwarded-For / X-Forwarded-Proto as common header-manipulation targets. [5]
    • Many frameworks can be configured to “trust proxy headers” (e.g., Django’s SECURE_PROXY_SSL_HEADER)—only enable that if only trusted proxies can reach the app/NGINX, otherwise the app may incorrectly treat an HTTP request as HTTPS. [6]

Minimal safe NGINX pattern (example)

# 1) Trust only your proxy/LB/CDN networks for client identity
set_real_ip_from  10.0.0.0/8;        # example: your LB subnet(s)
real_ip_header    X-Forwarded-For;
real_ip_recursive on;

location / {
  proxy_pass http://app;

  # 2) Do not pass through client-supplied forwarding headers; set your own
  proxy_set_header X-Forwarded-For   $proxy_add_x_forwarded_for;
  proxy_set_header X-Forwarded-Proto $scheme;
  proxy_set_header Host              $host;
}

Key decision: if TLS is terminated before NGINX, then $scheme will be http between LB→NGINX; in that case you should (a) restrict who can reach NGINX to the LB, and (b) pass the LB’s X-Forwarded-Proto onward only because it’s coming from a trusted source.

Sources: MDN on X-Forwarded-Proto [1]; NGINX Real IP module (set_real_ip_from, real_ip_header, real_ip_recursive) [2]; NGINX proxy module (proxy_set_header, $proxy_add_x_forwarded_for) [3]; RFC 7239 and its security considerations [4]; OWASP testing guide mentioning X-Forwarded-* header manipulation targets [5]; Django docs on trusting X-Forwarded-Proto only when it comes from your proxy [6].

🏁 Script executed:

cat -n nginx/nginx.conf

Repository: LucasSantana-Dev/Lucky

Length of output: 2125

Configure trusted proxy validation or use $scheme instead of client-supplied X-Forwarded-Proto.

This configuration preserves client-provided X-Forwarded-Proto headers without validation. Since nginx.conf lacks set_real_ip_from directives to declare trusted proxies, any client can inject this header. The backend uses it to construct OAuth redirect URIs—an attacker injecting X-Forwarded-Proto: http could force insecure redirects.

Replace the map with:

map $http_x_forwarded_proto $proxy_x_forwarded_proto {
    default $scheme;
}

Or, if nginx sits behind a trusted proxy (Cloudflare, AWS ALB, etc.), add trusted proxy validation:

set_real_ip_from <proxy_subnet>;
real_ip_header X-Forwarded-For;
real_ip_recursive on;

Then configure the app/backend to only trust X-Forwarded-Proto when coming from nginx.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@nginx/nginx.conf` around lines 3 - 6, The map definition for
proxy_x_forwarded_proto currently trusts client-supplied
$http_x_forwarded_proto; change it so the map uses $scheme as the default (i.e.,
always derive proto from nginx's scheme) or, if you must accept upstream
X-Forwarded-Proto, declare trusted proxies by adding set_real_ip_from entries
for your proxy subnets and enable real_ip_header and real_ip_recursive so nginx
only uses X-Forwarded-* from those proxies; also ensure the backend only trusts
X-Forwarded-Proto when coming from the validated proxy chain. Reference the map
block (map $http_x_forwarded_proto $proxy_x_forwarded_proto) and the directives
set_real_ip_from, real_ip_header, and real_ip_recursive when applying the fix.


server {
listen 80;
server_name _;
Expand All @@ -14,7 +19,7 @@ server {
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-Proto $scheme;
proxy_set_header X-Forwarded-Proto $proxy_x_forwarded_proto;
proxy_read_timeout 300s;
}

Expand All @@ -27,7 +32,7 @@ server {
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-Proto $scheme;
proxy_set_header X-Forwarded-Proto $proxy_x_forwarded_proto;
proxy_cache_bypass $http_upgrade;
}

Expand All @@ -40,7 +45,7 @@ server {
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-Proto $scheme;
proxy_set_header X-Forwarded-Proto $proxy_x_forwarded_proto;
proxy_cache_bypass $http_upgrade;
}
}
25 changes: 18 additions & 7 deletions packages/backend/src/middleware/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
import express, { type Express } from 'express'
import cors from 'cors'
import cookieParser from 'cookie-parser'
import { existsSync } from 'node:fs'
import path from 'path'
import { fileURLToPath } from 'url'
import { setupSessionMiddleware } from './session'
import { requestLogger } from './requestLogger'
import { getFrontendOrigins } from '../utils/frontendOrigin'

const __filename = fileURLToPath(import.meta.url)
const __dirname = path.dirname(__filename)

export function setupMiddleware(app: Express): void {
const configuredOrigins = getFrontendOrigins()
const isProduction = process.env.NODE_ENV === 'production'

if (isProduction) {
app.set('trust proxy', 1)
}

const isAllowedOrigin = (origin: string): boolean => {
if (configuredOrigins.includes(origin)) {
Expand Down Expand Up @@ -59,9 +61,18 @@ export function setupMiddleware(app: Express): void {
app.use(cookieParser())
setupSessionMiddleware(app)

// Only serve static files in production mode
const isProduction = process.env.NODE_ENV === 'production'
if (isProduction) {
app.use(express.static(path.join(__dirname, '../public')))
const monorepoPublicPath = path.join(
process.cwd(),
'packages',
'backend',
'public',
)
const localPublicPath = path.join(process.cwd(), 'public')
const staticPath = existsSync(monorepoPublicPath)
? monorepoPublicPath
: localPublicPath

app.use(express.static(staticPath))
}
}
Loading