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

[rmdes]

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.

[paulrobertlloyd]

@rmdes I have rebased your fork with main, and am seeing the failing test as a result. I’m not clear what needs to change to make this PR pass tests. If you can update it, I will then merge this in.

[rmdes]

checking now..

[rmdes]

Fixed and pushed — d4aadda2.

The failure was 200-authorization-profile-no-grant-type.js, the test added in #891. It signs a code carrying neither client_id nor redirect_uri:

const code = signToken({
  access_token: "token",
  me: "https://website.example",
  scope: "create update delete media",
  token_type: "Bearer",
});

This PR treats such a code as invalid, because consent.js always signs both claims into a real one — so a code without them is not one this server could have issued. That check is what makes the client binding work, so relaxing it would undo the fix.

The seven tests that already signed codes this way were updated in this branch when it was written. #891 added an eighth after that, and your rebase brought it in unchanged. It now signs the same two claims as its siblings:

client_id: "https://auth-endpoint.example",
redirect_uri: "https://auth-endpoint.example/redirect",

Nothing in lib/ changed. Locally on the rebased branch: packages/endpoint-auth 71 pass / 0 fail, packages/indiekit 111 pass / 0 fail, npm run lint exits 0.

[paulrobertlloyd]

If we’re no longer storing these in locals, can we not remove where they are set in lib/controllers/authorization.js?

[rmdes]

I checked before removing them, and I think they need to stay — they are still read, just not by this middleware.

What this PR dropped from code.js is the line const { client, usePkce } = request.app.locals. But authorization.js sets those two for the consent screen, and views/consent.njk still consumes both:

{{ authorize({
  client: client,          ← consent.njk:14
  ...
{{ warningText({
  text: __("auth.consent.pkce.text", { client: client.name })   ← :20
}) if not usePkce }}                                            ← :21

app.locals is merged into every render, so those come straight from the assignments at authorization.js:94 and :97. Removing either would render the consent form without the client name, and would show the "this client is not using PKCE" warning unconditionally.

So the assignments have one remaining consumer — the view — and this PR only removes the second consumer, which was the one reading cross-request state to make a security decision. Happy to remove them if you would rather the view got them explicitly via response.render(...) locals instead, but that felt like a separate change to this fix.

[paulrobertlloyd]

I note that lib/controllers/authorization.js tests for the presence of both code_challenge and code_challenge_method. Should we continue to do that here?

[rmdes]

Good catch — there is a real gap here, though I think mirroring authorization.js exactly would be the wrong direction. Three things I checked:

1. The code does carry the method. consent.js signs both, each conditionally:

...(code_challenge && { code_challenge }),
...(code_challenge_method && { code_challenge_method }),

So it is available to read here. Today this middleware ignores it and calls verifyCode(code_verifier, code_challenge), which falls back to the default — meaning the method is effectively hardcoded to sha256.

2. But it cannot be passed straight through. verifyCode feeds its third argument to createHash, and the value clients send per the spec is S256, not an OpenSSL digest name:

createHash("S256")  -> throws: Digest method not supported
createHash("sha256") -> OK

So honouring the method needs a mapping (S256 → sha256), not just forwarding the claim. That is why I left it alone rather than half-wiring it.

3. Requiring both would weaken this check. authorization.js sets usePkce = code_challenge && code_challenge_method, so PKCE counts as "in use" only when both are present. If this middleware adopted the same test, a code carrying a code_challenge but no method would skip verification entirely. Verifying whenever a challenge is present fails closed instead — if a challenge was issued, it has to be satisfied.

For what it is worth, RFC 7636 §4.6 treats a missing method as plain (compare the verifier literally) rather than as "no PKCE", which also argues against skipping.

Which would you like? I would suggest either leaving it as-is for this PR and handling the method properly in its own change, or doing it here as:

const { code_challenge, code_challenge_method } = request.verifiedToken;
if (code_challenge) {
  const method = code_challenge_method === "S256" ? "sha256" : code_challenge_method;
  ...
}

I would rather not fold a PKCE behaviour change into a client-binding fix without you picking the direction, since plain support is a spec question of its own.