Repository navigation
Makes redirect route dynamic - #36
Conversation
WalkthroughAdds safe redirect state handling to Google OAuth: accepts a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Passport
participant Google
rect rgb(240,248,255)
Note over Client,Server: Initiate OAuth
Client->>Server: GET /api/v1/auth/google?redirectTo=/path
Server->>Server: validate /path (starts with '/', not '//')
Server->>Server: base64-encode -> state
Server->>Passport: passport.authenticate(..., state)
Passport->>Google: Redirect to Google with state
end
rect rgb(245,255,240)
Note over Google,Server: Callback after consent
Google->>Server: GET /auth/google/callback?state=<base64>&code=...
Server->>Server: try base64-decode(state)
Server->>Server: validate decoded path (safe relative)
alt decoded path valid
Server-->>Client: 302 Redirect -> decoded path
else decode fails or invalid
Server-->>Client: 302 Redirect -> /default (or /login on failure)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/routes/auth.test.js (2)
8-32: Refactor to avoid duplicating production logic in test mocks.The
createMockedAuthRoutesfunction duplicates the exact redirect logic fromsrc/routes/auth.js. This creates maintenance burden and defeats the purpose of tests catching regressions—when production code is fixed, tests must also be updated.Consider these alternatives:
- Option 1: Import and test the actual routes from
authRoutes, mocking onlypassport.authenticateat the module level- Option 2: Extract the state decoding/validation logic into a separate testable function that both production and tests can use
Example for Option 2:
// In src/utils/redirectValidator.js export const getValidRedirectUrl = (state, fallback = '/default') => { let finalUrl = fallback; if (state) { try { const decoded = Buffer.from(state, 'base64').toString('utf-8'); if (decoded && decoded.startsWith('/') && !decoded.startsWith('//')) { finalUrl = decoded; } } catch (err) { console.error('Error decoding state:', err); } } return finalUrl; };Then use this function in both production routes and test mocks, and add unit tests specifically for
getValidRedirectUrl.
245-403: Add security test cases for open redirect vulnerability.The current implementation accepts any URL starting with
httpor/(lines 31-32 in src/routes/auth.js), creating an open redirect vulnerability. The tests do not validate security boundaries. Critical missing test cases:
- External URLs (e.g.,
https://evil.com) should be rejected, not redirected to- Protocol-relative URLs (e.g.,
//evil.com) should be rejected- Only relative paths (e.g.,
/api/v1/events) should be acceptedAdd these security-focused test cases:
describe('Security - Open Redirect Prevention', () => { it('deve rejeitar redirect para domínio externo via https', async () => { const app = createTestApp(); const maliciousUrl = 'https://evil.com/phishing'; const encodedState = Buffer.from(maliciousUrl).toString('base64'); const mockAuthenticate = (req, res, next) => { req.user = { /* mock user */ }; next(); }; const mockedRoutes = createMockedAuthRoutes(mockAuthenticate); app.use('/api/v1', mockedRoutes); const response = await request(app) .get(`/api/v1/auth/google/callback?state=${encodedState}`); expect(response.status).toBe(302); expect(response.headers.location).toBe('/default'); }); it('deve rejeitar protocol-relative URL (//evil.com)', async () => { const app = createTestApp(); const maliciousUrl = '//evil.com/phishing'; const encodedState = Buffer.from(maliciousUrl).toString('base64'); const mockAuthenticate = (req, res, next) => { req.user = { /* mock user */ }; next(); }; const mockedRoutes = createMockedAuthRoutes(mockAuthenticate); app.use('/api/v1', mockedRoutes); const response = await request(app) .get(`/api/v1/auth/google/callback?state=${encodedState}`); expect(response.status).toBe(302); expect(response.headers.location).toBe('/default'); }); it('deve aceitar apenas paths relativos válidos', async () => { const app = createTestApp(); const validPath = '/api/v1/events'; const encodedState = Buffer.from(validPath).toString('base64'); const mockAuthenticate = (req, res, next) => { req.user = { /* mock user */ }; next(); }; const mockedRoutes = createMockedAuthRoutes(mockAuthenticate); app.use('/api/v1', mockedRoutes); const response = await request(app) .get(`/api/v1/auth/google/callback?state=${encodedState}`); expect(response.status).toBe(302); expect(response.headers.location).toBe(validPath); }); });Note: The test at lines 84-109 expects a redirect to
https://extensaoads2.sj.ifsc.edu.br/api/v1/events, which demonstrates the current vulnerability. Update the implementation to reject absolute URLs before adding these tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/auth.js(1 hunks)tests/routes/auth.test.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/routes/auth.test.js (1)
src/routes/auth.js (6)
router(4-4)req(7-7)state(10-10)state(24-24)finalUrl(25-25)decoded(29-29)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/routes/auth.js (1)
24-47: State decode / redirect flow looks good; mirror the query normalization hereThe callback now properly restricts redirects to absolute‑relative paths (
/…but not//…), which addresses the previous open‑redirect concern. To harden it against odd query shapes (multiplestateparams), normalizestatethe same way asredirectTobefore decoding:(req, res) => { - // Recupera o redirectTo do state parameter - const state = req.query.state; + // Recupera o redirectTo do state parameter + const rawState = req.query.state; + const state = Array.isArray(rawState) ? rawState[0] : rawState; let finalUrl = '/default'; - if (state) { + if (typeof state === 'string' && state) { try { const decoded = Buffer.from(state, 'base64').toString('utf-8'); // Valida se o resultado é um path relativo seguro (não permite URLs externas) if (decoded && decoded.startsWith('/') && !decoded.startsWith('//')) { finalUrl = decoded; } } catch (err) { console.error('Error decoding state:', err); } } res.redirect(finalUrl); }Functionally you keep the same redirect behavior, but avoid surprises from non‑string
statevalues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/routes/auth.js (2)
6-23: Harden path validation against traversal and injection.The current validation correctly prevents open redirect vulnerabilities by restricting to relative paths. However, consider adding additional checks to prevent potential path traversal and injection attacks:
router.get('/auth/google', (req, res, next) => { const rawRedirectTo = req.query.redirectTo; const redirectTo = Array.isArray(rawRedirectTo) ? rawRedirectTo[0] : rawRedirectTo; let validatedRedirect = ''; - if (typeof redirectTo === 'string' && redirectTo.startsWith('/') && !redirectTo.startsWith('//')) { + if ( + typeof redirectTo === 'string' && + redirectTo.startsWith('/') && + !redirectTo.startsWith('//') && + !redirectTo.includes('..') && + !/[\r\n\0]/.test(redirectTo) + ) { validatedRedirect = redirectTo; } const state = validatedRedirect ? Buffer.from(validatedRedirect).toString('base64') : ''; passport.authenticate('google', { scope: ['profile', 'email'], state: state })(req, res, next); });This prevents:
- Path traversal sequences like
/../admin- CRLF injection via newlines
- Null byte injection
29-47: Apply the same hardening to callback validation.To maintain consistency with the initiation route, apply the same additional validation checks here:
(req, res) => { // Recupera o redirectTo do state parameter const state = req.query.state; let finalUrl = '/default'; if (state) { try { const decoded = Buffer.from(state, 'base64').toString('utf-8'); // Valida se o resultado é um path relativo seguro (não permite URLs externas) - if (decoded && decoded.startsWith('/') && !decoded.startsWith('//')) { + if ( + decoded && + decoded.startsWith('/') && + !decoded.startsWith('//') && + !decoded.includes('..') && + !/[\r\n\0]/.test(decoded) + ) { finalUrl = decoded; } } catch (err) { console.error('Error decoding state:', err); } } res.redirect(finalUrl); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/auth.js(2 hunks)
🔇 Additional comments (2)
src/routes/auth.js (2)
59-71: LGTM!The
/meendpoint implementation is clean and follows best practices. Authentication check is correct, and the response provides appropriate user information.
92-102: LGTM!The
/statusendpoint is well-implemented and provides a clean way for clients to check authentication state.
Descrição
Torna rota de redirect dinâmica .
Tipos de Mudanças
Como o teste foi realizado?
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.