Improved PKCE support

This commit is contained in:
Konstantin Wohlwend 2026-06-11 10:25:35 -07:00
parent 1999ad8be3
commit be01ae733e
12 changed files with 169 additions and 30 deletions

View File

@ -57,8 +57,8 @@ export const GET = createSmartRouteHandler({
scope: yupString().defined(),
state: yupString().defined(),
grant_type: yupString().oneOf(["authorization_code"]).defined(),
code_challenge: yupString().defined(),
code_challenge_method: yupString().defined(),
code_challenge: yupString().matches(/^[A-Za-z0-9._~-]{43,128}$/).defined(),
code_challenge_method: yupString().oneOf(["S256"]).defined(),
response_type: yupString().defined(),
}).noUnknown(/* Allow unknown query params such as ttclid, other stuff that's being injected by browsers */ false).defined(),
}),

View File

@ -8,7 +8,7 @@ import { getApiUrlForRequest } from "@/lib/request-api-url";
import { Tenancy, getTenancy } from "@/lib/tenancies";
import { oauthCookieSchema } from "@/lib/tokens";
import { createOAuthServer, getProvider } from "@/oauth";
import { PrismaClientTransaction, getPrismaClientForTenancy, globalPrismaClient } from "@/prisma-client";
import { PrismaClientTransaction, getPrismaClientForTenancy, globalPrismaClient, isPrismaError } from "@/prisma-client";
import { createSmartRouteHandler } from "@/route-handlers/smart-route-handler";
import { InvalidClientError, InvalidScopeError, Request as OAuthRequest, Response as OAuthResponse } from "@node-oauth/oauth2-server";
import { KnownError, KnownErrors } from "@hexclave/shared";
@ -108,14 +108,19 @@ const handler = createSmartRouteHandler({
const apiUrl = getApiUrlForRequest(fullReq);
const innerState = query.state ?? (body as any)?.state ?? "";
const outerInfoDB = await globalPrismaClient.oAuthOuterInfo.findUnique({
where: {
innerState: innerState,
},
});
if (!outerInfoDB) {
throw new StatusError(StatusError.BadRequest, "Invalid OAuth state. Please try signing in again.");
let outerInfoDB;
try {
outerInfoDB = await globalPrismaClient.oAuthOuterInfo.delete({
where: {
innerState: innerState,
},
});
} catch (e) {
if (isPrismaError(e, "DEPENDENT_RECORD_NOT_FOUND")) {
// No matching outer info (never existed, expired-and-swept, or already consumed by a concurrent/replayed callback).
throw new StatusError(StatusError.BadRequest, "Invalid OAuth state. Please try signing in again.");
}
throw e;
}
let outerInfo: Awaited<ReturnType<typeof oauthCookieSchema.validate>>;

View File

@ -183,6 +183,10 @@ export const teamInvitationsCrudHandlers = createLazyProxy(() => createCrudHandl
await teamInvitationCodeHandler.revokeCode({
tenancy: auth.tenancy,
id: params.id,
dataFilter: {
path: ['team_id'],
equals: teamId,
},
});
},
}));

View File

@ -303,11 +303,15 @@ export class OAuthModel implements AuthorizationCodeModel {
throw new KnownErrors.RedirectUrlNotWhitelisted(code.redirectUri);
}
if (!code.codeChallenge || code.codeChallengeMethod !== "S256") {
throw new HexclaveAssertionError("Refusing to persist an OAuth authorization code without a valid S256 PKCE challenge; this should have been rejected at the authorize endpoint.", { hasChallenge: !!code.codeChallenge, codeChallengeMethod: code.codeChallengeMethod });
}
await globalPrismaClient.projectUserAuthorizationCode.create({
data: {
authorizationCode: code.authorizationCode,
codeChallenge: code.codeChallenge || "",
codeChallengeMethod: code.codeChallengeMethod || "",
codeChallenge: code.codeChallenge,
codeChallengeMethod: code.codeChallengeMethod,
redirectUri: code.redirectUri,
expiresAt: code.expiresAt,
projectUserId: user.id,

View File

@ -629,6 +629,9 @@ export const PRISMA_ERROR_CODES = {
UNIQUE_CONSTRAINT_VIOLATION: "P2002",
FOREIGN_CONSTRAINT_VIOLATION: "P2003",
GENERIC_CONSTRAINT_VIOLATION: "P2004",
// Thrown by `delete`/`update` (and relation requirements) when the targeted row
// doesn't exist — e.g. when two requests race to consume the same single-use row.
DEPENDENT_RECORD_NOT_FOUND: "P2025",
} as const;
export function isPrismaError(error: unknown, code: keyof typeof PRISMA_ERROR_CODES): error is Prisma.PrismaClientKnownRequestError {

View File

@ -31,6 +31,7 @@ type ListCodesOptions<Data, AlreadyParsed extends boolean = true> = ProjectBranc
type RevokeCodeOptions<AlreadyParsed extends boolean = true> = ProjectBranchCombo<AlreadyParsed> & {
id: string,
dataFilter?: Prisma.JsonFilter<"VerificationCode"> | undefined,
}
type CodeObject<Data, Method extends {}, CallbackUrl extends string | URL | undefined> = {
@ -289,7 +290,26 @@ export function createVerificationCodeHandler<
const { project, branchId } = parseProjectBranchCombo(revokeOptions);
const tenancy = await getSoleTenancyFromProjectBranch(project.id, branchId);
// Record deletion for external DB sync if this is a TEAM_INVITATION code
const { count } = await globalPrismaClient.verificationCode.deleteMany({
where: {
projectId: project.id,
branchId,
id: revokeOptions.id,
type: options.type,
data: revokeOptions.dataFilter,
},
});
if (count === 0) {
// Either the code doesn't exist or it didn't match the authorized scope.
// Return the same error in both cases so callers can't probe for the
// existence of codes outside their scope.
throw new KnownErrors.VerificationCodeNotFound();
}
// Record deletion for external DB sync if this is a TEAM_INVITATION code.
// Done only after a row was actually deleted, so we don't emit a tombstone
// for a code that was never ours to delete.
if (options.type === 'TEAM_INVITATION') {
await recordExternalDbSyncDeletion(globalPrismaClient, {
tableName: "VerificationCode_TEAM_INVITATION",
@ -299,16 +319,6 @@ export function createVerificationCodeHandler<
verificationCodeId: revokeOptions.id,
});
}
await globalPrismaClient.verificationCode.delete({
where: {
projectId_branchId_id: {
projectId: project.id,
branchId,
id: revokeOptions.id,
},
},
});
},
async validateCode(tenancyIdAndCode: string) {
const fullCodeParts = tenancyIdAndCode.split('_');

View File

@ -8,7 +8,7 @@ import { filterUndefined, omit } from "@hexclave/shared/dist/utils/objects";
import { wait } from "@hexclave/shared/dist/utils/promises";
import { nicify } from "@hexclave/shared/dist/utils/strings";
import * as jose from "jose";
import { createHmac, randomUUID } from "node:crypto";
import { createHash, createHmac, randomUUID } from "node:crypto";
import { expect } from "vitest";
import { Context, Mailbox, NiceRequestInit, NiceResponse, STACK_BACKEND_BASE_URL, STACK_INTERNAL_PROJECT_ADMIN_KEY, STACK_INTERNAL_PROJECT_CLIENT_KEY, STACK_INTERNAL_PROJECT_ID, STACK_INTERNAL_PROJECT_SERVER_KEY, STACK_SVIX_SERVER_URL, generatedEmailSuffix, localRedirectUrl, niceFetch, updateCookiesFromResponse } from "../helpers";
import { localhostUrl, withPortPrefix } from "../helpers/ports";
@ -745,6 +745,9 @@ export namespace Auth {
export namespace OAuth {
export const testCodeVerifier = "some-code-challenge";
export const testCodeChallenge = createHash("sha256").update(testCodeVerifier).digest("base64url");
export async function getAuthorizeQuery(options: TurnstileTestOptions & {
forceBranchId?: string,
includeClientSecret?: boolean,
@ -766,8 +769,8 @@ export namespace Auth {
response_type: "code",
state: "this-is-some-state",
grant_type: "authorization_code",
code_challenge: "some-code-challenge",
code_challenge_method: "plain",
code_challenge: Auth.OAuth.testCodeChallenge,
code_challenge_method: "S256",
token: userAuth?.accessToken ?? undefined,
bot_challenge_token: options.turnstileToken ?? mockTurnstileTokens.oauthOk,
bot_challenge_phase: options.turnstilePhase,

View File

@ -252,6 +252,42 @@ it("should reject public client secret sentinel when publishable keys are requir
`);
});
it("should reject an empty code_challenge (PKCE cannot be disabled)", async ({ expect }) => {
const response = await niceBackendFetch("/api/v1/auth/oauth/authorize/spotify", {
redirect: "manual",
query: {
...await Auth.OAuth.getAuthorizeQuery(),
code_challenge: "",
},
});
expect(response.status).toBe(400);
expect(response.body.code).toBe("SCHEMA_ERROR");
});
it("should reject a non-S256 code_challenge_method", async ({ expect }) => {
const response = await niceBackendFetch("/api/v1/auth/oauth/authorize/spotify", {
redirect: "manual",
query: {
...await Auth.OAuth.getAuthorizeQuery(),
code_challenge_method: "plain",
},
});
expect(response.status).toBe(400);
expect(response.body.code).toBe("SCHEMA_ERROR");
});
it("should reject a code_challenge that is too short to be a real S256 challenge", async ({ expect }) => {
const response = await niceBackendFetch("/api/v1/auth/oauth/authorize/spotify", {
redirect: "manual",
query: {
...await Auth.OAuth.getAuthorizeQuery(),
code_challenge: "too-short",
},
});
expect(response.status).toBe(400);
expect(response.body.code).toBe("SCHEMA_ERROR");
});
it("should fail if an invalid redirect URL is provided", async ({ expect }) => {
const response = await niceBackendFetch("/api/v1/auth/oauth/authorize/spotify", {
redirect: "manual",

View File

@ -429,6 +429,71 @@ it("allows team admins to revoke invitations", async ({ expect }) => {
});
it("cannot revoke another team's invitation by passing a team_id the caller controls", async ({ expect }) => {
await Project.createAndSwitch();
const { userId: attackerId } = await Auth.fastSignUp();
// Team A: attacker holds $remove_members here.
const { teamId: teamA } = await Team.create();
await niceBackendFetch(`/api/v1/team-permissions/${teamA}/${attackerId}/$remove_members`, {
accessType: "server",
method: "POST",
body: {},
});
// Team B: has a pending invitation. (Created by the same actor only so the test
// can read the invitation id; the attack is passing teamA as the team_id.)
const { teamId: teamB } = await Team.create();
await niceBackendFetch(`/api/v1/team-permissions/${teamB}/${attackerId}/$invite_members`, {
accessType: "server",
method: "POST",
body: {},
});
const { sendTeamInvitationResponse } = await Team.sendInvitation("victim-invite@example.com", teamB);
const teamBInvitationId = sendTeamInvitationResponse.body.id;
// Revoke team B's invitation while authorizing against team A. Must fail: the
// invitation does not belong to team A, so the delete is not performed.
const revokeResponse = await niceBackendFetch(`/api/v1/team-invitations/${teamBInvitationId}?team_id=${teamA}`, {
accessType: "client",
method: "DELETE",
});
expect(revokeResponse).toMatchInlineSnapshot(`
NiceResponse {
"status": 404,
"body": {
"code": "VERIFICATION_CODE_NOT_FOUND",
"error": "The verification code does not exist for this project.",
},
"headers": Headers {
"x-stack-known-error": "VERIFICATION_CODE_NOT_FOUND",
<some fields may have been hidden>,
},
}
`);
// Team B's invitation must still exist.
const listInvitationsResponse = await niceBackendFetch(`/api/v1/team-invitations?team_id=${teamB}`, {
accessType: "server",
method: "GET",
});
expect(listInvitationsResponse.status).toBe(200);
expect(listInvitationsResponse.body.items).toHaveLength(1);
expect(listInvitationsResponse.body.items[0].id).toBe(teamBInvitationId);
// And revoking with the correct team_id still works (no regression).
await niceBackendFetch(`/api/v1/team-permissions/${teamB}/${attackerId}/$remove_members`, {
accessType: "server",
method: "POST",
body: {},
});
const correctRevoke = await niceBackendFetch(`/api/v1/team-invitations/${teamBInvitationId}?team_id=${teamB}`, {
accessType: "client",
method: "DELETE",
});
expect(correctRevoke.status).toBe(200);
});
it("requires $remove_members permission to revoke invitations", async ({ expect }) => {
const { userId: inviter } = await Auth.fastSignUp();
const { teamId } = await createAndAddCurrentUserWithoutMemberPermission();

View File

@ -3176,7 +3176,10 @@
"name": "code_challenge_method",
"in": "query",
"schema": {
"type": "string"
"type": "string",
"enum": [
"S256"
]
},
"required": true
},

View File

@ -2390,7 +2390,10 @@
"name": "code_challenge_method",
"in": "query",
"schema": {
"type": "string"
"type": "string",
"enum": [
"S256"
]
},
"required": true
},

View File

@ -3136,7 +3136,10 @@
"name": "code_challenge_method",
"in": "query",
"schema": {
"type": "string"
"type": "string",
"enum": [
"S256"
]
},
"required": true
},