From 0fb4eb6878297d38ad385364b18ee99d784bee91 Mon Sep 17 00:00:00 2001 From: Xiangzhe Date: Sun, 23 Aug 2026 12:50:04 -0300 Subject: [PATCH] fix(security): A2A REST auth + task owner scoping (GHSA-jcm5-6wpp-wjj8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The REST task routes (/api/a2a/tasks, /api/a2a/tasks/[id], /[id]/cancel) had NO authentication call at all — open regardless of configuration — and the task manager stored tasks in an owner-less Map, so any caller could read or cancel any task by id over either the JSON-RPC or the REST surface. - New shared src/lib/a2a/authenticate.ts (the v54m JSON-RPC posture, lifted so both surfaces cannot drift) + src/app/api/a2a/_auth.ts implementing the full posture matrix: REQUIRE_API_KEY=true demands a valid key (management session also passes via alwaysRequireAuth); requireLogin=true accepts management or a valid key; the keyless local-first default stays open by design. - Tasks bind to an owner (hashed API key) at creation; get/cancel/list are owner-scoped. Another principal's task answers with the same not-found a missing one would (no existence oracle). Ownerless tasks (keyless posture) stay visible to everyone; management/operator view sees all tasks. - Callers discriminate the auth failure with instanceof Response, never instanceof NextResponse — createErrorResponse() returns a plain Response, which silently fell through to the handler (caught by the 401-vs-404 test). --- src/app/a2a/route.ts | 49 ++------ src/app/api/a2a/_auth.ts | 51 ++++++++ src/app/api/a2a/tasks/[id]/cancel/route.ts | 15 ++- src/app/api/a2a/tasks/[id]/route.ts | 21 +++- src/app/api/a2a/tasks/route.ts | 27 +++- src/lib/a2a/authenticate.ts | 53 ++++++++ src/lib/a2a/taskManager.ts | 40 +++++- tests/unit/a2a-task-owner-idor.test.ts | 136 +++++++++++++++++++++ tests/unit/a2a-tasks-auth.test.ts | 12 +- 9 files changed, 345 insertions(+), 59 deletions(-) create mode 100644 src/app/api/a2a/_auth.ts create mode 100644 src/lib/a2a/authenticate.ts create mode 100644 tests/unit/a2a-task-owner-idor.test.ts diff --git a/src/app/a2a/route.ts b/src/app/a2a/route.ts index af7d93a2e5..dfe3fc73a7 100644 --- a/src/app/a2a/route.ts +++ b/src/app/a2a/route.ts @@ -10,15 +10,13 @@ * Auth: Bearer token via Authorization header */ -import { timingSafeEqual } from "node:crypto"; import { NextRequest, NextResponse } from "next/server"; import { getTaskManager } from "@/lib/a2a/taskManager"; import { logRoutingDecision } from "@/lib/a2a/routingLogger"; import { createA2AStream, SSE_HEADERS } from "@/lib/a2a/streaming"; import { A2A_SKILL_HANDLERS, executeA2ATaskWithState } from "@/lib/a2a/taskExecution"; import { getSettings } from "@/lib/db/settings"; -import { isRequireApiKeyEnabled } from "@/shared/utils/featureFlags"; -import { extractApiKey, isValidApiKey } from "@/sse/services/auth"; +import { authenticateA2ARequest, resolveA2AOwner } from "@/lib/a2a/authenticate"; // ============ A2A v1.0 ↔ v0.3 compatibility layer ============ // A2A 1.0 renamed the JSON-RPC methods (message/send → SendMessage, @@ -55,7 +53,7 @@ function buildV1Task( ? result.artifacts .map((a) => a && typeof a === "object" && typeof (a as { content?: unknown }).content === "string" - ? ((a as { content: string }).content) + ? (a as { content: string }).content : "" ) .filter((s) => s.length > 0) @@ -124,39 +122,13 @@ function toMessageArray(raw: unknown): A2AMessage[] | null { // ============ Auth ============ -/** - * Constant-time comparison of the presented bearer token against the configured - * key. A plain `===` short-circuits on the first differing byte, leaking the - * length of the shared prefix through response timing; `timingSafeEqual` does - * not. It requires equal-length buffers, so mismatched lengths are rejected up - * front (the length itself is not secret). - */ -function tokensMatch(provided: string, expected: string): boolean { - const a = Buffer.from(provided); - const b = Buffer.from(expected); - if (a.length !== b.length) return false; - return timingSafeEqual(a, b); -} - async function authenticate(req: NextRequest): Promise { // /a2a is outside the authz proxy matcher, so the REQUIRE_API_KEY posture the // pipeline enforces for /v1 never ran here — the route accepted every caller // whenever OMNIROUTE_API_KEY was unset, which is the shipped default - // (GHSA-v54m-6rm3-p565). Apply the same posture directly: when a client key is - // required, demand a valid OmniRoute key; otherwise honor the legacy explicit - // A2A key; otherwise stay keyless (the same local-first default as /v1). - const apiKey = extractApiKey(req); - if (isRequireApiKeyEnabled()) { - return apiKey ? await isValidApiKey(apiKey) : false; - } - - const configuredKey = process.env.OMNIROUTE_API_KEY; - if (configuredKey) { - return apiKey ? tokensMatch(apiKey, configuredKey) : false; - } - - // No API key required and none configured — allow (keyless local-first). - return true; + // (GHSA-v54m-6rm3-p565). The shared helper applies the same posture on both + // the JSON-RPC and the REST task surfaces (GHSA-jcm5-6wpp-wjj8). + return authenticateA2ARequest(req); } // ============ JSON-RPC Helpers ============ @@ -213,6 +185,9 @@ export async function POST(req: NextRequest) { if (disabledResponse) return disabledResponse; const tm = getTaskManager(); + // GHSA-jcm5-6wpp-wjj8: scope every task read/mutation below to the caller's + // owner id (hashed API key; undefined under the keyless local-first posture). + const callerOwner = resolveA2AOwner(req); // A2A 1.0 method-name compatibility (SendMessage → message/send, etc.) const isV1Method = method in V1_METHOD_ALIASES; @@ -236,7 +211,7 @@ export async function POST(req: NextRequest) { return jsonRpcError(id, -32601, `Unknown skill: ${skill}`); } - const task = tm.createTask({ skill, messages, metadata: params?.metadata }); + const task = tm.createTask({ skill, messages, metadata: params?.metadata }, callerOwner); try { tm.updateTask(task.id, "working"); const result = await handler(task); @@ -302,7 +277,7 @@ export async function POST(req: NextRequest) { return jsonRpcError(id, -32601, `Unknown skill: ${skill}`); } - const task = tm.createTask({ skill, messages, metadata: params?.metadata }); + const task = tm.createTask({ skill, messages, metadata: params?.metadata }, callerOwner); tm.updateTask(task.id, "working"); const stream = createA2AStream( @@ -323,7 +298,7 @@ export async function POST(req: NextRequest) { const taskId = params?.taskId || params?.id; if (!taskId) return jsonRpcError(id, -32602, "Invalid params: taskId required"); - const task = tm.getTask(taskId); + const task = tm.getTask(taskId, callerOwner); if (!task) return jsonRpcError(id, -32601, `Task not found: ${taskId}`); return jsonRpcResult(id, { task }); @@ -335,7 +310,7 @@ export async function POST(req: NextRequest) { if (!taskId) return jsonRpcError(id, -32602, "Invalid params: taskId required"); try { - const task = tm.cancelTask(taskId); + const task = tm.cancelTask(taskId, callerOwner); return jsonRpcResult(id, { task: { id: task.id, state: task.state } }); } catch (err) { const msg = err instanceof Error ? err.message : String(err); diff --git a/src/app/api/a2a/_auth.ts b/src/app/api/a2a/_auth.ts new file mode 100644 index 0000000000..2ec286db91 --- /dev/null +++ b/src/app/api/a2a/_auth.ts @@ -0,0 +1,51 @@ +/** + * Shared authorization for the REST A2A task routes (GHSA-jcm5-6wpp-wjj8). + * + * Dual audience: the dashboard calls these routes with a management session, + * A2A clients with an inference API key. Posture matrix: + * + * - REQUIRE_API_KEY=true: a valid OmniRoute key is mandatory (the same + * posture the /v1 inference plane enforces); a management session also + * passes (dashboard), via alwaysRequireAuth so requireLogin=false cannot + * bypass it. + * - otherwise + requireLogin=true: management session, or a valid key. + * - otherwise + requireLogin=false (local-first default): open, by design. + * + * Callers authenticated by key are owner-scoped — another principal's tasks + * answer as if they did not exist. Management/operator view sees all tasks. + */ + +import { requireManagementAuth } from "@/lib/api/requireManagementAuth"; +import { extractApiKey, isValidApiKey } from "@/sse/services/auth"; +import { isRequireApiKeyEnabled } from "@/shared/utils/featureFlags"; +import { resolveA2AOwner } from "@/lib/a2a/authenticate"; + +export interface A2ARestAuth { + /** Owner scope for task reads/mutations; undefined = operator view (all tasks). */ + owner: string | undefined; +} + +/** + * NOTE: the failure branch is whatever requireManagementAuth returns — today a + * plain `Response` from createErrorResponse(), NOT a NextResponse. Callers must + * test with `instanceof Response` (NextResponse extends Response), never + * `instanceof NextResponse`, or the 401 silently falls through to the handler. + */ +export async function authorizeA2ATaskRoute(request: Request): Promise { + const apiKey = extractApiKey(request); + + if (isRequireApiKeyEnabled()) { + if (apiKey && (await isValidApiKey(apiKey))) return { owner: resolveA2AOwner(request) }; + const managementError = await requireManagementAuth(request, { + invalidApiKeyStatus: 401, + alwaysRequireAuth: true, + }); + if (managementError === null) return { owner: undefined }; + return managementError; + } + + const managementError = await requireManagementAuth(request, { invalidApiKeyStatus: 401 }); + if (managementError === null) return { owner: undefined }; + if (apiKey && (await isValidApiKey(apiKey))) return { owner: resolveA2AOwner(request) }; + return managementError; +} diff --git a/src/app/api/a2a/tasks/[id]/cancel/route.ts b/src/app/api/a2a/tasks/[id]/cancel/route.ts index 9919626f39..bc3558b06e 100644 --- a/src/app/api/a2a/tasks/[id]/cancel/route.ts +++ b/src/app/api/a2a/tasks/[id]/cancel/route.ts @@ -1,14 +1,23 @@ import { NextResponse } from "next/server"; import { getTaskManager } from "@/lib/a2a/taskManager"; +import { authorizeA2ATaskRoute } from "@/app/api/a2a/_auth"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; -export async function POST(_request: Request, { params }: { params: Promise<{ id: string }> }) { +export async function POST(request: Request, { params }: { params: Promise<{ id: string }> }) { + // GHSA-jcm5-6wpp-wjj8: this route had no auth call at all. The owner check + // happens inside cancelTask: another principal's task throws the same + // "not found" a missing one would (no existence oracle). + const auth = await authorizeA2ATaskRoute(request); + if (auth instanceof Response) return auth; try { const { id } = await params; const tm = getTaskManager(); - const task = tm.cancelTask(id); + const task = tm.cancelTask(id, auth.owner); return NextResponse.json({ task: { id: task.id, state: task.state } }); } catch (error) { - const message = error instanceof Error ? error.message : "Failed to cancel A2A task"; + const message = sanitizeErrorMessage( + error instanceof Error ? error.message : "Failed to cancel A2A task" + ); const status = message.includes("not found") ? 404 : 400; return NextResponse.json({ error: message }, { status }); } diff --git a/src/app/api/a2a/tasks/[id]/route.ts b/src/app/api/a2a/tasks/[id]/route.ts index ae3906171e..2d5c1bf0c3 100644 --- a/src/app/api/a2a/tasks/[id]/route.ts +++ b/src/app/api/a2a/tasks/[id]/route.ts @@ -1,17 +1,30 @@ import { NextResponse } from "next/server"; import { getTaskManager } from "@/lib/a2a/taskManager"; +import { authorizeA2ATaskRoute } from "@/app/api/a2a/_auth"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; -export async function GET(_request: Request, { params }: { params: Promise<{ id: string }> }) { +export async function GET(request: Request, { params }: { params: Promise<{ id: string }> }) { + // GHSA-jcm5-6wpp-wjj8: this route had no auth call at all — open regardless + // of configuration. Another principal's task answers 404, same as a missing + // one, so an IDOR probe cannot tell the two apart. + const auth = await authorizeA2ATaskRoute(request); + if (auth instanceof Response) return auth; try { const { id } = await params; const tm = getTaskManager(); - const task = tm.getTask(id); + const task = tm.getTask(id, auth.owner); if (!task) { return NextResponse.json({ error: `Task not found: ${id}` }, { status: 404 }); } return NextResponse.json({ task }); } catch (error) { - const message = error instanceof Error ? error.message : "Failed to load A2A task"; - return NextResponse.json({ error: message }, { status: 500 }); + return NextResponse.json( + { + error: sanitizeErrorMessage( + error instanceof Error ? error.message : "Failed to load A2A task" + ), + }, + { status: 500 } + ); } } diff --git a/src/app/api/a2a/tasks/route.ts b/src/app/api/a2a/tasks/route.ts index ddd1ad60f1..18353dfa7d 100644 --- a/src/app/api/a2a/tasks/route.ts +++ b/src/app/api/a2a/tasks/route.ts @@ -3,6 +3,7 @@ import { NextResponse } from "next/server"; import { z } from "zod"; import { getTaskManager, type TaskState } from "@/lib/a2a/taskManager"; +import { authorizeA2ATaskRoute } from "@/app/api/a2a/_auth"; import { createConductorTask } from "@/lib/conductor/hubProxy"; import { getSettings } from "@/lib/db/settings"; @@ -22,6 +23,11 @@ function parseIntParam(value: string | null, fallback: number): number { } export async function GET(request: Request) { + // GHSA-jcm5-6wpp-wjj8: the list route had no auth call at all. Management + // (or the keyless posture) sees every task; a bare API key must be valid + // and is owner-scoped. + const auth = await authorizeA2ATaskRoute(request); + if (auth instanceof Response) return auth; try { const { searchParams } = new URL(request.url); const stateParam = searchParams.get("state"); @@ -36,7 +42,7 @@ export async function GET(request: Request) { const tm = getTaskManager(); const total = tm.countTasks({ state, skill }); - const tasks = tm.listTasks({ state, skill, limit, offset }); + const tasks = tm.listTasks({ state, skill, limit, offset }, auth.owner); return NextResponse.json({ tasks, @@ -104,7 +110,10 @@ export function authenticateA2A(request: Request): boolean { */ export async function POST(request: Request) { if (!authenticateA2A(request)) { - return NextResponse.json({ error: "Unauthorized: missing or invalid API key" }, { status: 401 }); + return NextResponse.json( + { error: "Unauthorized: missing or invalid API key" }, + { status: 401 } + ); } const settings = await getSettings(); if (settings.a2aEnabled !== true) { @@ -122,12 +131,18 @@ export async function POST(request: Request) { } const parsed = delegationSchema.safeParse(raw); if (!parsed.success) { - return NextResponse.json({ error: "Invalid A2A task: provide messages[] (and metadata.conductor)" }, { status: 400 }); + return NextResponse.json( + { error: "Invalid A2A task: provide messages[] (and metadata.conductor)" }, + { status: 400 } + ); } const { skill, messages, metadata } = parsed.data; if (skill !== "conductor" && !skill.startsWith("conductor-cli-")) { return NextResponse.json( - { error: "Only Conductor fleet skills are delegable here (conductor / conductor-cli-)" }, + { + error: + "Only Conductor fleet skills are delegable here (conductor / conductor-cli-)", + }, { status: 400 } ); } @@ -138,7 +153,9 @@ export async function POST(request: Request) { { status: 400 } ); } - const prompt = [...messages].reverse().find((m) => m.role === "user")?.content ?? messages[messages.length - 1].content; + const prompt = + [...messages].reverse().find((m) => m.role === "user")?.content ?? + messages[messages.length - 1].content; const created = await createConductorTask({ repoUrl: conductor.repo.url, diff --git a/src/lib/a2a/authenticate.ts b/src/lib/a2a/authenticate.ts new file mode 100644 index 0000000000..b57d4082cc --- /dev/null +++ b/src/lib/a2a/authenticate.ts @@ -0,0 +1,53 @@ +/** + * Shared A2A authentication + caller-owner resolution (GHSA-jcm5-6wpp-wjj8). + * + * The JSON-RPC router (/a2a) grew its own authenticate() for GHSA-v54m, but + * the REST task routes under /api/a2a/tasks/ had no auth call at all. Both + * surfaces now share this single implementation so they cannot drift again: + * same REQUIRE_API_KEY posture as /v1, same keyless local-first default, and + * a stable owner id (hashed API key) used to scope task visibility. + */ + +import { createHash, timingSafeEqual } from "crypto"; +import type { NextRequest } from "next/server"; +import { extractApiKey, isValidApiKey } from "@/sse/services/auth"; +import { isRequireApiKeyEnabled } from "@/shared/utils/featureFlags"; + +function tokensMatch(provided: string, expected: string): boolean { + const a = Buffer.from(provided); + const b = Buffer.from(expected); + if (a.length !== b.length) return false; + return timingSafeEqual(a, b); +} + +/** + * Whether the request may use the A2A surface at all. Mirrors the JSON-RPC + * posture: when a client key is required, demand a valid OmniRoute key; + * otherwise honor the legacy explicit A2A key; otherwise stay keyless (the + * same local-first default as /v1). + */ +export async function authenticateA2ARequest(req: NextRequest | Request): Promise { + const apiKey = extractApiKey(req as NextRequest); + if (isRequireApiKeyEnabled()) { + return apiKey ? await isValidApiKey(apiKey) : false; + } + + const configuredKey = process.env.OMNIROUTE_API_KEY; + if (configuredKey) { + return apiKey ? tokensMatch(apiKey, configuredKey) : false; + } + + // No API key required and none configured — allow (keyless local-first). + return true; +} + +/** + * Owner id for task scoping (GHSA-jcm5-6wpp-wjj8): a stable hash of the + * caller's API key, or `undefined` when the call carries no key (keyless + * posture — ownerless tasks stay visible to everyone, by design). + */ +export function resolveA2AOwner(req: NextRequest | Request): string | undefined { + const apiKey = extractApiKey(req as NextRequest); + if (!apiKey) return undefined; + return createHash("sha256").update(apiKey).digest("hex").slice(0, 32); +} diff --git a/src/lib/a2a/taskManager.ts b/src/lib/a2a/taskManager.ts index 390bcda03d..a21ac57207 100644 --- a/src/lib/a2a/taskManager.ts +++ b/src/lib/a2a/taskManager.ts @@ -45,6 +45,13 @@ export interface A2ATask { createdAt: string; updatedAt: string; expiresAt: string; + /** + * GHSA-jcm5-6wpp-wjj8: principal that created the task (hashed API key). + * `undefined` = created under the keyless local-first posture — such tasks + * stay visible to every caller, matching the pre-owner behavior. Tasks WITH + * an owner are only returned/cancelled/listed for the same owner. + */ + owner?: string; } export interface TaskListFilter { @@ -91,7 +98,7 @@ export class A2ATaskManager { } } - createTask(input: TaskInput): A2ATask { + createTask(input: TaskInput, owner?: string): A2ATask { const now = new Date(); const task: A2ATask = { id: randomUUID(), @@ -104,19 +111,31 @@ export class A2ATaskManager { createdAt: now.toISOString(), updatedAt: now.toISOString(), expiresAt: new Date(now.getTime() + this.ttlMs).toISOString(), + ...(owner !== undefined ? { owner } : {}), }; this.tasks.set(task.id, task); return task; } - getTask(taskId: string): A2ATask | undefined { + /** + * Owner scoping (GHSA-jcm5-6wpp-wjj8): a task carrying an owner is visible + * only to that owner. Ownerless tasks (keyless posture, or created before + * this field existed) stay visible to everyone — no behavior change there. + */ + private isVisibleTo(task: A2ATask, owner?: string): boolean { + return task.owner === undefined || task.owner === owner; + } + + getTask(taskId: string, owner?: string): A2ATask | undefined { const task = this.tasks.get(taskId); if (task && new Date(task.expiresAt) < new Date()) { if (task.state === "submitted" || task.state === "working") { this.updateTask(taskId, "failed", undefined, "Task expired"); } } - return this.tasks.get(taskId); + const current = this.tasks.get(taskId); + if (!current || !this.isVisibleTo(current, owner)) return undefined; + return current; } updateTask( @@ -142,7 +161,15 @@ export class A2ATaskManager { return task; } - cancelTask(taskId: string): A2ATask { + cancelTask(taskId: string, owner?: string): A2ATask { + // Owner check BEFORE the mutation (GHSA-jcm5-6wpp-wjj8): a caller must not + // cancel another principal's task by id. Uses the same not-found error as + // a missing task so an IDOR probe cannot distinguish "exists but not + // yours" from "does not exist". + const task = this.tasks.get(taskId); + if (!task || !this.isVisibleTo(task, owner)) { + throw new Error(`Task ${taskId} not found`); + } return this.updateTask(taskId, "cancelled", undefined, "Cancelled by client"); } @@ -153,8 +180,11 @@ export class A2ATaskManager { return tasks.length; } - listTasks(filter?: TaskListFilter): A2ATask[] { + listTasks(filter?: TaskListFilter, owner?: string): A2ATask[] { let tasks = [...this.tasks.values()]; + // GHSA-jcm5-6wpp-wjj8: when an owner scope is supplied, owned tasks of + // other principals are hidden; ownerless tasks remain visible (posture). + if (owner !== undefined) tasks = tasks.filter((t) => this.isVisibleTo(t, owner)); if (filter?.state) tasks = tasks.filter((t) => t.state === filter.state); if (filter?.skill) tasks = tasks.filter((t) => t.skill === filter.skill); tasks.sort((a, b) => new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime()); diff --git a/tests/unit/a2a-task-owner-idor.test.ts b/tests/unit/a2a-task-owner-idor.test.ts new file mode 100644 index 0000000000..365744ed33 --- /dev/null +++ b/tests/unit/a2a-task-owner-idor.test.ts @@ -0,0 +1,136 @@ +/** + * GHSA-jcm5-6wpp-wjj8 — A2A task IDOR + unauthenticated REST task routes. + * + * Two gaps closed here: + * 1. The REST routes /api/a2a/tasks/[id] and /api/a2a/tasks/[id]/cancel had + * NO auth call at all — open regardless of configuration. They now share + * the JSON-RPC surface's authentication (REQUIRE_API_KEY posture). + * 2. Tasks lived in an owner-less Map: any caller could read/cancel any + * task by id. Tasks now bind to an owner (hashed API key) at creation and + * reads/cancels/lists are owner-scoped. Ownerless tasks (keyless + * local-first posture) stay visible to everyone — by design. + * + * Run with: + * node --import tsx/esm --test tests/unit/a2a-task-owner-idor.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"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omni-a2a-idor-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "a2a-idor-test-secret"; +process.env.OMNIROUTE_DISABLE_REDIS_AUTH_CACHE = "1"; + +const core = await import("../../src/lib/db/core.ts"); +const apiKeysDb = await import("../../src/lib/db/apiKeys.ts"); +const { A2ATaskManager, getTaskManager } = await import("../../src/lib/a2a/taskManager.ts"); +const { resolveA2AOwner } = await import("../../src/lib/a2a/authenticate.ts"); +const restGet = await import("../../src/app/api/a2a/tasks/[id]/route.ts"); + +const ORIGINAL_REQUIRE = process.env.REQUIRE_API_KEY; + +after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + if (ORIGINAL_REQUIRE === undefined) delete process.env.REQUIRE_API_KEY; + else process.env.REQUIRE_API_KEY = ORIGINAL_REQUIRE; +}); + +function makeManager() { + const tm = new A2ATaskManager(5); + // Prevent the per-instance cleanup interval from keeping the process alive. + clearInterval((tm as unknown as { cleanupInterval: NodeJS.Timeout }).cleanupInterval); + return tm; +} + +describe("A2ATaskManager — owner scoping (GHSA-jcm5)", () => { + it("another principal cannot READ an owned task (same undefined as missing)", () => { + const tm = makeManager(); + const task = tm.createTask({ skill: "smart-routing", messages: [] }, "owner-a"); + assert.equal(tm.getTask(task.id, "owner-a")?.id, task.id, "the owner still reads it"); + assert.equal(tm.getTask(task.id, "owner-b"), undefined, "another owner gets undefined"); + }); + + it("another principal cannot CANCEL an owned task (not-found error, no existence oracle)", () => { + const tm = makeManager(); + const task = tm.createTask({ skill: "smart-routing", messages: [] }, "owner-a"); + assert.throws(() => tm.cancelTask(task.id, "owner-b"), /not found/); + assert.equal(tm.getTask(task.id, "owner-a")?.state, "submitted", "task untouched"); + assert.equal(tm.cancelTask(task.id, "owner-a").state, "cancelled", "the owner can cancel"); + }); + + it("owner-scoped listTasks hides other principals' owned tasks", () => { + const tm = makeManager(); + tm.createTask({ skill: "s1", messages: [] }, "owner-a"); + const mine = tm.createTask({ skill: "s1", messages: [] }, "owner-b"); + const listed = tm.listTasks(undefined, "owner-b"); + assert.deepEqual( + listed.map((t) => t.id), + [mine.id] + ); + // No owner scope (management/dashboard path) still sees everything. + assert.equal(tm.listTasks(undefined).length, 2); + }); + + it("ownerless tasks stay visible to everyone (keyless local-first posture)", () => { + const tm = makeManager(); + const task = tm.createTask({ skill: "smart-routing", messages: [] }); + assert.equal(tm.getTask(task.id, "anyone")?.id, task.id); + assert.equal(tm.getTask(task.id)?.id, task.id); + assert.equal(tm.cancelTask(task.id, "anyone").state, "cancelled"); + }); +}); + +describe("REST /api/a2a/tasks/[id] — authentication (GHSA-jcm5)", () => { + it("rejects an unkeyed call when REQUIRE_API_KEY=true (was: no auth at all)", async () => { + process.env.REQUIRE_API_KEY = "true"; + delete process.env.OMNIROUTE_API_KEY; + const res = await restGet.GET(new Request("http://localhost/api/a2a/tasks/abc") as never, { + params: Promise.resolve({ id: "abc" }), + }); + assert.equal(res.status, 401); + }); + + it("serves a keyed call under REQUIRE_API_KEY=true", async () => { + process.env.REQUIRE_API_KEY = "true"; + const key = await apiKeysDb.createApiKey("a2a-rest-client", "machine-rest", []); + const res = await restGet.GET( + new Request("http://localhost/api/a2a/tasks/definitely-missing", { + headers: { authorization: `Bearer ${key.key}` }, + }) as never, + { params: Promise.resolve({ id: "definitely-missing" }) } + ); + // Authenticated — the 404 now comes from the task lookup, not the auth gate. + assert.equal(res.status, 404); + }); + + it("keyed caller gets 404 for another principal's task (route-level IDOR, GHSA-jcm5)", async () => { + process.env.REQUIRE_API_KEY = "true"; + const tm = getTaskManager(); + // A task owned by a DIFFERENT principal than the caller's key hash. + const foreign = tm.createTask({ skill: "smart-routing", messages: [] }, "some-other-owner"); + const key = await apiKeysDb.createApiKey("a2a-rest-idor", "machine-idor", []); + const req = new Request(`http://localhost/api/a2a/tasks/${foreign.id}`, { + headers: { authorization: `Bearer ${key.key}` }, + }); + const res = await restGet.GET(req as never, { params: Promise.resolve({ id: foreign.id }) }); + assert.equal(res.status, 404, "another principal's task is invisible"); + + // And the same task IS visible to its owner (owner hash derived from the key). + const owned = tm.createTask( + { skill: "smart-routing", messages: [] }, + resolveA2AOwner(req as never) + ); + const res2 = await restGet.GET( + new Request(`http://localhost/api/a2a/tasks/${owned.id}`, { + headers: { authorization: `Bearer ${key.key}` }, + }) as never, + { params: Promise.resolve({ id: owned.id }) } + ); + assert.equal(res2.status, 200, "the owner reads its own task"); + }); +}); diff --git a/tests/unit/a2a-tasks-auth.test.ts b/tests/unit/a2a-tasks-auth.test.ts index 5569905d66..53d3a975c3 100644 --- a/tests/unit/a2a-tasks-auth.test.ts +++ b/tests/unit/a2a-tasks-auth.test.ts @@ -8,7 +8,9 @@ const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); const TASKS_ROUTE = path.resolve(__dirname, "../../src/app/api/a2a/tasks/route.ts"); -const A2A_ROUTE = path.resolve(__dirname, "../../src/app/a2a/route.ts"); +// GHSA-jcm5-6wpp-wjj8: the constant-time token comparison moved out of +// src/app/a2a/route.ts into the shared helper both surfaces now use. +const A2A_AUTH_HELPER = path.resolve(__dirname, "../../src/lib/a2a/authenticate.ts"); const source = fs.readFileSync(TASKS_ROUTE, "utf-8"); @@ -21,11 +23,11 @@ function hasImport(src: string, name: string, from: string): boolean { return pattern.test(src); } -test("tasks route uses the same constant-time contract as src/app/a2a/route.ts", () => { - const a2aSource = fs.readFileSync(A2A_ROUTE, "utf-8"); +test("tasks route uses the same constant-time contract as the shared A2A auth helper", () => { + const a2aSource = fs.readFileSync(A2A_AUTH_HELPER, "utf-8"); assert.ok( - hasImport(a2aSource, "timingSafeEqual", "node:crypto"), - "reference route imports timingSafeEqual" + hasImport(a2aSource, "timingSafeEqual", "crypto"), + "shared auth helper imports timingSafeEqual" ); assert.ok(