diff --git a/.env.development b/.env.development new file mode 100644 index 0000000..3a070b4 --- /dev/null +++ b/.env.development @@ -0,0 +1,22 @@ +# IPAM development-only environment overrides. +# Picked up by Vite automatically (`mode === 'development'`) and bundled into +# the client at dev-server start. Production builds do not read this file. +# +# Why this exists (NUL-231 / NUL-218): +# In dev, the SPA is served at http://localhost:5173 and the Hono backend +# listens at http://localhost:8787. Modern browsers treat those as +# different origins, so a cookie-bearing `fetch('http://localhost:8787/...')` +# is preflight-blocked by CORS (the Hono server has no Access-Control-Allow- +# Origin for the dev origin, by design — adding it would weaken prod). +# +# `vite.config.ts` adds `server.proxy['/api']` → :8787 so requests to +# `/api/*` issued against the Vite origin are forwarded server-side and +# arrive at the browser as same-origin. To use the proxy, the client must +# target the Vite origin, not :8787. Hence VITE_API_URL=http://localhost:5173. +# +# The same-origin proxy path means: +# - cookies (Lax) ride :5173 → :5173 → backend → :5173 without preflight; +# - the session cookie's sameSite=Lax semantics apply to dev too; +# - the production build (which is served same-origin behind nginx) is +# unaffected — it reads .env / .env.production, not this file. +VITE_API_URL=http://localhost:5173 diff --git a/playwright.config.ts b/playwright.config.ts index 3e8cf0b..29453fd 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -8,11 +8,19 @@ import { defineConfig, devices } from '@playwright/test' * in another terminal first, or let the npm script `test:e2e` do it. * * Hostname note: the dev stack is reachable on either `127.0.0.1` or - * `localhost`, but only `localhost` is in the Hono CORS allowlist - * (NUL-11 — pre-existing dev-server CORS gap). Hitting `127.0.0.1` would - * silently CORS-block every `/api/*` call, leaving the route-guard - * splash stuck and every test waiting 60 s for an input that never - * mounts. Pin `baseURL` + `PLAYWRIGHT_API_URL` to `localhost`. + * `localhost`. The Hono server deliberately serves no CORS headers + * (NUL-231 — adding dev CORS would weaken prod), so any browser fetch + * to http://127.0.0.1:8787 from a page on http://localhost:5173 (or + * vice versa) is preflight-blocked. Pin `baseURL` + `PLAYWRIGHT_API_URL` + * to `localhost` so every URL the browser sees has a single origin + * (after Vite proxies /api through to :8787 server-side). + * + * `--disable-web-security` on the projects below is intentionally kept + * because `tests/onboarding.spec.ts` issues fixture calls + * (`fetch(http://localhost:8787/...)` for the tour-reset PATCH) from + * inside the page evaluate; those are deliberately cross-origin and + * would need their own CORS or proxy rewrite to drop the flag. That + * refactor is out of scope for NUL-271 and tracked separately. * * Screenshot output: `test-results/onboarding/` so failures and the * deliberate capture step share a directory the CI workflow can archive. @@ -39,10 +47,11 @@ export default defineConfig({ ...devices['Desktop Chrome'], viewport: { width: 1280, height: 800 }, colorScheme: 'light', - // NUL-11 dev CORS gap: the Hono server does not return CORS headers, - // and the Vite dev server has no /api proxy. Real deploys sit behind - // a same-origin reverse proxy so this is dev-only. `--disable-web-security` - // is safe here because the dev stack only listens on localhost. + // NUL-231: the prod path is same-origin (nginx serves SPA + /api), + // and the dev path is now also same-origin through Vite's /api proxy. + // `--disable-web-security` is still passed because tests/onboarding.spec.ts + // also issues direct :8787 fixture calls inside page.evaluate() that + // are deliberately cross-origin; see the file-header comment. launchOptions: { args: ['--disable-web-security'] }, }, }, diff --git a/src/features/auth/route-guard.test.tsx b/src/features/auth/route-guard.test.tsx index 8e88c47..c76fd74 100644 --- a/src/features/auth/route-guard.test.tsx +++ b/src/features/auth/route-guard.test.tsx @@ -198,9 +198,11 @@ test('splash skeleton markup renders while /me is in a 401 error state', async ( throw err } - // 401 from /me — the component renders while the - // redirect effect runs. Once the effect fires (which we cannot - // observe under SSR), the location changes to /login. + const { __setLocation } = await import('../../../scripts/_test-mocks/router.mjs') + __setLocation({ pathname: '/login' }) + + // 401 from /me on the login route must not hide the destination form. + // The route guard's redirect effect treats /login as a no-op. globalThis.__routeGuardMeState = { isLoading: false, isSuccess: false, @@ -214,8 +216,11 @@ test('splash skeleton markup renders while /me is in a 401 error state', async ( const html = renderToStaticMarkup(createElement(AuthGuard, null, 'main-content')) - assert.match(html, /data-testid="skeleton"/) - assert.doesNotMatch(html, /main-content/) + // On /login the destination form must remain renderable even when the + // proxied /me request returns a typed 401. The guard's redirect effect + // already treats /login as a no-op; a splash here would hide the form. + assert.doesNotMatch(html, /data-testid="skeleton"/) + assert.match(html, /main-content/) }) test.afterEach(() => { diff --git a/src/features/auth/route-guard.tsx b/src/features/auth/route-guard.tsx index 30b5428..826f86c 100644 --- a/src/features/auth/route-guard.tsx +++ b/src/features/auth/route-guard.tsx @@ -103,11 +103,11 @@ export function AuthGuard({ children }: { children: ReactNode }) { } }, [currentPath, currentSearch, navigate, queryClient]) - if (me.isLoading) { + if (me.isLoading && currentPath !== '/login') { return } - if (me.isError) { + if (me.isError && currentPath !== '/login') { const status = (me.error as { status?: number } | null)?.status if (status === 401) return } diff --git a/src/features/dashboard/dashboard-page.tsx b/src/features/dashboard/dashboard-page.tsx index e40ff5e..b065e87 100644 --- a/src/features/dashboard/dashboard-page.tsx +++ b/src/features/dashboard/dashboard-page.tsx @@ -118,8 +118,8 @@ export function DashboardPage() { {fp.name} - {fp.rackPositions.length} rack - {fp.rackPositions.length === 1 ? '' : 's'} + {(fp.rackPositions ?? []).length} rack + {(fp.rackPositions ?? []).length === 1 ? '' : 's'} ))} diff --git a/src/server/__tests__/auth-and-tenant.test.ts b/src/server/__tests__/auth-and-tenant.test.ts index 0cfd737..5723bc0 100644 --- a/src/server/__tests__/auth-and-tenant.test.ts +++ b/src/server/__tests__/auth-and-tenant.test.ts @@ -528,4 +528,86 @@ test('unauthenticated POST /api/upload returns 401', async () => { const env = asEnvelope(await r.json()) assert.equal(env.error.code, 'unauthenticated') }) +}) + +// ============================================================================= +// 9. Logout actually clears the session cookie (NUL-230 regression) +// ============================================================================= +// +// Before the fix, the /api/auth/logout handler called clearSessionCookie(c) +// to schedule a Set-Cookie header, but then returned a bare +// `new Response(null, { status: 204 })` which dropped the prepared headers. +// The browser kept the old ipam_session cookie and the next /api/auth/me +// still returned 200, so the user appeared to still be logged in after +// logout. The fix uses c.body(null, 204) so the Set-Cookie header is +// actually included in the 204 response. + +test('POST /api/auth/logout returns 204 with a Set-Cookie that expires ipam_session', async () => { + await withFreshServer(async (h) => { + const cookie = await login(h.base, INTERNAL_ADMIN) + const r = await fetch(`${h.base}/api/auth/logout`, { + method: 'POST', + headers: { cookie }, + }) + assert.equal(r.status, 204) + const setCookie = r.headers.get('set-cookie') ?? '' + assert.ok( + setCookie.toLowerCase().includes('ipam_session='), + `expected logout response to include a Set-Cookie for ipam_session, got: ${setCookie}`, + ) + assert.ok( + /(?:expires=|max-age=0|max-age=-?\d+)/i.test(setCookie), + `expected logout response Set-Cookie to expire the cookie (expires= or max-age<=0), got: ${setCookie}`, + ) + }) +}) + +test('GET /api/auth/me after /api/auth/logout returns 401 to clients that drop the cookie', async () => { + await withFreshServer(async (h) => { + const cookie = await login(h.base, INTERNAL_ADMIN) + + // Sanity: /me works before logout. + const before = await fetch(`${h.base}/api/auth/me`, { headers: { cookie } }) + assert.equal(before.status, 200) + + // Logout. The response must clear the session cookie on the client + // (the previous /logout bug dropped the prepared Set-Cookie header). + const logout = await fetch(`${h.base}/api/auth/logout`, { + method: 'POST', + headers: { cookie }, + }) + assert.equal(logout.status, 204) + assert.ok( + (logout.headers.get('set-cookie') ?? '').toLowerCase().includes('ipam_session='), + 'precondition: logout response must include Set-Cookie for ipam_session', + ) + + // Simulate what a real browser does on receiving that Set-Cookie: + // the cookie is removed from the jar, so the next /me request goes + // out with no cookie. The server must then return 401. + const after = await fetch(`${h.base}/api/auth/me`, { + headers: { /* no cookie */ }, + }) + assert.equal( + after.status, + 401, + `expected /api/auth/me without cookie after logout to return 401, got ${after.status}`, + ) + }) +}) + +test('POST /api/auth/login still works after the logout fix (regression check)', async () => { + await withFreshServer(async (h) => { + // Login, logout, login again — the second login should still return + // 200 with a fresh session cookie, proving the logout fix didn't + // accidentally break the login path. + const cookie1 = await login(h.base, INTERNAL_ADMIN) + const logout = await fetch(`${h.base}/api/auth/logout`, { + method: 'POST', + headers: { cookie: cookie1 }, + }) + assert.equal(logout.status, 204) + const cookie2 = await login(h.base, INTERNAL_ADMIN) + assert.ok(cookie2.length > 0, 'second login should still return a session cookie') + }) }) \ No newline at end of file diff --git a/src/server/auth.ts b/src/server/auth.ts index 1ef0681..e44c611 100644 --- a/src/server/auth.ts +++ b/src/server/auth.ts @@ -315,8 +315,15 @@ authApp.post('/login', async (c) => { }) authApp.post('/logout', (c) => { + // NUL-230: use c.body(null, 204) so the Set-Cookie header prepared by + // clearSessionCookie(c) is actually sent. Returning a bare + // `new Response(null, { status: 204 })` discards c.res.headers, so the + // browser kept its old ipam_session cookie and the next /api/auth/me + // request still succeeded (the user appeared to be "still logged in" + // after logout). See src/server/__tests__/auth-and-tenant.test.ts for + // the regression test. clearSessionCookie(c) - return new Response(null, { status: 204 }) + return c.body(null, 204) }) authApp.get('/me', requireAuth, (c) => { diff --git a/vite.config.ts b/vite.config.ts index 1f95e83..8ed1f69 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -18,5 +18,16 @@ export default defineConfig({ server: { port: 5173, host: true, + // NUL-231: dev-only proxy so cookie-bearing /api fetches from :5173 + // reach the Hono backend on :8787 same-origin, avoiding browser CORS + // preflight. Production is already same-origin (nginx serves SPA + /api + // from one host), so this block has no production effect. + proxy: { + '/api': { + target: 'http://localhost:8787', + changeOrigin: false, + secure: false, + }, + }, }, })