Skip to content

fix(oauth): set ExpiresAt from JWT exp on stored ID tokens (#549)#620

Merged
QuentinBisson merged 4 commits into
mainfrom
fix-549-stale-id-token
May 7, 2026
Merged

fix(oauth): set ExpiresAt from JWT exp on stored ID tokens (#549)#620
QuentinBisson merged 4 commits into
mainfrom
fix-549-stale-id-token

Conversation

@QuentinBisson
Copy link
Copy Markdown
Contributor

@QuentinBisson QuentinBisson commented May 5, 2026

Closes part of #549.

storeIDTokenForSSO was persisting the proxy-side ID token with zero ExpiresAt, so IsExpiredWithMargin treated it as never-expiring even after the embedded JWT was past its exp. After ~30 minutes idle, downstream servers receive an expired token, reject with 401, and the SSE listeners loop on 1-second retries flooding the logs.

Reads the JWT's exp via pkgoauth.Expiry (introduced in #619) and persists it as the entry's ExpiresAt. JWTs without a parseable exp are refused at storage time and logged at warn level — the alternative would be to land a zero-ExpiresAt entry, recreating the same leak. muster's own ID tokens are minted by mcp-oauth and always carry exp, so this only fires on misconfiguration.

Tests cover the three branches: JWT with exp → stored with correct ExpiresAt; unparseable token → refused; JWT missing exp → refused.

This closes the silent-forwarding leg. On-demand upstream refresh (option b in the issue) needs an mcp-oauth API addition and will be filed as a follow-up.

@QuentinBisson
Copy link
Copy Markdown
Contributor Author

Self-review wrap-up — ready for an external pair of eyes.

Why this should merge: Real fix for #549. storeIDTokenForSSO was persisting the proxy-side ID token with zero ExpiresAt, so IsExpiredWithMargin treated it as never-expiring even after the embedded JWT was past exp. After ~30 minutes idle, downstream servers receive an expired token, reject with 401, and the SSE listeners loop on 1-second retries flooding the logs.

Reads the JWT's exp via pkgoauth.Expiry (from #619) and persists it as the entry's ExpiresAt. Tokens without a parseable exp are refused at storage time and logged at warn — landing a zero-ExpiresAt entry would recreate the same leak. muster's own ID tokens are minted by mcp-oauth and always carry exp, so the refuse path only fires on misconfiguration.

Test trio: JWT with exp → stored with correct ExpiresAt; unparseable token → refused; JWT missing exp → refused.

Self-review pass applied:

  • Updated PR body to match the code (previously claimed a "graceful fallback" that doesn't exist — the code refuses, the CHANGELOG says refuses, the tests assert refuses).
  • Switched to pkgoauth.Expiry (replacing the now-removed local getTokenExpiryTime from refactor(oauth): consolidate JWT payload parsing into pkg/oauth/jwt.go #619). Stack-order resolved.
  • Dropped narrative Issue #549 references from the godoc and test comment.

This closes the silent-forwarding leg. On-demand upstream refresh (option b in the issue) will be a follow-up. CI green.

Copy link
Copy Markdown
Contributor

@paurosello paurosello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tight scoped fix; refuse-to-store-on-parse-error matches issue option (a). Stacked on #619 — must merge that first.

storeIDTokenForSSO was persisting the proxy-side ID token with zero
ExpiresAt, so IsExpiredWithMargin treated it as never-expiring even
after the embedded JWT was past its `exp`. After ~30min idle, downstream
MCP servers receive an expired token and reject with 401, looping the
SSE listeners on 1-second retries.

Reuse the existing getTokenExpiryTime helper to populate ExpiresAt at
storage time. Tokens with no parseable exp still store with zero (current
behavior, graceful for hypothetical opaque tokens).

This closes the silent-forwarding leg of the bug. On-demand upstream
refresh (option b in the issue) needs an mcp-oauth API addition and is
filed as a follow-up.
The previous shape left ExpiresAt zero when getTokenExpiryTime couldn't
parse an exp claim, and IsExpiredWithMargin treats zero ExpiresAt as
'never expires' — reintroducing the #549 leak via a different code path.

storeIDTokenForSSO is only ever called with muster's own ID token (a JWT
issued by mcp-oauth, always carries exp). If we can't parse one out, the
token is malformed; refuse to store and force re-auth rather than landing
a never-expiring entry. Tests updated to assert the rejection.
…auth.Expiry

The companion site at injectExternalIDToken in internal/server/oauth_http.go
had the same #549 bug as storeIDTokenForSSO: forwarded bearer tokens were
mirrored into the OAuth proxy store with zero ExpiresAt, so
IsExpiredWithMargin treated entries as never-expiring on the SSO-passthrough
path even after the embedded JWT exp had passed.

Both sites now read the exp claim via pkgoauth.Expiry, which surfaces the
underlying decode error and ErrTokenExpMissing. Tokens that don't parse or
carry no exp are refused at the storage gate and logged at WARN, forcing
re-auth instead of landing a never-expiring entry.

Adds a round-trip test asserting that a stored entry with past exp is
IsExpiredWithMargin-expired (the contract that #549 broke at the consumer
end) and updates the CHANGELOG entry to mention both call sites.
@QuentinBisson QuentinBisson force-pushed the fix-549-stale-id-token branch from a4fae67 to 299f87c Compare May 7, 2026 23:42
@QuentinBisson QuentinBisson enabled auto-merge (squash) May 7, 2026 23:44
@QuentinBisson QuentinBisson merged commit d473198 into main May 7, 2026
5 checks passed
@QuentinBisson QuentinBisson deleted the fix-549-stale-id-token branch May 7, 2026 23:45
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