diff --git a/release-notes.md b/release-notes.md deleted file mode 100644 index 77545143..00000000 --- a/release-notes.md +++ /dev/null @@ -1,7 +0,0 @@ -# Release Notes - -## v2.9.11 - -### Bug Fixes - -- **OIDC-only mode: resolved login/logout loop** — When password authentication is disabled, logging out no longer triggers an immediate re-authentication loop. After logout, users land on the login page and must manually click "Sign in with {provider}" to start the OIDC flow. Also fixed a secondary loop that could occur on the OIDC callback page under React 18 StrictMode, where the auth code exchange would be interrupted before completing, causing the app to redirect back to the identity provider instead of landing on the dashboard. (#491) diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index 4994ec20..4af7f35c 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -1867,6 +1867,15 @@ function runMigrations(db: Database.Database): void { CREATE INDEX IF NOT EXISTS idx_idempotency_keys_created ON idempotency_keys(created_at); `); }, + // SEC-H6: revoke all OAuth tokens issued before audience binding was + // enforced. mcp/index.ts now unconditionally checks audience; tokens + // with audience=null would be permanently rejected by the check, so + // removing them here avoids leaving dead rows and makes the intent clear. + () => { + const hasCol = db.prepare("SELECT name FROM pragma_table_info('oauth_tokens') WHERE name = 'audience'").get(); + if (!hasCol) return; + db.prepare('DELETE FROM oauth_tokens WHERE audience IS NULL').run(); + }, ]; if (currentVersion < migrations.length) { diff --git a/server/src/mcp/index.ts b/server/src/mcp/index.ts index 4d215260..e46c03db 100644 --- a/server/src/mcp/index.ts +++ b/server/src/mcp/index.ts @@ -180,11 +180,10 @@ function verifyToken(authHeader: string | undefined): VerifyTokenResult | null { if (token.startsWith('trekoa_')) { const result = getUserByAccessToken(token); if (!result) return null; - // RFC 8707: if the token carries an audience, it must match this resource endpoint - if (result.audience !== null) { - const expected = `${(getAppUrl() || '').replace(/\/+$/, '')}/mcp`; - if (result.audience !== expected) return null; - } + // RFC 8707: audience must always match this resource endpoint. + // Pre-audit tokens with audience=null are revoked by the SEC-H6 migration. + const expected = `${(getAppUrl() || '').replace(/\/+$/, '')}/mcp`; + if (result.audience !== expected) return null; return { user: result.user, scopes: result.scopes, clientId: result.clientId, isStaticToken: false }; } diff --git a/server/src/routes/oidc.ts b/server/src/routes/oidc.ts index 85145c5a..dcd21a78 100644 --- a/server/src/routes/oidc.ts +++ b/server/src/routes/oidc.ts @@ -112,7 +112,7 @@ router.get('/callback', async (req: Request, res: Response) => { tokenData.id_token, doc, config.clientId, - doc.issuer || config.issuer, + config.issuer, ); if (idVerify.ok !== true) { const reason = 'error' in idVerify ? idVerify.error : 'unknown'; diff --git a/server/src/services/oidcService.ts b/server/src/services/oidcService.ts index efc1f7b8..db0ef8ad 100644 --- a/server/src/services/oidcService.ts +++ b/server/src/services/oidcService.ts @@ -140,6 +140,12 @@ export async function discover(issuer: string, discoveryUrl?: string | null): Pr const res = await fetch(url); if (!res.ok) throw new Error('Failed to fetch OIDC discovery document'); const doc = (await res.json()) as OidcDiscoveryDoc; + // Validate that the discovery doc's issuer matches the operator-configured + // one. A MITM or compromised doc could otherwise supply a crafted issuer + // that passes jwt.verify() because we used doc.issuer as the expected value. + if (doc.issuer && doc.issuer !== issuer) { + throw new Error(`OIDC discovery issuer mismatch: expected "${issuer}", got "${doc.issuer}"`); + } doc._issuer = url; discoveryCache = doc; discoveryCacheTime = Date.now(); diff --git a/server/tests/integration/oidc.test.ts b/server/tests/integration/oidc.test.ts index 1c7ae1b1..d57a935f 100644 --- a/server/tests/integration/oidc.test.ts +++ b/server/tests/integration/oidc.test.ts @@ -223,6 +223,49 @@ describe('GET /api/auth/oidc/callback', () => { expect(res.headers.location).toContain('oidc_error=token_failed'); }); + it('OIDC-010a: missing id_token in token response → redirects with no_id_token error', async () => { + mockDiscover.mockResolvedValueOnce(MOCK_DISCOVERY_DOC); + mockExchangeCode.mockResolvedValueOnce({ access_token: 'tok', _ok: true, _status: 200 }); // no id_token + + const state = oidcService.createState('http://localhost:3001/api/auth/oidc/callback'); + + const res = await request(app).get(`/api/auth/oidc/callback?code=anycode&state=${state}`); + + expect(res.status).toBe(302); + expect(res.headers.location).toContain('oidc_error=no_id_token'); + }); + + it('OIDC-010b: verifyIdToken failure → redirects with id_token_invalid error', async () => { + mockDiscover.mockResolvedValueOnce(MOCK_DISCOVERY_DOC); + mockExchangeCode.mockResolvedValueOnce({ access_token: 'tok', id_token: 'bad.id.token', _ok: true, _status: 200 }); + mockVerifyIdToken.mockResolvedValueOnce({ ok: false, error: 'signature_or_claim_mismatch: invalid signature' }); + + const state = oidcService.createState('http://localhost:3001/api/auth/oidc/callback'); + + const res = await request(app).get(`/api/auth/oidc/callback?code=anycode&state=${state}`); + + expect(res.status).toBe(302); + expect(res.headers.location).toContain('oidc_error=id_token_invalid'); + }); + + it('OIDC-010c: userinfo.sub does not match id_token.sub → redirects with subject_mismatch error', async () => { + mockDiscover.mockResolvedValueOnce(MOCK_DISCOVERY_DOC); + mockExchangeCode.mockResolvedValueOnce({ access_token: 'tok', id_token: 'fake.id.token', _ok: true, _status: 200 }); + mockVerifyIdToken.mockResolvedValueOnce({ ok: true, claims: { sub: 'sub-from-token' } }); + mockGetUserInfo.mockResolvedValueOnce({ + sub: 'sub-different-from-userinfo', + email: 'alice@example.com', + name: 'Alice', + }); + + const state = oidcService.createState('http://localhost:3001/api/auth/oidc/callback'); + + const res = await request(app).get(`/api/auth/oidc/callback?code=anycode&state=${state}`); + + expect(res.status).toBe(302); + expect(res.headers.location).toContain('oidc_error=subject_mismatch'); + }); + it('OIDC-010: registration disabled for new user → redirects with registration_disabled error', async () => { // Need at least one existing user so isFirstUser=false createUser(testDb, { email: 'existing@example.com' }); diff --git a/server/tests/unit/middleware/idempotency.test.ts b/server/tests/unit/middleware/idempotency.test.ts index e45a9f17..28bebada 100644 --- a/server/tests/unit/middleware/idempotency.test.ts +++ b/server/tests/unit/middleware/idempotency.test.ts @@ -134,6 +134,35 @@ describe('applyIdempotency', () => { expect(rows['fail-key:3:POST:/api/test']).toBeUndefined(); }); + it('returns 400 when X-Idempotency-Key exceeds 128 characters', () => { + const longKey = 'a'.repeat(129); + const req = makeReq('POST', { 'x-idempotency-key': longKey }); + const res = makeRes(); + const next = vi.fn(); + applyIdempotency(req, res, next, 1); + expect(next).not.toHaveBeenCalled(); + expect(res.json as ReturnType).toHaveBeenCalledWith( + expect.objectContaining({ error: expect.stringContaining('128') }), + ); + }); + + it('does NOT cache response body exceeding 256 KiB', () => { + const req = makeReq('POST', { 'x-idempotency-key': 'big-key' }); + const res = makeRes(200); + const originalJsonSpy = res.json as ReturnType; + const largePayload = { data: 'x'.repeat(256 * 1024 + 1) }; + const next = vi.fn(() => { + // res.json is now the wrapper; calling it exercises the size-cap branch + res.json(largePayload); + }); + applyIdempotency(req, res, next, 5); + expect(next).toHaveBeenCalledOnce(); + // Underlying spy was called (response reached the client) + expect(originalJsonSpy).toHaveBeenCalledWith(largePayload); + // But NOT stored in the idempotency store + expect(rows['big-key:5:POST:/api/test']).toBeUndefined(); + }); + it('handles PUT, PATCH, and DELETE the same as POST', () => { for (const method of ['PUT', 'PATCH', 'DELETE'] as const) { const req = makeReq(method, { 'x-idempotency-key': `key-${method}` });