Skip to content

fix(server): answer 401 when a token's user or namespace is gone - #6848

Merged
gustavosbarreto merged 3 commits into
masterfrom
fix/authn-stale-jwt-401
Aug 3, 2026
Merged

fix(server): answer 401 when a token's user or namespace is gone#6848
gustavosbarreto merged 3 commits into
masterfrom
fix/authn-stale-jwt-401

Conversation

@otavio

@otavio otavio commented Aug 3, 2026

Copy link
Copy Markdown
Member

What

The in-process authenticator now yields no identity when it cannot resolve the
role or admin flag behind a JWT, so a stale token gets a 401 instead of a 500.

Why

Resolve propagated the raw store error from GetUserRole and GetUserAdmin.
Those errors carry Layer: "store", which the echo error handler maps to 500,
so every authenticated request with a token referencing deleted entities
answered 500 and the client was never told to re-authenticate. A database reset
without clearing the browser session reproduces it on every GET /api/namespaces.

Before auth moved into the API process, the nginx edge proxy turned any non-2xx
from the /internal/auth subrequest into a 401 and never leaked the store's own
status codes. 7fb6ef42 restored that behaviour for API keys; this restores it
for bearer tokens.

Closes #6844

Changes

  • middleware/authn.go: both lookups in the *authorizer.UserClaims branch
    return nil, nil on error. The middleware's fail-closed path then answers 401.
    A single comment above the branch carries the rationale for both sites rather
    than repeating the one on the API-key branch.
  • middleware/authn_test.go: new. Covers the happy path, a missing namespace,
    a missing user, and — pinning the deliberate breadth of the rule — a store
    transport error and a cancelled request. Two middleware-level tests assert the
    401 on a protected route and that an anonymous route still reaches the handler
    with forged identity headers scrubbed.

Testing

The blanket swallow is the point of contention worth a reviewer's attention: a
store outage now logs users out with a 401 rather than surfacing a 5xx you can
alert on. That matches the edge proxy's old behaviour and the API-key branch
directly above (whose comment already calls out "including when its store was
down"), but it is broader than "the entity does not exist". The two
non-not-found test cases exist so any future narrowing is visible in CI.

The in-process authenticator propagated the store's own error when the JWT
referenced entities that no longer exist, so the error handler mapped it to a
500 and the client was never told to re-authenticate. Before auth moved into
the API process, the edge proxy turned any non-2xx from the authentication
subrequest into a 401.

Fixes: #6844
@otavio
otavio requested a review from a team as a code owner August 3, 2026 20:13
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 1m 42s —— View job


Code Review Complete

Reviewed 2 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

The nil, nil //nolint:nilerr returns are consistent with the API-key branch above them, the middleware's fail-closed path correctly turns a nil identity into a 401 on protected routes, and the new tests cover the happy path, missing namespace/user, store outage, cancellation, and the middleware-level 401 plus forged-header scrubbing on an anonymous route. The deliberate breadth of the error swallow (store outage → 401 instead of an alertable 5xx) is called out in both the comment and the PR description, and the two non-not-found test cases pin it so any future narrowing is visible in CI.


To request another review round, comment `/review`.

Yielding no identity on a failed role or admin lookup makes a store outage
indistinguishable from a stale token: both surface only as a rise in 401s.
@otavio

otavio commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 1m 19s —— View job


Code Review Complete

Reviewed 2 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

I verified the package compiles, the mocks.MockService satisfies the AuthnService interface used by the new tests, and the logic:

  • The return nil, nil //nolint:nilerr sites in the *authorizer.UserClaims branch mirror the API-key branch directly above, and the middleware's fail-closed path (identity == nil && !anonymous → 401) correctly turns the nil identity into a 401 on protected routes.
  • Swallowing the store error is now paired with a Warn log at both sites, so the deliberately-broad rule (store outage → 401 instead of an alertable 5xx) remains observable — matching the rationale in the comment and the PR description.
  • The new tests cover the happy path, missing namespace, missing user, store outage, and cancellation at the Resolve level, plus middleware-level assertions for the 401 on a protected route and forged X-ID header scrubbing on an anonymous route.

The two non-not-found test cases (store is unreachable, request is cancelled) pin the intended breadth so any future narrowing of the rule shows up in CI.


To request another review round, comment `/review`.

Completes the previous commit: the API key branch swallowed its error on the
same rationale and was the last undiagnosable one left in Resolve.
@otavio

otavio commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 3m 20s —— View job


Code Review Complete

Reviewed 2 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

Verified this round:

  • Fail-closed path is correct. Resolve now returns nil, nil on a stale token, so Middleware hits identity == nil && !anonymous401 on protected routes, and proceeds with a nil (scrubbed) identity on anonymous ones. No partially-populated Identity can escape the error branch.
  • No forged-header leak. Identity.WriteTo (gateway/identity.go:42-49) unconditionally Dels every identity header before its if i == nil { return } guard, so the identity.WriteTo call on a possibly-nil identity is both nil-safe and still scrubs forged X-* headers — as the new anonymous-route test asserts.
  • No credential logging. Both log.WithError(err) sites log only user_id/tenant_id plus the store error; the bearer token and API key never reach the log.
  • Tests compile and match call order. mocks.MockService satisfies AuthnService; the not-found/outage cases mock only GetUserRole().Once() (correct, since Resolve returns before GetUserAdmin), and the happy/user-gone/cancel cases mock both. RoleFromString("owner")RoleOwner matches the expected identity.
  • No cross-repo break. AuthnService is unchanged, Resolve is called only from Middleware, and nothing in cloud/ depends on the previous 500 or on error propagation through this middleware.

The nil, nil //nolint:nilerr swallow-and-log is intentional, consistent with the API-key branch above it, and pinned by the two non-not-found test cases so any future narrowing shows up in CI.


To request another review round, comment `/review`.

@gustavosbarreto
gustavosbarreto merged commit e94cacc into master Aug 3, 2026
38 checks passed
@gustavosbarreto
gustavosbarreto deleted the fix/authn-stale-jwt-401 branch August 3, 2026 21:15
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.

fix(server): authn middleware leaks store errors as 500 for stale JWTs

2 participants