mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-21 22:52:19 +03:00
fix(security): close 4 STILL-REAL advisory findings (ACP RCE hardening, db-backups tier, uppercase authz bypass, spawn-veto drift) (#11028)
⭐5 — 4 achados STILL-REAL de advisories de segurança, cada um com TDD (RED→GREEN) e crédito ao reporter original: ACP RCE hardening (resolveVersionProbe), db-backups Tier-2 allowlist, uppercase authz bypass (matcher case-insensitive), spawn-veto drift (chatgpt-web-codex-doctor). typecheck/lint limpos, suíte authz/acp/cors verde. UNSTABLE é o base-red inherited #9985, já documentado no corpo da PR.
This commit is contained in:
committed by
GitHub
parent
c130f2aa1c
commit
60829241fd
@@ -200,6 +200,14 @@ let _customAgentDefs: CustomAgentDef[] = [];
|
||||
|
||||
const DISALLOWED_VERSION_COMMAND_CHARS = /[;&|<>`$\r\n]/;
|
||||
|
||||
// A version probe only ever needs a version flag. For untrusted (client-registered)
|
||||
// custom agents the binary-match check alone is not enough: the caller controls both
|
||||
// `binary` and `versionCommand`, so a matching interpreter with an eval-style argument
|
||||
// (`node -e …`, `python -c …`, `ruby -e …`) reaches execFileSync as arbitrary code
|
||||
// execution without any shell metacharacter. Restricting the args to a recognized
|
||||
// version flag closes that path — see GHSA-jphr-2gw7-xrwp / GHSA-hf57-cqmx-p4gr.
|
||||
const SAFE_VERSION_PROBE_ARG = /^(-v|-V|--version|-version|version|--ver)$/;
|
||||
|
||||
/**
|
||||
* Set custom agent definitions from settings.
|
||||
*/
|
||||
@@ -300,6 +308,12 @@ export function resolveVersionProbe(
|
||||
if (!allowed.has(normalizedCommand)) {
|
||||
return null;
|
||||
}
|
||||
|
||||
// Untrusted probe: allow only a bare binary or a single recognized version
|
||||
// flag, so a matching interpreter cannot smuggle an eval/exec argument.
|
||||
if (args.length > 1 || (args.length === 1 && !SAFE_VERSION_PROBE_ARG.test(args[0]))) {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
return { command, args };
|
||||
|
||||
28
src/proxy.ts
28
src/proxy.ts
@@ -24,6 +24,14 @@ export async function proxy(request: NextRequest) {
|
||||
return runAuthzPipeline(request, { enforce: true });
|
||||
}
|
||||
|
||||
// Next compiles the middleware/proxy matcher from `regexp.source` only, dropping
|
||||
// path-to-regexp's default case-insensitive flag — so a lowercase literal like
|
||||
// `/v1/:path*` never matches `/V1/...`, while the rewrite matcher (flag kept)
|
||||
// still routes it to the handler. That skipped the authz pipeline entirely
|
||||
// (GHSA-jvqc-mp9f-q936). Expressing the case-insensitivity inside a custom
|
||||
// path-to-regexp group (`([vV]1)`) survives the flag-drop because it needs no
|
||||
// flag. Keep these in sync with the client-API aliases in
|
||||
// next.config.mjs rewrites and src/server/authz/classify.ts.
|
||||
export const config = {
|
||||
matcher: [
|
||||
"/",
|
||||
@@ -31,15 +39,15 @@ export const config = {
|
||||
"/home",
|
||||
"/home/:path*",
|
||||
"/api/:path*",
|
||||
"/v1/:path*",
|
||||
"/v1",
|
||||
"/v1beta/:path*",
|
||||
"/v1beta",
|
||||
"/chat/:path*",
|
||||
"/responses/:path*",
|
||||
"/responses",
|
||||
"/codex/:path*",
|
||||
"/codex",
|
||||
"/models",
|
||||
"/:v1seg([vV]1)/:path*",
|
||||
"/:v1seg([vV]1)",
|
||||
"/:v1betaseg([vV]1[bB][eE][tT][aA])/:path*",
|
||||
"/:v1betaseg([vV]1[bB][eE][tT][aA])",
|
||||
"/:chatseg([cC][hH][aA][tT])/:path*",
|
||||
"/:respseg([rR][eE][sS][pP][oO][nN][sS][eE][sS])/:path*",
|
||||
"/:respseg([rR][eE][sS][pP][oO][nN][sS][eE][sS])",
|
||||
"/:codexseg([cC][oO][dD][eE][xX])/:path*",
|
||||
"/:codexseg([cC][oO][dD][eE][xX])",
|
||||
"/:modelsseg([mM][oO][dD][eE][lL][sS])",
|
||||
],
|
||||
};
|
||||
|
||||
@@ -16,30 +16,39 @@ function normalizePathname(rawPath: string): { path: string; reason?: Classifica
|
||||
if (!path.startsWith("/")) path = "/" + path;
|
||||
if (path.length > 1 && path.endsWith("/")) path = path.slice(0, -1);
|
||||
|
||||
if (path === "/codex" || path.startsWith("/codex/")) {
|
||||
// Client-API aliases are matched case-insensitively on the control segment.
|
||||
// Next's rewrite layer accepts `/V1/...`, `/CODEX`, etc. and routes them to
|
||||
// the client handler, so the classifier must recognize the same casing —
|
||||
// otherwise an uppercase alias falls through to the management fallback and
|
||||
// the request is treated as a different route class than it is actually
|
||||
// dispatched to (GHSA-jvqc-mp9f-q936). Only the leading control segment is
|
||||
// lowercased for detection; the original-case tail is preserved.
|
||||
const lower = path.toLowerCase();
|
||||
|
||||
if (lower === "/codex" || lower.startsWith("/codex/")) {
|
||||
return { path: "/api/v1/responses", reason: "client_api_codex_alias" };
|
||||
}
|
||||
|
||||
if (path === "/v1/v1" || path.startsWith("/v1/v1/")) {
|
||||
if (lower === "/v1/v1" || lower.startsWith("/v1/v1/")) {
|
||||
const tail = path.slice("/v1/v1".length) || "";
|
||||
return { path: "/api/v1" + tail, reason: "client_api_double_prefix" };
|
||||
}
|
||||
|
||||
if (path === "/v1beta" || path.startsWith("/v1beta/")) {
|
||||
if (lower === "/v1beta" || lower.startsWith("/v1beta/")) {
|
||||
const tail = path.slice("/v1beta".length) || "";
|
||||
return { path: "/api/v1beta" + tail, reason: "client_api_alias" };
|
||||
}
|
||||
|
||||
if (path === "/v1" || path.startsWith("/v1/")) {
|
||||
if (lower === "/v1" || lower.startsWith("/v1/")) {
|
||||
const tail = path.slice("/v1".length) || "";
|
||||
return { path: "/api/v1" + tail, reason: "client_api_alias" };
|
||||
}
|
||||
|
||||
for (const { alias, canonical } of CLIENT_API_ALIAS_PREFIXES) {
|
||||
if (path === alias) {
|
||||
if (lower === alias) {
|
||||
return { path: canonical, reason: "client_api_alias" };
|
||||
}
|
||||
if (path.startsWith(alias + "/")) {
|
||||
if (lower.startsWith(alias + "/")) {
|
||||
return { path: canonical + path.slice(alias.length), reason: "client_api_alias" };
|
||||
}
|
||||
}
|
||||
|
||||
@@ -119,6 +119,11 @@ export const ALWAYS_PROTECTED_API_PATHS: ReadonlyArray<string> = [
|
||||
"/api/shutdown",
|
||||
"/api/providers/health-autopilot/actions",
|
||||
"/api/settings/database",
|
||||
// Full-database export/import: a credential dump and an irreversible replace.
|
||||
// Must stay authenticated even under requireLogin=false, for the same reason
|
||||
// /api/settings/database already does. isAlwaysProtectedPath matches on a path
|
||||
// boundary, so this covers export, exportAll and import. (GHSA-mghq-58h3-qcqj)
|
||||
"/api/db-backups",
|
||||
];
|
||||
|
||||
export function isLoopbackHost(hostHeader: string | null): boolean {
|
||||
|
||||
@@ -52,6 +52,7 @@ export const SPAWN_CAPABLE_PATTERNS: ReadonlyArray<RegExp> = [
|
||||
/^\/api\/providers\/[^/]+\/login\/?$/, // pre-existing gap: in LOCAL_ONLY_API_PATTERNS today but never in a spawn-capable deny-list
|
||||
/^\/api\/providers\/[^/]+\/refresh-cursor\/?$/, // spawns cursor-agent via renewal.ts (Hard Rules #15 + #17)
|
||||
/^\/api\/providers\/cursor\/agent-availability\/?$/, // static path (no dynamic segment), but kept in this array alongside its /api/providers/ siblings rather than the flat SPAWN_CAPABLE_PREFIXES array — spawns cursor-agent status via checkCursorAgentAvailability()/getCachedCursorAgentAvailability() (Hard Rules #15 + #17)
|
||||
/^\/api\/providers\/[^/]+\/chatgpt-web-codex-doctor\/?$/, // spawns via getTunnelRuntimeStatus() → spawnSync("...","runtimes status") (open-sse/executors/chatgpt-web-codex/tunnelClient.ts). Mirrors LOCAL_ONLY_API_PATTERNS in routeGuard.ts; keep the two in sync (GHSA-9q3h-mjm5-f4gj).
|
||||
];
|
||||
|
||||
/**
|
||||
|
||||
@@ -100,3 +100,32 @@ test("POST /api/acp/agents rejects unsafe version commands for authenticated ses
|
||||
assert.equal(response.status, 400);
|
||||
assert.match(body.error, /Invalid versionCommand/i);
|
||||
});
|
||||
|
||||
test("POST /api/acp/agents rejects an interpreter eval payload (GHSA-jphr-2gw7-xrwp)", async () => {
|
||||
// Exact shape of the advisory PoC: binary + versionCommand both name `node`,
|
||||
// so the binary-match check passes, but the `-e` eval argument must still be
|
||||
// refused before it can reach execFileSync("node", ["-e", ...]).
|
||||
process.env.JWT_SECRET = "acp-agents-jwt-secret";
|
||||
await localDb.updateSettings({ requireLogin: true, password: "hashed-password" });
|
||||
const token = await createSessionToken();
|
||||
|
||||
const response = await routeModule.POST(
|
||||
makeRequest(
|
||||
"POST",
|
||||
{
|
||||
id: "anonrce",
|
||||
name: "anonrce",
|
||||
binary: "node",
|
||||
versionCommand: 'node -e "process.exit(1)"',
|
||||
providerAlias: "anonrce",
|
||||
spawnArgs: [],
|
||||
protocol: "stdio",
|
||||
},
|
||||
token
|
||||
)
|
||||
);
|
||||
const body = (await response.json()) as { error?: string };
|
||||
|
||||
assert.equal(response.status, 400);
|
||||
assert.match(body.error ?? "", /Invalid versionCommand/i);
|
||||
});
|
||||
|
||||
@@ -32,6 +32,41 @@ test("resolveVersionProbe rejects shell metacharacters in version commands", ()
|
||||
assert.equal(probe, null);
|
||||
});
|
||||
|
||||
// Regression guard — GHSA-jphr-2gw7-xrwp / GHSA-hf57-cqmx-p4gr (ACP custom-agent
|
||||
// RCE). A client-registered custom agent controls both `binary` and
|
||||
// `versionCommand`; the binary-match check alone still admits an eval-style
|
||||
// argument on a matching interpreter, which reaches execFileSync as arbitrary
|
||||
// code execution (no shell metacharacter required). A version *probe* only ever
|
||||
// needs a version flag, so untrusted probes must reject non-version arguments.
|
||||
test("resolveVersionProbe rejects interpreter eval arguments on a matching binary", () => {
|
||||
assert.equal(resolveVersionProbe("node", 'node -e "process.exit(1)"', true), null);
|
||||
assert.equal(resolveVersionProbe("node", "node --eval 1", true), null);
|
||||
assert.equal(resolveVersionProbe("python3", 'python3 -c "import os"', true), null);
|
||||
assert.equal(resolveVersionProbe("ruby", 'ruby -e "puts 1"', true), null);
|
||||
// Any extra argument beyond a single version flag is refused for a probe.
|
||||
assert.equal(resolveVersionProbe("node", "node --version --eval 1", true), null);
|
||||
});
|
||||
|
||||
test("resolveVersionProbe still accepts legitimate version flags for custom agents", () => {
|
||||
assert.deepEqual(resolveVersionProbe("node", "node --version", true), {
|
||||
command: "node",
|
||||
args: ["--version"],
|
||||
});
|
||||
assert.deepEqual(resolveVersionProbe("my-agent", "my-agent -v", true), {
|
||||
command: "my-agent",
|
||||
args: ["-v"],
|
||||
});
|
||||
assert.deepEqual(resolveVersionProbe("my-agent", "my-agent version", true), {
|
||||
command: "my-agent",
|
||||
args: ["version"],
|
||||
});
|
||||
// Bare binary with no arguments is a valid probe too.
|
||||
assert.deepEqual(resolveVersionProbe("my-agent", "my-agent", true), {
|
||||
command: "my-agent",
|
||||
args: [],
|
||||
});
|
||||
});
|
||||
|
||||
test("shouldUseShellForVersionProbe preserves Windows npm wrapper detection", () => {
|
||||
assert.equal(shouldUseShellForVersionProbe("codex", "win32"), true);
|
||||
assert.equal(
|
||||
|
||||
@@ -55,15 +55,19 @@ test("proxy.ts delegates to runAuthzPipeline with enforce: true", () => {
|
||||
test("proxy.ts config.matcher covers every /api/* route plus dashboard and v1 aliases", () => {
|
||||
const content = fs.readFileSync("src/proxy.ts", "utf8");
|
||||
// Required prefixes — drop one and the corresponding routes go unguarded.
|
||||
// The client-API aliases use a case-insensitive path-to-regexp group
|
||||
// (`([vV]1)`) so `/V1/...` reaches the pipeline too — see
|
||||
// GHSA-jvqc-mp9f-q936 and tests/unit/authz/proxy-matcher-case.test.ts for the
|
||||
// semantic (compiled-matcher) coverage assertions.
|
||||
const requiredMatchers = [
|
||||
'"/api/:path*"',
|
||||
'"/dashboard/:path*"',
|
||||
'"/v1/:path*"',
|
||||
'"/v1beta/:path*"',
|
||||
'"/chat/:path*"',
|
||||
'"/responses/:path*"',
|
||||
'"/codex/:path*"',
|
||||
'"/models"',
|
||||
'"/:v1seg([vV]1)/:path*"',
|
||||
'"/:v1betaseg([vV]1[bB][eE][tT][aA])/:path*"',
|
||||
'"/:chatseg([cC][hH][aA][tT])/:path*"',
|
||||
'"/:respseg([rR][eE][sS][pP][oO][nN][sS][eE][sS])/:path*"',
|
||||
'"/:codexseg([cC][oO][dD][eE][xX])/:path*"',
|
||||
'"/:modelsseg([mM][oO][dD][eE][lL][sS])"',
|
||||
];
|
||||
for (const matcher of requiredMatchers) {
|
||||
assert.ok(
|
||||
|
||||
76
tests/unit/authz/proxy-matcher-case.test.ts
Normal file
76
tests/unit/authz/proxy-matcher-case.test.ts
Normal file
@@ -0,0 +1,76 @@
|
||||
import { test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import { createRequire } from "node:module";
|
||||
|
||||
import { config } from "../../../src/proxy.ts";
|
||||
import { classifyRoute } from "../../../src/server/authz/classify.ts";
|
||||
|
||||
// Regression guard — GHSA-jvqc-mp9f-q936 (case-sensitive authz-matcher bypass).
|
||||
//
|
||||
// Next.js compiles the middleware/proxy matcher from `regexp.source` only,
|
||||
// dropping path-to-regexp's default case-insensitive flag, so a lowercase
|
||||
// literal like `/v1/:path*` does NOT match `/V1/...`. The rewrite matcher keeps
|
||||
// the flag, so `/V1/chat/completions` was still rewritten to the handler while
|
||||
// skipping the authz pipeline entirely — an unauthenticated inference bypass.
|
||||
//
|
||||
// The fix expresses the case-insensitivity inside a path-to-regexp custom group
|
||||
// (`/:seg([vV]1)/:path*`), which survives the flag-drop because it needs no
|
||||
// flag. This test compiles the matcher exactly the way Next does and asserts the
|
||||
// uppercase / mixed-case client aliases are covered.
|
||||
|
||||
const require = createRequire(import.meta.url);
|
||||
const { tryToParsePath } = require("next/dist/lib/try-to-parse-path.js");
|
||||
|
||||
function compiledMatcherRegexes(): RegExp[] {
|
||||
return (config.matcher as string[]).map((entry) => {
|
||||
const parsed = tryToParsePath(entry);
|
||||
// Mirror Next's middleware-route-matcher: source only, no flags.
|
||||
return new RegExp(parsed.regexStr as string);
|
||||
});
|
||||
}
|
||||
|
||||
function isMatchedByProxy(path: string): boolean {
|
||||
return compiledMatcherRegexes().some((re) => re.test(path));
|
||||
}
|
||||
|
||||
test("proxy matcher still covers the canonical lowercase client aliases", () => {
|
||||
for (const p of [
|
||||
"/v1/chat/completions",
|
||||
"/v1/models",
|
||||
"/v1beta/models",
|
||||
"/responses",
|
||||
"/codex/x",
|
||||
"/models",
|
||||
]) {
|
||||
assert.equal(isMatchedByProxy(p), true, `expected proxy matcher to cover ${p}`);
|
||||
}
|
||||
});
|
||||
|
||||
test("proxy matcher covers uppercase / mixed-case client aliases (GHSA-jvqc-mp9f-q936)", () => {
|
||||
for (const p of [
|
||||
"/V1/chat/completions",
|
||||
"/V1/models",
|
||||
"/V1BETA/models",
|
||||
"/CHAT/completions",
|
||||
"/RESPONSES",
|
||||
"/CODEX/x",
|
||||
"/MODELS",
|
||||
"/Responses/x",
|
||||
"/v1BeTa/models",
|
||||
]) {
|
||||
assert.equal(
|
||||
isMatchedByProxy(p),
|
||||
true,
|
||||
`uppercase alias ${p} must reach the authz pipeline, not skip it`
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
test("classifyRoute treats uppercase client aliases as CLIENT_API, not management fallback", () => {
|
||||
assert.equal(classifyRoute("/V1/chat/completions", "POST").routeClass, "CLIENT_API");
|
||||
assert.equal(classifyRoute("/V1BETA/models", "GET").routeClass, "CLIENT_API");
|
||||
assert.equal(classifyRoute("/MODELS", "GET").routeClass, "CLIENT_API");
|
||||
assert.equal(classifyRoute("/CODEX", "POST").routeClass, "CLIENT_API");
|
||||
// Lowercase behavior is unchanged.
|
||||
assert.equal(classifyRoute("/v1/chat/completions", "POST").routeClass, "CLIENT_API");
|
||||
});
|
||||
@@ -6,6 +6,7 @@ import {
|
||||
isAlwaysProtectedPath,
|
||||
isLoopbackHost,
|
||||
} from "../../../src/server/authz/routeGuard.ts";
|
||||
import { SPAWN_CAPABLE_PATTERNS } from "../../../src/shared/constants/spawnCapablePrefixes.ts";
|
||||
import { managementPolicy } from "../../../src/server/authz/policies/management.ts";
|
||||
import { getMachineTokenSync } from "../../../src/lib/machineToken.ts";
|
||||
import { CLI_TOKEN_HEADER } from "../../../src/server/authz/headers.ts";
|
||||
@@ -50,6 +51,27 @@ test("isLocalOnlyBypassableByManageScope: non-local-only routes are not bypassab
|
||||
assert.equal(isLocalOnlyBypassableByManageScope("/api/settings"), false);
|
||||
});
|
||||
|
||||
test("SPAWN_CAPABLE_PATTERNS covers every regex-tier LOCAL_ONLY spawn route (GHSA-9q3h-mjm5-f4gj)", () => {
|
||||
// The manage-scope bypass veto's precise early-deny keys on
|
||||
// SPAWN_CAPABLE_PATTERNS, so every LOCAL_ONLY_API_PATTERNS entry (a
|
||||
// spawn-capable regex route) must have a matching pattern here — otherwise the
|
||||
// two layers drift and a spawn route loses its exact early-deny. This guards
|
||||
// against the chatgpt-web-codex-doctor drift and any future one.
|
||||
const spawnRoutes = [
|
||||
"/api/providers/acct-1/login",
|
||||
"/api/providers/acct-1/refresh-cursor",
|
||||
"/api/providers/acct-1/chatgpt-web-codex-doctor",
|
||||
];
|
||||
for (const p of spawnRoutes) {
|
||||
assert.equal(isLocalOnlyPath(p), true, `${p} must be LOCAL_ONLY`);
|
||||
assert.equal(
|
||||
SPAWN_CAPABLE_PATTERNS.some((re) => re.test(p)),
|
||||
true,
|
||||
`${p} is a LOCAL_ONLY spawn route but SPAWN_CAPABLE_PATTERNS does not cover it`
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
test("isAlwaysProtectedPath: /api/shutdown is always protected", () => {
|
||||
assert.equal(isAlwaysProtectedPath("/api/shutdown"), true);
|
||||
});
|
||||
@@ -58,6 +80,15 @@ test("isAlwaysProtectedPath: /api/settings/database is always protected", () =>
|
||||
assert.equal(isAlwaysProtectedPath("/api/settings/database"), true);
|
||||
});
|
||||
|
||||
test("isAlwaysProtectedPath: /api/db-backups is always protected (GHSA-mghq-58h3-qcqj)", () => {
|
||||
// Full-database read/replace must require auth even when requireLogin=false —
|
||||
// the same Tier-2 trade-off /api/settings/database already makes. The single
|
||||
// prefix entry covers export, exportAll, import and future siblings.
|
||||
assert.equal(isAlwaysProtectedPath("/api/db-backups/export"), true);
|
||||
assert.equal(isAlwaysProtectedPath("/api/db-backups/exportAll"), true);
|
||||
assert.equal(isAlwaysProtectedPath("/api/db-backups/import"), true);
|
||||
});
|
||||
|
||||
test("isAlwaysProtectedPath: ordinary settings routes are not always protected", () => {
|
||||
assert.equal(isAlwaysProtectedPath("/api/settings"), false);
|
||||
assert.equal(isAlwaysProtectedPath("/api/settings/proxy"), false);
|
||||
|
||||
Reference in New Issue
Block a user