Repository navigation
fix(auth): stabilize oauth redirect/session handling and api origin - #131
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR implements OAuth session persistence using a resilient Redis adapter for ioredis compatibility, introduces context-aware OAuth redirect URI handling with forwarded-header support, adds smart frontend API base inference for same-origin requests on specific hosts, configures production trust-proxy settings, and improves E2E test resilience with deterministic role-based locators. Changes
Sequence DiagramsequenceDiagram
participant Client as User/Browser
participant Backend as Backend Server
participant Session as Session Store<br/>(Redis/Fallback)
participant OAuth as Discord OAuth
participant TokenSvc as Token Exchange
Client->>Backend: GET /api/auth/discord (initiate OAuth)
Backend->>Backend: Compute redirect URI<br/>(from forwarded headers/<br/>env/production default)
Backend->>Session: Store oauthRedirectUri in session
Session-->>Backend: Session saved
Backend-->>Client: Redirect to Discord OAuth URL
Client->>OAuth: Authorize at Discord
OAuth-->>Client: Redirect to callback with code
Client->>Backend: GET /api/auth/callback?code=...
Backend->>Session: Retrieve oauthRedirectUri<br/>from session
Session-->>Backend: oauthRedirectUri (or fallback)
Backend->>TokenSvc: Exchange code for token<br/>with stored redirectUri
TokenSvc-->>Backend: Access token response
Backend->>Session: setSession with user info
Session-->>Backend: Session updated
Backend-->>Client: Redirect to authenticated state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for regal-bunny-0c8efe ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Size Change: +42 B (+0.01%) Total Size: 291 kB
ℹ️ View Unchanged
|
LucasSantana-Dev
force-pushed
the
fix/login-oauth-session-origin
branch
from
March 10, 2026 03:40
d8dd87e to
22e7867
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/backend/tests/integration/api.test.ts (1)
80-92:⚠️ Potential issue | 🟡 MinorThis integration test still skips the stateful half of the flow.
It calls
/api/auth/callbackdirectly, so it never proves that/api/auth/discordstoredreq.session.oauthRedirectUriand that the callback reused the same value. Please drive the request through the start route first and carry the returned cookie into the callback assertion.Based on learnings "Add integration tests where appropriate" and "Test behavior, not implementation details".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/tests/integration/api.test.ts` around lines 80 - 92, The test currently hits /api/auth/callback directly and skips the stateful start of the flow; update the test to first request GET /api/auth/discord, capture the Set-Cookie/session returned, then call GET /api/auth/callback with that cookie and the query code (MOCK_AUTH_CODE) so the request.session.oauthRedirectUri set by the discord start route is preserved; assert the redirect contains authenticated=true and that mockDiscordOAuth.exchangeCodeForToken was called with MOCK_AUTH_CODE and a redirect URI containing '/api/auth/callback' (or the captured redirect value) to verify the session-stored redirectUri was reused.packages/backend/src/routes/auth.ts (1)
23-43:⚠️ Potential issue | 🟠 MajorFail the OAuth start request when the session cannot be saved.
req.session.save()logs errors but still resolves, so this route can redirect to Discord with anoauthRedirectUrithat was never persisted. On the callback,req.session.oauthRedirectUrimay be missing and the token exchange can use a different redirect URI than the authorize step.Suggested fix
req.session.oauthInitiated = true req.session.oauthRedirectUri = getOAuthRedirectUri(req) - await new Promise<void>((resolve) => { + await new Promise<void>((resolve, reject) => { req.session.save((err) => { if (err) { errorLog({ message: 'Error saving session on OAuth init:', error: err, }) + reject(err) + return } else { debugLog({ message: 'Session initialized for OAuth', data: { sessionId: req.sessionID }, }) } resolve() }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/routes/auth.ts` around lines 23 - 43, The route currently always proceeds even when req.session.save(err) fails, which can lead to an unset oauthRedirectUri; update the save logic in the OAuth init flow to treat save errors as fatal: in the Promise passed to req.session.save (and around getOAuthRedirectUri / req.session.oauthRedirectUri usage) reject or return an HTTP error (e.g., 500) when err is non-null, log the error via errorLog and do not continue to compute clientId or redirectUri/redirect to Discord; only resolve/continue and call debugLog when save succeeds so the downstream token exchange can rely on req.session.oauthRedirectUri being persisted.
🧹 Nitpick comments (2)
packages/frontend/src/services/apiBase.test.ts (1)
14-37: Good test coverage, consider adding edge cases.The parameterized tests cover the key scenarios. Consider adding test cases for:
- Apex homeserver domain (
luk-homeserver.com.br)- Localhost behavior (if relevant for local dev)
Optional: additional test cases
{ hostname: 'panel.luk-homeserver.com.br', expected: 'https://api.luk-homeserver.com.br/api', }, + { + hostname: 'luk-homeserver.com.br', + expected: 'https://api.luk-homeserver.com.br/api', + }, + { + hostname: 'localhost', + expected: '/api', + }, ](🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/services/apiBase.test.ts` around lines 14 - 37, Add edge-case cases to the parameterized tests that exercise inferApiBase: extend the test.each in apiBase.test.ts to include a case for the apex homeserver hostname "luk-homeserver.com.br" with expected "https://api.luk-homeserver.com.br/api" (matching the existing panel-derived rule) and a case for local development such as "localhost" (and optionally "127.0.0.1") with the expected local API behavior (e.g., "/api" or the local host API URL your inferApiBase implementation returns); ensure these new entries use the same test title ("infers API base for $hostname") and call inferApiBase(...) the same way as the other cases so the function name inferApiBase and the existing test harness are reused.packages/backend/src/services/DiscordOAuthService.ts (1)
55-70: MakeredirectUrirequired inexchangeCodeForToken.The callback now computes the exact redirect URI it wants to reuse. Leaving this parameter optional with
redirectUri ?? this.getRedirectUri()reopens the mismatch as soon as another caller forgets the second argument.♻️ Tighten the API contract
async exchangeCodeForToken( code: string, - redirectUri?: string, + redirectUri: string, ): Promise<TokenResponse> { @@ - redirect_uri: redirectUri ?? this.getRedirectUri(), + redirect_uri: redirectUri, }), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/services/DiscordOAuthService.ts` around lines 55 - 70, exchangeCodeForToken currently accepts an optional redirectUri and falls back to getRedirectUri(), which risks mismatches; change the method signature of exchangeCodeForToken to require redirectUri (remove the ?), remove the fallback use of redirectUri ?? this.getRedirectUri() and pass redirectUri directly into the URLSearchParams, and update any callers to provide the computed redirect URI; ensure TokenResponse typing remains unchanged and adjust any tests or usages referencing getRedirectUri fallback accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nginx/nginx.conf`:
- Around line 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.
In `@packages/backend/src/middleware/session.ts`:
- Around line 194-200: The touch method declares its callback as () => void but
passes it to execute which expects a SessionCallback (error/data arguments),
causing a type mismatch; update touch's signature to accept a SessionCallback
(e.g., callback: SessionCallback = () => {}) and keep the default no-op but
typed correctly, or alternatively adapt the call to wrap the provided zero-arg
callback into a SessionCallback wrapper before calling this.execute('touch',
[sid, sessionData], ...), referencing the touch method, execute method, and
SessionCallback/session.SessionData types to locate and fix the inconsistency.
In `@packages/backend/src/utils/oauthRedirectUri.ts`:
- Around line 3-4: The file defines a hard-coded
CANONICAL_PRODUCTION_REDIRECT_URI which must be removed: read the canonical
backend origin from a validated source (e.g., WEBAPP_BACKEND_URL or the shared
config) instead of using 'https://lucky-api.lucassantana.tech', validate it as a
well-formed URL/origin, and use its origin to build the OAuth callback; update
any code that references CANONICAL_PRODUCTION_REDIRECT_URI (and the logic around
lines referencing 43-53) to throw/fail-fast if the env/shared-config value is
missing or malformed in production-like environments so callbacks never silently
go to the hard-coded host.
In `@packages/backend/tests/integration/routes/auth.test.ts`:
- Around line 145-165: The test "should resolve callback redirect uri from
forwarded host when env is unset" mutates process.env.WEBAPP_REDIRECT_URI but
only restores it conditionally at the end; wrap the request/assertion logic in a
try/finally and restore process.env.WEBAPP_REDIRECT_URI in the finally block so
the original value is always reset (keep references to
process.env.WEBAPP_REDIRECT_URI, mockSuccessfulOAuthFlow(), and the assertion
using getDiscordOAuthMock().exchangeCodeForToken intact).
In `@packages/backend/tests/unit/middleware/index.test.ts`:
- Around line 1-25: The tests mutate process.env.NODE_ENV but don't guarantee
restoration on failure; update the test file so NODE_ENV is always restored by
wrapping each test's mutation in a try/finally or add an afterEach hook that
resets NODE_ENV, referencing the existing tests that call setupMiddleware(app)
and assert app.get('trust proxy') so the originalNodeEnv captured before
mutation is reinstated unconditionally.
In `@packages/frontend/src/services/api.ts`:
- Around line 87-93: The 401 interceptor in services/api.ts currently redirects
using a relative path (globalThis.window.location.assign('/api/auth/discord')),
causing inconsistent OAuth entry points versus the explicit login URL returned
by getDiscordLoginUrl() and NORMALIZED_API_BASE; update the interceptor to build
the redirect using the same base as NORMALIZED_API_BASE (or simply
call/getDiscordLoginUrl() to obtain the full URL) and then call
window.location.assign(...) with that full URL so both manual login and
401-triggered login use the identical OAuth endpoint.
In `@packages/frontend/tests/e2e/servers-page.spec.ts`:
- Around line 113-115: The test registers an extra page.route handler that calls
route.continue(), which bypasses the existing mock installed by
setupMockApiResponses(page); replace route.continue() with route.fallback() in
the anonymous handler (the async route => { ... } callback) so the request is
passed to the next matching handler in the chain, preserving the existing mock
for '/api/guilds' after the artificial delay.
---
Outside diff comments:
In `@packages/backend/src/routes/auth.ts`:
- Around line 23-43: The route currently always proceeds even when
req.session.save(err) fails, which can lead to an unset oauthRedirectUri; update
the save logic in the OAuth init flow to treat save errors as fatal: in the
Promise passed to req.session.save (and around getOAuthRedirectUri /
req.session.oauthRedirectUri usage) reject or return an HTTP error (e.g., 500)
when err is non-null, log the error via errorLog and do not continue to compute
clientId or redirectUri/redirect to Discord; only resolve/continue and call
debugLog when save succeeds so the downstream token exchange can rely on
req.session.oauthRedirectUri being persisted.
In `@packages/backend/tests/integration/api.test.ts`:
- Around line 80-92: The test currently hits /api/auth/callback directly and
skips the stateful start of the flow; update the test to first request GET
/api/auth/discord, capture the Set-Cookie/session returned, then call GET
/api/auth/callback with that cookie and the query code (MOCK_AUTH_CODE) so the
request.session.oauthRedirectUri set by the discord start route is preserved;
assert the redirect contains authenticated=true and that
mockDiscordOAuth.exchangeCodeForToken was called with MOCK_AUTH_CODE and a
redirect URI containing '/api/auth/callback' (or the captured redirect value) to
verify the session-stored redirectUri was reused.
---
Nitpick comments:
In `@packages/backend/src/services/DiscordOAuthService.ts`:
- Around line 55-70: exchangeCodeForToken currently accepts an optional
redirectUri and falls back to getRedirectUri(), which risks mismatches; change
the method signature of exchangeCodeForToken to require redirectUri (remove the
?), remove the fallback use of redirectUri ?? this.getRedirectUri() and pass
redirectUri directly into the URLSearchParams, and update any callers to provide
the computed redirect URI; ensure TokenResponse typing remains unchanged and
adjust any tests or usages referencing getRedirectUri fallback accordingly.
In `@packages/frontend/src/services/apiBase.test.ts`:
- Around line 14-37: Add edge-case cases to the parameterized tests that
exercise inferApiBase: extend the test.each in apiBase.test.ts to include a case
for the apex homeserver hostname "luk-homeserver.com.br" with expected
"https://api.luk-homeserver.com.br/api" (matching the existing panel-derived
rule) and a case for local development such as "localhost" (and optionally
"127.0.0.1") with the expected local API behavior (e.g., "/api" or the local
host API URL your inferApiBase implementation returns); ensure these new entries
use the same test title ("infers API base for $hostname") and call
inferApiBase(...) the same way as the other cases so the function name
inferApiBase and the existing test harness are reused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0840e42d-f8ea-4862-9586-3e86a53fe713
📒 Files selected for processing (23)
CHANGELOG.mdREADME.mdnginx/nginx.confpackages/backend/src/middleware/index.tspackages/backend/src/middleware/session.tspackages/backend/src/routes/auth.tspackages/backend/src/routes/authCallback.tspackages/backend/src/services/DiscordOAuthService.tspackages/backend/src/types/session.d.tspackages/backend/src/utils/oauthRedirectUri.tspackages/backend/tests/integration/api.test.tspackages/backend/tests/integration/routes/auth.test.tspackages/backend/tests/setup.tspackages/backend/tests/unit/middleware/index.test.tspackages/backend/tests/unit/middleware/session.test.tspackages/backend/tests/unit/middleware/sessionRedisAdapter.test.tspackages/backend/tests/unit/utils/oauthRedirectUri.test.tspackages/frontend/src/services/api.tspackages/frontend/src/services/apiBase.test.tspackages/frontend/src/services/apiBase.tspackages/frontend/tests/e2e/dashboard-page.spec.tspackages/frontend/tests/e2e/servers-page.spec.tspackages/frontend/tests/e2e/track-history-page.spec.ts
Comment on lines
+3
to
+6
| map $http_x_forwarded_proto $proxy_x_forwarded_proto { | ||
| default $http_x_forwarded_proto; | ||
| '' $scheme; | ||
| } |
There was a problem hiding this comment.
🧩 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)
-
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]
-
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-Protospecifically:- 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.
- If NGINX terminates TLS: set it from NGINX’s own view:
- When NGINX proxies to an app, explicitly set headers with
-
Prefer the standardized
Forwardedheader when possible- RFC 7239 defines
Forwarded: ...; proto=httpsand is explicit that these fields cannot be relied upon unless proxies are trusted, and that intermediaries should handle/replace existing forwarded info carefully. [4]
- RFC 7239 defines
-
Assume attackers will try header-based bypasses
- OWASP testing guidance explicitly calls out
X-Forwarded-For/X-Forwarded-Protoas 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]
- OWASP testing guidance explicitly calls out
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.confRepository: 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.
Comment on lines
+194
to
+200
| touch( | ||
| sid: string, | ||
| sessionData: session.SessionData, | ||
| callback: () => void = () => {}, | ||
| ): void { | ||
| this.execute('touch', [sid, sessionData], callback as SessionCallback) | ||
| } |
There was a problem hiding this comment.
Touch callback signature mismatch.
The touch method's callback type is () => void, but execute calls it with error/data arguments via SessionCallback. While this works at runtime (extra arguments are ignored), the type inconsistency could cause confusion.
Suggested fix for type consistency
touch(
sid: string,
sessionData: session.SessionData,
- callback: () => void = () => {},
+ callback: (error?: unknown) => void = () => {},
): void {
this.execute('touch', [sid, sessionData], callback as SessionCallback)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| touch( | |
| sid: string, | |
| sessionData: session.SessionData, | |
| callback: () => void = () => {}, | |
| ): void { | |
| this.execute('touch', [sid, sessionData], callback as SessionCallback) | |
| } | |
| touch( | |
| sid: string, | |
| sessionData: session.SessionData, | |
| callback: (error?: unknown) => void = () => {}, | |
| ): void { | |
| this.execute('touch', [sid, sessionData], callback as SessionCallback) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/src/middleware/session.ts` around lines 194 - 200, The touch
method declares its callback as () => void but passes it to execute which
expects a SessionCallback (error/data arguments), causing a type mismatch;
update touch's signature to accept a SessionCallback (e.g., callback:
SessionCallback = () => {}) and keep the default no-op but typed correctly, or
alternatively adapt the call to wrap the provided zero-arg callback into a
SessionCallback wrapper before calling this.execute('touch', [sid, sessionData],
...), referencing the touch method, execute method, and
SessionCallback/session.SessionData types to locate and fix the inconsistency.
Comment on lines
+3
to
+4
| const CANONICAL_PRODUCTION_REDIRECT_URI = | ||
| 'https://lucky-api.lucassantana.tech/api/auth/callback' |
There was a problem hiding this comment.
Remove the hard-coded production callback origin.
If WEBAPP_BACKEND_URL is missing or malformed in a production-like deployment, this silently sends OAuth callbacks to lucky-api.lucassantana.tech. That can break auth for every non-canonical deployment and route authorization codes to the wrong backend. Fail fast here, or read the canonical backend origin from validated shared config instead of hardcoding a host.
🔧 Safer direction
-const CANONICAL_PRODUCTION_REDIRECT_URI =
- 'https://lucky-api.lucassantana.tech/api/auth/callback'
-
const getCanonicalProductionRedirectUri = (): string => {
const backendUrl = process.env.WEBAPP_BACKEND_URL
- if (!backendUrl) {
- return CANONICAL_PRODUCTION_REDIRECT_URI
- }
-
- try {
- return new URL('/api/auth/callback', backendUrl).toString()
- } catch {
- return CANONICAL_PRODUCTION_REDIRECT_URI
- }
+ if (!backendUrl) {
+ throw new Error('WEBAPP_BACKEND_URL must be configured in production')
+ }
+
+ return new URL('/api/auth/callback', backendUrl).toString()
}As per coding guidelines "Store secrets, ports, and hosts in environment variables (.env, .env.example) and never hardcode them".
Also applies to: 43-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/src/utils/oauthRedirectUri.ts` around lines 3 - 4, The file
defines a hard-coded CANONICAL_PRODUCTION_REDIRECT_URI which must be removed:
read the canonical backend origin from a validated source (e.g.,
WEBAPP_BACKEND_URL or the shared config) instead of using
'https://lucky-api.lucassantana.tech', validate it as a well-formed URL/origin,
and use its origin to build the OAuth callback; update any code that references
CANONICAL_PRODUCTION_REDIRECT_URI (and the logic around lines referencing 43-53)
to throw/fail-fast if the env/shared-config value is missing or malformed in
production-like environments so callbacks never silently go to the hard-coded
host.
Comment on lines
+145
to
+165
| test('should resolve callback redirect uri from forwarded host when env is unset', async () => { | ||
| const originalRedirectUri = process.env.WEBAPP_REDIRECT_URI | ||
| delete process.env.WEBAPP_REDIRECT_URI | ||
|
|
||
| const mockSessionService = sessionService as jest.Mocked< | ||
| typeof sessionService | ||
| > | ||
| mockSessionService.setSession.mockResolvedValue() | ||
| mockSuccessfulOAuthFlow() | ||
|
|
||
| const response = await request(app) | ||
| await request(app) | ||
| .get('/api/auth/callback') | ||
| .query({ code: MOCK_AUTH_CODE }) | ||
| .set('Cookie', ['sessionId=callback_session_id']) | ||
| .set('x-forwarded-proto', 'https') | ||
| .set('x-forwarded-host', 'lucky.lucassantana.tech') | ||
| .expect(302) | ||
|
|
||
| expect(response.headers.location).toContain('authenticated=true') | ||
| expect(mockDiscordOAuth.exchangeCodeForToken).toHaveBeenCalledWith( | ||
| expect(getDiscordOAuthMock().exchangeCodeForToken).toHaveBeenCalledWith( | ||
| MOCK_AUTH_CODE, | ||
| 'https://lucky.lucassantana.tech/api/auth/callback', | ||
| ) | ||
| expect(mockDiscordOAuth.getUserInfo).toHaveBeenCalledWith( | ||
| MOCK_TOKEN_RESPONSE.access_token, | ||
| ) | ||
| expect(mockSessionService.setSession).toHaveBeenCalled() | ||
|
|
||
| if (originalRedirectUri) { | ||
| process.env.WEBAPP_REDIRECT_URI = originalRedirectUri | ||
| } |
There was a problem hiding this comment.
Restore WEBAPP_REDIRECT_URI in finally here too.
If the request or assertion fails, this test leaves global env state mutated and can poison the rest of the suite.
Suggested fix
test('should resolve callback redirect uri from forwarded host when env is unset', async () => {
const originalRedirectUri = process.env.WEBAPP_REDIRECT_URI
delete process.env.WEBAPP_REDIRECT_URI
- mockSuccessfulOAuthFlow()
-
- await request(app)
- .get('/api/auth/callback')
- .query({ code: MOCK_AUTH_CODE })
- .set('x-forwarded-proto', 'https')
- .set('x-forwarded-host', 'lucky.lucassantana.tech')
- .expect(302)
-
- expect(getDiscordOAuthMock().exchangeCodeForToken).toHaveBeenCalledWith(
- MOCK_AUTH_CODE,
- 'https://lucky.lucassantana.tech/api/auth/callback',
- )
-
- if (originalRedirectUri) {
- process.env.WEBAPP_REDIRECT_URI = originalRedirectUri
- }
+ try {
+ mockSuccessfulOAuthFlow()
+
+ await request(app)
+ .get('/api/auth/callback')
+ .query({ code: MOCK_AUTH_CODE })
+ .set('x-forwarded-proto', 'https')
+ .set('x-forwarded-host', 'lucky.lucassantana.tech')
+ .expect(302)
+
+ expect(
+ getDiscordOAuthMock().exchangeCodeForToken,
+ ).toHaveBeenCalledWith(
+ MOCK_AUTH_CODE,
+ 'https://lucky.lucassantana.tech/api/auth/callback',
+ )
+ } finally {
+ if (originalRedirectUri) {
+ process.env.WEBAPP_REDIRECT_URI = originalRedirectUri
+ } else {
+ delete process.env.WEBAPP_REDIRECT_URI
+ }
+ }
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/tests/integration/routes/auth.test.ts` around lines 145 -
165, The test "should resolve callback redirect uri from forwarded host when env
is unset" mutates process.env.WEBAPP_REDIRECT_URI but only restores it
conditionally at the end; wrap the request/assertion logic in a try/finally and
restore process.env.WEBAPP_REDIRECT_URI in the finally block so the original
value is always reset (keep references to process.env.WEBAPP_REDIRECT_URI,
mockSuccessfulOAuthFlow(), and the assertion using
getDiscordOAuthMock().exchangeCodeForToken intact).
Comment on lines
+1
to
+25
| import { describe, test, expect } from '@jest/globals' | ||
| import express from 'express' | ||
| import { setupMiddleware } from '../../../src/middleware' | ||
|
|
||
| describe('Middleware setup', () => { | ||
| test('should enable trust proxy in production', () => { | ||
| const originalNodeEnv = process.env.NODE_ENV | ||
| process.env.NODE_ENV = 'production' | ||
| const app = express() | ||
|
|
||
| setupMiddleware(app) | ||
|
|
||
| expect(app.get('trust proxy')).toBe(1) | ||
| process.env.NODE_ENV = originalNodeEnv | ||
| }) | ||
|
|
||
| test('should not force trust proxy outside production', () => { | ||
| const originalNodeEnv = process.env.NODE_ENV | ||
| process.env.NODE_ENV = 'test' | ||
| const app = express() | ||
|
|
||
| setupMiddleware(app) | ||
|
|
||
| expect(app.get('trust proxy')).not.toBe(1) | ||
| process.env.NODE_ENV = originalNodeEnv |
There was a problem hiding this comment.
Always restore NODE_ENV, even when the test fails.
If setupMiddleware(app) or an assertion throws, the mutated env leaks into later tests. An afterEach or try/finally keeps the suite isolated.
🧪 Safer test isolation
-import { describe, test, expect } from '@jest/globals'
+import { describe, test, expect, afterEach } from '@jest/globals'
import express from 'express'
import { setupMiddleware } from '../../../src/middleware'
describe('Middleware setup', () => {
+ const originalNodeEnv = process.env.NODE_ENV
+
+ afterEach(() => {
+ process.env.NODE_ENV = originalNodeEnv
+ })
+
test('should enable trust proxy in production', () => {
- const originalNodeEnv = process.env.NODE_ENV
process.env.NODE_ENV = 'production'
const app = express()
setupMiddleware(app)
expect(app.get('trust proxy')).toBe(1)
- process.env.NODE_ENV = originalNodeEnv
})
test('should not force trust proxy outside production', () => {
- const originalNodeEnv = process.env.NODE_ENV
process.env.NODE_ENV = 'test'
const app = express()
setupMiddleware(app)
expect(app.get('trust proxy')).not.toBe(1)
- process.env.NODE_ENV = originalNodeEnv
})
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/tests/unit/middleware/index.test.ts` around lines 1 - 25,
The tests mutate process.env.NODE_ENV but don't guarantee restoration on
failure; update the test file so NODE_ENV is always restored by wrapping each
test's mutation in a try/finally or add an afterEach hook that resets NODE_ENV,
referencing the existing tests that call setupMiddleware(app) and assert
app.get('trust proxy') so the originalNodeEnv captured before mutation is
reinstated unconditionally.
Comment on lines
+87
to
93
| if ( | ||
| status === 401 && | ||
| typeof globalThis !== 'undefined' && | ||
| 'window' in globalThis | ||
| ) { | ||
| globalThis.window.location.assign('/api/auth/discord') | ||
| } |
There was a problem hiding this comment.
Inconsistent OAuth redirect paths may break homeserver deployments.
On homeserver hosts (e.g., panel.luk-homeserver.com.br), NORMALIZED_API_BASE resolves to https://api.luk-homeserver.com.br/api. The explicit login via getDiscordLoginUrl() (line 115) returns the full URL, but this 401 interceptor redirects to the relative path /api/auth/discord, which resolves to the current origin instead.
This creates two different OAuth entry points:
- Manual login:
https://api.luk-homeserver.com.br/api/auth/discord - 401 redirect:
https://panel.luk-homeserver.com.br/api/auth/discord
Consider using the same base URL for consistency:
Proposed fix
if (
status === 401 &&
typeof globalThis !== 'undefined' &&
'window' in globalThis
) {
- globalThis.window.location.assign('/api/auth/discord')
+ globalThis.window.location.assign(`${NORMALIZED_API_BASE}/auth/discord`)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/frontend/src/services/api.ts` around lines 87 - 93, The 401
interceptor in services/api.ts currently redirects using a relative path
(globalThis.window.location.assign('/api/auth/discord')), causing inconsistent
OAuth entry points versus the explicit login URL returned by
getDiscordLoginUrl() and NORMALIZED_API_BASE; update the interceptor to build
the redirect using the same base as NORMALIZED_API_BASE (or simply
call/getDiscordLoginUrl() to obtain the full URL) and then call
window.location.assign(...) with that full URL so both manual login and
401-triggered login use the identical OAuth endpoint.
Comment on lines
113
to
115
| await page.route('**/api/guilds', async (route) => { | ||
| await page.waitForTimeout(1000) | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)) | ||
| await route.continue() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Playwright 1.57, when multiple page.route('**/api/guilds')handlers match the same request, doesroute.continue()go straight to the network whileroute.fallback() passes control to the next matching handler?
💡 Result:
Yes.
route.continue()immediately sends the request to the network, and no other matching route handlers will run. [1]route.fallback()defers to the next matching handler in the chain (and only after all applicable handlers does it go to the network). [1]
Source:
[1] Playwright API docs — Route.continue() / Route.fallback() (playwright.dev)
Citations:
Use route.fallback() instead of route.continue() to chain route handlers properly.
The setupMockApiResponses(page) call in beforeEach already registers a handler for /api/guilds. Calling route.continue() sends the request directly to the network and skips the existing mock handler entirely. Use route.fallback() to pass control to the next matching handler in the chain instead.
Suggested fix
await page.route('**/api/guilds', async (route) => {
await new Promise((resolve) => setTimeout(resolve, 1000))
- await route.continue()
+ await route.fallback()
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await page.route('**/api/guilds', async (route) => { | |
| await page.waitForTimeout(1000) | |
| await new Promise((resolve) => setTimeout(resolve, 1000)) | |
| await route.continue() | |
| await page.route('**/api/guilds', async (route) => { | |
| await new Promise((resolve) => setTimeout(resolve, 1000)) | |
| await route.fallback() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/frontend/tests/e2e/servers-page.spec.ts` around lines 113 - 115, The
test registers an extra page.route handler that calls route.continue(), which
bypasses the existing mock installed by setupMockApiResponses(page); replace
route.continue() with route.fallback() in the anonymous handler (the async route
=> { ... } callback) so the request is passed to the next matching handler in
the chain, preserving the existing mock for '/api/guilds' after the artificial
delay.
|
|
This was referenced Mar 10, 2026
LucasSantana-Dev
added a commit
that referenced
this pull request
May 13, 2026
…131) * fix(auth): stabilize oauth redirect uri and e2e selectors * fix(auth): adapt redis sessions and enforce same-origin api base * fix(frontend): reduce sonar duplication and harden browser redirect checks * fix(auth): harden callback canonicalization and proxy/session middleware * fix(ci): address sonar hotspot and duplication in auth/session changes * test(auth): increase coverage for session fallback and api client
LucasSantana-Dev
added a commit
that referenced
this pull request
Jul 10, 2026
…1761) Closes the two real gaps found by gap analysis against web-app issue #131 (everything else already shipped in PR #1609): trigger moved Monday→Sunday 12:00 UTC (9h BRT) with the week-window + idempotency math re-anchored coherently, and a 'Novos guias da semana' section fed from the web-app guides feed (≤3 bullets, omitted when empty, fail-soft on fetch error — digest always sends). 29/29 spec tests; full bot suite 2637/0. Cross-repo: Criativaria-Projects/web-app#131. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Moves the weekly digest to Sunday 12:00 UTC and adds a “Novos guias da semana” section powered by the guides RSS feed. Re-anchors the week window to Sunday to keep idempotency correct and closes gaps in `Criativaria-Projects/web-app#131`. - **New Features** - Trigger now runs on Sunday at 12:00 UTC; week math anchored to Sunday. - Fetch up to 3 new guides from `CRIATIVARIA_GUIDES_FEED_URL` via `rss-parser` (last 7 days); render title + link. - Omit the guides section when empty and fail soft on fetch errors (digest still sends). - **Bug Fixes** - Validate RSS dates; skip undated, unparsable, future, and out-of-window items. - Handle unsorted feeds without dropping newer in-window items. - Enforce Discord embed limits: truncate titles to 80 chars with ellipsis and cap field value at 1024 chars; skip only the overlong bullet and keep later items. <sup>Written for commit 12b484c. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/1761?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Weekly digests are now sent on Sundays at 12:00 UTC. * Digests can include a “New guides this week” section (up to three RSS items), shown only when available. * Guide titles are trimmed to fit embed limits, with an overall guides field size cap. * **Bug Fixes** * RSS feed problems no longer block digests; the guides section is omitted and an error is logged. * Duplicate weekly digests prevention now aligns with the Sunday-anchored schedule. * **Tests** * Expanded coverage for RSS-guides inclusion/exclusion and fail-soft behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Summary by CodeRabbit
Bug Fixes
Documentation