diff --git a/apps/backend/src/app/api/latest/auth/oauth/authorize/[provider_id]/route.tsx b/apps/backend/src/app/api/latest/auth/oauth/authorize/[provider_id]/route.tsx index 0fbcb8be7..7d621dc09 100644 --- a/apps/backend/src/app/api/latest/auth/oauth/authorize/[provider_id]/route.tsx +++ b/apps/backend/src/app/api/latest/auth/oauth/authorize/[provider_id]/route.tsx @@ -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(), }), diff --git a/apps/backend/src/app/api/latest/auth/oauth/callback/[provider_id]/route.tsx b/apps/backend/src/app/api/latest/auth/oauth/callback/[provider_id]/route.tsx index aae9990b1..7fd915275 100644 --- a/apps/backend/src/app/api/latest/auth/oauth/callback/[provider_id]/route.tsx +++ b/apps/backend/src/app/api/latest/auth/oauth/callback/[provider_id]/route.tsx @@ -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>; diff --git a/apps/backend/src/app/api/latest/team-invitations/crud.tsx b/apps/backend/src/app/api/latest/team-invitations/crud.tsx index 3188e05ba..3c3ff6c0a 100644 --- a/apps/backend/src/app/api/latest/team-invitations/crud.tsx +++ b/apps/backend/src/app/api/latest/team-invitations/crud.tsx @@ -183,6 +183,10 @@ export const teamInvitationsCrudHandlers = createLazyProxy(() => createCrudHandl await teamInvitationCodeHandler.revokeCode({ tenancy: auth.tenancy, id: params.id, + dataFilter: { + path: ['team_id'], + equals: teamId, + }, }); }, })); diff --git a/apps/backend/src/oauth/model.tsx b/apps/backend/src/oauth/model.tsx index dadcd9621..355523bd5 100644 --- a/apps/backend/src/oauth/model.tsx +++ b/apps/backend/src/oauth/model.tsx @@ -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, diff --git a/apps/backend/src/prisma-client.tsx b/apps/backend/src/prisma-client.tsx index c2b17b105..0471048ba 100644 --- a/apps/backend/src/prisma-client.tsx +++ b/apps/backend/src/prisma-client.tsx @@ -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 { diff --git a/apps/backend/src/route-handlers/verification-code-handler.tsx b/apps/backend/src/route-handlers/verification-code-handler.tsx index 8200c90c3..fe7dd4c2b 100644 --- a/apps/backend/src/route-handlers/verification-code-handler.tsx +++ b/apps/backend/src/route-handlers/verification-code-handler.tsx @@ -31,6 +31,7 @@ type ListCodesOptions = ProjectBranc type RevokeCodeOptions = ProjectBranchCombo & { id: string, + dataFilter?: Prisma.JsonFilter<"VerificationCode"> | undefined, } type CodeObject = { @@ -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('_'); diff --git a/apps/e2e/tests/backend/backend-helpers.ts b/apps/e2e/tests/backend/backend-helpers.ts index 0ac8c4608..3275b4f69 100644 --- a/apps/e2e/tests/backend/backend-helpers.ts +++ b/apps/e2e/tests/backend/backend-helpers.ts @@ -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, diff --git a/apps/e2e/tests/backend/endpoints/api/v1/auth/oauth/authorize.test.ts b/apps/e2e/tests/backend/endpoints/api/v1/auth/oauth/authorize.test.ts index 467bce1a9..63358b0d7 100644 --- a/apps/e2e/tests/backend/endpoints/api/v1/auth/oauth/authorize.test.ts +++ b/apps/e2e/tests/backend/endpoints/api/v1/auth/oauth/authorize.test.ts @@ -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", diff --git a/apps/e2e/tests/backend/endpoints/api/v1/team-invitations.test.ts b/apps/e2e/tests/backend/endpoints/api/v1/team-invitations.test.ts index d49373d79..a02759f90 100644 --- a/apps/e2e/tests/backend/endpoints/api/v1/team-invitations.test.ts +++ b/apps/e2e/tests/backend/endpoints/api/v1/team-invitations.test.ts @@ -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", +