Skip to content

security(consent): refuse a delegated token at /authorize/consent - #815

Merged
lakhansamani merged 1 commit into
mainfrom
security/consent-delegated-token-denied
Oct 14, 2026
Merged

lakhansamani merged 1 commit into
mainfrom
security/consent-delegated-token-denied

Conversation

@lakhansamani

Copy link
Copy Markdown
Contributor

Closes the last of the six advisories that were sitting in triage. GHSA-hhhh-p743-q3cp.

The gap

ConsentHandler authorizes the approval on one condition: tokenData.UserID == pending.UserID. GetUserIDFromSessionOrAccessToken resolves an RFC 8693 delegated token to the delegating user's sub — by design, so that a correctly-gated caller can treat a delegated token as "this user, with this reduced scope". That identity check therefore passes for an agent acting on the user's behalf.

It is the right answer for a caller that consults internal/delegatedscope, and the wrong one here: /authorize/consent is a plain gin route (http_routes.go) behind neither of the two places that gate actually enforces — the gRPC auth interceptor and the GraphQL field middleware. tokenData.Scope and tokenData.ActorID are never read.

The fix: fail closed, no new scope

A guard rejecting any caller with a non-empty ActorID.

Deliberately not a delegatedscope table entry. Approving a consent grant is not attenuable: the approval confers whatever scope the third-party client requested, which is unbounded by the agent's own ceiling. An agent capped at openid could hand a third party far more than openid, so a ceiling that permitted this would stop being a ceiling. That is why there is no scope for the operation rather than a scope nobody is granted — matching delegatedscope's own documented default ("an operation absent from the table is DENIED to delegated callers") and mirroring enforceDelegatedScope's not-delegatable branch, including its metric label.

Human-in-the-loop approval for a headless agent is CIBA + RAR (Wave 3 in the delegation design), not this endpoint. Noted in the code comment so the refusal reads as "use the right primitive" rather than "unsupported".

Nothing in the browser flow changes

The resolver checks the session cookie before the bearer token (auth_token.go), and the session branch never sets ActorID. So a first-party caller — including an agent driving the user's own browser with their cookie — takes the session path and is unaffected. Only a cookie-less caller presenting a delegated bearer token is refused.

There is a test for exactly this (a first-party session still approves and mints a code), because "does this break agent flows?" is the first question the change invites.

Measured impact — narrower than the report states

The advisory says the attack works "with no session cookie at all" and that "the resulting authorization code is delivered to the client's redirect_uri". Tested on main before the fix:

  • The consent approval does succeed with an openid-only delegated token and no cookie — the core finding is real, and the grant is recorded.
  • But the caller is redirected to the authorizer's own login UI, not to the client. The pre-generated handoff code it can see in that URL is not redeemable: exchanging it at /oauth/token returns 400 invalid_grant, because it is not yet bound to an authenticated user.
  • A with-session control confirms the test setup is sound: the same request with a cookie does mint a code to the client, so the only missing element is the session.

Real exposure: consent can be pre-approved for one exact request (the grant is single-use and keyed to user + client + exact query), so a later visit from the victim's own browser skips the consent screen. Serious enough to fix — that screen is the only protection for a self-asserted client — but the caller obtains nothing usable itself. The advisory's severity and impact text should be corrected before publication.

Testing

internal/integration_tests/consent_delegated_test.go:

  1. a delegated token is refused (403) — fails before the fix
  2. a first-party session still approves and the flow mints a code at the client's registered redirect_uri
  3. the login-handoff code is not redeemable without authentication

The client is RFC 7591 self-registered (ClientKindDynamic) on purpose: it resolves from the registry with no outbound fetch, so /authorize reaches the session and consent-grant checks. A CIMD client_id that serves no document is refused with invalid_client before either check — which is what made an earlier draft of this test inconclusive.

Mutation-verified: disabling the guard (in a form that still compiles) makes test 1 fail.

Local: go build ./..., go vet ./... clean; make test 0 failures; the three pre-existing consent tests (TestCIMDConsentFlow, TestConsentHandlerSecurityProperties, DCR) all still pass.

GetUserIDFromSessionOrAccessToken resolves an RFC 8693 delegated token
to the DELEGATING USER's sub, by design, so ConsentHandler's identity
check (tokenData.UserID == pending.UserID) passes for an agent acting
on the user's behalf. That is correct for a caller that consults
internal/delegatedscope, and wrong here: this is a plain gin route
behind neither of the two gates that enforce it — the gRPC auth
interceptor and the GraphQL field middleware.

Approving a consent grant is not attenuable, which is why this fails
closed rather than gaining a scope: the approval confers whatever
scope the THIRD-PARTY client requested, unbounded by the agent's own
ceiling, so an agent capped at `openid` could hand a third party far
more than `openid`. A ceiling that permitted this would stop being a
ceiling. Mirrors enforceDelegatedScope's not-delegatable branch and
delegatedscope's documented default: absent from the table is DENIED.

Nothing in the browser flow changes. The resolver checks the session
cookie BEFORE the bearer token and the session branch never sets
ActorID, so a first-party caller — including an agent driving the
user's own browser with their cookie — still approves. Only a
cookie-less caller presenting a delegated bearer token is refused.

Measured bounds of the issue, which the advisory overstates: the
approval records a grant keyed to (user, client, exact query) and
returns a 302 into /authorize, which reads its session from the
cookie only. A cookie-less caller lands on the login UI, and the
pre-generated handoff code it can see is NOT redeemable — exchanging
it returns invalid_grant. So the caller obtains nothing usable; the
exposure is that a later visit from the victim's own browser skips
the consent screen.

Human-in-the-loop approval for a headless agent is CIBA + RAR
(Wave 3), not this endpoint.

Tests: a delegated token is refused; a first-party session still
approves and mints a code at the client's redirect_uri; the handoff
code is not redeemable. The self-registered (RFC 7591) client is
deliberate — a CIMD client_id that serves no document is refused
with invalid_client BEFORE the session and grant checks, which makes
any conclusion drawn from it worthless.

Mutation-verified: disabling the guard makes the first test fail.
@lakhansamani
lakhansamani merged commit 0e5fbdd into main Oct 14, 2026
4 checks passed
lakhansamani added a commit that referenced this pull request Oct 14, 2026
Sixteen PRs merged since the 2.4.1 tag; [Unreleased] documented three
of them and none of the five advisory fixes. A security release whose
notes omit its security fixes gives nobody a reason to upgrade, which
is the operator-facing half of the failure SECURITY.md triage exists
to prevent.

Renames [Unreleased] to 2.4.2 and documents all of it:

- Security: the six advisories closed (#814, #799, #798, #796 x2,
  #815) and the three reachable dependency advisories (#797, #807,
  #811), plus the Go 1.27 builder (#800).
- Fixed: the audit-filter parity bugs (#802) and the Cassandra index
  probe that never waited (#810).
- Changed: direct-gRPC x-authorizer-url now yields to --url (#814),
  and the dropped Cassandra audit_log(created_at) index (#812).
- Added: the audit plumbing (#806), stated as writing no new records
  and changing no behaviour, plus two deployment entries.
- Upgrade notes: eight items, including the three that were missing
  entirely — that ANY two audit filters now scan on Cassandra (not
  only the timestamp range, which the previous note implied), the
  longer first start on ScyllaDB now that probes genuinely block, and
  the manual DROP INDEX for the removed index.

Section order leads with Security, matching 2.4.1 — the file orders by
what matters for the release at hand rather than a fixed sequence.

Deliberately omitted: #791 (scorecard action bump) and #809 (the
previous changelog edit). Neither is user-facing; Keep a Changelog
asks for notable changes, not a commit log.

The ScyllaDB view-build note is scoped to ScyllaDB: global secondary
indexes are materialized views there, while Apache Cassandra builds
local 2i with no view-build step and no nodetool viewbuildstatus.
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