security: close SEC-H4/H6 gaps from second-pass review

- SEC-H6: remove conditional audience check in mcp/index.ts — audience is
  now always enforced against the mcpResource URL. Add migration to revoke
  pre-existing oauth_tokens with audience=NULL so dead rows don't linger.
- SEC-H4: validate doc.issuer against config.issuer inside discover() to
  prevent a MITM'd discovery doc from supplying a crafted expected issuer.
  verifyIdToken caller now passes config.issuer as ground truth, not
  doc.issuer.
- tests: cover three new OIDC callback failure paths (no_id_token,
  id_token_invalid, subject_mismatch) and two idempotency caps (key length
  >128 chars returns 400, body >256 KiB skips caching).
This commit is contained in:
jubnl
2026-04-20 21:35:30 +02:00
parent 9f57ab4517
commit 20bf9c2312
7 changed files with 92 additions and 13 deletions
+9
View File
@@ -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) {
+4 -5
View File
@@ -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 };
}
+1 -1
View File
@@ -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';
+6
View File
@@ -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();