Skip to content

fix(endpoint-auth): bind code exchange to the code’s own claims, not application state - #961

Merged
paulrobertlloyd merged 2 commits into
mainfrom
fix/code-binding-from-claims
Sep 7, 2026
Merged

paulrobertlloyd merged 2 commits into
mainfrom
fix/code-binding-from-claims

Conversation

@rmdes

@rmdes rmdes commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Fixes #892.

codeValidator took the client an authorization code was issued to from request.app.locals.client. That is application-wide state, holding whichever authorization request last reached the server, so it can be absent (500) or refer to a different client than the one the code was issued to (a code redeemable by another client).

The change

Verify the code first, then compare the request against the claims it carries:

request.verifiedToken = verifyToken(code);

if (!request.verifiedToken.client_id || !request.verifiedToken.redirect_uri) {
  throw IndiekitError.unauthorized(__("UnauthorizedError.invalidToken"));
}

if (getCanonicalUrl(client_id) !== getCanonicalUrl(String(request.verifiedToken.client_id))) { … }
if (redirect_uri !== request.verifiedToken.redirect_uri) { … }

consent.js:82 already signs both into the code, so nothing new has to be recorded. PKCE now keys off the challenge in the code rather than app.locals.usePkce, which removes the last read of application state here.

validateRedirect is no longer called at redemption: redirect_uri is validated against the client's metadata during the authorization request, so what remains is matching the value the code was issued for. That is a behaviour change worth a look — it trades a network-dependent check for an equality one.

Tests

Two added, both failing on main:

  • 401-token-grant-no-authorization-request.js — an exchange with no preceding authorization request. main returns 500 with a TypeError.
  • 401-token-grant-code-issued-to-other-client.js — a code issued to one client, redeemed by another that began its own authorization request first. main returns 200 with an access token.

Seven existing tests signed codes carrying neither client_id nor redirect_uri, which consent.js cannot produce, so they exercised codes that cannot occur. They now sign the claims a real code carries. Two of them — invalid client_id and invalid redirect_uri — assert the same statuses and messages as before, now reached through the code's claims rather than through app.locals.

node --test in packages/endpoint-auth: 68 tests, 68 passing. Reverting only code.js and keeping the tests fails exactly the two new ones. main is 66/66 before this change.

Relationship to the other open branches

Independent of #884, #887 and #889. This branch is cut from main, as is #891, and both change packages/endpoint-auth/lib/middleware/code.js.

I described that overlap as a trivial rebase when opening this. Having since merged the two locally, that was understated, so to correct it: code.js does merge cleanly — #891 adds a grant_type allowance to the required-parameter list, this one replaces the checks that follow it, and git resolves that without conflict. But the merged result fails a test, and it is #891's own:

200-authorization-profile-no-grant-type.js, added there, signs a code with neither client_id nor redirect_uri — the same gap as the seven fixtures this PR updates. It passes on either branch alone and fails on both together, because this PR starts rejecting codes missing those claims. Adding them to that fixture resolves it, after which the combined branches give 70 tests, 70 passing.

So whichever merges second needs a one-line fixture change as well as the merge, and CI on the second branch will go red until it is made. Nothing structural — but it will not go green on the merge alone, which is what "trivial rebase" implied.


Replaces #893, which was opened from a branch on rmdes/indiekit. PRs from a fork do not receive the Localazy readKey secret, so the "Download localisations" step fails and every step after it — install, lint and the whole test suite — is skipped. Same two commits, rebased onto current main, on a branch here so CI actually runs.

`codeValidator` read the client an authorization code was issued to from
`request.app.locals.client`, set by the authorization request in
`authorization.js`. `app.locals` is shared by every request an Express
application handles, so it holds whichever authorization request happened last
on the server, which need not be the one that produced the code being redeemed.

Two consequences.

A code exchange arriving with no preceding authorization request finds
`client` undefined and fails at `client.id` with an unhandled TypeError and a
500, reachable unauthenticated:

    {"error":"TypeError",
     "error_description":"Cannot read properties of undefined (reading 'id')"}

More seriously, the check that a code is being redeemed by the client it was
issued to can be satisfied by any client that begins its own authorization
request first, because both sides of the comparison then refer to that client.
A code issued to one client is redeemed by another, and an access token is
returned for the profile URL and scope the code carries.

The authorization code already records what is needed: `consent.js` signs
`client_id` and `redirect_uri` into it. Verify the code first, then compare the
request against those claims rather than against application state, and treat a
code missing either claim as invalid. Whether PKCE applies is likewise recorded
in the code, by the presence of the challenge it was issued with, so that no
longer depends on `app.locals.usePkce` either.

`validateRedirect` is no longer called here: `redirect_uri` was checked against
the client's metadata during the authorization request, so matching the value
recorded in the code is what remains to be done at redemption.

Adds a test for each failure. Eight existing tests signed codes carrying
neither `client_id` nor `redirect_uri` — codes this server cannot issue — and
now sign the claims a real code would carry.
Both comments explained the change by reference to `app.locals`, which
this middleware no longer touches. Describe what the code does instead:
the exchange is tied to the authorization request that produced the code,
because the claims are read from the code itself.
@paulrobertlloyd
paulrobertlloyd merged commit c3427e3 into main Sep 7, 2026
1 check passed
@paulrobertlloyd
paulrobertlloyd deleted the fix/code-binding-from-claims branch September 7, 2026 22:38
@paulrobertlloyd paulrobertlloyd added code quality Making things easier to understand plugin-endpoint Endpoint plug-in labels Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code quality Making things easier to understand plugin-endpoint Endpoint plug-in

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IndieAuth: authorization code binding relies on application-wide state (500 on missing state; code redeemable by another client)

2 participants