mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(oidc): sign in after SSO on a success flag, not the deprecated token
The token in the sign-in bodies was declared deprecated and "read by
nothing of ours", but the web client still decided an SSO sign-in by
reading data.token from GET /api/auth/oidc/exchange, an endpoint the
deprecation notice left out. Removing the field later, as documented,
would have broken SSO sign-in.
The exchange now answers { success: true, token } and the login hook
signs in when the response is OK and success is true, so it never reads
token. The field stays for API clients and is now on the deprecation
list in the API docs, the controller comments, server/CLAUDE.md and the
wiki (Security-Hardening, OIDC-SSO). Adding a field to the body breaks
no existing client.
This commit is contained in:
@@ -63,7 +63,7 @@ describe('LoginPage — OIDC redirect preservation', () => {
|
||||
beforeEach(() => {
|
||||
server.use(
|
||||
http.get('/api/auth/oidc/exchange', () =>
|
||||
HttpResponse.json({ token: 'mock-oidc-token' })
|
||||
HttpResponse.json({ success: true, token: 'mock-oidc-token' })
|
||||
),
|
||||
);
|
||||
});
|
||||
|
||||
@@ -299,7 +299,7 @@ describe('useLogin — OIDC callback', () => {
|
||||
it('FE-LOGIN-HOOK-016: navigates to the stashed redirect after a successful code exchange', async () => {
|
||||
sessionStorage.setItem('oidc_redirect', '/oauth/consent?client_id=foo');
|
||||
setSearch('?oidc_code=code-1');
|
||||
server.use(http.get('/api/auth/oidc/exchange', () => HttpResponse.json({ token: 'tok' })));
|
||||
server.use(http.get('/api/auth/oidc/exchange', () => HttpResponse.json({ success: true, token: 'tok' })));
|
||||
|
||||
const { result } = renderLogin();
|
||||
|
||||
@@ -316,7 +316,7 @@ describe('useLogin — OIDC callback', () => {
|
||||
server.use(
|
||||
http.get('/api/auth/oidc/exchange', () => {
|
||||
exchanges += 1;
|
||||
return HttpResponse.json({ token: 'tok' });
|
||||
return HttpResponse.json({ success: true, token: 'tok' });
|
||||
}),
|
||||
);
|
||||
|
||||
@@ -333,6 +333,28 @@ describe('useLogin — OIDC callback', () => {
|
||||
expect(mockNavigate).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('FE-LOGIN-HOOK-211: signs in on the success flag alone, never reading the deprecated token', async () => {
|
||||
setSearch('?oidc_code=code-flag');
|
||||
server.use(http.get('/api/auth/oidc/exchange', () => HttpResponse.json({ success: true })));
|
||||
|
||||
const { result } = renderLogin();
|
||||
await waitFor(() => expect(mockNavigate).toHaveBeenCalledWith(START_DESTINATION_ROUTE, { replace: true }));
|
||||
expect(auth.loadUser).toHaveBeenCalled();
|
||||
await waitFor(() => expect(result.current.isLoading).toBe(false));
|
||||
});
|
||||
|
||||
it('FE-LOGIN-HOOK-212: a refused exchange is a failure whatever its body holds', async () => {
|
||||
setSearch('?oidc_code=code-refused');
|
||||
server.use(
|
||||
http.get('/api/auth/oidc/exchange', () => HttpResponse.json({ success: true, error: 'Invalid or expired code' }, { status: 400 })),
|
||||
);
|
||||
|
||||
const { result } = renderLogin();
|
||||
await waitFor(() => expect(result.current.error).toBe('Invalid or expired code'));
|
||||
expect(mockNavigate).not.toHaveBeenCalled();
|
||||
expect(auth.loadUser).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('FE-LOGIN-HOOK-017: surfaces the error the exchange endpoint reports', async () => {
|
||||
setSearch('?oidc_code=code-2');
|
||||
server.use(http.get('/api/auth/oidc/exchange', () => HttpResponse.json({ error: 'State mismatch' })));
|
||||
@@ -994,7 +1016,7 @@ describe('OIDC-only auto-redirect suppression', () => {
|
||||
server.use(
|
||||
http.get('/api/auth/oidc/exchange', async () => {
|
||||
await new Promise(r => { release = r; });
|
||||
return HttpResponse.json({ token: 'tok' });
|
||||
return HttpResponse.json({ success: true, token: 'tok' });
|
||||
}),
|
||||
);
|
||||
setSearch('?oidc_code=CODE-1');
|
||||
|
||||
@@ -207,10 +207,12 @@ export function useLogin() {
|
||||
exchangeInitiated.current = true
|
||||
setIsLoading(true)
|
||||
fetch('/api/auth/oidc/exchange?code=' + encodeURIComponent(oidcCode), { credentials: 'include' })
|
||||
.then(r => r.json())
|
||||
.then(async data => {
|
||||
.then(async r => ({ ok: r.ok, data: (await r.json()) as { success?: boolean; error?: string } }))
|
||||
.then(async ({ ok, data }) => {
|
||||
window.history.replaceState({}, '', '/login')
|
||||
if (data.token) {
|
||||
// The exchange set the session cookie; the deprecated `token` in the
|
||||
// body is never read, so it can go without breaking SSO sign-in.
|
||||
if (ok && data.success === true) {
|
||||
await loadUser()
|
||||
const savedRedirect = sessionStorage.getItem('oidc_redirect') || START_DESTINATION_ROUTE
|
||||
sessionStorage.removeItem('oidc_redirect')
|
||||
|
||||
+1
-1
@@ -96,7 +96,7 @@ SQLite is the engine today and Postgres is the planned second one. Code under `s
|
||||
## Cross-cutting pieces
|
||||
|
||||
- **`src/nest/common/`**: the global exception filter (error envelope), the Zod validation pipe, the idempotency interceptor (`X-Idempotency-Key` replay), and `validate-route-guards.ts` (boot-time ratchet refusing any public route not on its allow-list).
|
||||
- **Auth is default-deny**: the global auth guard and the MFA policy guard run as `APP_GUARD`s, followed by the managed-install guard (`managed.guard.ts`), which refuses the routes a centrally administered install withholds from its own admin — it runs last so strangers still get a 401, never a route-revealing 403. Routes opt out with `@Public()`/`@OptionalAuth()`. The global guard stands down for routes declaring their own guard chain, so `@UseGuards(JwtAuthGuard, TripAccessGuard)` + `@RequirePermission` stacks work. Session is a cookie (`trek_session`); `@CurrentUser()` reads the user. Every session token carries a `jti` naming its `user_sessions` row, and `verifyJwtAndLoadUser` refuses one whose row is revoked, expired or gone (logout, `/api/auth/sessions`, a password change or reset, an MFA disable all end rows; `session-purge.job.ts` deletes the dead ones nightly). A token without a `jti` predates the table and passes until it expires, so an upgrade signs nobody out. The `token` field in the login-type JSON bodies is deprecated: kept for API clients, read by nothing of ours. **Never gate a multipart upload with a guard** — guards run before the parser and the client sees ECONNRESET instead of your 403; check in the handler.
|
||||
- **Auth is default-deny**: the global auth guard and the MFA policy guard run as `APP_GUARD`s, followed by the managed-install guard (`managed.guard.ts`), which refuses the routes a centrally administered install withholds from its own admin — it runs last so strangers still get a 401, never a route-revealing 403. Routes opt out with `@Public()`/`@OptionalAuth()`. The global guard stands down for routes declaring their own guard chain, so `@UseGuards(JwtAuthGuard, TripAccessGuard)` + `@RequirePermission` stacks work. Session is a cookie (`trek_session`); `@CurrentUser()` reads the user. Every session token carries a `jti` naming its `user_sessions` row, and `verifyJwtAndLoadUser` refuses one whose row is revoked, expired or gone (logout, `/api/auth/sessions`, a password change or reset, an MFA disable all end rows; `session-purge.job.ts` deletes the dead ones nightly). A token without a `jti` predates the table and passes until it expires, so an upgrade signs nobody out. The `token` field in the login-type JSON bodies and in the SSO exchange (`GET /api/auth/oidc/exchange`, which also answers `success: true`) is deprecated: kept for API clients, read by nothing of ours. **Never gate a multipart upload with a guard** — guards run before the parser and the client sees ECONNRESET instead of your 403; check in the handler.
|
||||
- **Logging**: `src/nest/audit/audit-log.logger.ts` (`logInfo`/`logWarn`/`logError`/`logDebug`) prints each line to the console and queues it for `data/logs/trek.log`, which `log-file.ts` appends in batches off the event loop and rotates at 10 MB over five files; `index.ts` flushes what is queued on exit. Never write the log file synchronously from a request path.
|
||||
- **Request correlation** (`src/nest/common/request-correlation.ts`): every HTTP request runs under an id (a well-formed incoming `X-Request-Id`, else a fresh UUID) that the response carries in `X-Request-Id`, and every log line written inside it is tagged `[http <id>]`. The exception filter leaves a 5xx on `res.locals` so the access log writes the request line and the stack as one entry. The other entry points (MCP tool/resource/prompt calls via the registry's `around` seam, WebSocket messages in `TrekWsAdapter`, plugin RPC dispatches in the supervisor, cron ticks in `CronRegistrarService`) each run under their own id through `traceEntry()` (`nest/audit/entry-trace.logger.ts`), which logs one line per call: debug on success, debug as `skipped` when the call declines its work on purpose (a cron tick whose lease another process holds), warn or error with the reason on failure. A new entry point goes through `traceEntry()` too.
|
||||
- **Process safety nets** (`src/index.ts`, `src/shutdown.ts`): `unhandledRejection` and `uncaughtException` are logged to trek.log and end in the same orderly shutdown as SIGTERM, with exit code 1; a second fatal error during it exits at once. A shutdown first marks `ReadinessService` (`nest/health/`) as draining, so `/api/health/ready` answers 503 for the whole drain. The HTTP server keeps idle keep-alive connections for `HTTP_KEEP_ALIVE_TIMEOUT_MS` (95 s, above the 60 s of nginx and the 90 s of Traefik) with the headers timeout one second above it (`applyProxyTimeouts` in `bootstrap.ts`).
|
||||
|
||||
@@ -27,10 +27,11 @@ const delay = (ms: number) => new Promise((r) => setTimeout(r, ms));
|
||||
* forgot response, the audit writes and the JWT httpOnly cookie set/clear via
|
||||
* the shared cookie service.
|
||||
*
|
||||
* The `token` in the login, register, demo-login and MFA bodies is deprecated:
|
||||
* the httpOnly cookie is the session, and the web app has never read the field.
|
||||
* It stays for API clients that took it as a bearer token, and the API docs
|
||||
* mark it; nothing here or in the client relies on it.
|
||||
* The `token` in the login, register, demo-login, MFA and passkey bodies (and
|
||||
* in the SSO exchange, oidc.controller.ts) is deprecated: the httpOnly cookie
|
||||
* is the session, and the web app does not read the field. It stays for API
|
||||
* clients that took it as a bearer token, and the API docs mark it; nothing
|
||||
* here or in the client relies on it.
|
||||
*/
|
||||
@Controller('api/auth')
|
||||
export class AuthPublicController {
|
||||
|
||||
@@ -236,7 +236,9 @@ export class OidcController {
|
||||
return;
|
||||
}
|
||||
this.oidc.setAuthCookie(res, result.token, req, result.remember);
|
||||
res.json({ token: result.token });
|
||||
// `success` is what the web app reads. `token` is deprecated like the one
|
||||
// in the login bodies: kept for API clients, the cookie is the session.
|
||||
res.json({ success: true, token: result.token });
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -27,8 +27,9 @@ export function setupApiDocs(app: INestApplication): void {
|
||||
+ '`GET /api/auth/sessions` lists the signed-in sessions, `DELETE /api/auth/sessions/{id}` ends one and '
|
||||
+ '`POST /api/auth/sessions/revoke-others` ends all but the current one.\n\n'
|
||||
+ '**Deprecated:** the `token` field in the bodies of `POST /api/auth/login`, `/register`, `/demo-login`, '
|
||||
+ '`/mfa/verify-login` and `/passkey/login/verify`. It is still sent, for clients that read it, but will be '
|
||||
+ 'removed in a future major version. Use the `trek_session` cookie the same response sets.',
|
||||
+ '`/mfa/verify-login`, `/passkey/login/verify` and `GET /api/auth/oidc/exchange`. It is still sent, for '
|
||||
+ 'clients that read it, but will be removed in a future major version. Use the `trek_session` cookie the '
|
||||
+ 'same response sets; the SSO exchange also answers `success: true`.',
|
||||
)
|
||||
.setVersion(version)
|
||||
.addBearerAuth({ type: 'http', scheme: 'bearer', bearerFormat: 'JWT' }, 'session')
|
||||
|
||||
@@ -103,11 +103,11 @@ describe('OIDC e2e (real cookie service)', () => {
|
||||
expect(res.body).toEqual({ error: 'Code required' });
|
||||
});
|
||||
|
||||
it('GET /exchange sets the httpOnly trek_session cookie + returns the token', async () => {
|
||||
it('GET /exchange sets the httpOnly trek_session cookie + answers success (and the deprecated token)', async () => {
|
||||
consumeAuthCode.mockResolvedValue({ token: 'jwt.value' });
|
||||
const res = await request(server).get('/api/auth/oidc/exchange').query({ code: 'good' });
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body).toEqual({ token: 'jwt.value' });
|
||||
expect(res.body).toEqual({ success: true, token: 'jwt.value' });
|
||||
const setCookie = res.headers['set-cookie'] as unknown as string[];
|
||||
expect(setCookie.some((c) => c.startsWith('trek_session=') && /HttpOnly/i.test(c))).toBe(true);
|
||||
});
|
||||
|
||||
@@ -332,7 +332,7 @@ describe('GET /api/auth/oidc/exchange', () => {
|
||||
.set('Cookie', `trek_oidc_exchange=${binding}`);
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.token).toBe(fakeToken);
|
||||
expect(res.body).toEqual({ success: true, token: fakeToken });
|
||||
expect(res.headers['set-cookie']).toBeDefined();
|
||||
const cookieHeader = Array.isArray(res.headers['set-cookie'])
|
||||
? res.headers['set-cookie'].join(';')
|
||||
|
||||
@@ -488,7 +488,7 @@ describe('OidcController /exchange', () => {
|
||||
const req3 = reqEx();
|
||||
await ctl(svc({ consumeAuthCode: vi.fn().mockResolvedValue({ token: 'jwt' }), setAuthCookie })).exchange('x', req3, r3);
|
||||
expect(setAuthCookie).toHaveBeenCalledWith(r3, 'jwt', req3, undefined);
|
||||
expect(r3.body).toEqual({ token: 'jwt' });
|
||||
expect(r3.body).toEqual({ success: true, token: 'jwt' });
|
||||
});
|
||||
|
||||
it('forwards the stored remember flag to setAuthCookie', async () => {
|
||||
@@ -498,7 +498,7 @@ describe('OidcController /exchange', () => {
|
||||
const r = reqEx();
|
||||
await ctl(svc({ consumeAuthCode: vi.fn().mockResolvedValue({ token: 'jwt', remember }), setAuthCookie })).exchange('x', r, res);
|
||||
expect(setAuthCookie).toHaveBeenCalledWith(res, 'jwt', r, remember);
|
||||
expect(res.body).toEqual({ token: 'jwt' });
|
||||
expect(res.body).toEqual({ success: true, token: 'jwt' });
|
||||
}
|
||||
});
|
||||
|
||||
|
||||
+1
-1
@@ -12,7 +12,7 @@ OpenID Connect (OIDC) lets users log in with an existing identity provider — G
|
||||
2. You are redirected to your identity provider's login page.
|
||||
3. Authenticate and grant consent.
|
||||
4. The provider redirects back to TREK at `GET /api/auth/oidc/callback`. If this is your first login, an account is created automatically (subject to registration settings).
|
||||
5. The server issues a short-lived one-time code and redirects your browser to `/login?oidc_code=<code>`. The frontend immediately exchanges that code at `GET /api/auth/oidc/exchange?code=<code>` to obtain the session.
|
||||
5. The server issues a short-lived one-time code and redirects your browser to `/login?oidc_code=<code>`. The frontend immediately exchanges that code at `GET /api/auth/oidc/exchange?code=<code>` to obtain the session: the exchange sets the `trek_session` cookie and answers `{ "success": true }`. The same body still carries the session JWT as `token` for API clients; that field is deprecated and will be removed in a future major version.
|
||||
The code is only half of what the exchange needs: the callback also sets a one-minute `HttpOnly` cookie (`trek_oidc_exchange`) holding a secret that never appears in a URL, and the exchange requires both. A code copied out of the address bar, out of history, or out of a proxy log is therefore worthless in any other browser, and it is spent by the first attempt to redeem it whether that attempt succeeds or not. If your reverse proxy strips cookies on the way back from the identity provider, SSO login will fail here with `Invalid or expired code`.
|
||||
6. Your `trek_session` cookie is set and you land on the dashboard.
|
||||
|
||||
|
||||
@@ -56,7 +56,7 @@ Sessions issued before this was introduced carry no session id. They keep workin
|
||||
|
||||
A backup restore and the hourly demo reset replace the whole database file. Both carry the sessions that are active at that moment into the new file, for every account it holds under the same id and email, so neither signs anybody out by itself.
|
||||
|
||||
> **Deprecated:** the login, registration, demo-login, MFA and passkey sign-in responses still carry the session JWT as `token` in the JSON body, for API clients that read it. The web app does not use it; the session is the httpOnly cookie the same response sets. The field will be removed in a future major version.
|
||||
> **Deprecated:** the login, registration, demo-login, MFA and passkey sign-in responses and the SSO code exchange (`GET /api/auth/oidc/exchange`) still carry the session JWT as `token` in the JSON body, for API clients that read it. The web app does not use it; the session is the httpOnly cookie the same response sets, and the SSO exchange also answers `success: true`, which is what the web app checks. The field will be removed in a future major version.
|
||||
|
||||
## Password Policy
|
||||
|
||||
|
||||
Reference in New Issue
Block a user