mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-17 20:32:25 +03:00
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 <mail@hartmark.se>
109 lines
3.7 KiB
TypeScript
109 lines
3.7 KiB
TypeScript
import { describe, it, before } from "node:test";
|
|
import assert from "node:assert";
|
|
|
|
// `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<string, string> = {};
|
|
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"));
|
|
|
|
assert.equal(parsed.ok, true);
|
|
if (!parsed.ok) return;
|
|
assert.equal(parsed.limit, 20);
|
|
});
|
|
|
|
it("parses an explicit positive integer limit", () => {
|
|
const parsed = parseFilesListQuery(new URLSearchParams("limit=2&order=asc&purpose=batch"));
|
|
|
|
assert.equal(parsed.ok, true);
|
|
if (!parsed.ok) return;
|
|
assert.equal(parsed.limit, 2);
|
|
assert.equal(parsed.order, "asc");
|
|
assert.equal(parsed.purpose, "batch");
|
|
});
|
|
|
|
it("rejects non-integer, zero, and oversized limits", async () => {
|
|
for (const rawLimit of ["abc", "1.5", "-1", "0", "10001"]) {
|
|
const parsed = parseFilesListQuery(
|
|
new URLSearchParams(`limit=${encodeURIComponent(rawLimit)}`)
|
|
);
|
|
assert.equal(parsed.ok, false, `limit=${rawLimit} should be rejected`);
|
|
if (parsed.ok) continue;
|
|
assert.equal(parsed.response.status, 400);
|
|
const body = await parsed.response.json();
|
|
assert.equal(body.error.type, "invalid_request_error");
|
|
}
|
|
});
|
|
|
|
it("returns only the requested number of files over HTTP", async () => {
|
|
const created = [
|
|
createFile({
|
|
bytes: 1,
|
|
filename: "test-files-limit-http-a.txt",
|
|
purpose: "assistants",
|
|
content: Buffer.from("a"),
|
|
mimeType: "text/plain",
|
|
apiKeyId,
|
|
}),
|
|
createFile({
|
|
bytes: 1,
|
|
filename: "test-files-limit-http-b.txt",
|
|
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", {
|
|
headers: authHeaders,
|
|
})
|
|
);
|
|
assert.equal(response.status, 200);
|
|
const body = await response.json();
|
|
assert.equal(body.object, "list");
|
|
assert.equal(body.data.length, 1);
|
|
assert.equal(body.has_more, true);
|
|
} finally {
|
|
for (const file of created) deleteFile(file.id);
|
|
}
|
|
});
|
|
|
|
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", { 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");
|
|
});
|
|
});
|