Skip to content

Commit 14e1e6b

Browse files
author
YourName
committed
fix(security): honor all outbound proxy variables
1 parent 64342c9 commit 14e1e6b

8 files changed

Lines changed: 82 additions & 35 deletions

File tree

docs-site/src/content/docs/reference/configuration.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,8 @@ transport. Without an outbound proxy, opencodex resolves the provider hostname o
8282
only to that validated address. HTTPS keeps the original hostname for Host, SNI, and certificate
8383
verification; certificate verification cannot be disabled by provider config.
8484

85-
When `HTTP_PROXY` or `HTTPS_PROXY` applies, these two operations keep Bun's native fetch so existing
86-
proxy behavior is not silently bypassed. URL/literal checks still run. Successful local DNS answers
85+
When `HTTP_PROXY`, `HTTPS_PROXY`, or `ALL_PROXY` applies, these two operations keep Bun's native fetch
86+
so existing proxy behavior is not silently bypassed. URL/literal checks still run. Successful local DNS answers
8787
are classified, but a local DNS failure is allowed through because proxy-only networks commonly
8888
delegate name resolution to the proxy. The proxy chooses the final route, DNS answer, and peer, so
8989
opencodex logs that this path cannot pin or verify the proxy-selected peer. This is an explicit
@@ -94,6 +94,8 @@ Private/local provider destinations require both `allowPrivateNetwork: true` and
9494
automatically. A LAN provider such as `192.168.1.50` must be added explicitly; otherwise connection
9595
tests and model discovery reject it with an actionable message instead of sending it to the proxy.
9696
Metadata and link-local destinations remain blocked even when `allowPrivateNetwork` is enabled.
97+
The safety guard accepts exact hosts, domain suffixes, optional ports, bracketed IPv6, and `*` in
98+
`NO_PROXY`; it does not interpret CIDR entries, so list each private provider host or address explicitly.
9799

98100
Both direct and proxied diagnostic paths reject redirects and report a credential-stripped target;
99101
configure the final provider URL directly. Ordinary provider requests, streaming responses, and

src/cli/doctor.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { dirname, join } from "node:path";
1313
import { getConfigDir, getConfigPath, readConfigDiagnostics, readPid, readRuntimePort, resolveEnvValue } from "../config";
1414
import { gracefulStopHost } from "../lib/process-control";
1515
import { maskAccountId } from "../lib/privacy";
16+
import { PROXY_ENV_KEYS, proxyEnvPresent } from "../lib/proxy-env";
1617
import { configuredAdminToken } from "../lib/admin-secrets";
1718
import { readCodexTokens } from "../codex/auth-collision";
1819
import { collectOrcaCodexHomeDiagnostic, resolveCodexHomeDir as resolveCodexHomeDirImpl, isWslRuntime, listWslWindowsCodexHomes, wslAutomountRoot, type CodexHomeDeps } from "../codex/home";
@@ -285,17 +286,15 @@ export function collectWslDualInstall(deps: WslDualInstallDeps = {}): WslDualIns
285286
};
286287
}
287288

288-
const PROXY_KEYS = ["HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY", "NO_PROXY"] as const;
289-
290289
export type ProxyEnvRow = { key: string; present: boolean };
291290
export type EnvMap = Record<string, string | undefined>;
292291

293292
/** Report only presence/absence of proxy env vars - never the value (it may
294293
* embed credentials). Checks both upper- and lower-case forms. */
295294
export function collectProxyEnv(env: EnvMap = process.env): ProxyEnvRow[] {
296-
return PROXY_KEYS.map(key => ({
295+
return PROXY_ENV_KEYS.map(key => ({
297296
key,
298-
present: !!(env[key]?.trim() || env[key.toLowerCase()]?.trim()),
297+
present: proxyEnvPresent(key, env),
299298
}));
300299
}
301300

src/lib/provider-outbound.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
resolvePublicAddresses,
77
} from "./destination-policy";
88
import { pinnedHttpGet } from "./pinned-http";
9+
import { outboundProxyConfigured } from "./proxy-env";
910
import { publicProviderBaseUrl } from "./provider-url";
1011

1112
type ProviderGetInit = Omit<RequestInit, "body" | "method" | "redirect">;
@@ -25,11 +26,8 @@ function pickPinnedAddress(addresses: Array<{ address: string; family: number }>
2526
return addresses.find(address => address.family === 4) ?? addresses[0]!;
2627
}
2728

28-
function configuredProxyFor(url: URL): boolean {
29-
const values = url.protocol === "https:"
30-
? [process.env.HTTPS_PROXY, process.env.https_proxy]
31-
: [process.env.HTTP_PROXY, process.env.http_proxy];
32-
return values.some(value => Boolean(value?.trim()));
29+
function configuredProxyFor(): boolean {
30+
return outboundProxyConfigured();
3331
}
3432

3533
function normalizeProxyHostname(hostname: string): string {
@@ -111,6 +109,9 @@ export async function providerOutboundGet(
111109
dependencies: ProviderOutboundDependencies = {},
112110
): Promise<Response> {
113111
if (provider.fetch) {
112+
// A caller-owned executor cannot be peer-pinned here. This branch keeps literal/config
113+
// checks and redirect blocking, but does not provide the resolved-address guarantees of
114+
// the built-in transport. Main-request migration must define that executor contract first.
114115
const assessment = assessUrlDestination(url);
115116
if (assessment?.kind === "metadata" || assessment?.kind === "link-local" || assessment?.kind === "unspecified") {
116117
throw new ProviderOutboundPolicyError(`provider URL targets ${assessment.detail}`);
@@ -125,7 +126,7 @@ export async function providerOutboundGet(
125126
return provider.fetch(url, { ...init, method: "GET", redirect: "manual" });
126127
}
127128
const parsed = new URL(url);
128-
const proxyConfigured = configuredProxyFor(parsed);
129+
const proxyConfigured = configuredProxyFor();
129130
const resolveAddresses = dependencies.resolveAddresses ?? resolvePublicAddresses;
130131
const pinnedGet = dependencies.pinnedGet ?? pinnedHttpGet;
131132
let resolved: Awaited<ReturnType<typeof resolvePublicAddresses>>;

src/lib/proxy-env.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
export const OUTBOUND_PROXY_ENV_KEYS = ["HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY"] as const;
2+
export const PROXY_ENV_KEYS = [...OUTBOUND_PROXY_ENV_KEYS, "NO_PROXY"] as const;
3+
4+
export type ProxyEnvKey = typeof PROXY_ENV_KEYS[number];
5+
export type ProxyEnvMap = Record<string, string | undefined>;
6+
7+
export function proxyEnvPresent(
8+
key: ProxyEnvKey,
9+
env: ProxyEnvMap = process.env,
10+
): boolean {
11+
return Boolean(env[key]?.trim() || env[key.toLowerCase()]?.trim());
12+
}
13+
14+
export function outboundProxyConfigured(
15+
env: ProxyEnvMap = process.env,
16+
): boolean {
17+
return OUTBOUND_PROXY_ENV_KEYS.some(key => proxyEnvPresent(key, env));
18+
}

structure/04_transports-and-sidecars.md

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,17 @@
44

55
Provider connection tests and live model discovery share the GET-only provider outbound wrapper.
66
Direct HTTP(S) resolves once and pins the validated address; HTTPS preserves the original Host/SNI
7-
and always verifies certificates. Proxy-configured requests stay on Bun fetch so HTTP(S)_PROXY and
8-
NO_PROXY semantics remain authoritative. The wrapper classifies successful local DNS answers, but
7+
and always verifies certificates. Proxy-configured requests stay on Bun fetch so HTTP(S)_PROXY,
8+
ALL_PROXY, and NO_PROXY semantics remain authoritative. The wrapper classifies successful local DNS answers, but
99
only a typed DNS-resolution failure degrades to proxy resolution; every literal, metadata, and
1010
resolved-address policy error still rejects. Proxy mode logs once that the proxy-selected peer
1111
cannot be pinned. Private destinations additionally require allowPrivateNetwork plus NO_PROXY.
1212

1313
Both paths reject redirects and expose only credential-stripped final-address guidance. This phase
1414
does not cover ordinary requests, streaming, retries, or per-hop redirect review on those paths.
15+
Caller-owned `provider.fetch` executors are also deferred: they receive literal/config checks and
16+
redirect blocking, but cannot inherit DNS classification or peer pinning without a verified-peer
17+
executor contract. Main-request migration must not treat that branch as fixed-transport equivalent.
1518

1619
## Responses HTTP/SSE
1720

tests/fixtures/provider-outbound-e2e.ts

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,12 @@ import type { AddressInfo } from "node:net";
33
import { saveConfig } from "../../src/config";
44
import { fetchProviderModels } from "../../src/codex/catalog/provider-fetch";
55
import { providerOutboundGet } from "../../src/lib/provider-outbound";
6+
import { PROXY_ENV_KEYS } from "../../src/lib/proxy-env";
67
import { handleManagementAPI } from "../../src/server/management-api";
78
import type { OcxConfig } from "../../src/types";
89
import { ManagementRequest as Request } from "../helpers/management-auth";
910

10-
const proxyKeys = [
11-
"HTTP_PROXY",
12-
"HTTPS_PROXY",
13-
"ALL_PROXY",
14-
"http_proxy",
15-
"https_proxy",
16-
"all_proxy",
17-
] as const;
11+
const proxyKeys = PROXY_ENV_KEYS.flatMap(key => [key, key.toLowerCase()]);
1812

1913
async function listen(server: ReturnType<typeof createServer>): Promise<number> {
2014
await new Promise<void>((resolve, reject) => {
@@ -95,6 +89,15 @@ try {
9589
models: [],
9690
}, 0);
9791

92+
for (const key of proxyKeys) delete process.env[key];
93+
process.env.ALL_PROXY = proxyUrl;
94+
const allProxyResponse = await providerOutboundGet(
95+
"all-proxy",
96+
{ baseUrl: "http://all-proxy-only.invalid/v1", allowPrivateNetwork: false },
97+
"http://all-proxy-only.invalid/v1/models",
98+
);
99+
const allProxy = { status: allProxyResponse.status, body: await allProxyResponse.text() };
100+
98101
const localConfig = {
99102
port: 0,
100103
hostname: "127.0.0.1",
@@ -108,6 +111,8 @@ try {
108111
},
109112
},
110113
} as OcxConfig;
114+
process.env.NO_PROXY = "localhost,127.0.0.1,::1,[::1]";
115+
process.env.no_proxy = "localhost,127.0.0.1,::1,[::1]";
111116
const managementNoProxy = await probe(localConfig, "local");
112117

113118
for (const key of proxyKeys) delete process.env[key];
@@ -122,6 +127,7 @@ try {
122127

123128
console.log(JSON.stringify({
124129
outbound,
130+
allProxy,
125131
managementProxy,
126132
proxyModels: proxyModels.map(model => model.id),
127133
managementNoProxy,

tests/provider-outbound.test.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,9 @@ import { mkdtempSync, rmSync } from "node:fs";
33
import { tmpdir } from "node:os";
44
import { join } from "node:path";
55
import type { ProviderOutboundDependencies } from "../src/lib/provider-outbound";
6+
import { PROXY_ENV_KEYS } from "../src/lib/proxy-env";
67

7-
const proxyKeys = ["HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY", "http_proxy", "https_proxy", "no_proxy"] as const;
8+
const proxyKeys = PROXY_ENV_KEYS.flatMap(key => [key, key.toLowerCase()]);
89
const originalProxyEnv = Object.fromEntries(proxyKeys.map(key => [key, process.env[key]]));
910

1011
afterEach(() => {
@@ -162,8 +163,12 @@ describe("provider outbound GET transport", () => {
162163
new Response(child.stderr).text(),
163164
child.exited,
164165
]);
166+
if (exitCode !== 0) {
167+
throw new Error(`provider outbound fixture exited ${exitCode}: ${stderr.trim()}`);
168+
}
165169
const result = JSON.parse(stdout.trim()) as {
166170
outbound: { status: number; body: string };
171+
allProxy: { status: number; body: string };
167172
managementProxy: Record<string, unknown>;
168173
proxyModels: string[];
169174
managementNoProxy: Record<string, unknown>;
@@ -173,11 +178,14 @@ describe("provider outbound GET transport", () => {
173178
providerRequests: string[];
174179
};
175180

176-
expect(exitCode).toBe(0);
177181
expect(result.outbound).toEqual({
178182
status: 200,
179183
body: '{"data":[{"id":"proxied-model"}]}',
180184
});
185+
expect(result.allProxy).toEqual({
186+
status: 200,
187+
body: '{"data":[{"id":"proxied-model"}]}',
188+
});
181189
expect(result.managementProxy.ok).toBe(false);
182190
expect(String(result.managementProxy.error)).toContain("returned 302 redirect");
183191
expect(String(result.managementProxy.error)).toContain("http://final.example/v1/models");
@@ -191,6 +199,7 @@ describe("provider outbound GET transport", () => {
191199
"http://proxy-only.invalid/v1/models",
192200
"http://connection-proxy.invalid/v1/models",
193201
"http://proxy-models.invalid/v1/models",
202+
"http://all-proxy-only.invalid/v1/models",
194203
]);
195204
expect(result.providerRequests).toEqual(["/v1/models", "/v1/models", "/v1/models"]);
196205
expect(stderr).toContain("cannot be pinned locally");

tests/subagent-fallback-handle-responses.test.ts

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -296,29 +296,29 @@ describe("subagent fallback without primary auth cooldown failure", () => {
296296
expect(response.status).not.toBe(429);
297297
});
298298

299-
test("final-route auth failure does not leave a primary probe lease", async () => {
299+
test("final-route direct auth failure does not acquire a pool probe lease", async () => {
300300
const now = 1_800_000_000_000;
301301
Date.now = () => now;
302302
installPoolCredential("pool-a", "pool_acc", now);
303303
const cfg: OcxConfig = {
304304
port: 0,
305-
defaultProvider: "openai",
305+
defaultProvider: "xai",
306306
activeCodexAccountId: "pool-a",
307307
autoSwitchThreshold: 80,
308-
subagentModelFallback: ["openai-direct/gpt-5.5"],
308+
subagentModelFallback: ["gpt-5.5"],
309309
providers: {
310310
openai: {
311-
adapter: "openai-responses",
312-
baseUrl: "https://chatgpt.com/backend-api/codex",
313-
authMode: "forward",
314-
codexAccountMode: "pool",
315-
},
316-
"openai-direct": {
317311
adapter: "openai-responses",
318312
baseUrl: "https://chatgpt.com/backend-api/codex",
319313
authMode: "forward",
320314
codexAccountMode: "direct",
321315
},
316+
xai: {
317+
adapter: "openai-chat",
318+
baseUrl: "https://api.x.ai/v1",
319+
authMode: "key",
320+
apiKey: "xai-test",
321+
},
322322
},
323323
codexAccounts: [
324324
{ id: "main", email: "main@example.test", isMain: true },
@@ -328,8 +328,16 @@ describe("subagent fallback without primary auth cooldown failure", () => {
328328
updateAccountQuota("pool-a", 95, undefined, 20);
329329
const resetAt = Math.floor((now + 4 * 24 * 60 * 60_000) / 1000);
330330
recordCodexUpstreamOutcome(cfg, "pool-a", 429, { resetAt, now });
331+
const { noteSubagentModelFailure } = await import("../src/codex/subagent-model-fallback");
332+
noteSubagentModelFailure("xai/grok-4.5", "429", cfg);
331333

332-
// Omit authorization so direct-mode final auth fails — primary never leased.
334+
let fetchCalls = 0;
335+
globalThis.fetch = (async () => {
336+
fetchCalls += 1;
337+
throw new Error("must not dispatch");
338+
}) as typeof fetch;
339+
340+
// Omit authorization so the canonical Direct final route fails before dispatch.
333341
const response = await handleResponses(
334342
new Request("http://localhost/v1/responses", {
335343
method: "POST",
@@ -338,7 +346,7 @@ describe("subagent fallback without primary auth cooldown failure", () => {
338346
"x-openai-subagent": "collab_spawn",
339347
},
340348
body: JSON.stringify({
341-
model: "gpt-5.6-sol",
349+
model: "xai/grok-4.5",
342350
input: readableAgentInput(),
343351
stream: false,
344352
}),
@@ -348,6 +356,7 @@ describe("subagent fallback without primary auth cooldown failure", () => {
348356
);
349357

350358
expect(response.status).toBe(401);
359+
expect(fetchCalls).toBe(0);
351360
expect(getCodexUpstreamHealth("pool-a")?.probeLeaseId).toBeUndefined();
352361
});
353362
});

0 commit comments

Comments
 (0)