fix(auth): serve clock-skewed tokens anonymously in permissive mode (#133)

[piyalbasu]

TL;DR

Users whose device clock is badly wrong are currently locked out of every Freighter feature that talks to this service. Their requests are correctly signed and would be served happily if they sent no credentials at all — but because they do send one, and it carries a timestamp we disagree with, we reject them. That defeats the purpose of the permissive rollout mode, which exists precisely so the auth migration doesn't break anyone.

This makes permissive mode treat a wrong clock — in either direction, running fast or slow — as a failure to authenticate rather than a reason to refuse service: those requests are now served anonymously, exactly as if no credentials had been presented. A malformed or mis-bound credential is still rejected, and strict mode is unchanged.

In the first day of real traffic this affected ~5 devices making ~335 failed requests, each failing 100% of the time, out of 424 users on the new version. Widening the existing clock-tolerance setting cannot fix it: the clocks in question are wrong by hours, not seconds.

Closes #133. Mitigates #147 (does not close it — see below).

Implementation details (for agents)

What changed

• internal/api/middleware/auth.go — the ErrUnauthorized branch computes permitted := mode == auth.Permissive && (reason == auth.ReasonExpired || reason == auth.ReasonClockAhead). When true it records, logs, then breaks out of the switch to the existing next.ServeHTTP — the same anonymous path a no-token request already takes, with no userID attached. Everything else still renders 401 and returns.
• internal/auth/errors.go / claims.go — new ReasonClockAhead and ClockAheadError{AheadBy}; the two future-bound checks in Claims.Validate now return it instead of ReasonBadTiming.
• internal/metrics/metrics.go — new exported ResultInvalidPermitted = "invalid_permitted" result label.
• Log line keeps reason/detail/method/path, gains a permitted field, and its message changes from "rejected request with invalid auth token" to "invalid auth token" (the old wording would be false for served requests). Verified nothing in wallet-eng-runbooks greps either the old message or the metric.
• Design doc mode table updated, including why the original rationale was wrong.

Which reasons are permitted, and why the timing bucket got split

expired and clock_ahead — the two directions a clock can be wrong. clock_ahead is new in this PR, split out of bad_timing.

That split is the substance of the change. bad_timing was never a clock signal: it bundled two genuine clock checks (iat or exp too far ahead) with three malformed-token checks (missing exp/iat, exp preceding iat, over-long lifetime). Permitting the whole bucket would serve malformed tokens and make invalid_permitted — the counter that gates the strict flip — mix wrong clocks with client bugs. Rejecting the whole bucket, which an earlier revision of this PR did, leaves fast-clock users locked out for a reason unrelated to them. So neither side of the bucket was the right unit; the bucket was.

New ReasonClockAhead + ClockAheadError, the latter carrying AheadBy so the overshoot is diagnosable from logs exactly as ExpiredTokenError.ExpiredBy already is for lagging clocks. bad_timing keeps the malformed cases and still 401s.

One consequence worth naming explicitly

clock_ahead is assigned in Claims.Validate, which runs before signature verification — so a forged token dated into the future is now served, without its signature ever being checked.

That is safe rather than merely tolerated: permitting attaches no userID, so the request is byte-for-byte equivalent to one carrying no Authorization header, which these routes already serve anonymously. An attacker gains nothing they could not have by sending no token at all. TestAuth_PermissiveForgedTokenOutcomes pins all three forged-token outcomes and asserts the invariant that makes it safe — a forged token never yields a userID.

The cost is measurement, not access: read invalid_permitted{reason="clock_ahead"} as an upper bound on fast-clock clients rather than an exact count. expired has no such caveat (jwt/v5 returns on signature failure before validating claims, so it implies a verified signature).

Why this grants nothing

No userID enters the request context on this path, so no downstream authorization decision can consume the token. whoami is the only handler in the service that reads auth.UserIDFromContext, so for the four endpoints actually affected in prd (protocols, token-prices, collectibles, ledger-key/accounts) the response is identical authenticated or not.

Why leeway-widening was rejected

335 rejections resolve into ~5 fixed clock offsets — 3m04s, 11m05s, 15m14s, 3h01m20s, and exactly 8h00m00s — each tight to within a second across hundreds of samples over 24h, i.e. individual machines set wrong rather than a drift distribution. Coverage by leeway value: 2m (current) 0/335, 5m 0/335, 20m 107/335, 60m 107/335, 8h+ 335/335. Full coverage implies a ~16h replay window (2·leeway + 15s), and the flag is capped at 10m at startup anyway.

Metric consumers must be updated

Anything watching the invalid-token rate must now sum result="rejected" with result="invalid_permitted", or permitted skew reads as having vanished. This matters most for permissive→strict flip readiness. Nothing currently alerts on it (checked wallet-eng-runbooks), so there is no live alert to fix — but the strict-flip check must be written against the sum.

Verification

• go build ./... clean; go test ./... fully green (19 packages); go vet and gofmt clean; all 8 CI checks green.
• Tests written first and confirmed load-bearing: with the new tests and the middleware reverted, permissive/expired and permitted expired fail; with it restored they pass. permissive/wrong-key and every required/* row pass in both states, which is what proves the narrowness and that strict is untouched.
• TestAuth_PermissiveServesEveryObservedProdClockLag replays the five real offsets from the prd logs through the middleware and asserts each is served, carries no userID, and counts as invalid_permitted/expired. This pins the fix to the incident data rather than to a chosen threshold.
• TestAuth_PermissiveServesArbitrarilyLargeClockOffsetEitherDirection asserts ±24h/±30d/±365d offsets are served — the property that distinguishes this from widening the leeway, since there is no bound left to re-tune. The fast-clock rows matter most: prod showed only lagging clocks, but across five devices, which cannot support a conclusion that fast clocks do not occur.
• TestAuth_PermissiveStillRejectsMalformedTiming asserts the complement — an over-long lifetime is bad_timing, still 401s, and counts as rejected rather than permitted skew.
• internal/api route-wiring test still asserts 401 in permissive using not-a-real-token — that is malformed, so it correctly still 401s; comment tightened so it doesn't imply broader coverage than it has.

Follow-ups / out of scope

1. This does nothing for strict, which stays fail-closed by design. The Blockaid migration (SE-12017) is driving that flip, so this is a rollout-window mitigation with an expiry date.
2. The real fix is client-side: derive a clock offset from the Date response header and sign with Date.now() + offset, re-derived per response so it self-corrects when a clock is fixed. buildAuthJwt already takes an injectable now and authedFetch already has a 401-retry hook (which today rebuilds from the same wrong clock, hence the deterministic double failure).
3. That fix needs one line of server CORS first: Access-Control-Expose-Headers: Date in internal/api/middleware/header.go. Date is not CORS-safelisted, and the extension manifest is MV3 with no host_permissions, so JS currently reads null. Mobile is unaffected (no CORS). Not included here to keep the change reviewable, but it blocks the client work and can ship immediately.
4. Silent-failure caveat: this converts a loud failure into a quiet one, which is the right trade only while no gated route consumes identity — true in prd today (DB_ENABLED=false, WALLET_BACKEND_ROUTES_ENABLED=false), not true once contacts or balances turn on.
5. Consider a freighter_auth_clock_skew_seconds histogram on timing rejections. The skew magnitude currently exists only in log text, so tracking whether the population is shrinking requires parsing logs rather than reading a dashboard.

[Copilot]

Pull request overview

Allows clock-skewed JWT requests to proceed anonymously in permissive auth mode while preserving strict-mode rejection.

Changes:

• Permits expired and bad_timing tokens anonymously in permissive mode.
• Adds distinct metrics and expanded middleware tests.
• Updates auth design documentation.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
internal/metrics/metrics.go	Adds the invalid_permitted result.
internal/api/serve_test.go	Clarifies malformed-token behavior.
internal/api/middleware/auth.go	Implements timing-failure fallback.
internal/api/middleware/auth_test.go	Tests timing behavior and metrics.
docs/design/2026-06-22-jwt-auth-middleware.md	Documents revised auth semantics.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[aristidesstaffieri]

Suggestion: make the fail-open predicate type-based rather than string-based.

No live bug here — I checked all 10 &VerificationError{...} constructions in internal/auth and none carries ReasonExpired or ReasonClockAhead, so current behavior is exactly what the PR documents. This is about the property being structural instead of conventional.

Both clock failures have dedicated types, and Reason() already discriminates them with errors.As before flattening to a string. The predicate then keys on the flattened value, and Reason()'s third branch returns ve.Reason verbatim — a plain string field set at 10 sites across claims.go, parser.go, verifier.go. So the fail-open set isn't "the two clock error types," it's "anything whose reason string matches," which anyone editing internal/auth can extend without opening this file.

What makes me want to fix it rather than note it: the mechanism already produced exactly this failure, in this branch. 5b32ac7 read

permitted := mode == auth.Permissive &&
    (reason == auth.ReasonExpired || reason == auth.ReasonBadTiming)

which served tokens with no exp claim at all, and 2h-lifetime tokens, and counted them as invalid_permitted. 0c1478b fixed it — but it was caught by review, not by a test: git show 5b32ac7:internal/api/middleware/auth_test.go | grep -c bad_timing → 0. That commit updated the truth table to assert the new 200s, so the suite encoded the behavior rather than resisting it. TestAuth_PermissiveStillRejectsMalformedTiming landed later, in dda760f.

And the next one would be equally invisible: three of the four remaining bad_timing sub-cases have no test anywhere in the repo (missing exp, missing iat, exp precedes iat — only over-long lifetime is covered). Meanwhile the prevailing idiom in claims.go is &VerificationError{Reason: ...} — 6 of 8 returns — so a new check written in the file's own style is the fail-open-capable form by default. Concretely: Claims.Validate doesn't check nbf, and a future nbf is a fast-clock symptom, so whoever adds that check reaches for ReasonClockAhead in the surrounding style and has added a pre-signature fail-open path that reads as correct in review.

Worth being clear about the stakes, since the PR is already careful about this: not a privilege escalation either way — no userID attaches, and TestAuth_PermissiveForgedTokenOutcomes pins that well. The damage is to invalid_permitted, which the design doc makes the gate for the strict flip. Contaminating it with malformed tokens is exactly the harm 0c1478b was written to undo.

Suggested change — four lines, no new concepts, errors already imported, and reason stays as-is for RecordAuth and the log line:

var expired *auth.ExpiredTokenError
var ahead *auth.ClockAheadError
permitted := mode == auth.Permissive &&
    (errors.As(err, &expired) || errors.As(err, &ahead))

Extending the fail-open set then requires defining a new error type and naming it here — a diff a reviewer actually sees.

[aristidesstaffieri]

Non-blocking: two tests I'd add for the permitted fall-through with a request body.

The fall-through changes a structural property of this middleware. Before it, next.ServeHTTP was reachable from exactly two arms — success (body read and reset) and ErrNoToken in permissive (verifier returns at verifier.go:52/:59, before readAndResetBody ever runs). At 7c6eecd, lines 61/84/95/104 all return. So the invariant was: if the verifier touched the body, verification succeeded. The permitted break retires that — a handler can now run after parseJWT returned an error, on a request whose body the verifier already drained.

It's correct today, because readAndResetBody (verifier.go:71) runs before parseJWT (verifier.go:77) and unconditionally swaps in a bytes.Reader over the full bytes. That ordering is now load-bearing for handler correctness and reads like an implementation detail.

The refactor that would break it is specific and, I'd argue, newly attractive: hoist a cheap timing pre-check above the body read — parse unverified, check iat/exp, bail before draining. This PR establishes that skewed devices fail ~100% of the time, so "stop draining bodies for the population we know fails timing" is a natural thought. Pre-PR it was safe; those requests 401'd and the body was never needed. Post-PR the handler runs and gets EOF. TestVerifyHTTPRequest_Valid stays green through it, because on success timing passes and the body is read as before.

Worth sizing because of which routes ride on it: of the four endpoints your description names as affected in prd, three are POSTs with bodies — token-prices, collectibles, ledger-key/accounts. That's the majority of the payload of this change, and it's the untested half of the new path. A regression there surfaces as io.EOF inside the handler — a 400, a 500, or an empty-result 200 depending on the handler — and only for skewed-clock users, i.e. the population already labeled as broken. Which is exactly the reading that would stop anyone from looking.

1. internal/auth — the one that actually pins the ordering. Works with the existing helpers as-is, since skewedClaims sets BodyHash: HashBody(body):

func TestVerifyHTTPRequest_ResetsBodyOnValidationFailure(t *testing.T) {
	_, priv, sub := newKeypair(t)
	body := []byte(`{"x":1}`)
	// Lagging clock well past the leeway: fails as expired, and methodAndPath /
	// bodyHash both still match so nothing else can be the reason.
	token := mint(t, priv, skewedClaims(sub, "POST /api/v1/thing", body, -(ClockSkewLeeway + time.Minute)))

	r := newRequest(t, http.MethodPost, "/api/v1/thing", body, token)
	_, err := NewVerifier(ClockSkewLeeway).VerifyHTTPRequest(r)
	require.Error(t, err)
	require.Equal(t, ReasonExpired, Reason(err))

	got, err := readAll(r)
	require.NoError(t, err)
	assert.Equal(t, body, got)
}

Direct complement to TestVerifyHTTPRequest_Valid:295, and it's the test the pre-check hoist would trip.

2. Middleware level — the end-to-end statement. Also needs no helper changes, for a slightly subtle reason: Claims.Validate returns clock_ahead at claims.go:84, before the bodyHash check at :113, so mintToken's HashBody(nil) doesn't matter on the fast-clock path.

func TestAuth_PermittedRequestReachesHandlerWithIntactBody(t *testing.T) {
	pub, priv, err := ed25519.GenerateKey(nil)
	require.NoError(t, err)
	sub := hex.EncodeToString(pub)

	body := []byte(`{"addresses":["GABC"]}`)
	token := mintToken(t, priv, sub, "POST "+authTestPath, auth.MaxTokenLifetime, time.Now().Add(time.Hour))

	var got []byte
	next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		got, _ = io.ReadAll(r.Body)
		w.WriteHeader(http.StatusOK)
	})
	handler := Auth(auth.NewVerifier(auth.ClockSkewLeeway), auth.Permissive, nil)(next)

	r := httptest.NewRequest(http.MethodPost, authTestPath, bytes.NewReader(body))
	r.Header.Set("Authorization", "Bearer "+token)
	rr := httptest.NewRecorder()
	handler.ServeHTTP(rr, r)

	require.Equal(t, http.StatusOK, rr.Code)
	assert.Equal(t, body, got, "a permitted request must reach its handler with an intact body")
}

One observation if you want the expired direction covered at the middleware level too: that path does reach the bodyHash check, so it needs a token bound to the real body — and authtest.MintToken hardcodes HashBody(nil) (authtest.go:24). A body []byte parameter (or a MintTokenWithBody sibling) would unblock it, and is probably worth having regardless: that hardcoded nil is why every middleware test is nil-body, which is how this gap stayed invisible across 5b32ac7, 1f52ffa, and dda760f.

[aristidesstaffieri]

The iss recorded on this line is unverified, and the flip criterion added in this PR reads it per-client.

The middleware has two sources for iss. The success arm labels from identity.Issuer, which is signature-verified (auth.go:57-60). This arm labels from IssuerFromRequestUnverified. That split used to coincide with serve-vs-reject, which is the assumption baked into the helper's own doc comment: "use it ONLY for a bounded, best-effort observability label (metric client bucket, log field) on the rejection path — never for an authentication or authorization decision." The permitted fall-through moves it onto the serving path.

For expired, an authentic issuer exists and is discarded. parseJWT reaches ExpiredTokenError only after ParseWithClaims verified the signature, so claims.Issuer is authentic at that point. verifier.go:76 then returns Identity{}, err, and this arm re-derives a weaker version by parsing the token a third time. For clock_ahead there's no alternative — no signature check ran, and decodePublicKey (parser.go:45) never ran either, so sub isn't even a validated key. That half is irreducibly caller-chosen.

What it affects, in order:

1. Per-client attribution. Design doc line 86 gates the flip on "the timing reasons reaching ~0 across both iss values." In aggregate that criterion fails safe — an inflated count can only read not ready, never ready-when-it-isn't. The per-client split has no such property: it is wrong in whichever direction the sender chose, and it's the number you'd use to decide which client team still needs to ship the Date-header offset fix. SanitizeClient bounds the label cardinality but not the attribution.

2. The count is unattributable, at one request per increment. No key, no signature, no valid sub required — POST /api/v1/token-prices with a future-dated token claiming any iss. The practical cost isn't that someone does this deliberately; it's that the metric alone cannot distinguish five real fast-clock devices from one script, so investigating a nonzero count has to leave the dashboard for logs and source IPs. With the flip on a deadline (SE-12017 per follow-up 1), that's the number the decision rests on.

3. Log ambiguity on 200s. iss now means "verified" for authenticated requests and "spoofable" for permitted ones, with user_id's absence as the only tell. An aggregation by iss filtered to 200s mixes the two.

expired was 100% of the real population — the regression test records 335 expired against 0 fast-clock — so recovering the verified issuer for that half yields an authentic per-iss signal covering every observed device, and leaves clock_ahead as the explicitly noisy bucket. Line 86 could then gate on invalid_permitted{reason="expired"} per-iss instead of on a blended number that inherits the weaker guarantee.

Suggested change — mirror how ExpiredBy already carries diagnostic detail out of the failure:

type ExpiredTokenError struct {
	ExpiredBy time.Duration
	Issuer    string // signature-verified: ParseWithClaims checks the signature before claims
	Err       error
}

claims is in scope where it's constructed in parseJWT, so it's a one-word addition — and deliberately not a signature change to VerifyHTTPRequest, since returning a partial Identity on error would invite reading UserID off it. Then here:

iss := auth.IssuerFromRequestUnverified(r)
var expired *auth.ExpiredTokenError
if errors.As(err, &expired) && expired.Issuer != "" {
	iss = expired.Issuer
}

If you take the type-based predicate suggested on line 117, that errors.As is already on this path, so this comes nearly free on top.

For the log field, the cheapest fix is a distinct name on the permitted path — iss_unverified rather than iss, or a verified boolean beside it — so the trust level is self-describing rather than inferred from user_id's absence.