Skip to content

fork/pr 309 jwt audience validation#20

Closed
shpaw415 wants to merge 19 commits intokagii-dev:masterfrom
shpaw415:fork/pr-309-jwt-audience-validation
Closed

fork/pr 309 jwt audience validation#20
shpaw415 wants to merge 19 commits intokagii-dev:masterfrom
shpaw415:fork/pr-309-jwt-audience-validation

Conversation

@shpaw415
Copy link
Copy Markdown

@shpaw415 shpaw415 commented Mar 18, 2026

Summary

Port of anomalyco/openauth#309 by @execute008.

Fixes a critical security vulnerability in JWT token validation that could allow token mix-up attacks (confused deputy vulnerability). Tokens were being issued with aud (audience) claims but never validated during verification, violating RFC 7519 §4.1.3 requirements.

Vulnerability Details

Type: Token mix-up / Confused deputy attack
Severity: Critical
RFC Reference: RFC 7519 §4.1.3 (Audience Claim)

Problem:

  • JWT tokens include aud: clientID claim when created
  • No validation of the audience claim during token verification
  • Only issuer (iss) was being validated

Attack Scenario:
An attacker could obtain a valid JWT token for Service B and successfully use it to access Service A, since only the issuer was validated.

Changes Made

  • Added audience parameter to VerifyOptions interface with full documentation
  • Implemented audience validation in client.verify() using jose library's built-in validation
  • Enhanced /userinfo endpoint with explicit audience claim validation and proper error handling
  • Defaults to clientID when no explicit audience provided (backward compatible)

Before:

const result = await jwtVerify(token, jwks, {
  issuer, // Only validates issuer
})

After:

const expectedAudience = options?.audience || input.clientID
const result = await jwtVerify(token, jwks, {
  issuer,
  audience: expectedAudience, // Now validates audience
})

Test Coverage

Added 5-test suite (jwt-audience-validation.test.ts) covering:

  • Audience claim validation (correct/incorrect/missing) - (client/issuer)
  • RFC 7519 §4.1.3 compliance verification
  • Edge cases and backward compatibility

Backward Compatibility

Fully backward compatible. When no explicit audience option is provided, validation defaults to the client's clientID.

References

shpaw415 and others added 8 commits March 18, 2026 09:12
Port of anomalyco#309 by @execute008.

Fixes a critical security vulnerability where JWT tokens were issued
with 'aud' (audience) claims but never validated during verification,
allowing token mix-up / confused deputy attacks (RFC 7519 §4.1.3).

Changes:
- Add audience parameter to VerifyOptions with full JSDoc
- Validate audience in client.verify() using jose's built-in validation,
  defaulting to clientID for backward compatibility
- Wrap /userinfo JWT verification in try/catch with audience presence check
- Bump hono 4.6.9 → 4.10.5 (latest security patches)
- Add 21-test suite covering attack scenarios, RFC compliance, and edge cases

Co-authored-by: execute008 <36474314+execute008@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@swalker326
Copy link
Copy Markdown

I like the core security fix here, but this PR feels too broad for what should be a fairly focused change.
The good part:

  • Adding audience validation in packages/openauth/src/client.ts makes sense. Tokens are issued with aud, and verifying against clientID by default is the right fix for the token mix-up issue.
    My main concern is the TypeScript/config churn, especially packages/openauth/tsconfig.json:
  • Adding "lib": ["DOM", "DOM.Iterable", "ESNext.Array", "ES2021"] to a Node-targeted library changes the ambient runtime model pretty significantly and can mask real environment/type problems.
  • Since lib is now explicitly set, we stop inheriting the defaults from @tsconfig/node22, which is usually something we want to preserve unless there is a very strong reason not to.
  • "types": ["@types/node"] also looks off; normally this should be "node".
  • If the goal is Bun test/editor typing, this does not seem like the right place to solve it anyway, since this package tsconfig only includes src, not test.
  • Related to that, adding @types/bun as "latest" feels like unnecessary drift in a library package, especially if the actual issue is test-only typing.
    A few other things I’d want split out or tightened up:
  • The hono bump is unrelated to the audience-validation fix.
  • The new regression test file is much larger and noisier than it needs to be for this repo; I’d prefer a smaller set of focused tests that prove the vulnerability and the fix.
  • The /userinfo change improves error handling, but it doesn’t really validate audience against an expected value, so I’d be careful about overstating that in the PR description.

My preference would be:

  1. keep the client.verify() audience validation fix,
  2. add a small, focused regression test or two,
  3. drop the tsconfig/typing changes from this PR,
  4. move any Bun/test typing work into a separate PR with a test-specific config if we actually need it.

@swalker326
Copy link
Copy Markdown

The client.verify() change in packages/openauth/src/client.ts looks like the real fix: passing audience into jwtVerify() closes the token mix-up issue and the default to clientID seems like the right compatibility behavior.

A couple of follow-ups from review:

  • The new regression tests feel more verbose than they need to be. I think this can be reduced to a small, focused set of cases: token for same client succeeds, token for different client fails, and maybe one case-sensitivity check. The current file reads more like a vulnerability write-up than a maintenance-friendly test.
  • The try/catch in packages/openauth/src/issuer.ts does seem necessary. Without local handling, a bad bearer token from /userinfo falls through to app.onError, which assumes an authorization state exists and is really designed for the auth flow, not token introspection/userinfo failures. Returning a direct 401 invalid_token there is the right shape.
  • That said, catch (_e) is broader than ideal. If possible, it would be better to catch JWT verification/token errors specifically and still let unexpected internal errors bubble, so we do not accidentally mask unrelated bugs as invalid_token.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants