feat(auth-service): redirect / to /account#102
Conversation
The bare auth-service hostname previously returned a 404 "Cannot GET /" page, which was confusing for users bookmarking or mistyping the URL. Mount a tiny router that 303-redirects / to /account, matching the redirect status used elsewhere in this service. /account itself enforces auth and bounces to /account/login when unauthenticated, so the redirect chain ends somewhere useful regardless of session state. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 548f4ad The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThe pull request adds a redirect handler for the auth service root URL. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
|
🚅 Deployed to the ePDS-pr-102 environment in ePDS
|
Coverage Report for CI Build 24732639057Coverage increased (+0.05%) to 42.568%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/auth-service/src/__tests__/root-redirect.test.ts (1)
7-44: Considersupertestto match the repo convention for route integration tests.The coding guidelines call out
supertestfor covering route handlers, and it would let you drop the manuallisten(0)/server.close()/ ephemeral-port plumbing:Proposed simplification
-import { afterAll, beforeAll, describe, expect, it } from 'vitest' -import express, { type Express } from 'express' -import type { AddressInfo } from 'node:net' -import type { Server } from 'node:http' -import { createRootRouter } from '../routes/root.js' - -let server: Server -let baseUrl: string -let app: Express - -beforeAll(async () => { - app = express() - app.use(createRootRouter()) - await new Promise<void>((resolve) => { - server = app.listen(0, '127.0.0.1', () => { - resolve() - }) - }) - const addr = server.address() as AddressInfo - baseUrl = `http://127.0.0.1:${addr.port}` -}) - -afterAll(async () => { - await new Promise<void>((resolve, reject) => { - server.close((err) => { - if (err) reject(err) - else resolve() - }) - }) -}) - -describe('root router', () => { - it('redirects GET / to /account with 303 See Other', async () => { - const res = await fetch(`${baseUrl}/`, { redirect: 'manual' }) - expect(res.status).toBe(303) - expect(res.headers.get('location')).toBe('/account') - }) -}) +import { describe, expect, it } from 'vitest' +import express from 'express' +import request from 'supertest' +import { createRootRouter } from '../routes/root.js' + +describe('root router', () => { + it('redirects GET / to /account with 303 See Other', async () => { + const app = express() + app.use(createRootRouter()) + const res = await request(app).get('/').redirects(0) + expect(res.status).toBe(303) + expect(res.headers.location).toBe('/account') + }) +})The current test works and is correct; this is a convention alignment nit. As per coding guidelines: "Do not test route handlers with unit tests — cover them via
supertestintegration tests or e2e tests instead".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/__tests__/root-redirect.test.ts` around lines 7 - 44, Replace the manual listen/close ephemeral-port setup in root-redirect.test.ts with supertest-based integration testing: import supertest (or request) and use request(app) against the Express app created with createRootRouter() (remove the beforeAll/afterAll server start/stop logic and the baseUrl management), then call await request(app).get('/') with redirect disabled and assert the response status is 303 and the Location header equals '/account' to keep the existing expectations (refer to createRootRouter and the test block that currently does fetch).packages/auth-service/src/index.ts (1)
53-73: Root redirect is subject to CSRF + rate-limit middleware — consider mounting earlier.
createRootRouter()is mounted aftercsrfProtection,requestRateLimit, andcreateSecurityHeadersMiddleware. For a pureGET /→ 303 that any visitor (including bookmarked/mistyped landings) can hit, this means:
- Each
GET /consumes a slot from the 60/min/IP bucket, so a refresh loop or a load balancer's liveness probe pointed at/could get 429s instead of a redirect.- It runs the CSRF + security-header work for what is effectively a static redirect.
CSRF for GET is typically a no-op, so that's not a correctness issue, but if you want
/to behave like/health(cheap, always-on), consider mountingcreateRootRouter()before the middleware block — or at least beforerequestRateLimit. Keeping it where it is is also fine if you explicitly want/rate-limited; worth a conscious call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/index.ts` around lines 53 - 73, The root router (createRootRouter) is mounted after csrfProtection, requestRateLimit and createSecurityHeadersMiddleware causing GET / to be subject to CSRF and rate-limiting; move the createRootRouter() mounting earlier so the GET / redirect is handled before requestRateLimit (and optionally before csrfProtection/security headers) — i.e., call app.use(createRootRouter()) before app.use(requestRateLimit(...)) (or before the entire middleware block) so the cheap root redirect bypasses those middlewares unless you intentionally want it rate-limited.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/auth-service/src/__tests__/root-redirect.test.ts`:
- Around line 7-44: Replace the manual listen/close ephemeral-port setup in
root-redirect.test.ts with supertest-based integration testing: import supertest
(or request) and use request(app) against the Express app created with
createRootRouter() (remove the beforeAll/afterAll server start/stop logic and
the baseUrl management), then call await request(app).get('/') with redirect
disabled and assert the response status is 303 and the Location header equals
'/account' to keep the existing expectations (refer to createRootRouter and the
test block that currently does fetch).
In `@packages/auth-service/src/index.ts`:
- Around line 53-73: The root router (createRootRouter) is mounted after
csrfProtection, requestRateLimit and createSecurityHeadersMiddleware causing GET
/ to be subject to CSRF and rate-limiting; move the createRootRouter() mounting
earlier so the GET / redirect is handled before requestRateLimit (and optionally
before csrfProtection/security headers) — i.e., call app.use(createRootRouter())
before app.use(requestRateLimit(...)) (or before the entire middleware block) so
the cheap root redirect bypasses those middlewares unless you intentionally want
it rate-limited.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03b4d359-b82f-4e58-a6e4-4c225305c0b4
📒 Files selected for processing (4)
.changeset/root-redirect-to-account.mdpackages/auth-service/src/__tests__/root-redirect.test.tspackages/auth-service/src/index.tspackages/auth-service/src/routes/root.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — simple, self-contained redirect with a passing integration test.
Extended reasoning...
Overview
This PR touches four files: a new Express router (), its integration test, a one-line mount in , and a changeset entry. The sole behaviour change is replacing Express's default 404 on GET / with a 303 redirect to /account.
Security risks
None. The redirect target is a hard-coded relative path (/account); there is no user-controlled input and no open-redirect risk. The change does not touch auth, sessions, CSRF, or any permission-gated code.
Level of scrutiny
Low. This is a missing-route fill-in — the kind of one-liner that is hard to get wrong. The router is mounted first but only matches the exact path /, leaving all other routes unaffected.
Other factors
The PR ships with an integration test that verifies the exact status code (303) and Location header (/account) using redirect: 'manual'. The test plan notes 595/595 suite passes. The changeset includes a clear operator note about healthchecks. No outstanding reviewer comments.
1 participant
Summary
GET /on the auth service now 303-redirects to/account(which in turn bounces unauthenticated users to/account/login).createRootRouter()inpackages/auth-service/src/routes/root.tsplus an integration test that spins up a minimal Express app and assertsstatus === 303andlocation === '/account'withredirect: 'manual'.Operator note
If you have an external healthcheck pointed at
/expecting a 404, switch it to/health(unchanged, returns JSON status).Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
/healthendpoint instead of the root path.Tests