From 748ef7f78dc6ba6803a71ced88a81263ad5d5ac2 Mon Sep 17 00:00:00 2001 From: Ricardo Date: Thu, 20 Aug 2026 15:15:38 +0200 Subject: [PATCH 1/2] fix(endpoint-auth): bind code exchange to the code's own claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- packages/endpoint-auth/lib/middleware/code.js | 55 +++++++++++++------ .../200-authorization-profile-json.js | 2 + ...200-authorization-profile-no-grant-type.js | 2 + .../200-authorization-profile-url-encoded.js | 2 + .../test/integration/200-token-grant-json.js | 2 + .../200-token-grant-url-encoded.js | 2 + .../400-token-grant-invalid-pkce-code.js | 2 + .../400-token-grant-invalid-redirect-uri.js | 2 + ...token-grant-code-issued-to-other-client.js | 49 +++++++++++++++++ .../401-token-grant-invalid-client-id.js | 2 + ...01-token-grant-no-authorization-request.js | 36 ++++++++++++ 11 files changed, 138 insertions(+), 18 deletions(-) create mode 100644 packages/endpoint-auth/test/integration/401-token-grant-code-issued-to-other-client.js create mode 100644 packages/endpoint-auth/test/integration/401-token-grant-no-authorization-request.js diff --git a/packages/endpoint-auth/lib/middleware/code.js b/packages/endpoint-auth/lib/middleware/code.js index 56451f32c..36a66a7a3 100644 --- a/packages/endpoint-auth/lib/middleware/code.js +++ b/packages/endpoint-auth/lib/middleware/code.js @@ -2,7 +2,6 @@ import { IndiekitError } from "@indiekit/error"; import { getCanonicalUrl } from "@indiekit/util"; import { verifyCode } from "../pkce.js"; -import { validateRedirect } from "../redirect.js"; import { verifyToken } from "../token.js"; import { getRequestParameters } from "../utils.js"; @@ -12,7 +11,6 @@ import { getRequestParameters } from "../utils.js"; */ export const codeValidator = async (request, response, next) => { try { - const { client, usePkce } = request.app.locals; const parameters = getRequestParameters(request); const { client_id, code, code_verifier, grant_type, redirect_uri } = parameters; @@ -45,33 +43,54 @@ export const codeValidator = async (request, response, next) => { ); } - // Validate `client_id` against that provided in authorization request - if (getCanonicalUrl(client_id) !== getCanonicalUrl(client.id)) { + // Verify the code before reading anything from it + try { + request.verifiedToken = verifyToken(code); + } catch { throw IndiekitError.unauthorized( - response.locals.__("BadRequestError.invalidValue", "client_id"), + response.locals.__("UnauthorizedError.invalidToken"), ); } - // Validate `redirect_uri` - const validRedirect = await validateRedirect(redirect_uri, client_id); - if (!validRedirect) { - throw IndiekitError.badRequest( - response.locals.__("BadRequestError.invalidValue", "redirect_uri"), + // An authorization code records the client it was issued to and the + // redirect it was issued for. A code missing either cannot be checked + // against the request, so it is not one this server issued. + if ( + !request.verifiedToken.client_id || + !request.verifiedToken.redirect_uri + ) { + throw IndiekitError.unauthorized( + response.locals.__("UnauthorizedError.invalidToken"), ); } - // Verify token - try { - request.verifiedToken = verifyToken(code); - } catch { + // Validate `client_id` against the client the code was issued to. Reading + // this from the code rather than from application state is what ties the + // two requests together: `app.locals` is shared by every request the + // server handles, so it holds whichever authorization request happened + // last, which need not be the one that produced this code. + if ( + getCanonicalUrl(client_id) !== + getCanonicalUrl(String(request.verifiedToken.client_id)) + ) { throw IndiekitError.unauthorized( - response.locals.__("UnauthorizedError.invalidToken"), + response.locals.__("BadRequestError.invalidValue", "client_id"), + ); + } + + // Validate `redirect_uri` against the one the code was issued for. It was + // checked against the client's metadata during the authorization request, + // so matching it here is what remains to be done. + if (redirect_uri !== request.verifiedToken.redirect_uri) { + throw IndiekitError.badRequest( + response.locals.__("BadRequestError.invalidValue", "redirect_uri"), ); } - // PKCE (Proof Key for Code Exchange) - if (usePkce) { - const { code_challenge } = request.verifiedToken; + // PKCE (Proof Key for Code Exchange). Whether it applies is recorded in + // the code itself, by the presence of the challenge it was issued with. + const { code_challenge } = request.verifiedToken; + if (code_challenge) { const verifiedCode = verifyCode(code_verifier, code_challenge); if (!verifiedCode) { throw IndiekitError.unauthorized( diff --git a/packages/endpoint-auth/test/integration/200-authorization-profile-json.js b/packages/endpoint-auth/test/integration/200-authorization-profile-json.js index 544ad47f4..808f750ef 100644 --- a/packages/endpoint-auth/test/integration/200-authorization-profile-json.js +++ b/packages/endpoint-auth/test/integration/200-authorization-profile-json.js @@ -24,7 +24,9 @@ describe("endpoint-auth POST /auth", () => { it("Returns JSON profile", async () => { const code = signToken({ access_token: "token", + client_id: "https://auth-endpoint.example", me: "https://website.example", + redirect_uri: "https://auth-endpoint.example/redirect", scope: "create update delete media", token_type: "Bearer", }); diff --git a/packages/endpoint-auth/test/integration/200-authorization-profile-no-grant-type.js b/packages/endpoint-auth/test/integration/200-authorization-profile-no-grant-type.js index c64a9ed0f..98aa68a50 100644 --- a/packages/endpoint-auth/test/integration/200-authorization-profile-no-grant-type.js +++ b/packages/endpoint-auth/test/integration/200-authorization-profile-no-grant-type.js @@ -24,7 +24,9 @@ describe("endpoint-auth POST /auth", () => { it("Returns profile when `grant_type` omitted", async () => { const code = signToken({ access_token: "token", + client_id: "https://auth-endpoint.example", me: "https://website.example", + redirect_uri: "https://auth-endpoint.example/redirect", scope: "create update delete media", token_type: "Bearer", }); diff --git a/packages/endpoint-auth/test/integration/200-authorization-profile-url-encoded.js b/packages/endpoint-auth/test/integration/200-authorization-profile-url-encoded.js index 2578261d0..3a7347354 100644 --- a/packages/endpoint-auth/test/integration/200-authorization-profile-url-encoded.js +++ b/packages/endpoint-auth/test/integration/200-authorization-profile-url-encoded.js @@ -24,7 +24,9 @@ describe("endpoint-auth POST /auth", () => { it("Returns URL encoded profile", async () => { const code = signToken({ access_token: "token", + client_id: "https://auth-endpoint.example", me: "https://website.example", + redirect_uri: "https://auth-endpoint.example/redirect", scope: "create update delete media", token_type: "Bearer", }); diff --git a/packages/endpoint-auth/test/integration/200-token-grant-json.js b/packages/endpoint-auth/test/integration/200-token-grant-json.js index 4dd4d6a99..5e12e60ff 100644 --- a/packages/endpoint-auth/test/integration/200-token-grant-json.js +++ b/packages/endpoint-auth/test/integration/200-token-grant-json.js @@ -24,7 +24,9 @@ describe("endpoint-auth POST /auth/token", () => { it("Returns JSON access token", async () => { const code = signToken({ access_token: "token", + client_id: "https://auth-endpoint.example", me: "https://website.example", + redirect_uri: "https://auth-endpoint.example/redirect", scope: "create update delete media", token_type: "Bearer", }); diff --git a/packages/endpoint-auth/test/integration/200-token-grant-url-encoded.js b/packages/endpoint-auth/test/integration/200-token-grant-url-encoded.js index 06ef5958b..d7452ff32 100644 --- a/packages/endpoint-auth/test/integration/200-token-grant-url-encoded.js +++ b/packages/endpoint-auth/test/integration/200-token-grant-url-encoded.js @@ -24,7 +24,9 @@ describe("endpoint-auth POST /auth/token", () => { it("Returns URL encoded access token", async () => { const code = signToken({ access_token: "token", + client_id: "https://auth-endpoint.example", me: "https://website.example", + redirect_uri: "https://auth-endpoint.example/redirect", scope: "create update delete media", token_type: "Bearer", }); diff --git a/packages/endpoint-auth/test/integration/400-token-grant-invalid-pkce-code.js b/packages/endpoint-auth/test/integration/400-token-grant-invalid-pkce-code.js index 223260b7d..51cb09e29 100644 --- a/packages/endpoint-auth/test/integration/400-token-grant-invalid-pkce-code.js +++ b/packages/endpoint-auth/test/integration/400-token-grant-invalid-pkce-code.js @@ -29,8 +29,10 @@ describe("endpoint-auth POST /auth/token", () => { it("Returns 401 error fails PKCE code challenge", async () => { const code = signToken({ access_token: "token", + client_id: "https://auth-endpoint.example", code_challenge: codeChallenge, code_challenge_method: "S256", + redirect_uri: "https://auth-endpoint.example/redirect", scope: "create update delete media", token_type: "Bearer", }); diff --git a/packages/endpoint-auth/test/integration/400-token-grant-invalid-redirect-uri.js b/packages/endpoint-auth/test/integration/400-token-grant-invalid-redirect-uri.js index f82556248..f1dcdc517 100644 --- a/packages/endpoint-auth/test/integration/400-token-grant-invalid-redirect-uri.js +++ b/packages/endpoint-auth/test/integration/400-token-grant-invalid-redirect-uri.js @@ -24,7 +24,9 @@ describe("endpoint-auth POST /auth/token", () => { it("Returns 400 error invalid `redirect_uri`", async () => { const code = signToken({ access_token: "token", + client_id: "https://auth-endpoint.example", me: "https://website.example", + redirect_uri: "https://auth-endpoint.example/redirect", scope: "create update delete media", token_type: "Bearer", }); diff --git a/packages/endpoint-auth/test/integration/401-token-grant-code-issued-to-other-client.js b/packages/endpoint-auth/test/integration/401-token-grant-code-issued-to-other-client.js new file mode 100644 index 000000000..a5f16940a --- /dev/null +++ b/packages/endpoint-auth/test/integration/401-token-grant-code-issued-to-other-client.js @@ -0,0 +1,49 @@ +import { strict as assert } from "node:assert"; +import { after, before, describe, it } from "node:test"; + +import { mockAgent } from "@indiekit-test/mock-agent"; +import { testServer } from "@indiekit-test/server"; +import supertest from "supertest"; + +import { signToken } from "../../lib/token.js"; + +await mockAgent("endpoint-auth"); +const server = await testServer(); +const request = supertest.agent(server); + +describe("endpoint-auth POST /auth/token", () => { + // Begin an authorization request as this client, so that whatever + // application-wide state the server keeps refers to it. + before(async () => { + await request + .get("/auth") + .query({ client_id: "https://auth-endpoint.example" }) + .query({ redirect_uri: "https://auth-endpoint.example/redirect" }) + .query({ response_type: "code" }) + .query({ state: "12345" }); + }); + + // The authorization code records the client it was issued to. Redeeming it + // as a different client must fail, whatever authorization request happened + // most recently on the server. + it("Rejects a code issued to a different client", async () => { + const code = signToken({ + client_id: "https://other-client.example", + me: "https://website.example", + redirect_uri: "https://other-client.example/redirect", + scope: "create", + }); + const result = await request + .post("/auth/token") + .set("accept", "application/json") + .query({ client_id: "https://auth-endpoint.example" }) + .query({ code }) + .query({ grant_type: "authorization_code" }) + .query({ redirect_uri: "https://auth-endpoint.example/redirect" }); + + assert.notEqual(result.status, 200); + assert.equal(result.body.access_token, undefined); + }); + + after(() => server.close()); +}); diff --git a/packages/endpoint-auth/test/integration/401-token-grant-invalid-client-id.js b/packages/endpoint-auth/test/integration/401-token-grant-invalid-client-id.js index 49e48b205..5d2394b5f 100644 --- a/packages/endpoint-auth/test/integration/401-token-grant-invalid-client-id.js +++ b/packages/endpoint-auth/test/integration/401-token-grant-invalid-client-id.js @@ -24,7 +24,9 @@ describe("endpoint-auth POST /auth/token", () => { it("Returns 401 error invalid `client_id`", async () => { const code = signToken({ access_token: "token", + client_id: "https://auth-endpoint.example", me: "https://website.example", + redirect_uri: "https://website.example/redirect", scope: "create update delete media", token_type: "Bearer", }); diff --git a/packages/endpoint-auth/test/integration/401-token-grant-no-authorization-request.js b/packages/endpoint-auth/test/integration/401-token-grant-no-authorization-request.js new file mode 100644 index 000000000..a32a7132b --- /dev/null +++ b/packages/endpoint-auth/test/integration/401-token-grant-no-authorization-request.js @@ -0,0 +1,36 @@ +import { strict as assert } from "node:assert"; +import { after, describe, it } from "node:test"; + +import { testServer } from "@indiekit-test/server"; +import supertest from "supertest"; + +import { signToken } from "../../lib/token.js"; + +const server = await testServer(); +const request = supertest.agent(server); + +describe("endpoint-auth POST /auth/token", () => { + // No authorization request precedes this exchange, so nothing has populated + // `request.app.locals`. The response should describe the problem, not fail + // with an unhandled error. + it("Returns an error, not 500, with no preceding authorization request", async () => { + const code = signToken({ + client_id: "https://client.example", + me: "https://website.example", + redirect_uri: "https://client.example/redirect", + scope: "create", + }); + const result = await request + .post("/auth/token") + .set("accept", "application/json") + .query({ client_id: "https://other-client.example" }) + .query({ code }) + .query({ grant_type: "authorization_code" }) + .query({ redirect_uri: "https://other-client.example/redirect" }); + + assert.notEqual(result.status, 500); + assert.equal(result.body.error, "unauthorized"); + }); + + after(() => server.close()); +}); From b36ac3853cf1cf43b52bf0445cfc8eb08ad246d9 Mon Sep 17 00:00:00 2001 From: Ricardo Date: Sun, 30 Aug 2026 12:49:42 +0200 Subject: [PATCH 2/2] fix(endpoint-auth): stop describing state the middleware no longer reads 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. --- packages/endpoint-auth/lib/middleware/code.js | 6 ++---- .../integration/401-token-grant-no-authorization-request.js | 6 +++--- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/endpoint-auth/lib/middleware/code.js b/packages/endpoint-auth/lib/middleware/code.js index 36a66a7a3..a75cf49a3 100644 --- a/packages/endpoint-auth/lib/middleware/code.js +++ b/packages/endpoint-auth/lib/middleware/code.js @@ -65,10 +65,8 @@ export const codeValidator = async (request, response, next) => { } // Validate `client_id` against the client the code was issued to. Reading - // this from the code rather than from application state is what ties the - // two requests together: `app.locals` is shared by every request the - // server handles, so it holds whichever authorization request happened - // last, which need not be the one that produced this code. + // it from the code, rather than from state shared across requests, is what + // ties this exchange to the authorization request that produced the code. if ( getCanonicalUrl(client_id) !== getCanonicalUrl(String(request.verifiedToken.client_id)) diff --git a/packages/endpoint-auth/test/integration/401-token-grant-no-authorization-request.js b/packages/endpoint-auth/test/integration/401-token-grant-no-authorization-request.js index a32a7132b..accd130db 100644 --- a/packages/endpoint-auth/test/integration/401-token-grant-no-authorization-request.js +++ b/packages/endpoint-auth/test/integration/401-token-grant-no-authorization-request.js @@ -10,9 +10,9 @@ const server = await testServer(); const request = supertest.agent(server); describe("endpoint-auth POST /auth/token", () => { - // No authorization request precedes this exchange, so nothing has populated - // `request.app.locals`. The response should describe the problem, not fail - // with an unhandled error. + // No authorization request precedes this exchange, so the code itself has to + // carry everything needed to validate it. The response should describe the + // problem, not fail with an unhandled error. it("Returns an error, not 500, with no preceding authorization request", async () => { const code = signToken({ client_id: "https://client.example",