Skip to content

test(auth): drive JWKS bearer validation end-to-end; require sub, validate nbf#267

Open
smunini wants to merge 1 commit into
mainfrom
pr229-followup
Open

test(auth): drive JWKS bearer validation end-to-end; require sub, validate nbf#267
smunini wants to merge 1 commit into
mainfrom
pr229-followup

Conversation

@smunini

@smunini smunini commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Follow-up from the security review of #229. Stacked on #229 — base is fix/206-audience-required-claim, so merge that first (or retarget this to main afterwards).

Tests that pin behavior, not struct fields

#229's tests assert on the fields of the Validation struct. They are real regression tests (they fail without the fix), but a future jsonwebtoken upgrade that changed claim-validation semantics underneath us would leave them passing.

This adds the RS256 + wiremock JWKS harness the #229 description noted was missing, so authenticate() is exercised with genuinely signed tokens against a served JWKS. wiremock was already a dev-dependency; the only new fixture is a test RSA keypair.

Each protection was mutation-checked — removing it turns exactly the expected tests red:

Mutation Tests that fail
Drop the aud/iss required-claims (pre-#229 code) token_without_audience_is_rejected, token_without_issuer_is_rejected, token_with_non_string_audience_is_rejected
Drop the sub requirement token_without_subject_is_rejected, token_with_empty_subject_is_rejected
Drop validate_nbf not_yet_valid_token_is_rejected

valid_token_is_accepted and token_with_a_bad_signature_is_rejected guard against the suite passing vacuously.

Two gaps the harness made easy to close

sub is now required. The subject is what audit records and policy decisions are attributed to (and what bulk export ties job ownership to), but a token without one previously yielded a Principal with an empty-string subject. Note the required-claim check only asserts presence, so "sub": "" would satisfy it and still produce an anonymous principal — authenticate() rejects that explicitly as well.

nbf is now validated. It defaults to off in jsonwebtoken, so a not-yet-valid token was accepted. It is enforced only when the claim is present, so tokens that omit nbf are unaffected.

Also pinned: the non-string-aud bypass

Worth calling out because #229 closes it without mentioning it. A token with "aud": 42 fails to deserialize, and a failed parse was indistinguishable from an absent claim to the audience check — so it bypassed a configured HFS_AUTH_AUDIENCE exactly the way an omitted aud did. It is now covered by a test, so it can't silently regress.

Related: #266 (warn when HFS_AUTH_AUDIENCE is unset).

…idate nbf

Adds an RS256 + wiremock JWKS harness so the provider's accept/reject behavior
is tested through authenticate(), not just the shape of the Validation struct
(which would keep passing if a jsonwebtoken upgrade changed claim semantics).

Also closes two gaps the harness made easy to cover:
- sub is now a required claim, and a present-but-empty sub is rejected: the
  subject is what audit records and policy decisions are attributed to.
- nbf is now validated, so a not-yet-valid token is rejected. It is enforced
  only when present, so tokens that omit nbf are unaffected.

The non-string-aud case (e.g. "aud": 42) is also pinned: it fails to
deserialize, and a failed parse was indistinguishable from an absent claim to
the audience check, so it bypassed audience validation the same way an omitted
aud did.
Base automatically changed from fix/206-audience-required-claim to main July 14, 2026 18:34
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.

1 participant