From e7f9fec251fdc5df984750f0719309be983381f6 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Tue, 15 Sep 2026 13:25:13 -0300 Subject: [PATCH] =?UTF-8?q?fix(api):=20enforce=20API-key=20ownership=20on?= =?UTF-8?q?=20files=20and=20batches=20=E2=80=94=20null-owner=20records=20a?= =?UTF-8?q?nd=20anonymous=20listing=20(#13749)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GHSA-2jm2-mpx8-6523 and GHSA-m3hp-hq9g-fpmv, one root cause. `getApiKeyRequestScope()` never rejects: with the default REQUIRE_API_KEY=false the client-api policy admits both a missing and an invalid bearer as anonymous, and the scope comes back `{ apiKeyId: null, isSessionAuth: false }`. The `/v1/files` and `/v1/batches` routes then treated "null" as permissive in two different ways: - GHSA-m3hp — the list routes coerced `apiKeyId || undefined`, and the DB layer reads `undefined` as "no owner filter", so an anonymous or invalid-bearer caller got every tenant's file and batch metadata, the same unfiltered view as the operator's dashboard. - GHSA-2jm2 — the single-record checks were `record.apiKeyId !== null && …`, so a record with no owner short-circuited to "allowed" for any caller: read, download raw content, delete, cancel, or use as a batch input. Null-owner records are common — every dashboard-session upload, and every batch output file inheriting a session batch's owner, which carries model responses. `api_key_id` has existed since the table was created (migration 028), so a null owner is not a legacy row; it is an unattributable write. No doc described it as shared — API_REFERENCE says files are scoped per key — and batch_api.test.ts pinned the by-id exposure as expected behaviour. One rule now, in `_helpers/apiKeyScope.ts`: - `canAccessOwnedRecord(scope, owner)`: a dashboard session is the instance operator and may act on any record; an API key acts on its own records only; a null owner is denied to every non-session caller. Applied to files GET / DELETE / content, batches GET / DELETE / cancel, and the batch-create input-file check. - `resolveListScope(scope)`: an explicit union for list/count reads — scoped to the presented key (a key wins even alongside a session cookie), instance-wide only for a session without a key, and 401 otherwise, including for a bearer that does not resolve to a key. There is no default that widens a read. This follows the GHSA-wvxc shape already used by the delete-completed sweep. Behaviour change: the anonymous upload → batch → download flow no longer works without an API key, because a null owner cannot be attributed. Subsumes #13683: it moved `scopeCheck` into the shared helper so a session can cancel any batch — kept, and its test ported — but it also kept null-owner records open on the premise they predate ownership tracking, which migration 028 contradicts. Tests are red-first. batch_api's by-id case is flipped to 404 with a negative assertion; batch-deletion-route-logic now imports the real helper instead of a local copy that had silently diverged from production; the two integration tests present a real key, since their subject is limits and rate logging, not auth. Co-authored-by: Markus Hartung --- ...hes-ownership-null-owner-anonymous-list.md | 1 + docs/reference/API_REFERENCE.md | 12 +- src/app/api/v1/_helpers/apiKeyScope.ts | 78 +++ src/app/api/v1/batches/[id]/cancel/route.ts | 9 +- src/app/api/v1/batches/[id]/route.ts | 18 +- .../api/v1/batches/delete-completed/route.ts | 4 +- src/app/api/v1/batches/route.ts | 25 +- src/app/api/v1/files/[id]/content/route.ts | 7 +- src/app/api/v1/files/[id]/route.ts | 20 +- src/app/api/v1/files/route.ts | 15 +- src/lib/db/batches.ts | 11 +- .../integration/batch-e2e-rate-limit.test.ts | 28 +- .../files-api-limit-validation.test.ts | 41 +- .../batch-cancel-session-auth-scope.test.ts | 133 +++++ tests/unit/batch-deletion-route-logic.test.ts | 29 +- tests/unit/batch_api.test.ts | 11 +- .../files-batches-ownership-2jm2-m3hp.test.ts | 490 ++++++++++++++++++ 17 files changed, 861 insertions(+), 71 deletions(-) create mode 100644 changelog.d/fixes/0000-files-batches-ownership-null-owner-anonymous-list.md create mode 100644 tests/unit/batch-cancel-session-auth-scope.test.ts create mode 100644 tests/unit/files-batches-ownership-2jm2-m3hp.test.ts diff --git a/changelog.d/fixes/0000-files-batches-ownership-null-owner-anonymous-list.md b/changelog.d/fixes/0000-files-batches-ownership-null-owner-anonymous-list.md new file mode 100644 index 0000000000..a6f4d3f3b9 --- /dev/null +++ b/changelog.d/fixes/0000-files-batches-ownership-null-owner-anonymous-list.md @@ -0,0 +1 @@ +- **fix(api):** `/v1/files` and `/v1/batches` now enforce one ownership rule everywhere — a dashboard session is the instance operator, an API key acts on its own records only, and a record with no owner is denied to every non-session caller. Previously a file or batch whose `api_key_id` was null (a dashboard-session or anonymous upload, or a batch artifact inheriting one) could be read, downloaded, deleted, cancelled or used as a batch input by any other key or by an unauthenticated caller (GHSA-2jm2-mpx8-6523), and `GET /v1/files` / `GET /v1/batches` returned every tenant's records to an anonymous or invalid-bearer caller under the default `REQUIRE_API_KEY=false` (GHSA-m3hp-hq9g-fpmv) — both lists now fail closed with a `401`, and only a dashboard session without a key reads the whole instance. The same shared rule lets the dashboard cancel any batch, not just unowned ones. Behaviour change: the anonymous upload → batch → download flow no longer works without an API key, since a null owner cannot be attributed. Subsumes [#13683](https://github.com/diegosouzapw/OmniRoute/pull/13683) — thanks @hartmark diff --git a/docs/reference/API_REFERENCE.md b/docs/reference/API_REFERENCE.md index 0c470b525c..18cc55cacf 100644 --- a/docs/reference/API_REFERENCE.md +++ b/docs/reference/API_REFERENCE.md @@ -530,7 +530,12 @@ OpenAI-compatible files endpoint for batch input/output and file-purpose uploads | DELETE | `/v1/files/[id]` | Delete a file | | GET | `/v1/files/[id]/content` | Stream the raw file body back | -**Auth:** Bearer API key — files are scoped per-API-key via `getApiKeyRequestScope`. +**Auth:** Bearer API key — files are scoped per-API-key via `getApiKeyRequestScope`. A key +sees, downloads and deletes its own files only; a dashboard session without a key reads the +whole instance; a file with no owner (anonymous or dashboard-session upload) is denied to every +non-session caller. `GET /v1/files` rejects an anonymous caller — and a presented key that does +not resolve — with `401` even when `REQUIRE_API_KEY=false`, instead of listing every tenant's +files (GHSA-m3hp-hq9g-fpmv, GHSA-2jm2-mpx8-6523). --- @@ -546,7 +551,10 @@ OpenAI-compatible batch processing. | DELETE | `/v1/batches/[id]` | Delete a finished/failed batch | | POST | `/v1/batches/[id]/cancel` | Cancel an in-progress batch | -**Auth:** Bearer API key. Batches are scoped per-API-key. +**Auth:** Bearer API key. Batches are scoped per-API-key under the same three-way rule as +files: own key only, dashboard session instance-wide, null-owner records denied to every +non-session caller (retrieve, delete, cancel, and the `input_file_id` check on create). +`GET /v1/batches` rejects an anonymous caller with `401` even when `REQUIRE_API_KEY=false`. --- diff --git a/src/app/api/v1/_helpers/apiKeyScope.ts b/src/app/api/v1/_helpers/apiKeyScope.ts index 4d238a7818..21aba3fca9 100644 --- a/src/app/api/v1/_helpers/apiKeyScope.ts +++ b/src/app/api/v1/_helpers/apiKeyScope.ts @@ -1,6 +1,9 @@ +import { NextResponse } from "next/server"; import { getApiKeyMetadata } from "@/lib/db/apiKeys"; import { extractApiKey } from "@/sse/services/auth"; import { isDashboardSessionAuthenticated } from "@/shared/utils/apiAuth"; +import { CORS_HEADERS } from "@/shared/utils/cors"; +import { buildErrorBody } from "@omniroute/open-sse/utils/error"; export interface ApiKeyRequestScope { apiKey: string | null; @@ -26,3 +29,78 @@ export async function getApiKeyRequestScope(request: Request): Promise, + recordApiKeyId: string | null | undefined +): boolean { + if (scope.isSessionAuth) return true; + if (recordApiKeyId === null || recordApiKeyId === undefined) return false; + return recordApiKeyId === scope.apiKeyId; +} + +/** + * Owner scope of a CLIENT_API list/count read (`GET /v1/files`, `GET /v1/batches`). + * The intent is explicit on purpose, exactly like the `delete-completed` sweep: + * a caller is either scoped to the API key it presented, or it is the operator's + * dashboard session reading the whole instance, or it is rejected — there is no + * default that widens a read to every tenant (GHSA-m3hp-hq9g-fpmv). + */ +export type OwnedListScope = + | { mode: "api_key"; apiKeyId: string } + | { mode: "instance" } + | { mode: "rejected"; response: Response }; + +function unauthorized(message: string): Response { + return NextResponse.json(buildErrorBody(401, message), { status: 401, headers: CORS_HEADERS }); +} + +/** + * Resolve the {@link OwnedListScope} of a list/count request, failing closed: + * + * - a presented bearer that does not resolve to a key row (deleted, rotated, + * mistyped) → 401 "Invalid API key" — even when a session cookie is also + * present, so an unresolvable key never falls through to the session branch; + * - a resolved key → scoped to that key, even alongside a session cookie (the + * key wins, so a leaked or over-shared key can never widen a read); + * - a dashboard session WITHOUT a key → instance-wide (the operator's own + * dashboard is the one legitimate instance-wide reader); + * - anything else (anonymous under `REQUIRE_API_KEY=false`) → 401 + * "Authentication required". + * + * The list handlers used to coerce `apiKeyId || undefined`, and the DB layer + * reads `undefined` as "no owner filter" — so the anonymous caller landed in the + * same unfiltered bucket as the operator. + */ +export function resolveListScope(scope: ApiKeyRequestScope): OwnedListScope { + if (scope.apiKey && !scope.apiKeyId) { + return { mode: "rejected", response: unauthorized("Invalid API key") }; + } + if (scope.apiKeyId) { + return { mode: "api_key", apiKeyId: scope.apiKeyId }; + } + if (scope.isSessionAuth) { + return { mode: "instance" }; + } + return { mode: "rejected", response: unauthorized("Authentication required") }; +} diff --git a/src/app/api/v1/batches/[id]/cancel/route.ts b/src/app/api/v1/batches/[id]/cancel/route.ts index 3222f0f0d8..d441f9911c 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, canAccessOwnedRecord } from "@/app/api/v1/_helpers/apiKeyScope"; import { formatBatchResponse } from "../../formatBatchResponse"; export async function OPTIONS() { @@ -11,12 +11,15 @@ 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)) { + // The shared 3-way rule: the operator's dashboard (session auth) may cancel + // ANY batch — the old inline check 404'd every dashboard cancel of a + // key-owned batch (#13683) — a key cancels its own, and a null-owner batch + // is denied to a foreign key and to an anonymous caller (GHSA-2jm2-mpx8-6523). + if (!batch || !canAccessOwnedRecord(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 7ce3867906..b9d841f8f4 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, canAccessOwnedRecord } 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; @@ -24,7 +15,10 @@ export async function GET(request: Request, { params }: { params: Promise<{ id: const { id } = await params; const batch = getBatch(id); - if (!batch || !scopeCheck(scope, batch.apiKeyId)) { + // Session = operator, key = own rows only, null owner = denied + // (GHSA-2jm2-mpx8-6523): the previous local check let ANY caller read or + // delete an unowned batch by id. + if (!batch || !canAccessOwnedRecord(scope, batch.apiKeyId)) { return NextResponse.json( { error: { message: "Batch not found", type: "invalid_request_error" } }, { status: 404, headers: CORS_HEADERS } @@ -41,7 +35,7 @@ export async function DELETE(request: Request, { params }: { params: Promise<{ i const { id } = await params; const batch = getBatch(id); - if (!batch || !scopeCheck(scope, batch.apiKeyId)) { + if (!batch || !canAccessOwnedRecord(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/delete-completed/route.ts b/src/app/api/v1/batches/delete-completed/route.ts index fa60d98d61..5e095fc845 100644 --- a/src/app/api/v1/batches/delete-completed/route.ts +++ b/src/app/api/v1/batches/delete-completed/route.ts @@ -51,8 +51,8 @@ export async function DELETE(request: Request) { if (policy.rejection) return policy.rejection; // A presented API key always scopes the sweep to that key — even when the - // request also carries a dashboard session cookie — exactly like the - // list/count siblings (`apiKeyId || undefined`), so a leaked or over-shared + // request also carries a dashboard session cookie — the same rule the list + // siblings apply through `resolveListScope()`, so a leaked or over-shared // key can never widen a destructive sweep. Only a dashboard session WITHOUT a // key sweeps the whole instance; otherwise an ordinary key would delete every // tenant's completed batches and null out their file contents diff --git a/src/app/api/v1/batches/route.ts b/src/app/api/v1/batches/route.ts index f64a46d972..46e080aa26 100644 --- a/src/app/api/v1/batches/route.ts +++ b/src/app/api/v1/batches/route.ts @@ -3,7 +3,11 @@ import { createBatch, listBatches, countBatches } from "@/lib/db/batches"; import { getFile } from "@/lib/db/files"; import { v1BatchCreateSchema } from "@/shared/validation/schemas"; import { NextResponse } from "next/server"; -import { getApiKeyRequestScope } from "@/app/api/v1/_helpers/apiKeyScope"; +import { + getApiKeyRequestScope, + canAccessOwnedRecord, + resolveListScope, +} from "@/app/api/v1/_helpers/apiKeyScope"; import { formatBatchResponse } from "./formatBatchResponse"; import { parseBatchListLimit } from "./parseListLimit"; @@ -32,8 +36,12 @@ export async function POST(request: Request) { } const validated = validation.data; + // The batch runs LLM requests over the input file's content, so the caller + // must be allowed to READ that file: own key, or the operator's session. A + // null-owner input file is denied to a foreign key and to an anonymous + // caller alike (GHSA-2jm2-mpx8-6523). const inputFile = getFile(validated.input_file_id); - if (!inputFile || (inputFile.apiKeyId !== null && inputFile.apiKeyId !== apiKeyId)) { + if (!inputFile || !canAccessOwnedRecord(scope, inputFile.apiKeyId)) { return NextResponse.json( { error: { message: "Input file not found", type: "invalid_request_error" } }, { status: 400, headers: CORS_HEADERS } @@ -68,7 +76,14 @@ export async function POST(request: Request) { export async function GET(request: Request) { const scope = await getApiKeyRequestScope(request); if (scope.rejection) return scope.rejection; - const apiKeyId = scope.apiKeyId; + + // Key → own batches only; dashboard session without a key → instance-wide; + // anonymous / unresolvable bearer → 401. `listBatches`/`countBatches` read an + // absent owner as "every tenant", so the widening must be an explicit + // decision here, never a fallback (GHSA-m3hp-hq9g-fpmv). + const listScope = resolveListScope(scope); + if (listScope.mode === "rejected") return listScope.response; + const ownerFilter = listScope.mode === "api_key" ? listScope.apiKeyId : undefined; const url = new URL(request.url); const parsedLimit = parseBatchListLimit(url.searchParams.get("limit")); @@ -81,13 +96,13 @@ export async function GET(request: Request) { const limit = parsedLimit.limit; const after = url.searchParams.get("after") || undefined; - const batches = listBatches(apiKeyId || undefined, limit + 1, after); + const batches = listBatches(ownerFilter, limit + 1, after); const hasMore = batches.length > limit; const data = hasMore ? batches.slice(0, limit) : batches; const formattedData = data.map((b) => formatBatchResponse(b)); - const totalCount = countBatches(apiKeyId || undefined); + const totalCount = countBatches(ownerFilter); return NextResponse.json( { diff --git a/src/app/api/v1/files/[id]/content/route.ts b/src/app/api/v1/files/[id]/content/route.ts index 33bf4fdab4..73255c6982 100644 --- a/src/app/api/v1/files/[id]/content/route.ts +++ b/src/app/api/v1/files/[id]/content/route.ts @@ -1,7 +1,7 @@ import { CORS_HEADERS, handleCorsOptions } from "@/shared/utils/cors"; import { getFile, getFileContent } from "@/lib/db/files"; import { NextResponse } from "next/server"; -import { getApiKeyRequestScope } from "@/app/api/v1/_helpers/apiKeyScope"; +import { getApiKeyRequestScope, canAccessOwnedRecord } from "@/app/api/v1/_helpers/apiKeyScope"; export async function OPTIONS() { return handleCorsOptions(); @@ -10,12 +10,13 @@ export async function OPTIONS() { export async function GET(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 file = getFile(id); - if (!file || (file.apiKeyId !== null && file.apiKeyId !== apiKeyId && !scope.isSessionAuth)) { + // `getFileContent` has no ownership check of its own — this guard is the only + // thing between a caller and the raw bytes (GHSA-2jm2-mpx8-6523). + if (!file || !canAccessOwnedRecord(scope, file.apiKeyId)) { return NextResponse.json( { error: { message: "File not found", type: "invalid_request_error" } }, { status: 404, headers: CORS_HEADERS } diff --git a/src/app/api/v1/files/[id]/route.ts b/src/app/api/v1/files/[id]/route.ts index 953903cca4..91e47872e7 100644 --- a/src/app/api/v1/files/[id]/route.ts +++ b/src/app/api/v1/files/[id]/route.ts @@ -1,7 +1,7 @@ import { CORS_HEADERS, handleCorsOptions } from "@/shared/utils/cors"; import { getFile, deleteFile, formatFileResponse } from "@/lib/db/files"; import { NextResponse } from "next/server"; -import { getApiKeyRequestScope } from "@/app/api/v1/_helpers/apiKeyScope"; +import { getApiKeyRequestScope, canAccessOwnedRecord } from "@/app/api/v1/_helpers/apiKeyScope"; export async function OPTIONS() { return handleCorsOptions(); @@ -10,12 +10,14 @@ export async function OPTIONS() { export async function GET(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 file = getFile(id); - if (!file || (file.apiKeyId !== null && file.apiKeyId !== apiKeyId && !scope.isSessionAuth)) { + // Session = operator, key = own rows only, null owner = denied + // (GHSA-2jm2-mpx8-6523). A foreign or anonymous caller gets the same 404 as + // a missing id so the id space cannot be probed. + if (!file || !canAccessOwnedRecord(scope, file.apiKeyId)) { return NextResponse.json( { error: { message: "File not found", type: "invalid_request_error" } }, { status: 404, headers: CORS_HEADERS } @@ -28,21 +30,11 @@ export async function GET(request: Request, { params }: { params: Promise<{ id: export async function DELETE(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 file = getFile(id); - if (!file) { - return NextResponse.json( - { error: { message: "File not found", type: "invalid_request_error" } }, - { status: 404, headers: CORS_HEADERS } - ); - } - - // Allow session-authenticated (dashboard) requests to delete any file; - // for API-key-authenticated requests, enforce scope. - if (!scope.isSessionAuth && file.apiKeyId !== null && file.apiKeyId !== apiKeyId) { + if (!file || !canAccessOwnedRecord(scope, file.apiKeyId)) { return NextResponse.json( { error: { message: "File not found", type: "invalid_request_error" } }, { status: 404, headers: CORS_HEADERS } diff --git a/src/app/api/v1/files/route.ts b/src/app/api/v1/files/route.ts index 4550755a15..63d8380782 100644 --- a/src/app/api/v1/files/route.ts +++ b/src/app/api/v1/files/route.ts @@ -1,7 +1,7 @@ import { CORS_HEADERS, handleCorsOptions } from "@/shared/utils/cors"; import { createFile, listFiles, formatFileResponse, countFiles } from "@/lib/db/files"; import { NextResponse } from "next/server"; -import { getApiKeyRequestScope } from "@/app/api/v1/_helpers/apiKeyScope"; +import { getApiKeyRequestScope, resolveListScope } from "@/app/api/v1/_helpers/apiKeyScope"; export async function OPTIONS() { return handleCorsOptions(); @@ -130,7 +130,14 @@ export async function POST(request: Request) { export async function GET(request: Request) { const scope = await getApiKeyRequestScope(request); if (scope.rejection) return scope.rejection; - const apiKeyId = scope.apiKeyId; + + // Key → own files only; dashboard session without a key → instance-wide; + // anonymous / unresolvable bearer → 401. `listFiles`/`countFiles` read an + // absent owner as "every tenant", so the widening must be an explicit + // decision here, never a fallback (GHSA-m3hp-hq9g-fpmv). + const listScope = resolveListScope(scope); + if (listScope.mode === "rejected") return listScope.response; + const ownerFilter = listScope.mode === "api_key" ? listScope.apiKeyId : undefined; const { searchParams } = new URL(request.url); const parsed = parseFilesListQuery(searchParams); @@ -139,7 +146,7 @@ export async function GET(request: Request) { // We fetch limit + 1 to check if there are more items const files = listFiles({ - apiKeyId: apiKeyId || undefined, + apiKeyId: ownerFilter, purpose, limit: limit + 1, after, @@ -148,7 +155,7 @@ export async function GET(request: Request) { const hasMore = files.length > limit; const data = files.slice(0, limit); - const totalCount = countFiles({ apiKeyId: apiKeyId || undefined, purpose }); + const totalCount = countFiles({ apiKeyId: ownerFilter, purpose }); return NextResponse.json( { diff --git a/src/lib/db/batches.ts b/src/lib/db/batches.ts index 9d3a8e4531..5282313ac2 100644 --- a/src/lib/db/batches.ts +++ b/src/lib/db/batches.ts @@ -436,11 +436,12 @@ export const INSTANCE_SWEEP_CHUNK = 200; * widening the sweep, and a scope carrying BOTH `apiKeyId` and `allTenants` is * rejected rather than widened. * - * Batches whose `api_key_id` IS NULL are intentionally OUT of a key-scoped sweep. - * This diverges from `scopeCheck` in `src/app/api/v1/batches/[id]/route.ts`, - * which lets any key read/delete a single unowned batch by id: a bulk destructive - * sweep must never reach records the key does not own, so unowned batches are - * only swept by `{ allTenants: true }`. + * Batches whose `api_key_id` IS NULL are intentionally OUT of a key-scoped sweep: + * a bulk destructive sweep must never reach records the key does not own, so + * unowned batches are only swept by `{ allTenants: true }`. The single-item routes + * apply the same rule through `canAccessOwnedRecord` in + * `src/app/api/v1/_helpers/apiKeyScope.ts` (a null owner is denied to every + * non-session caller — GHSA-2jm2-mpx8-6523). * * In key mode the file half is owner-scoped too: only files whose api_key_id is * the caller's are soft-deleted; a referenced file another tenant owns (or an diff --git a/tests/integration/batch-e2e-rate-limit.test.ts b/tests/integration/batch-e2e-rate-limit.test.ts index 3460efbf95..686262d100 100644 --- a/tests/integration/batch-e2e-rate-limit.test.ts +++ b/tests/integration/batch-e2e-rate-limit.test.ts @@ -281,6 +281,12 @@ async function removeDirWithRetry(dir: string) { const relay = createFakeEmbeddingRelay(); let app: ReturnType; const RELAY_BASE = `http://127.0.0.1:${RELAY_PORT}`; +// The `/v1/files` + `/v1/batches` flow is owner-scoped: a file uploaded with no +// key has no owner, and a null-owner record is denied to every non-session +// caller (GHSA-2jm2-mpx8-6523 / GHSA-m3hp-hq9g-fpmv). Mint a real API key +// through the management API (open bootstrap mode, same path that seeds the +// provider node) and present it on every `/v1` call below. +let clientAuthHeaders: Record = {}; test.before(async () => { await relay.start(); @@ -307,6 +313,17 @@ test.before(async () => { `Failed to create provider node: ${nodeResp.status} ${JSON.stringify(nodeBody)}` ); } + + const keyResp = await fetch(`${app.baseUrl}/api/keys`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ name: "Batch E2E Test Key" }), + }); + const keyBody = (await keyResp.json().catch(() => null)) as { key?: string } | null; + if (!keyResp.ok || !keyBody?.key) { + throw new Error(`Failed to create API key: ${keyResp.status} ${JSON.stringify(keyBody)}`); + } + clientAuthHeaders = { Authorization: `Bearer ${keyBody.key}` }; }); test.after(async () => { @@ -348,6 +365,7 @@ test("batch E2E: upload file, create batch, verify rate-limit logs appear", asyn const uploadResp = await fetch(`${app.baseUrl}/api/v1/files`, { method: "POST", + headers: clientAuthHeaders, body: formData, }); assert.match( @@ -362,7 +380,7 @@ test("batch E2E: upload file, create batch, verify rate-limit logs appear", asyn // 2. Create batch via HTTP POST const batchResp = await fetch(`${app.baseUrl}/api/v1/batches`, { method: "POST", - headers: { "Content-Type": "application/json" }, + headers: { "Content-Type": "application/json", ...clientAuthHeaders }, body: JSON.stringify({ input_file_id: fileId, endpoint: "/v1/embeddings", @@ -381,7 +399,9 @@ test("batch E2E: upload file, create batch, verify rate-limit logs appear", asyn while (attempts < maxAttempts) { await sleep(2_000); attempts++; - const sr = await fetch(`${app.baseUrl}/api/v1/batches/${batchId}`); + const sr = await fetch(`${app.baseUrl}/api/v1/batches/${batchId}`, { + headers: clientAuthHeaders, + }); const text = await sr.text(); let sb: BatchResponse; try { @@ -433,7 +453,9 @@ test("batch E2E: upload file, create batch, verify rate-limit logs appear", asyn ); // 5. Verify batch results - const finalResp = await fetch(`${app.baseUrl}/api/v1/batches/${batchId}`); + const finalResp = await fetch(`${app.baseUrl}/api/v1/batches/${batchId}`, { + headers: clientAuthHeaders, + }); const finalBody = await readJsonForTest(finalResp, "Final batch fetch", app); assert.equal( finalBody.request_counts?.completed, diff --git a/tests/integration/files-api-limit-validation.test.ts b/tests/integration/files-api-limit-validation.test.ts index a697e07967..da24604309 100644 --- a/tests/integration/files-api-limit-validation.test.ts +++ b/tests/integration/files-api-limit-validation.test.ts @@ -1,9 +1,25 @@ -import { describe, it } from "node:test"; +import { describe, it, before } from "node:test"; import assert from "node:assert"; -import { createFile, deleteFile } from "@/lib/db/files"; -import { GET, parseFilesListQuery } from "@/app/api/v1/files/route"; + +// `GET /v1/files` fails closed for a caller that is neither an API key nor a +// dashboard session (GHSA-m3hp-hq9g-fpmv), so the HTTP cases below present a +// real key: the subject here is limit validation, not auth. +process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "files-limit-validation-secret"; + +const { createFile, deleteFile } = await import("@/lib/db/files"); +const { createApiKey } = await import("@/lib/db/apiKeys"); +const { GET, parseFilesListQuery } = await import("@/app/api/v1/files/route"); + +let authHeaders: Record = {}; +let apiKeyId = ""; describe("GET /v1/files limit validation", () => { + before(async () => { + const key = await createApiKey("files-limit-validation", "machine-files-limit", []); + apiKeyId = key.id; + authHeaders = { Authorization: `Bearer ${key.key}` }; + }); + it("defaults to 20 when limit is absent", () => { const parsed = parseFilesListQuery(new URLSearchParams("order=asc")); @@ -43,6 +59,7 @@ describe("GET /v1/files limit validation", () => { purpose: "assistants", content: Buffer.from("a"), mimeType: "text/plain", + apiKeyId, }), createFile({ bytes: 1, @@ -50,12 +67,15 @@ describe("GET /v1/files limit validation", () => { purpose: "assistants", content: Buffer.from("b"), mimeType: "text/plain", + apiKeyId, }), ]; try { const response = await GET( - new Request("http://localhost/v1/files?limit=1&purpose=assistants") + new Request("http://localhost/v1/files?limit=1&purpose=assistants", { + headers: authHeaders, + }) ); assert.equal(response.status, 200); const body = await response.json(); @@ -68,10 +88,21 @@ describe("GET /v1/files limit validation", () => { }); it("returns 400 over HTTP for an invalid limit instead of listing files", async () => { - const response = await GET(new Request("http://localhost/v1/files?limit=-1")); + const response = await GET( + new Request("http://localhost/v1/files?limit=-1", { headers: authHeaders }) + ); assert.equal(response.status, 400); const body = await response.json(); assert.equal(body.error.type, "invalid_request_error"); }); + + it("rejects an anonymous list with 401 before the limit is even looked at (GHSA-m3hp-hq9g-fpmv)", async () => { + const response = await GET(new Request("http://localhost/v1/files?limit=1")); + + assert.equal(response.status, 401); + const body = await response.json(); + assert.equal(body.error.message, "Authentication required"); + assert.equal(body.error.type, "authentication_error"); + }); }); 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 0000000000..120d3b19f2 --- /dev/null +++ b/tests/unit/batch-cancel-session-auth-scope.test.ts @@ -0,0 +1,133 @@ +/** + * `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 rule that `batches/[id]/route.ts` (GET/DELETE) and + * `deleteCompletedBatches()` (GHSA-wvxc-jp3v-5mg5) already share: session auth + * is 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 — the rule now + * shared as `canAccessOwnedRecord()` in `_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. The route-level proof (a real session cookie against the + * real handler) lives in tests/unit/files-batches-ownership-2jm2-m3hp.test.ts. + * + * Originally contributed in PR #13683 (@hartmark); folded into the + * GHSA-2jm2-mpx8-6523 / GHSA-m3hp-hq9g-fpmv fix, which subsumes it. + * + * Run with: + * node --import tsx/esm --test tests/unit/batch-cancel-session-auth-scope.test.ts + */ + +import { describe, it, after } from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +// Self-isolating: DATA_DIR points at a fresh temp dir BEFORE any `@/lib/db/*` +// module loads, so this file never touches ~/.omniroute. +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "batch-cancel-session-scope-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const { resetDbInstance } = await import("../../src/lib/db/core.ts"); +const { createFile } = await import("../../src/lib/db/files.ts"); +const { createBatch } = await import("../../src/lib/db/batches.ts"); +const { canAccessOwnedRecord } = await import("../../src/app/api/v1/_helpers/apiKeyScope.ts"); + +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("{}"), + apiKeyId, + }); + return createBatch({ + endpoint: "/v1/chat/completions", + completionWindow: "24h", + inputFileId: file.id, + status, + apiKeyId, + }); +} + +after(() => { + resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); +}); + +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: `!canAccessOwnedRecord(scope, batch.apiKeyId)` + const allowed = canAccessOwnedRecord({ 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 = canAccessOwnedRecord( + { 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 = canAccessOwnedRecord( + { 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 ownership rule 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( + /canAccessOwnedRecord\(\s*scope\s*,\s*batch\.apiKeyId\s*\)/.test(src), + "the route must delegate ownership to the shared canAccessOwnedRecord helper" + ); + }); +}); diff --git a/tests/unit/batch-deletion-route-logic.test.ts b/tests/unit/batch-deletion-route-logic.test.ts index b486145725..e3043fbc58 100644 --- a/tests/unit/batch-deletion-route-logic.test.ts +++ b/tests/unit/batch-deletion-route-logic.test.ts @@ -1,9 +1,18 @@ import { test } from "node:test"; import assert from "node:assert"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; // Tests for the business logic embedded in DELETE route handlers. // These verify every code path without importing Next.js route modules -// (which pull in pino/thread-stream — broken on Node 26). +// (which pull in pino/thread-stream — broken on Node 26). The ownership rule is +// the REAL shared helper, not a local copy: a copy drifted from production once +// (v3.8.4 tightened the copy, production stayed open — GHSA-2jm2-mpx8-6523). +// The helper's module pulls in the DB layer, so isolate DATA_DIR before it loads. +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "batch-deletion-route-logic-")); +process.env.DATA_DIR = TEST_DATA_DIR; +const { canAccessOwnedRecord } = await import("../../src/app/api/v1/_helpers/apiKeyScope.ts"); const TERMINAL = ["completed", "failed", "cancelled", "expired"]; @@ -12,15 +21,17 @@ function scopeCheck( recordApiKeyId: string | null | undefined, apiKeyId: string | null ): boolean { - if (isSessionAuth) return true; - if (recordApiKeyId === null || recordApiKeyId === undefined) return apiKeyId !== null; - return recordApiKeyId === apiKeyId; + return canAccessOwnedRecord({ isSessionAuth, apiKeyId }, recordApiKeyId); } function canDeleteBatch(status: string): boolean { return TERMINAL.includes(status); } +test.after(() => { + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); +}); + test("scopeCheck — session auth always passes", () => { assert.strictEqual(scopeCheck(true, "key-1", "key-1"), true); assert.strictEqual(scopeCheck(true, "key-1", "different-key"), true); @@ -28,11 +39,11 @@ test("scopeCheck — session auth always passes", () => { assert.strictEqual(scopeCheck(true, undefined, null), true); }); -test("scopeCheck — null record ApiKeyId requires an authenticated API key", () => { - assert.strictEqual(scopeCheck(false, null, null), false); - assert.strictEqual(scopeCheck(false, null, "any-key"), true); - assert.strictEqual(scopeCheck(false, undefined, null), false); - assert.strictEqual(scopeCheck(false, undefined, "any-key"), true); +test("scopeCheck — a null-owner record is denied to every non-session caller (GHSA-2jm2-mpx8-6523)", () => { + assert.strictEqual(scopeCheck(false, null, null), false, "anonymous"); + assert.strictEqual(scopeCheck(false, null, "any-key"), false, "any authenticated key"); + assert.strictEqual(scopeCheck(false, undefined, null), false, "anonymous, undefined owner"); + assert.strictEqual(scopeCheck(false, undefined, "any-key"), false, "any key, undefined owner"); }); test("scopeCheck — matching apiKeyId passes", () => { diff --git a/tests/unit/batch_api.test.ts b/tests/unit/batch_api.test.ts index ce765a8e12..fa65b1e952 100644 --- a/tests/unit/batch_api.test.ts +++ b/tests/unit/batch_api.test.ts @@ -815,7 +815,7 @@ test("Files and batches routes expose explicit CORS preflight handlers", async ( } }); -test("Batch by-id route exposes ownerless records to anonymous requests", async () => { +test("Batch by-id route hides ownerless records from anonymous requests (GHSA-2jm2-mpx8-6523)", async () => { const file = createFile({ bytes: 2, filename: "ownerless.jsonl", @@ -830,15 +830,18 @@ test("Batch by-id route exposes ownerless records to anonymous requests", async apiKeyId: null, }); + // A null owner is unattributable: only the operator's dashboard session may + // read it. An anonymous caller (no key, no session) gets the same 404 a + // foreign key gets — never the record. const response = await batchByIdRoute.GET( new Request(`http://localhost/api/v1/batches/${batch.id}`), { params: Promise.resolve({ id: batch.id }) } ); const body = await response.json(); - assert.strictEqual(response.status, 200); - assert.strictEqual(body.id, batch.id); - assert.strictEqual(body.status, "validating"); + assert.strictEqual(response.status, 404); + assert.strictEqual(body.error?.message, "Batch not found"); + assert.strictEqual(body.id, undefined, "the ownerless record must not be returned"); }); test("Batch Cancel API", async () => { diff --git a/tests/unit/files-batches-ownership-2jm2-m3hp.test.ts b/tests/unit/files-batches-ownership-2jm2-m3hp.test.ts new file mode 100644 index 0000000000..c5f4d183ff --- /dev/null +++ b/tests/unit/files-batches-ownership-2jm2-m3hp.test.ts @@ -0,0 +1,490 @@ +/** + * GHSA-2jm2-mpx8-6523 + GHSA-m3hp-hq9g-fpmv — route-level regression guard for the + * `/api/v1/files` and `/api/v1/batches` ownership model. + * + * Both advisories share one root cause: `getApiKeyRequestScope` resolves three + * different callers to the SAME `{ apiKeyId: null, isSessionAuth: false }` shape — + * an anonymous request, a request presenting an invalid/rotated bearer, and (with + * `isSessionAuth: true`) the operator's dashboard session — and the routes then + * treated "no key" as "no restriction": + * + * - the list routes coerced `apiKeyId || undefined`, which the DB layer reads as + * "instance-wide" — every tenant's file and batch metadata to an anonymous + * caller (GHSA-m3hp); + * - the single-item routes short-circuited to ALLOW when the record's own + * `api_key_id` was null, so a null-owner file (dashboard upload, anonymous + * upload, batch output inheriting a null owner) was readable, downloadable and + * deletable by anybody, and a foreign key could run a batch over it (GHSA-2jm2). + * + * The fix is one shared 3-way rule (`canAccessOwnedRecord` in + * `_helpers/apiKeyScope.ts`): a dashboard session is the instance operator and may + * act on any record; an API key may act on its own records only; a null-owner + * record is unattributable and is denied to every non-session caller. The list + * routes apply the same explicit 3-way scope as `delete-completed` and fail closed + * with a `buildErrorBody()` 401 when the caller is neither a key nor a session. + * + * Modelled on tests/unit/batches-delete-completed-route-scope.test.ts: drives the + * REAL route handlers with REAL credentials (API keys via `createApiKey`, a dashboard + * session via a signed `auth_token` cookie). Self-isolating: DATA_DIR points at a + * fresh temp dir BEFORE any `@/lib/db/*` module loads, so this file never touches + * ~/.omniroute. + */ +import { describe, it, after } from "node:test"; +import assert from "node:assert"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { SignJWT } from "jose"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "ownership-2jm2-m3hp-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "ownership-2jm2-api-secret"; +process.env.JWT_SECRET = "ownership-2jm2-jwt-secret"; + +const { resetDbInstance } = await import("../../src/lib/db/core.ts"); +const { createApiKey } = await import("../../src/lib/db/apiKeys.ts"); +const { createFile, getFile, getFileContent } = await import("../../src/lib/db/files.ts"); +const { createBatch, getBatch } = await import("../../src/lib/db/batches.ts"); +const { canAccessOwnedRecord } = await import("../../src/app/api/v1/_helpers/apiKeyScope.ts"); +const filesRoute = await import("../../src/app/api/v1/files/route.ts"); +const fileByIdRoute = await import("../../src/app/api/v1/files/[id]/route.ts"); +const fileContentRoute = await import("../../src/app/api/v1/files/[id]/content/route.ts"); +const batchesRoute = await import("../../src/app/api/v1/batches/route.ts"); +const batchByIdRoute = await import("../../src/app/api/v1/batches/[id]/route.ts"); +const batchCancelRoute = await import("../../src/app/api/v1/batches/[id]/cancel/route.ts"); + +type Headers = Record; +type ErrorBody = { error?: { message: string; type?: string; code?: string } }; +type ListBody = ErrorBody & { object?: string; data?: Array<{ id: string }>; total_count?: number }; + +async function sessionCookie(): Promise { + const secret = new TextEncoder().encode(process.env.JWT_SECRET); + const jwt = await new SignJWT({ authenticated: true, sub: "admin" }) + .setProtectedHeader({ alg: "HS256" }) + .setExpirationTime("1h") + .sign(secret); + return `auth_token=${jwt}`; +} + +function seedFile(apiKeyId: string | null, label: string) { + return createFile({ + bytes: label.length, + filename: `${label}.jsonl`, + purpose: "batch", + content: Buffer.from(label), + mimeType: "application/jsonl", + apiKeyId, + }); +} + +function seedBatch( + apiKeyId: string | null, + label: string, + status: "validating" | "completed" = "validating" +) { + const file = seedFile(apiKeyId, label); + const batch = createBatch({ + endpoint: "/v1/chat/completions", + completionWindow: "24h", + inputFileId: file.id, + status, + apiKeyId, + }); + return { file, batch }; +} + +const params = (id: string) => ({ params: Promise.resolve({ id }) }); + +async function listFilesVia(headers: Headers) { + const res = await filesRoute.GET( + new Request("http://localhost/api/v1/files?limit=100", { headers }) + ); + return { res, body: (await res.json()) as ListBody }; +} + +async function listBatchesVia(headers: Headers) { + const res = await batchesRoute.GET( + new Request("http://localhost/api/v1/batches?limit=100", { headers }) + ); + return { res, body: (await res.json()) as ListBody }; +} + +async function getFileVia(headers: Headers, id: string) { + return fileByIdRoute.GET( + new Request(`http://localhost/api/v1/files/${id}`, { headers }), + params(id) + ); +} + +async function getFileContentVia(headers: Headers, id: string) { + return fileContentRoute.GET( + new Request(`http://localhost/api/v1/files/${id}/content`, { headers }), + params(id) + ); +} + +async function deleteFileVia(headers: Headers, id: string) { + return fileByIdRoute.DELETE( + new Request(`http://localhost/api/v1/files/${id}`, { method: "DELETE", headers }), + params(id) + ); +} + +async function getBatchVia(headers: Headers, id: string) { + return batchByIdRoute.GET( + new Request(`http://localhost/api/v1/batches/${id}`, { headers }), + params(id) + ); +} + +async function deleteBatchVia(headers: Headers, id: string) { + return batchByIdRoute.DELETE( + new Request(`http://localhost/api/v1/batches/${id}`, { method: "DELETE", headers }), + params(id) + ); +} + +async function cancelBatchVia(headers: Headers, id: string) { + return batchCancelRoute.POST( + new Request(`http://localhost/api/v1/batches/${id}/cancel`, { method: "POST", headers }), + params(id) + ); +} + +async function createBatchVia(headers: Headers, inputFileId: string) { + const res = await batchesRoute.POST( + new Request("http://localhost/api/v1/batches", { + method: "POST", + headers: { "Content-Type": "application/json", ...headers }, + body: JSON.stringify({ + input_file_id: inputFileId, + endpoint: "/v1/chat/completions", + completion_window: "24h", + }), + }) + ); + return { res, body: (await res.json()) as ErrorBody & { id?: string } }; +} + +function assertAuthRequired401(res: Response, body: ErrorBody, label: string) { + assert.strictEqual(res.status, 401, `${label}: anonymous caller must be rejected`); + assert.strictEqual(body.error?.message, "Authentication required", label); + assert.strictEqual(body.error?.type, "authentication_error", `${label}: buildErrorBody() shape`); + assert.strictEqual(body.error?.code, "invalid_api_key", label); + assert.ok(!body.error?.message.includes("at /"), `${label}: stack trace leaked`); +} + +function assertInvalidKey401(res: Response, body: ErrorBody, label: string) { + assert.strictEqual(res.status, 401, `${label}: an unresolvable bearer must fail closed`); + assert.strictEqual(body.error?.message, "Invalid API key", label); + assert.strictEqual(body.error?.type, "authentication_error", `${label}: buildErrorBody() shape`); + assert.ok(!body.error?.message.includes("at /"), `${label}: stack trace leaked`); +} + +describe("canAccessOwnedRecord — the shared 3-way ownership rule", () => { + it("a dashboard session may act on any record, owned or not", () => { + assert.strictEqual( + canAccessOwnedRecord({ isSessionAuth: true, apiKeyId: null }, "key-1"), + true + ); + assert.strictEqual(canAccessOwnedRecord({ isSessionAuth: true, apiKeyId: "k" }, "key-1"), true); + assert.strictEqual(canAccessOwnedRecord({ isSessionAuth: true, apiKeyId: null }, null), true); + assert.strictEqual( + canAccessOwnedRecord({ isSessionAuth: true, apiKeyId: null }, undefined), + true + ); + }); + + it("a null-owner record is denied to every non-session caller — anonymous AND any key", () => { + assert.strictEqual(canAccessOwnedRecord({ isSessionAuth: false, apiKeyId: null }, null), false); + assert.strictEqual(canAccessOwnedRecord({ isSessionAuth: false, apiKeyId: "k" }, null), false); + assert.strictEqual( + canAccessOwnedRecord({ isSessionAuth: false, apiKeyId: null }, undefined), + false + ); + assert.strictEqual( + canAccessOwnedRecord({ isSessionAuth: false, apiKeyId: "k" }, undefined), + false + ); + }); + + it("a key may act on its own records only", () => { + assert.strictEqual(canAccessOwnedRecord({ isSessionAuth: false, apiKeyId: "k1" }, "k1"), true); + assert.strictEqual(canAccessOwnedRecord({ isSessionAuth: false, apiKeyId: "k2" }, "k1"), false); + assert.strictEqual(canAccessOwnedRecord({ isSessionAuth: false, apiKeyId: null }, "k1"), false); + }); +}); + +after(() => { + resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); +}); + +describe("GET /api/v1/files + GET /api/v1/batches — caller scope (GHSA-m3hp-hq9g-fpmv)", () => { + it("(a) no credential at all → 401 on both lists, nothing enumerated", async () => { + const keyA = await createApiKey("m3hp-a-key", "machine-m3hp-a", []); + seedBatch(keyA.id, "m3hp-a-victim"); + + const files = await listFilesVia({}); + assertAuthRequired401(files.res, files.body, "GET /v1/files"); + assert.strictEqual(files.body.data, undefined, "no file rows in a 401 body"); + + const batches = await listBatchesVia({}); + assertAuthRequired401(batches.res, batches.body, "GET /v1/batches"); + assert.strictEqual(batches.body.data, undefined, "no batch rows in a 401 body"); + }); + + it("(b) an invalid/rotated bearer → 401 on both lists — even alongside a session cookie", async () => { + const keyA = await createApiKey("m3hp-b-key", "machine-m3hp-b", []); + seedBatch(keyA.id, "m3hp-b-victim"); + const bogus = { Authorization: "Bearer sk-omni-this-key-was-rotated-away-m3hp" }; + + const files = await listFilesVia(bogus); + assertInvalidKey401(files.res, files.body, "GET /v1/files"); + const batches = await listBatchesVia(bogus); + assertInvalidKey401(batches.res, batches.body, "GET /v1/batches"); + + const withSession = { ...bogus, cookie: await sessionCookie() }; + const files2 = await listFilesVia(withSession); + assertInvalidKey401(files2.res, files2.body, "GET /v1/files + session cookie"); + const batches2 = await listBatchesVia(withSession); + assertInvalidKey401(batches2.res, batches2.body, "GET /v1/batches + session cookie"); + }); + + it("(c) key A lists only A's rows — B's and null-owner rows never appear", async () => { + const keyA = await createApiKey("m3hp-c-key-a", "machine-m3hp-ca", []); + const keyB = await createApiKey("m3hp-c-key-b", "machine-m3hp-cb", []); + const own = seedBatch(keyA.id, "m3hp-c-own"); + const other = seedBatch(keyB.id, "m3hp-c-other"); + const unowned = seedBatch(null, "m3hp-c-unowned"); + const headers = { Authorization: `Bearer ${keyA.key}` }; + + const files = await listFilesVia(headers); + assert.strictEqual(files.res.status, 200); + const fileIds = new Set(files.body.data!.map((f) => f.id)); + assert.ok(fileIds.has(own.file.id), "key A sees its own file"); + assert.ok(!fileIds.has(other.file.id), "key B's file must not leak to key A"); + assert.ok(!fileIds.has(unowned.file.id), "the null-owner file must not leak to key A"); + assert.strictEqual(files.body.total_count, files.body.data!.length); + assert.ok(files.body.data!.every((f) => getFile(f.id)?.apiKeyId === keyA.id)); + + const batches = await listBatchesVia(headers); + assert.strictEqual(batches.res.status, 200); + const batchIds = new Set(batches.body.data!.map((b) => b.id)); + assert.ok(batchIds.has(own.batch.id), "key A sees its own batch"); + assert.ok(!batchIds.has(other.batch.id), "key B's batch must not leak to key A"); + assert.ok(!batchIds.has(unowned.batch.id), "the null-owner batch must not leak to key A"); + assert.strictEqual(batches.body.total_count, batches.body.data!.length); + assert.ok(batches.body.data!.every((b) => getBatch(b.id)?.apiKeyId === keyA.id)); + }); + + it("(d) a dashboard session WITHOUT a key lists the whole instance", async () => { + const keyA = await createApiKey("m3hp-d-key-a", "machine-m3hp-da", []); + const keyB = await createApiKey("m3hp-d-key-b", "machine-m3hp-db", []); + const a = seedBatch(keyA.id, "m3hp-d-a"); + const b = seedBatch(keyB.id, "m3hp-d-b"); + const unowned = seedBatch(null, "m3hp-d-unowned"); + const headers = { cookie: await sessionCookie() }; + + const files = await listFilesVia(headers); + assert.strictEqual(files.res.status, 200); + const fileIds = new Set(files.body.data!.map((f) => f.id)); + for (const f of [a.file, b.file, unowned.file]) { + assert.ok(fileIds.has(f.id), `session sees ${f.filename}`); + } + + const batches = await listBatchesVia(headers); + assert.strictEqual(batches.res.status, 200); + const batchIds = new Set(batches.body.data!.map((x) => x.id)); + for (const x of [a.batch, b.batch, unowned.batch]) { + assert.ok(batchIds.has(x.id), `session sees batch ${x.id}`); + } + }); + + it("(e) a request carrying BOTH a session cookie and key A stays scoped to key A (the key wins)", async () => { + const keyA = await createApiKey("m3hp-e-key-a", "machine-m3hp-ea", []); + const keyB = await createApiKey("m3hp-e-key-b", "machine-m3hp-eb", []); + const own = seedBatch(keyA.id, "m3hp-e-own"); + const other = seedBatch(keyB.id, "m3hp-e-other"); + const unowned = seedBatch(null, "m3hp-e-unowned"); + const headers = { Authorization: `Bearer ${keyA.key}`, cookie: await sessionCookie() }; + + const files = await listFilesVia(headers); + assert.strictEqual(files.res.status, 200); + const fileIds = new Set(files.body.data!.map((f) => f.id)); + assert.ok(fileIds.has(own.file.id)); + assert.ok(!fileIds.has(other.file.id), "a session cookie never widens a key's file list"); + assert.ok(!fileIds.has(unowned.file.id)); + + const batches = await listBatchesVia(headers); + assert.strictEqual(batches.res.status, 200); + const batchIds = new Set(batches.body.data!.map((b) => b.id)); + assert.ok(batchIds.has(own.batch.id)); + assert.ok(!batchIds.has(other.batch.id), "a session cookie never widens a key's batch list"); + assert.ok(!batchIds.has(unowned.batch.id)); + }); +}); + +describe("single-item routes — null-owner records (GHSA-2jm2-mpx8-6523)", () => { + it("(f) files: a null-owner file is 404 (metadata, content, delete) for a foreign key and for anonymous; 200 for a session", async () => { + const keyB = await createApiKey("2jm2-f-key-b", "machine-2jm2-fb", []); + const file = seedFile(null, "2jm2-f-null-owner"); + const foreign = { Authorization: `Bearer ${keyB.key}` }; + const anon = {}; + + for (const [label, headers] of [ + ["foreign key", foreign], + ["anonymous", anon], + ] as const) { + const meta = await getFileVia(headers, file.id); + assert.strictEqual(meta.status, 404, `${label}: GET /v1/files/{id} on a null-owner file`); + + const content = await getFileContentVia(headers, file.id); + assert.strictEqual(content.status, 404, `${label}: GET /v1/files/{id}/content`); + const contentBody = (await content.json()) as ErrorBody; + assert.strictEqual(contentBody.error?.message, "File not found", label); + + const del = await deleteFileVia(headers, file.id); + assert.strictEqual(del.status, 404, `${label}: DELETE /v1/files/{id}`); + assert.ok(getFile(file.id), `${label}: the null-owner file must survive`); + assert.strictEqual( + getFileContent(file.id)?.toString(), + "2jm2-f-null-owner", + `${label}: the null-owner file content must not be nulled` + ); + } + + const session = { cookie: await sessionCookie() }; + const meta = await getFileVia(session, file.id); + assert.strictEqual(meta.status, 200, "session: GET /v1/files/{id} on a null-owner file"); + const content = await getFileContentVia(session, file.id); + assert.strictEqual(content.status, 200, "session: GET /v1/files/{id}/content"); + assert.strictEqual(await content.text(), "2jm2-f-null-owner"); + const del = await deleteFileVia(session, file.id); + assert.strictEqual(del.status, 200, "session: DELETE /v1/files/{id}"); + assert.strictEqual(getFile(file.id), null, "session delete takes effect"); + }); + + it("(f) files: key-owned files keep the owner-only rule — owner 200, foreign key 404, anonymous 404, session 200", async () => { + const keyA = await createApiKey("2jm2-f2-key-a", "machine-2jm2-f2a", []); + const keyB = await createApiKey("2jm2-f2-key-b", "machine-2jm2-f2b", []); + const file = seedFile(keyA.id, "2jm2-f2-owned"); + + assert.strictEqual( + (await getFileVia({ Authorization: `Bearer ${keyA.key}` }, file.id)).status, + 200 + ); + assert.strictEqual( + (await getFileVia({ Authorization: `Bearer ${keyB.key}` }, file.id)).status, + 404 + ); + assert.strictEqual((await getFileVia({}, file.id)).status, 404); + assert.strictEqual((await getFileVia({ cookie: await sessionCookie() }, file.id)).status, 200); + assert.strictEqual( + (await getFileContentVia({ Authorization: `Bearer ${keyB.key}` }, file.id)).status, + 404 + ); + assert.strictEqual((await deleteFileVia({}, file.id)).status, 404); + assert.ok(getFile(file.id), "an anonymous delete on a key-owned file is a no-op"); + }); + + it("(f) batches: a null-owner batch is 404 (get, delete, cancel) for a foreign key and for anonymous; 200 for a session", async () => { + const keyB = await createApiKey("2jm2-fb-key-b", "machine-2jm2-fbb", []); + const terminal = seedBatch(null, "2jm2-fb-null-terminal", "completed"); + const live = seedBatch(null, "2jm2-fb-null-live", "validating"); + const foreign = { Authorization: `Bearer ${keyB.key}` }; + + for (const [label, headers] of [ + ["foreign key", foreign], + ["anonymous", {}], + ] as const) { + assert.strictEqual( + (await getBatchVia(headers, terminal.batch.id)).status, + 404, + `${label}: GET /v1/batches/{id} on a null-owner batch` + ); + assert.strictEqual( + (await deleteBatchVia(headers, terminal.batch.id)).status, + 404, + `${label}: DELETE /v1/batches/{id} on a null-owner batch` + ); + assert.ok(getBatch(terminal.batch.id), `${label}: the null-owner batch must survive`); + assert.ok(getFile(terminal.file.id), `${label}: its input file must survive`); + assert.strictEqual( + (await cancelBatchVia(headers, live.batch.id)).status, + 404, + `${label}: POST /v1/batches/{id}/cancel on a null-owner batch` + ); + assert.strictEqual(getBatch(live.batch.id)?.status, "validating", `${label}: not cancelled`); + } + + const session = { cookie: await sessionCookie() }; + assert.strictEqual((await getBatchVia(session, terminal.batch.id)).status, 200); + assert.strictEqual((await cancelBatchVia(session, live.batch.id)).status, 200); + assert.strictEqual( + getBatch(live.batch.id)?.status, + "cancelling", + "session cancel takes effect" + ); + assert.strictEqual((await deleteBatchVia(session, terminal.batch.id)).status, 200); + assert.strictEqual(getBatch(terminal.batch.id), null, "session delete takes effect"); + }); + + it("(g) POST /api/v1/batches: a foreign key or an anonymous caller cannot run a batch over a null-owner input file; the owner and a session can", async () => { + const keyA = await createApiKey("2jm2-g-key-a", "machine-2jm2-ga", []); + const keyB = await createApiKey("2jm2-g-key-b", "machine-2jm2-gb", []); + const unownedInput = seedFile(null, "2jm2-g-null-input"); + const ownedInput = seedFile(keyA.id, "2jm2-g-owned-input"); + + for (const [label, headers] of [ + ["foreign key", { Authorization: `Bearer ${keyB.key}` }], + ["anonymous", {}], + ] as const) { + const { res, body } = await createBatchVia(headers, unownedInput.id); + assert.strictEqual(res.status, 400, `${label}: batch over a null-owner input file`); + assert.strictEqual(body.error?.message, "Input file not found", label); + assert.strictEqual(body.id, undefined, `${label}: no batch created`); + } + + // Key B still cannot use key A's file (the pre-existing owner rule). + const foreignOwned = await createBatchVia( + { Authorization: `Bearer ${keyB.key}` }, + ownedInput.id + ); + assert.strictEqual(foreignOwned.res.status, 400, "key B over key A's input file"); + + // The owner can. + const owner = await createBatchVia({ Authorization: `Bearer ${keyA.key}` }, ownedInput.id); + assert.strictEqual(owner.res.status, 200, "key A over its own input file"); + assert.strictEqual(getBatch(owner.body.id!)?.apiKeyId, keyA.id); + + // The operator's session can — over the null-owner file AND over a key-owned one. + const session = { cookie: await sessionCookie() }; + const sessionUnowned = await createBatchVia(session, unownedInput.id); + assert.strictEqual(sessionUnowned.res.status, 200, "session over the null-owner input file"); + const sessionOwned = await createBatchVia(session, ownedInput.id); + assert.strictEqual(sessionOwned.res.status, 200, "session over key A's input file"); + }); + + it("(h) POST /api/v1/batches/{id}/cancel: a dashboard session cancels a KEY-owned batch (#13683); the owner can; a foreign key cannot", async () => { + const keyA = await createApiKey("2jm2-h-key-a", "machine-2jm2-ha", []); + const keyB = await createApiKey("2jm2-h-key-b", "machine-2jm2-hb", []); + const bySession = seedBatch(keyA.id, "2jm2-h-session", "validating"); + const byOwner = seedBatch(keyA.id, "2jm2-h-owner", "validating"); + + assert.strictEqual( + (await cancelBatchVia({ Authorization: `Bearer ${keyB.key}` }, bySession.batch.id)).status, + 404, + "a foreign key cannot cancel key A's batch" + ); + assert.strictEqual(getBatch(bySession.batch.id)?.status, "validating"); + + const session = await cancelBatchVia({ cookie: await sessionCookie() }, bySession.batch.id); + assert.strictEqual(session.status, 200, "the operator's dashboard cancels any batch"); + assert.strictEqual(getBatch(bySession.batch.id)?.status, "cancelling"); + + const owner = await cancelBatchVia({ Authorization: `Bearer ${keyA.key}` }, byOwner.batch.id); + assert.strictEqual(owner.status, 200, "the owning key cancels its own batch"); + assert.strictEqual(getBatch(byOwner.batch.id)?.status, "cancelling"); + }); +});