Skip to content

Reject malformed prior act claims at token exchange - #6114

Merged
JAORMX merged 2 commits into
mainfrom
fix-invalid-act
Jul 28, 2026
Merged

Reject malformed prior act claims at token exchange#6114
JAORMX merged 2 commits into
mainfrom
fix-invalid-act

Conversation

@jhrozek

@jhrozek jhrozek commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

RFC 8693 §4.1 requires the act claim to be a JSON object. The token exchange
handler copied a subject token's prior act into the newly minted token without
checking its shape, and the depth gate could not catch it either: actChainDepth
returned 0 for anything that was not a map, making "absent" and "malformed"
indistinguishable. A subject token carrying "act": "some-agent" therefore made
us mint act: {"sub": "<actor>", "act": "some-agent"} — a spec violation emitted
by us, and one the consuming side has flagged since #6046 (DelegationChain.Malformed),
so the symptom was already visible in audit logs while the cause sat on the
issuance side.

  • Reuse the shared audit-side parser adopted in Adopt toolhive-core canonical delegation-chain schema #6107 instead of teaching the
    bespoke walker about shapes. It reports the depth and the conformance verdict
    in one pass, so the two can no longer disagree — which is what the bug was.
  • Reject with invalid_grant when the prior chain is malformed, consistent with
    the adjacent depth, consent, and expiry gates in the same handler.
  • Delete actChainDepth; the replaced call site was its only user.

Closes #6093

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

Scoped the unit run to the touched package with the Taskfile's flags
(go test -ldflags=-extldflags=-Wl,-w -race ./pkg/authserver/server/tokenexchange/...) —
green. The pre-existing depth cases pass unmodified, which is the regression
proof that swapping the walker did not move the depth boundary.

Manual verification that the new tests pin the fix and not merely the parser:
disabled only the new chain.Malformed gate and re-ran. Exactly the three reject
cases fail, and both accept cases plus the depth cases still pass:

--- FAIL: .../non-object_act_rejected
--- FAIL: .../non-object_nested_act_rejected
--- FAIL: .../non-string_sub_in_act_hop_rejected

Changes

File Change
pkg/authserver/server/tokenexchange/handler.go Gate the prior act on coreaudit.ParseDelegationChain; delete actChainDepth
pkg/authserver/server/tokenexchange/handler_test.go Drop TestActChainDepth; add 5 table cases + an actFormValues helper

Does this introduce a user-facing change?

No. A token exchange that would previously have minted a non-conformant act
claim is now rejected with invalid_grant, but no such exchange is reachable
today: NewMultiIssuerTokenValidator has no non-test callers, and a self-issued
subject token's act can only have been minted by this same handler as a
map[string]any. This is a conformance fix landing ahead of the multi-issuer
path (TODO(#5989)) that would make it reachable.

Special notes for reviewers

The new gate is monotonically stricter, not merely equivalent. With
maxDepth == maxDelegationDepth the parser appends exactly one hop per level, so
len(chain.Chain) == min(depth, 10) for a well-formed chain and
len(chain.Chain) >= 10 ⟺ actChainDepth(...) >= 10. A new accept would require
!Malformed && len < 10, reachable only via the same absent-nested-act exit at
the same level the old walker returned from — so nothing previously rejected is
newly accepted. Two inputs change which rejection fires (never accept↔reject):
an over-cap chain with a non-object act past the cap, and an over-cap chain
with a non-string sub in the first 10 hops, both now reported as malformed
rather than too deep.

Three deliberate decisions worth a second opinion:

  1. invalid_grant, not invalid_request. RFC 8693 §2.2.2 nominally says a
    bad subject_token MUST yield invalid_request, but the same section allows
    that "other error codes may also be used, as appropriate". invalid_grant
    matches the three adjacent gates in this handler and the issue's own
    recommendation. Noted in a code comment so it does not get "fixed" later.
    Heads-up that the sibling malformed-may_act rejection surfaces as
    invalid_request — reconciling the two is Harden act/may_act handling on the external-issuer token exchange path #6113's business.
  2. Scope is wider than the issue text. The issue names non-object act;
    Malformed also covers a non-string sub/iss on a hop. Rejecting those too
    comes free from the parser, is the same class of violation (RFC 7519 §4.1.2
    fixes sub as a string), and avoids maintaining a reason allowlist. It is
    stricter than any consumer needs, since §4.1 makes prior hops informational
    only — but we are the issuer and would be re-signing the violation.
  3. MalformedReason is surfaced in the client-visible hint. It is a closed
    four-value enum written only from constants, so no token-derived data can
    reach it; the raw offending values stay in core's unexported per-hop map. The
    code comment says never to interpolate claim contents there.

Two tolerated shapes are now pinned by tests rather than left implicit:
act: {} and a hop with "act": null are both accepted, because §4.1 only
requires an object and sub is conventional rather than mandatory. If core's
parser tightens, a test breaks instead of a user.

Follow-ups filed as #6113 (sub-issue of #5194), deliberately out of scope
here: may_act shape validation missing on the external-issuer path (the direct
sibling of this bug, on the receiving side); our own minted hop carrying no
iss; and the act subtree being depth-capped but not size-capped.

Generated with Claude Code

RFC 8693 §4.1 requires the act claim to be a JSON object, but the token
exchange handler copied a subject token's prior act into the newly minted
token without checking its shape. The depth gate did not catch it either:
actChainDepth returned 0 for any non-map, making "absent" and "malformed"
indistinguishable, so a subject token carrying act: "some-agent" produced
a minted token asserting a delegation chain we ourselves would flag as
non-conformant on the consuming side.

Reuse the shared audit-side parser adopted in #6107 rather than teaching
the bespoke walker about shapes: it reports the depth and the conformance
verdict in one pass, so the two cannot disagree. The exchange is now
rejected with invalid_grant, consistent with the adjacent depth, consent,
and expiry gates.

The new gate is monotonically stricter than the old one: a well-formed
chain yields len(Chain) == min(depth, cap), so the depth boundary is
unchanged and nothing previously rejected is newly accepted.

Closes #6093

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Jul 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.42%. Comparing base (9ce78fa) to head (d1602ab).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6114      +/-   ##
==========================================
- Coverage   72.43%   72.42%   -0.02%     
==========================================
  Files         734      733       -1     
  Lines       75871    75826      -45     
==========================================
- Hits        54960    54916      -44     
+ Misses      17019    17017       -2     
- Partials     3892     3893       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The gate rejects on every reason ParseDelegationChain reports and surfaces
the reason in the client-visible hint, but only three of the four were
covered. Add the non-string iss case so the fourth reason and its hint are
pinned too.

The hop keeps a valid sub: the parser reports the first violation it finds
and checks iss before sub, so a hop with both non-string would report only
iss_not_string and leave this branch unpinned.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Jul 28, 2026
@JAORMX
JAORMX merged commit 735225f into main Jul 28, 2026
47 checks passed
@JAORMX
JAORMX deleted the fix-invalid-act branch July 28, 2026 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Auth server can mint an act claim that violates RFC 8693 §4.1

2 participants