diff --git a/src/app/api/v1/_helpers/apiKeyScope.ts b/src/app/api/v1/_helpers/apiKeyScope.ts index 4d238a7818b..b599fcb7924 100644 --- a/src/app/api/v1/_helpers/apiKeyScope.ts +++ b/src/app/api/v1/_helpers/apiKeyScope.ts @@ -26,3 +26,21 @@ export async function getApiKeyRequestScope(request: Request): Promise, + recordApiKeyId: string | null | undefined +): boolean { + if (scope.isSessionAuth) return true; + if (recordApiKeyId === null || recordApiKeyId === undefined) return true; + return recordApiKeyId === scope.apiKeyId; +} diff --git a/src/app/api/v1/batches/[id]/cancel/route.ts b/src/app/api/v1/batches/[id]/cancel/route.ts index 3222f0f0d81..11615242f50 100644 --- a/src/app/api/v1/batches/[id]/cancel/route.ts +++ b/src/app/api/v1/batches/[id]/cancel/route.ts @@ -1,7 +1,7 @@ import { CORS_HEADERS, handleCorsOptions } from "@/shared/utils/cors"; import { getBatch, updateBatch } from "@/lib/db/batches"; import { NextResponse } from "next/server"; -import { getApiKeyRequestScope } from "@/app/api/v1/_helpers/apiKeyScope"; +import { getApiKeyRequestScope, scopeCheck } from "@/app/api/v1/_helpers/apiKeyScope"; import { formatBatchResponse } from "../../formatBatchResponse"; export async function OPTIONS() { @@ -11,12 +11,11 @@ export async function OPTIONS() { export async function POST(request: Request, { params }: { params: Promise<{ id: string }> }) { const scope = await getApiKeyRequestScope(request); if (scope.rejection) return scope.rejection; - const apiKeyId = scope.apiKeyId; const { id } = await params; const batch = getBatch(id); - if (!batch || (batch.apiKeyId !== null && batch.apiKeyId !== apiKeyId)) { + if (!batch || !scopeCheck(scope, batch.apiKeyId)) { return NextResponse.json( { error: { message: "Batch not found", type: "invalid_request_error" } }, { status: 404, headers: CORS_HEADERS } diff --git a/src/app/api/v1/batches/[id]/route.ts b/src/app/api/v1/batches/[id]/route.ts index 7ce3867906d..04e1a5de473 100644 --- a/src/app/api/v1/batches/[id]/route.ts +++ b/src/app/api/v1/batches/[id]/route.ts @@ -1,22 +1,13 @@ import { CORS_HEADERS, handleCorsOptions } from "@/shared/utils/cors"; import { getBatch, deleteBatch } from "@/lib/db/batches"; import { NextResponse } from "next/server"; -import { getApiKeyRequestScope } from "@/app/api/v1/_helpers/apiKeyScope"; +import { getApiKeyRequestScope, scopeCheck } from "@/app/api/v1/_helpers/apiKeyScope"; import { formatBatchResponse } from "../formatBatchResponse"; export async function OPTIONS() { return handleCorsOptions(); } -function scopeCheck( - scope: { isSessionAuth: boolean; apiKeyId: string | null }, - recordApiKeyId: string | null | undefined -): boolean { - if (scope.isSessionAuth) return true; - if (recordApiKeyId === null || recordApiKeyId === undefined) return true; - return recordApiKeyId === scope.apiKeyId; -} - export async function GET(request: Request, { params }: { params: Promise<{ id: string }> }) { const scope = await getApiKeyRequestScope(request); if (scope.rejection) return scope.rejection; diff --git a/tests/unit/batch-cancel-session-auth-scope.test.ts b/tests/unit/batch-cancel-session-auth-scope.test.ts new file mode 100644 index 00000000000..6fc152e2ccd --- /dev/null +++ b/tests/unit/batch-cancel-session-auth-scope.test.ts @@ -0,0 +1,109 @@ +/** + * `POST /api/v1/batches/[id]/cancel` rejected the dashboard's own + * session-authenticated caller as "Batch not found" (404) for any batch + * owned by a non-null api_key_id -- which in practice is every batch created + * through the default `env-key`, i.e. every real batch on the instance. + * Cancelling from the dashboard silently did nothing. + * + * Root cause: the route carried its own inline ownership check + * (`batch.apiKeyId !== null && batch.apiKeyId !== apiKeyId`) instead of the + * canonical `scopeCheck()` that two sibling implementations already get + * right: `batches/[id]/route.ts` (GET/DELETE) and `deleteCompletedBatches()` + * (GHSA-wvxc-jp3v-5mg5) both treat session auth as the instance-wide + * operator, able to act on any record regardless of which API key owns it. + * The inline check never granted that exemption, so a session-authenticated + * caller (`apiKeyId === null`) was treated as a mismatched key the instant + * `batch.apiKeyId` was non-null. + * + * This test proves the fix at the ownership-decision boundary (`scopeCheck`, + * now shared via `_helpers/apiKeyScope.ts`) against a batch shaped exactly + * like the two that were actually stuck in production (`api_key_id: "env-key"`), + * and proves the route source no longer contains the buggy inline check. + * + * Run with: + * node --import tsx/esm --test tests/unit/batch-cancel-session-auth-scope.test.ts + */ + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { createFile } from "@/lib/db/files"; +import { createBatch } from "@/lib/db/batches"; +import { scopeCheck } from "@/app/api/v1/_helpers/apiKeyScope"; + +function seedBatch(apiKeyId: string | null, status: "validating" | "in_progress", tag: string) { + const file = createFile({ + bytes: 10, + filename: `cancel-scope-${tag}.jsonl`, + purpose: "batch", + content: Buffer.from("{}"), + }); + return createBatch({ + endpoint: "/v1/chat/completions", + completionWindow: "24h", + inputFileId: file.id, + status, + apiKeyId, + }); +} + +describe("cancel route ownership scoping", () => { + it("session auth (dashboard) may cancel a batch owned by an API key", () => { + const batch = seedBatch("env-key", "in_progress", "a1"); + + // Exactly the check cancel/route.ts now runs: `!scopeCheck(scope, batch.apiKeyId)` + const allowed = scopeCheck({ isSessionAuth: true, apiKeyId: null }, batch.apiKeyId); + + assert.equal(allowed, true, "the operator's dashboard must be able to cancel any batch"); + }); + + it("an unrelated API key may not cancel someone else's batch", () => { + const batch = seedBatch("env-key", "validating", "a2"); + + const allowed = scopeCheck({ isSessionAuth: false, apiKeyId: "other-key" }, batch.apiKeyId); + + assert.equal(allowed, false, "a foreign API key must not be able to cancel this batch"); + }); + + it("the owning API key may cancel its own batch", () => { + const batch = seedBatch("key-owns-this", "validating", "a3"); + + const allowed = scopeCheck({ isSessionAuth: false, apiKeyId: "key-owns-this" }, batch.apiKeyId); + + assert.equal(allowed, true, "the owning API key must be able to cancel its own batch"); + }); + + it("the original buggy inline check would have rejected the session-auth caller", () => { + const batch = seedBatch("env-key", "in_progress", "a4"); + + // This is the exact predicate cancel/route.ts used to run before the fix. + const apiKeyId: string | null = null; // session auth + const rejectedByOldCheck = !batch || (batch.apiKeyId !== null && batch.apiKeyId !== apiKeyId); + + assert.equal( + rejectedByOldCheck, + true, + "documents the regression: the old inline check 404'd every dashboard cancel" + ); + }); +}); + +describe("the route uses the shared scopeCheck instead of its old inline predicate", () => { + it("cancel/route.ts no longer carries the buggy apiKeyId !== null inline check", async () => { + const { readFileSync } = await import("node:fs"); + const { fileURLToPath } = await import("node:url"); + const src = readFileSync( + fileURLToPath( + new URL("../../src/app/api/v1/batches/[id]/cancel/route.ts", import.meta.url) + ), + "utf8" + ); + assert.ok( + !/batch\.apiKeyId\s*!==\s*null\s*&&\s*batch\.apiKeyId\s*!==\s*apiKeyId/.test(src), + "the route still carries the old inline ownership check that 404s session auth" + ); + assert.ok( + /scopeCheck\(\s*scope\s*,\s*batch\.apiKeyId\s*\)/.test(src), + "the route must delegate ownership to the shared scopeCheck helper" + ); + }); +});