fix(proxy): #6246 stop the v3.8.44 proxy IP-leak + over-deactivation regression (#6296)

Merged into release/v3.8.45. Reds pré-existentes classificados: dast-smoke (infra), check:file-size (drift de baseline de god-files congelados), e 3 testes de contagem agentSkills stale (43→44 já corrigidos no tip pelo #6186 — somem no squash sobre o tip). Núcleo do fix #6246 (proxy IP-leak + over-deactivation).
This commit is contained in:
Diego Rodrigues de Sa e Souza
2026-07-05 17:26:38 -03:00
committed by GitHub
parent dc5ae96905
commit f680aacff0
7 changed files with 451 additions and 23 deletions

View File

@@ -15,6 +15,8 @@
### 🐛 Bug Fixes
- **fix(proxy):** stop the v3.8.44 proxy regression that leaked the real IP and disabled healthy proxies ([#6246](https://github.com/diegosouzapw/OmniRoute/issues/6246)). Two coupled defects from the new health scheduler: (1) **IP leak** — when a proxy assigned to a connection was marked `inactive`, resolution fell through to a **direct** egress instead of blocking, exposing the operator's real IP; (2) **over-deactivation** — the sweep flipped a proxy to `inactive` on the **first** failed probe and counted our own 5s timeout / a probe-target `5xx` as the proxy's fault, so healthy paid proxies vanished from egress selection ("my proxies are not being used anymore"). Fix: the sweep decision is extracted into a pure, network-free `decideProxyHealthAction` (`src/lib/proxyHealth/decision.ts`) — by default the health check now **only counts/logs and never downgrades status** (a proxy is downgraded/removed only with `PROXY_AUTO_REMOVE=true`, after `PROXY_AUTO_REMOVE_AFTER` **consecutive** conclusive failures); probes are classified tri-state so an inconclusive result (our timeout, or a `5xx` from the probe target) never penalizes the proxy, and the probe timeout is raised 5s→15s. Separately, `safeResolveProxy` now **fails closed** via the existing policy: a connection whose assigned proxy is dead is blocked instead of leaking direct (`hasBlockingProxyAssignment`), honoring the explicit `proxy off` toggles and the `PROXY_FAIL_OPEN=true` opt-out. Existing proxies stuck `inactive` by the old behavior need a one-time manual re-activate (the operator owns proxy status). Regression guards: `tests/unit/proxy-health-decide-action-6246.test.ts`, `tests/unit/proxy-assigned-unavailable-6246.test.ts`.
- **chatcore (tools): stop the default 128-tool cap from silently dropping opencode's `task`/MCP tools.** opencode (used as an MCP/agent host) sends a large tool list; when it exceeds the speculative `MAX_TOOLS_LIMIT` (128) default, `truncateToolList` did a blind `tools.slice(0, 128)`, dropping every tool past index 128 — including opencode's built-in `task` tool (subagent launch) and many MCP tools, so models routed through OmniRoute could no longer spawn subagents or reach part of their tools. The cap exists to avoid upstream `400`s for providers with real hard limits (e.g. grok-cli 200), so it is kept for those: detection of the opencode client (`isOpencodeClient` — any `x-opencode-*` header, or `opencode` in the user-agent) now only bypasses the **speculative 128 default**, never a known provider ceiling. Precedence is explicit — a proactive/detected provider limit always truncates (even for opencode); otherwise opencode forwards its full tool list; otherwise the unchanged 128 default applies to every other client. Refactors `getEffectiveToolLimit` into `getKnownToolLimit(provider) ?? DEFAULT_LIMIT` (byte-identical for existing callers) and fixes a cosmetic debug-log that reported the truncated count instead of the original. Regression guard: `tests/unit/tool-limit-detector.test.ts`.
- **fix(mitm):** the macOS MITM-cert install check now matches the system keychain again. `security find-certificate -a -Z` prints the SHA-1 as a colon-less hex string, but the installed-check compared it against `getCertFingerprint()`'s colon-separated form, so the substring match never hit — the cert was reported as not-installed and re-prompted for the sudo install on every run. Fingerprints are now normalized (colons stripped, upper-cased) on both sides via the extracted `macCertOutputHasFingerprint` helper. Regression guard: `tests/unit/mitm-cert-mac-fingerprint.test.ts`. ([#6204](https://github.com/diegosouzapw/OmniRoute/pull/6204), closes [#6134](https://github.com/diegosouzapw/OmniRoute/issues/6134) — thanks @rianonehub)

View File

@@ -632,6 +632,59 @@ export async function resolveProxyForScopeFromRegistry(scope: string, scopeId?:
}
}
/**
* #6246 fail-closed guard. Returns true when a connection would egress DIRECTLY
* ONLY because its ASSIGNED proxy (account/provider/global scope) is dead/inactive
* — i.e. the request must be BLOCKED, not silently sent on the real IP.
*
* Callers use this after `resolveProxyForConnection` returns a direct result: if
* the operator assigned a proxy but every assigned proxy is dead, leaking the IP
* is worse than failing the request. An explicit "proxy off" (global or per
* connection) is a deliberate direct choice and is NOT treated as a leak. Read-only
* and best-effort: any DB error fails OPEN (returns false) so a guard never breaks
* the request path.
*/
export function hasBlockingProxyAssignment(connectionId: string): boolean {
try {
const db = getDbInstance();
// Explicit global "proxy off" → direct is intended, never a leak.
const globalRow = db
.prepare("SELECT value FROM key_value WHERE namespace = 'settings' AND key = 'proxyEnabled'")
.get() as { value?: string } | undefined;
if (globalRow?.value) {
try {
if (JSON.parse(globalRow.value) === false) return false;
} catch {
/* malformed → treat as enabled */
}
}
// Explicit per-connection "proxy off" → direct is intended.
const conn = db
.prepare("SELECT provider, proxy_enabled FROM provider_connections WHERE id = ?")
.get(connectionId) as { provider?: string | null; proxy_enabled?: number } | undefined;
if (conn && conn.proxy_enabled === 0) return false;
const provider = conn?.provider ?? null;
// A proxy is assigned to this connection at some scope, but every assigned
// proxy is dead (the alive filter would have resolved a live one already).
const dead = db
.prepare(
`SELECT 1 FROM proxy_assignments a JOIN proxy_registry p ON p.id = a.proxy_id
WHERE ((a.scope = 'account' AND a.scope_id = ?)
OR (a.scope = 'provider' AND a.scope_id = ?)
OR (a.scope = 'global'))
AND NOT ${PROXY_ALIVE_PREDICATE}
LIMIT 1`
)
.get(connectionId, provider);
return !!dead;
} catch {
return false;
}
}
export async function migrateLegacyProxyConfigToRegistry(options?: { force?: boolean }) {
const force = options?.force === true;
const db = getDbInstance();

View File

@@ -0,0 +1,77 @@
/**
* Pure, network-free decision for the proxy health scheduler (#6246).
*
* Separated from the sweep so the status/removal policy can be unit-tested
* exhaustively without any I/O. The sweep classifies each probe into a tri-state
* {@link ProxyProbeOutcome} and applies the returned {@link ProxyHealthDecision}.
*
* Policy (agreed for #6246):
* A — downgrade only after `removeAfter` CONSECUTIVE conclusive failures.
* B — an `inconclusive` probe (our own timeout/abort, or the probe TARGET
* erroring) never penalizes: it neither counts nor changes status.
* C — by DEFAULT (auto-remove off) the health check NEVER mutates a proxy's
* status. It only counts failures for logging. A proxy is downgraded to
* `inactive` (and removed) only when the operator opts in via
* PROXY_AUTO_REMOVE=true. This mirrors how accounts are only auto-disabled
* when the operator allows it — the operator owns their (often paid) proxies.
*/
export type ProxyProbeOutcome = "ok" | "fail" | "inconclusive";
export interface ProxyHealthDecisionInput {
/** Tri-state result of the reachability probe for this proxy. */
outcome: ProxyProbeOutcome;
/** Consecutive failure count recorded BEFORE this probe. */
priorFailures: number;
/** PROXY_AUTO_REMOVE === "true" — operator opted into status management. */
autoRemove: boolean;
/** Consecutive conclusive failures required before a downgrade/removal. */
removeAfter: number;
}
export interface ProxyHealthDecision {
/** New consecutive-failure count to persist for this proxy. */
failures: number;
/** Whether to drop this proxy from the consecutive-failure map. */
clearFailures: boolean;
/** Status to write, or `null` to leave the operator-controlled status untouched. */
setStatus: "active" | "inactive" | null;
/** Whether to auto-remove the proxy (only ever true when autoRemove is on). */
remove: boolean;
}
export function decideProxyHealthAction(input: ProxyHealthDecisionInput): ProxyHealthDecision {
const { outcome, priorFailures, autoRemove, removeAfter } = input;
const threshold = Number.isFinite(removeAfter) && removeAfter > 0 ? removeAfter : 3;
// B: inconclusive probes are neutral — do not touch count or status.
if (outcome === "inconclusive") {
return { failures: priorFailures, clearFailures: false, setStatus: null, remove: false };
}
// Success: reset the streak. Only (re)assert "active" when the operator has
// opted into status management; otherwise never touch the user's status (C).
if (outcome === "ok") {
return {
failures: 0,
clearFailures: true,
setStatus: autoRemove ? "active" : null,
remove: false,
};
}
// Conclusive failure.
const failures = priorFailures + 1;
// C: default mode only counts/logs — never downgrades.
if (!autoRemove) {
return { failures, clearFailures: false, setStatus: null, remove: false };
}
// A: downgrade + remove only once the consecutive threshold is reached.
if (failures >= threshold) {
return { failures, clearFailures: false, setStatus: "inactive", remove: true };
}
return { failures, clearFailures: false, setStatus: null, remove: false };
}

View File

@@ -14,8 +14,16 @@
import { deleteProxyById, listProxies, updateProxy } from "@/lib/localDb";
import { createProxyDispatcher, clearDispatcherCache } from "@omniroute/open-sse/utils/proxyDispatcher";
import { fetch as undiciFetch } from "undici";
import {
decideProxyHealthAction,
type ProxyProbeOutcome,
} from "./decision.ts";
const TEST_TIMEOUT_MS = 5000;
// #6246: a HEAD to the public probe target through a legit (often loaded) proxy
// can exceed a few seconds; the old 5s ceiling produced false negatives that
// flipped healthy proxies to inactive. Raise it and treat our own timeout as
// inconclusive (see testOneProxy) rather than a proxy failure.
const TEST_TIMEOUT_MS = 15000;
// Reachability probe target for proxy health checks. Configurable so operators
// can point it at an internal/self-hosted endpoint instead of the public default.
const TEST_URL = process.env.PROXY_HEALTH_TEST_URL || "https://httpbin.org/ip";
@@ -65,7 +73,21 @@ function isBackgroundServicesDisabled(): boolean {
return ["1", "true", "yes", "on"].includes(raw.trim().toLowerCase());
}
async function testOneProxy(proxy: { id: string; type: string; host: string; port: number }): Promise<boolean> {
/**
* Reachability probe for one proxy, classified into a tri-state so the pure
* decision layer can apply the #6246 policy:
* - "ok" — the proxy relayed and the target answered (<500).
* - "inconclusive" — NOT the proxy's fault: our own timeout/abort, or the probe
* TARGET returned a 5xx (the proxy connected fine). Never
* penalizes the proxy.
* - "fail" — a proxy-level connection error (refused/unreachable/TLS).
*/
async function testOneProxy(proxy: {
id: string;
type: string;
host: string;
port: number;
}): Promise<ProxyProbeOutcome> {
const proxyUrl = `${proxy.type}://${proxy.host}:${proxy.port}`;
const controller = new AbortController();
const timeout = setTimeout(() => controller.abort(), TEST_TIMEOUT_MS);
@@ -77,9 +99,13 @@ async function testOneProxy(proxy: { id: string; type: string; host: string; por
dispatcher,
headers: { "User-Agent": "OmniRoute/1.0" },
});
return resp.status < 500;
// A 5xx from the probe target means the proxy DID relay — the target is at
// fault, not the proxy. Do not penalize the proxy for that.
return resp.status < 500 ? "ok" : "inconclusive";
} catch {
return false;
// Our own deadline elapsed → inconclusive (slow, not necessarily dead).
// Any other error is a genuine proxy-level connection failure.
return controller.signal.aborted ? "inconclusive" : "fail";
} finally {
clearTimeout(timeout);
}
@@ -95,44 +121,54 @@ async function sweep(): Promise<void> {
let tested = 0;
let alive = 0;
let inconclusive = 0;
let removed = 0;
for (let i = 0; i < proxies.length; i += CONCURRENCY) {
const batch = proxies.slice(i, i + CONCURRENCY);
const results = await Promise.allSettled(
batch.map(async (proxy) => {
const ok = await testOneProxy(proxy);
return { id: proxy.id, ok };
const outcome = await testOneProxy(proxy);
return { id: proxy.id, outcome };
})
);
for (const result of results) {
if (result.status !== "fulfilled") continue;
const { id, ok } = result.value;
const { id, outcome } = result.value;
tested++;
if (outcome === "ok") alive++;
else if (outcome === "inconclusive") inconclusive++;
if (ok) {
alive++;
failureMap.delete(id);
await updateProxy(id, { status: "active" }).catch(() => {});
} else {
const failures = (failureMap.get(id) ?? 0) + 1;
failureMap.set(id, failures);
await updateProxy(id, { status: "inactive" }).catch(() => {});
const decision = decideProxyHealthAction({
outcome,
priorFailures: failureMap.get(id) ?? 0,
autoRemove,
removeAfter,
});
if (autoRemove && failures >= removeAfter) {
if (await deleteProxyById(id, { force: true }).catch(() => false)) {
failureMap.delete(id);
removed++;
try { clearDispatcherCache(); } catch { /* non-critical */ }
}
if (decision.clearFailures) failureMap.delete(id);
else failureMap.set(id, decision.failures);
// #6246 (policy C): only mutate the operator-owned status when the decision
// explicitly asks for it. By default (auto-remove off) setStatus is null, so
// a transient probe failure never flips a healthy proxy to inactive.
if (decision.setStatus) {
await updateProxy(id, { status: decision.setStatus }).catch(() => {});
}
if (decision.remove) {
if (await deleteProxyById(id, { force: true }).catch(() => false)) {
failureMap.delete(id);
removed++;
try { clearDispatcherCache(); } catch { /* non-critical */ }
}
}
}
}
console.log(
`${LOG_PREFIX} Sweep complete: ${tested} tested, ${alive} alive, ${removed} auto-removed`
`${LOG_PREFIX} Sweep complete: ${tested} tested, ${alive} alive, ${inconclusive} inconclusive, ${removed} auto-removed`
);
}

View File

@@ -28,6 +28,7 @@ import {
type AppliedProxySink,
} from "@omniroute/open-sse/utils/proxyFetch.ts";
import { resolveProxyForConnection } from "@/lib/localDb";
import { hasBlockingProxyAssignment } from "@/lib/db/proxies";
import {
CircuitBreakerOpenError,
getCircuitBreaker,
@@ -686,7 +687,22 @@ export function decideProxyResolutionFailure(
export async function safeResolveProxy(connectionId: string, apiKeyId?: string) {
try {
return await resolveProxyForConnection(connectionId, apiKeyId);
const resolved = await resolveProxyForConnection(connectionId, apiKeyId);
// #6246: a connection that resolves to DIRECT only because its assigned proxy
// is dead/inactive must fail closed — egressing on the real IP leaks it. Reuse
// the existing proxy-resolution-failure policy (blocks by default; PROXY_FAIL_OPEN
// opts back into direct). Explicit "proxy off" is not a leak (see the guard).
if (!(resolved as { proxy?: unknown } | null)?.proxy && hasBlockingProxyAssignment(connectionId)) {
return decideProxyResolutionFailure(
Object.assign(
new Error(
"PROXY_ASSIGNED_UNAVAILABLE: assigned proxy is inactive/unreachable; refusing to egress on a direct connection"
),
{ code: "PROXY_ASSIGNED_UNAVAILABLE" }
)
);
}
return resolved;
} catch (proxyErr) {
return decideProxyResolutionFailure(proxyErr);
}

View File

@@ -0,0 +1,141 @@
/**
* TDD — #6246 IP leak (part 1, fail-closed).
*
* When a proxy is ASSIGNED to a connection (account/provider/global scope) but is
* dead/inactive, `resolveProxyForConnection` returns a direct result (no alive
* proxy resolved). The chat path used to egress DIRECTLY in that case, leaking
* the operator's real IP. The fix is a fail-closed guard: if the only reason a
* connection resolves to direct is that its ASSIGNED proxy is dead, block instead
* of leaking. Explicit "proxy off" toggles are a deliberate direct choice and must
* NOT be treated as a leak.
*
* This test exercises the pure DB predicate `hasBlockingProxyAssignment`, which
* encodes exactly that decision (honoring the global + connection proxy toggles).
*/
import test 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(), "omniroute-proxy-6246-"));
process.env.DATA_DIR = TEST_DATA_DIR;
process.env.API_KEY_SECRET = "test-secret";
const core = await import("../../src/lib/db/core.ts");
const proxiesDb = await import("../../src/lib/db/proxies.ts");
const providersDb = await import("../../src/lib/db/providers.ts");
async function resetStorage() {
core.resetDbInstance();
fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true });
fs.mkdirSync(TEST_DATA_DIR, { recursive: true });
}
function setGlobalProxyEnabled(enabled: boolean) {
const db = core.getDbInstance();
db.prepare(
"INSERT OR REPLACE INTO key_value (namespace, key, value) VALUES ('settings', 'proxyEnabled', ?)"
).run(JSON.stringify(enabled));
}
async function makeConnection(): Promise<string> {
const conn = await providersDb.createProviderConnection({
provider: "openai",
authType: "apiKey",
name: `Conn ${Date.now()} ${Math.random()}`,
apiKey: "sk-test",
});
return (conn as { id: string }).id;
}
test.after(async () => {
core.resetDbInstance();
fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true });
});
test("BLOCKS: an account proxy assigned but marked inactive (the IP-leak case)", async () => {
await resetStorage();
const connId = await makeConnection();
const proxy = await proxiesDb.createProxy({
name: "Dead paid proxy",
type: "http",
host: "127.0.0.1",
port: 9001,
});
await proxiesDb.updateProxy(proxy!.id, { status: "inactive" });
await proxiesDb.assignProxyToScope("account", connId, proxy!.id);
assert.equal(
proxiesDb.hasBlockingProxyAssignment(connId),
true,
"a dead assigned proxy must block, not fall back to a direct egress"
);
});
test("ALLOWS DIRECT: a connection with no proxy assignment at all", async () => {
await resetStorage();
const connId = await makeConnection();
assert.equal(
proxiesDb.hasBlockingProxyAssignment(connId),
false,
"no assignment = user never configured a proxy = direct is legitimate"
);
});
test("NOT BLOCKING: an assigned proxy that is still ALIVE", async () => {
await resetStorage();
const connId = await makeConnection();
const proxy = await proxiesDb.createProxy({
name: "Live proxy",
type: "http",
host: "127.0.0.1",
port: 9002,
});
await proxiesDb.assignProxyToScope("account", connId, proxy!.id);
assert.equal(
proxiesDb.hasBlockingProxyAssignment(connId),
false,
"an alive assigned proxy resolves normally; nothing to block"
);
});
test("EXPLICIT DIRECT: global proxyEnabled=false is a deliberate choice, not a leak", async () => {
await resetStorage();
const connId = await makeConnection();
const proxy = await proxiesDb.createProxy({
name: "Dead proxy",
type: "http",
host: "127.0.0.1",
port: 9003,
});
await proxiesDb.updateProxy(proxy!.id, { status: "inactive" });
await proxiesDb.assignProxyToScope("account", connId, proxy!.id);
setGlobalProxyEnabled(false);
assert.equal(
proxiesDb.hasBlockingProxyAssignment(connId),
false,
"operator turned proxying off globally — direct is intended, do not block"
);
});
test("BLOCKS: a dead GLOBAL proxy assignment blocks any connection", async () => {
await resetStorage();
const connId = await makeConnection();
const proxy = await proxiesDb.createProxy({
name: "Dead global proxy",
type: "http",
host: "127.0.0.1",
port: 9004,
});
await proxiesDb.updateProxy(proxy!.id, { status: "error" });
await proxiesDb.assignProxyToScope("global", null, proxy!.id);
assert.equal(
proxiesDb.hasBlockingProxyAssignment(connId),
true,
"a dead global proxy assignment must block, not leak direct"
);
});

View File

@@ -0,0 +1,103 @@
/**
* TDD — #6246 proxy health regression (part 2, A+B+C).
*
* Before this fix the sweep marked a proxy `inactive` on the FIRST failed probe,
* unconditionally, and treated any error (including our own timeout or the probe
* TARGET being down) as a proxy failure. That flipped healthy paid proxies to
* inactive, which then dropped them from egress selection ("my proxies are not
* being used anymore").
*
* The decision is extracted into a pure, network-free function so it can be
* unit-tested exhaustively:
* A — only downgrade after `removeAfter` CONSECUTIVE conclusive failures.
* B — an inconclusive probe (our timeout / probe-target error) never penalizes.
* C — by default (PROXY_AUTO_REMOVE off) the health check NEVER mutates status;
* it only counts/logs. Status downgrade happens only when auto-remove is on.
*/
import test from "node:test";
import assert from "node:assert/strict";
const { decideProxyHealthAction } = await import("../../src/lib/proxyHealth/decision.ts");
test("C: default (autoRemove off) never mutates status on failure — only counts", () => {
const d = decideProxyHealthAction({
outcome: "fail",
priorFailures: 0,
autoRemove: false,
removeAfter: 3,
});
assert.equal(d.setStatus, null, "must not downgrade status by default");
assert.equal(d.remove, false);
assert.equal(d.failures, 1, "still counts the failure for logging");
});
test("C: default (autoRemove off) never downgrades even after many failures", () => {
const d = decideProxyHealthAction({
outcome: "fail",
priorFailures: 9,
autoRemove: false,
removeAfter: 3,
});
assert.equal(d.setStatus, null);
assert.equal(d.remove, false);
assert.equal(d.failures, 10);
});
test("A: with autoRemove on, does NOT downgrade before the consecutive threshold", () => {
const d = decideProxyHealthAction({
outcome: "fail",
priorFailures: 1, // this probe makes it 2, threshold is 3
autoRemove: true,
removeAfter: 3,
});
assert.equal(d.setStatus, null, "2 < 3 failures must not flip inactive");
assert.equal(d.remove, false);
assert.equal(d.failures, 2);
});
test("A: with autoRemove on, downgrades + removes at the consecutive threshold", () => {
const d = decideProxyHealthAction({
outcome: "fail",
priorFailures: 2, // this probe makes it 3 == threshold
autoRemove: true,
removeAfter: 3,
});
assert.equal(d.setStatus, "inactive");
assert.equal(d.remove, true);
assert.equal(d.failures, 3);
});
test("B: an inconclusive probe never penalizes (no count bump, no status change)", () => {
const d = decideProxyHealthAction({
outcome: "inconclusive",
priorFailures: 2,
autoRemove: true,
removeAfter: 3,
});
assert.equal(d.setStatus, null, "inconclusive must not touch status");
assert.equal(d.remove, false);
assert.equal(d.failures, 2, "failure streak is preserved, not incremented");
assert.equal(d.clearFailures, false);
});
test("ok: resets the failure streak; re-activates only when autoRemove manages status", () => {
const onAuto = decideProxyHealthAction({
outcome: "ok",
priorFailures: 2,
autoRemove: true,
removeAfter: 3,
});
assert.equal(onAuto.clearFailures, true);
assert.equal(onAuto.setStatus, "active");
assert.equal(onAuto.failures, 0);
const offAuto = decideProxyHealthAction({
outcome: "ok",
priorFailures: 2,
autoRemove: false,
removeAfter: 3,
});
assert.equal(offAuto.clearFailures, true);
assert.equal(offAuto.setStatus, null, "default mode never touches user-controlled status");
assert.equal(offAuto.failures, 0);
});