From 06334f76cfc86e064d536ba29d26d9a6dd602386 Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 20 Sep 2026 14:20:58 +0900 Subject: [PATCH 1/4] fix(transport): decide DNS pinning by whether the proxy applies to the request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Carries the proxy-applies decision onto current dev and tightens the cases exact-head review found uncovered. The DNS-pinned provider transport used to leave pinning whenever ANY proxy variable was present (outboundProxyConfigured). Presence is not application: with only a scheme-mismatched variable set, an https: request still downgraded to the unpinned fetch even though Bun fetch would never use that proxy for it, and a local DNS failure silently degraded to the same unpinned fetch. The benchmark/fake-IP admission, the transport downgrade, the DNS-failure degradation and the private-network NO_PROXY demand now key on one snapshot: whether a proxy actually applies to this request (a usable, scheme-matched proxy variable that NO_PROXY does not exempt). effectiveProxyFor models that decision. Two corrections to the carried model: a non-SOCKS ALL_PROXY counts for plain http: targets on every CI platform, not just POSIX — the provider-outbound e2e drives that exact request through the proxy on Linux, macOS and Windows — and a present-but-unusable scheme-matched variable fails closed instead of falling through to ALL_PROXY, because no usable proxy is guaranteed either way and keeping the pinned transport is the safe direction. The Mihomo IPv6 fake-IP gate deliberately does not move to the new snapshot. Its documented condition is stricter — a scheme-matched variable or a SOCKS5 ALL_PROXY, with a non-SOCKS ALL_PROXY never counting — and admission pins the fetch to that value explicitly, so it now reads schemeMatchedProxyFor. That keeps every documented and tested #3462 behaviour byte-identical, including a SOCKS URL written into a scheme variable remaining a valid explicit binding. Regressions pin a scheme-mismatched variable keeping the pinned transport with benchmark answers rejected, a NO_PROXY match keeping it, a mismatched variable not demanding NO_PROXY for private providers, a DNS failure with only a mismatched variable surfacing instead of degrading, and the degradation surviving for the proxy that genuinely applies. Every caller of providerOutboundGet/Post — provider discovery, the model-catalog gather, quota probes, ollama show and the management model-refresh routes — shares this single decision function. The main inference dispatch and OAuth token exchange do not use the DNS-pinned transport today and are unchanged. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> --- src/lib/provider-outbound.ts | 32 ++-- src/lib/proxy-env.ts | 60 ++++++- tests/providers/provider-outbound.test.ts | 188 +++++++++++++++++++++- 3 files changed, 265 insertions(+), 15 deletions(-) diff --git a/src/lib/provider-outbound.ts b/src/lib/provider-outbound.ts index 799c8d69fa6..d87893cfa4f 100644 --- a/src/lib/provider-outbound.ts +++ b/src/lib/provider-outbound.ts @@ -7,7 +7,7 @@ import { resolvePublicAddresses, } from "./destination-policy"; import { pinnedHttpGet, pinnedHttpPost } from "./pinned-http"; -import { configuredOutboundFetch, effectiveProxyFor, noProxyMatches, normalizeProxyHostname, outboundProxyConfigured } from "./proxy-env"; +import { configuredOutboundFetch, effectiveProxyFor, noProxyMatches, normalizeProxyHostname, schemeMatchedProxyFor } from "./proxy-env"; import { publicProviderBaseUrl } from "./provider-url"; type ProviderGetInit = Omit; @@ -186,13 +186,23 @@ async function providerOutboundRequest( return provider.fetch(url, { ...init, method, redirect: "manual" }); } const parsed = postUrl ?? new URL(url); - const proxyConfigured = outboundProxyConfigured(); - // Snapshot the scheme-matched proxy once, before the DNS await, so admission and transport - // below reason about the same value. `null` here means "no proxy fetch would actually use", - // even if some other proxy variable is set. + // Snapshot the proxy fetch would actually use once, before the DNS await, so admission + // and transport below reason about the same value. `null` here means "no proxy fetch + // would actually use", even if some other proxy variable is set. const effectiveProxy = effectiveProxyFor(parsed); + // The request leaves the DNS-pinned transport only when a proxy will actually carry it: + // a proxy variable fetch would use for this URL that NO_PROXY does not exempt. + // A scheme-mismatched or unusable variable, a NO_PROXY match, or an ALL_PROXY + // this target's scheme cannot use must not downgrade pinning or admit + // proxy-only DNS answers. + const proxyApplies = effectiveProxy !== null && !noProxyMatches(parsed); const isCanonicalUrl = dependencies.isCanonicalUrl ?? (() => false); - const allowMihomoIpv6FakeIp = (effectiveProxy !== null && !noProxyMatches(parsed)) + // The IPv6 fake-IP gate keeps its stricter documented condition — a + // scheme-matched variable or a SOCKS5 ALL_PROXY, never a non-SOCKS + // ALL_PROXY — even when proxyApplies admits one for the transport + // decision, because admission binds the fetch to this value explicitly. + const bindingProxy = schemeMatchedProxyFor(parsed); + const allowMihomoIpv6FakeIp = (bindingProxy !== null && !noProxyMatches(parsed)) || transparentFakeIpException(url, parsed, isCanonicalUrl, name); const resolveAddresses = dependencies.resolveAddresses ?? resolvePublicAddresses; const pinnedGet = dependencies.pinnedGet ?? pinnedHttpGet; @@ -216,7 +226,7 @@ async function providerOutboundRequest( // proof is on the final request URL — not the provider name — because an // OAuth/forward name matches any baseUrl by design while the bearer is // pinned to the registry destination independently. - allowBenchmarkAddresses: (proxyConfigured && !noProxyMatches(parsed)) + allowBenchmarkAddresses: proxyApplies || transparentFakeIpException(url, parsed, isCanonicalUrl, name), // Mihomo IPv6 fake-IP (fdfe:dcba:9876::/48) answers are admitted either when bound // to a scheme-matched proxy (#3462) or under the TUN transparency exception for a @@ -229,21 +239,21 @@ async function providerOutboundRequest( if (!dnsResolutionFailed) { throw new ProviderOutboundPolicyError(error instanceof Error ? error.message : "provider destination was blocked"); } - if (!proxyConfigured) throw error; + if (!proxyApplies) throw error; warnProxyBoundaryOnce(); warnProxyDnsDegradationOnce(); return configuredOutboundFetch(url, { ...init, method, redirect: "manual" }); } // A canonical TUN exception with no scheme-matched proxy must retain the // validated address, even when an unrelated HTTP_PROXY/ALL_PROXY is present. - if (proxyConfigured && !resolved.privateNetwork && (effectiveProxy !== null || !allowMihomoIpv6FakeIp)) { + if (proxyApplies && !resolved.privateNetwork) { warnProxyBoundaryOnce(); // When the Mihomo exception could have admitted an answer, pin the transport to the // proxy the admission assumed instead of letting fetch re-infer it from the environment. - const proxy = (allowMihomoIpv6FakeIp && effectiveProxy) ? effectiveProxy : undefined; + const proxy = (allowMihomoIpv6FakeIp && bindingProxy) ? bindingProxy : undefined; return configuredOutboundFetch(url, { ...init, method, redirect: "manual", ...(proxy ? { proxy } : {}) }); } - if (proxyConfigured && resolved.privateNetwork && !noProxyMatches(parsed)) { + if (proxyApplies && resolved.privateNetwork) { const hostname = normalizeProxyHostname(parsed.hostname); throw new Error( `provider URL resolves to a private-network destination; add ${hostname} to NO_PROXY before using allowPrivateNetwork with an outbound proxy`, diff --git a/src/lib/proxy-env.ts b/src/lib/proxy-env.ts index aebf62730bc..3c3e80052dd 100644 --- a/src/lib/proxy-env.ts +++ b/src/lib/proxy-env.ts @@ -86,12 +86,30 @@ export function outboundProxyConfigured( return OUTBOUND_PROXY_ENV_KEYS.some(key => proxyEnvPresent(key, env)); } +/** + * The value when `raw` is a proxy URL Bun fetch can actually use, else null. + * Bun rejects unparseable values and non-http(s) schemes (UnsupportedProxyProtocol), + * so admitting them as "the proxy that applies" would only downgrade DNS pinning. + */ +function usableHttpProxyUrl(raw: string | undefined): string | null { + if (!raw) return null; + try { + const scheme = new URL(raw).protocol; + return scheme === "http:" || scheme === "https:" ? raw : null; + } catch { + return null; + } +} + /** * The proxy URL selected by configured outbound fetch for `url`, or null when none applies. * * Bun selects by scheme: `HTTPS_PROXY` for `https:` targets, `HTTP_PROXY` for `http:`. - * A SOCKS5 `ALL_PROXY` is selected by the explicit wrapper first; other ALL_PROXY - * schemes remain excluded because the native HTTP fetch does not honor them. + * A SOCKS5 `ALL_PROXY` is selected by the explicit wrapper first. A non-SOCKS + * `ALL_PROXY` is still honoured by the native fetch for plain `http:` targets on + * every platform the CI matrix covers — the provider-outbound e2e drives exactly that + * request through the proxy on Linux, macOS and Windows. For `https:` targets the + * SOCKS wrapper remains the only `ALL_PROXY` route this module counts. * Presence of *some* proxy variable (`outboundProxyConfigured`) is not that guarantee. */ export function effectiveProxyFor( @@ -107,8 +125,44 @@ export function effectiveProxyFor( // The installed SOCKS wrapper takes this route before Bun sees scheme proxies. const socksProxy = socks5ProxyFromEnv(env); if (socksProxy) return socksProxy; + const schemeValue = env[key]?.trim() || env[key.toLowerCase()]?.trim(); + if (schemeValue) { + // A present but unusable scheme-matched variable fails closed: it is not a + // proxy Bun fetch can use, and it must not fall through to ALL_PROXY either. + // If Bun would have used ALL_PROXY here, keeping the DNS-pinned transport is + // the safe direction; if it would not, this is exactly right. + return usableHttpProxyUrl(schemeValue); + } + if (url.protocol !== "http:") return null; + return usableHttpProxyUrl(env.ALL_PROXY?.trim() || env.all_proxy?.trim()); +} + +/** + * The proxy a request can be explicitly bound to for fake-IP admission, or + * null: a scheme-matched variable or a SOCKS5 `ALL_PROXY`. This is the + * stricter documented gate for Mihomo IPv6 fake-IP answers — a non-SOCKS + * `ALL_PROXY` does not count here even when `effectiveProxyFor` reports it, + * because admission pins the transport to the returned value and the gate's + * contract is stated in those terms. The scheme-matched value counts only as + * a usable binding — a SOCKS or http(s) URL; anything else would admit a + * fake-IP answer nothing can resolve. + */ +export function schemeMatchedProxyFor( + url: URL, + env: ProxyEnvMap = process.env, +): string | null { + const key: ProxyEnvKey | null = url.protocol === "https:" + ? "HTTPS_PROXY" + : url.protocol === "http:" + ? "HTTP_PROXY" + : null; + if (!key) return null; + const socksProxy = socks5ProxyFromEnv(env); + if (socksProxy) return socksProxy; const value = env[key]?.trim() || env[key.toLowerCase()]?.trim(); - return value ? value : null; + if (!value) return null; + if (isSocks5ProxyUrl(value)) return value; + return usableHttpProxyUrl(value); } export function isSocks5ProxyUrl(proxy: string): boolean { diff --git a/tests/providers/provider-outbound.test.ts b/tests/providers/provider-outbound.test.ts index c0d5f59a2a9..a56ca4aa105 100644 --- a/tests/providers/provider-outbound.test.ts +++ b/tests/providers/provider-outbound.test.ts @@ -3,6 +3,7 @@ import { mkdtempSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { pathToFileURL } from "node:url"; +import { DestinationDnsResolutionError } from "../../src/lib/destination-policy"; import type { ProviderOutboundDependencies } from "../../src/lib/provider-outbound"; import { PROXY_ENV_KEYS } from "../../src/lib/proxy-env"; import { removeTreeWithRetry } from "../helpers/remove-tree"; @@ -234,6 +235,153 @@ describe("provider outbound GET transport", () => { } }); + test("scheme-mismatched proxy variables keep the DNS-pinned transport", async () => { + const originalFetch = globalThis.fetch; + const fetchMock = mock(async () => new Response("unexpected", { status: 500 })) as typeof fetch; + globalThis.fetch = fetchMock; + try { + for (const { url, proxyKey } of [ + { url: "http://provider.example/v1/models", proxyKey: "HTTPS_PROXY" }, + { url: "https://provider.example/v1/models", proxyKey: "HTTP_PROXY" }, + ] as const) { + for (const key of proxyKeys) delete process.env[key]; + process.env[proxyKey] = "http://127.0.0.1:9"; + const { providerOutboundGet } = await import("../../src/lib/provider-outbound"); + const resolveOptions: { allowBenchmarkAddresses?: boolean }[] = []; + const { dependencies, captured } = directDependencies(new Response(null, { status: 204 })); + dependencies.resolveAddresses = mock(async (_url: string, options?: { allowBenchmarkAddresses?: boolean }) => { + resolveOptions.push({ allowBenchmarkAddresses: options?.allowBenchmarkAddresses }); + return { + hostname: "provider.example", + addresses: [{ address: "93.184.216.34", family: 4 }], + privateNetwork: false, + }; + }) as ProviderOutboundDependencies["resolveAddresses"]; + + const response = await providerOutboundGet( + "custom", + { baseUrl: new URL(url).origin + "/v1" }, + url, + {}, + dependencies, + ); + + expect(response.status).toBe(204); + expect(captured.address).toBe("93.184.216.34"); + expect(resolveOptions).toEqual([{ allowBenchmarkAddresses: false }]); + } + expect(fetchMock).not.toHaveBeenCalled(); + } finally { + globalThis.fetch = originalFetch; + } + }); + + test("a NO_PROXY match keeps the request on the DNS-pinned transport", async () => { + for (const key of proxyKeys) delete process.env[key]; + process.env.HTTPS_PROXY = "http://127.0.0.1:9"; + process.env.NO_PROXY = "provider.example"; + const originalFetch = globalThis.fetch; + const fetchMock = mock(async () => new Response("unexpected", { status: 500 })) as typeof fetch; + globalThis.fetch = fetchMock; + try { + const { providerOutboundGet } = await import("../../src/lib/provider-outbound"); + const { dependencies, captured } = directDependencies(new Response(null, { status: 204 })); + + const response = await providerOutboundGet( + "custom", + { baseUrl: "https://provider.example/v1" }, + "https://provider.example/v1/models", + {}, + dependencies, + ); + + expect(response.status).toBe(204); + expect(captured.address).toBe("93.184.216.34"); + expect(fetchMock).not.toHaveBeenCalled(); + } finally { + globalThis.fetch = originalFetch; + } + }); + + test("a scheme-mismatched proxy variable does not demand NO_PROXY for private providers", async () => { + for (const key of proxyKeys) delete process.env[key]; + process.env.HTTP_PROXY = "http://127.0.0.1:9"; + const { providerOutboundGet } = await import("../../src/lib/provider-outbound"); + const { dependencies, captured } = directDependencies(new Response(null, { status: 200 }), { + privateNetwork: true, + address: "192.168.1.50", + }); + + const response = await providerOutboundGet( + "ollama-lan", + { baseUrl: "https://ollama.lan:11434/v1", allowPrivateNetwork: true }, + "https://ollama.lan:11434/v1/models", + {}, + dependencies, + ); + + expect(response.status).toBe(200); + expect(captured.address).toBe("192.168.1.50"); + }); + + test("DNS failure with only a scheme-mismatched proxy rethrows instead of degrading to an unpinned fetch", async () => { + for (const key of proxyKeys) delete process.env[key]; + process.env.HTTP_PROXY = "http://127.0.0.1:9"; + const originalFetch = globalThis.fetch; + const fetchMock = mock(async () => new Response("unexpected", { status: 500 })) as typeof fetch; + globalThis.fetch = fetchMock; + try { + const { providerOutboundGet } = await import("../../src/lib/provider-outbound"); + const { dependencies } = directDependencies(new Response(null, { status: 204 })); + dependencies.resolveAddresses = mock(async () => { + throw new DestinationDnsResolutionError("getaddrinfo ENOTFOUND provider.example"); + }) as ProviderOutboundDependencies["resolveAddresses"]; + + // A proxy variable fetch would never use for this https: target must not + // license the unpinned degradation path: the DNS failure surfaces as-is. + await expect(providerOutboundGet( + "custom", + { baseUrl: "https://provider.example/v1" }, + "https://provider.example/v1/models", + {}, + dependencies, + )).rejects.toBeInstanceOf(DestinationDnsResolutionError); + expect(fetchMock).not.toHaveBeenCalled(); + } finally { + globalThis.fetch = originalFetch; + } + }); + + test("DNS failure behind a scheme-matched proxy still degrades to the proxy fetch", async () => { + for (const key of proxyKeys) delete process.env[key]; + process.env.HTTPS_PROXY = "http://127.0.0.1:9"; + const originalFetch = globalThis.fetch; + const fetchMock = mock(async () => new Response('{"data":[]}', { status: 200 })) as typeof fetch; + globalThis.fetch = fetchMock; + try { + const { providerOutboundGet } = await import("../../src/lib/provider-outbound"); + const { dependencies } = directDependencies(new Response(null, { status: 204 })); + dependencies.resolveAddresses = mock(async () => { + throw new DestinationDnsResolutionError("getaddrinfo ENOTFOUND provider.example"); + }) as ProviderOutboundDependencies["resolveAddresses"]; + + // The proxy the request will actually use may resolve names the local + // resolver cannot, so the degradation stays for the route that applies. + const response = await providerOutboundGet( + "custom", + { baseUrl: "https://provider.example/v1" }, + "https://provider.example/v1/models", + {}, + dependencies, + ); + + expect(response.status).toBe(200); + expect(fetchMock).toHaveBeenCalledTimes(1); + } finally { + globalThis.fetch = originalFetch; + } + }); + test("built-in ollama admits loopback discovery without an explicit allowPrivateNetwork flag (#758)", async () => { for (const key of proxyKeys) delete process.env[key]; const { providerOutboundGet } = await import("../../src/lib/provider-outbound"); @@ -571,7 +719,7 @@ describe("#3462 Mihomo IPv6 fake-IP admission is gated on the scheme-matched pro }); describe("effectiveProxyFor picks the variable Bun fetch actually honours", () => { - test("scheme-matched selection; HTTP ALL_PROXY is never consulted", async () => { + test("scheme-matched selection; HTTP ALL_PROXY only counts for http: targets", async () => { const { effectiveProxyFor } = await import("../../src/lib/proxy-env"); const https = new URL("https://opencode.ai/zen/v1/models"); const http = new URL("http://ollama.lan:11434/v1/models"); @@ -582,9 +730,47 @@ describe("effectiveProxyFor picks the variable Bun fetch actually honours", () = expect(effectiveProxyFor(https, { ALL_PROXY: "socks5://127.0.0.1:1080" })).toBe("socks5://127.0.0.1:1080"); expect(effectiveProxyFor(http, { HTTP_PROXY: "http://p:5" })).toBe("http://p:5"); expect(effectiveProxyFor(http, { HTTPS_PROXY: "http://p:6" })).toBeNull(); + // Bun's native fetch honours a non-SOCKS ALL_PROXY for plain http: targets on + // every platform the CI matrix covers (the provider-outbound e2e proves the + // request reaches the proxy); https: targets only ever use the socks5 wrapper. + expect(effectiveProxyFor(http, { ALL_PROXY: "http://p:7" })).toBe("http://p:7"); + expect(effectiveProxyFor(http, { all_proxy: "http://p:13" })).toBe("http://p:13"); + expect(effectiveProxyFor(http, { ALL_PROXY: "ftp://p:8" })).toBeNull(); + expect(effectiveProxyFor(http, { ALL_PROXY: "http://" })).toBeNull(); + // A malformed or non-http(s) scheme-matched variable is not a proxy Bun fetch + // can use either: it must not count as "the proxy that applies". + expect(effectiveProxyFor(http, { HTTP_PROXY: "http://" })).toBeNull(); + expect(effectiveProxyFor(http, { HTTP_PROXY: "not a url" })).toBeNull(); + expect(effectiveProxyFor(https, { HTTPS_PROXY: "http://" })).toBeNull(); + expect(effectiveProxyFor(https, { HTTPS_PROXY: "socks5://p:9" })).toBeNull(); expect(effectiveProxyFor(https, { HTTPS_PROXY: " " })).toBeNull(); + // A present-but-unusable scheme-matched variable fails closed rather than + // falling through to ALL_PROXY: no usable proxy is guaranteed either way, + // so the DNS-pinned transport must stay. + expect(effectiveProxyFor(http, { HTTP_PROXY: "not a url", ALL_PROXY: "http://p:10" })).toBeNull(); + expect(effectiveProxyFor(https, { HTTPS_PROXY: "ftp://p:11", ALL_PROXY: "http://p:12" })).toBeNull(); expect(effectiveProxyFor(new URL("ftp://x/"), { HTTPS_PROXY: "http://p:7", HTTP_PROXY: "http://p:7" })).toBeNull(); }); + + test("schemeMatchedProxyFor keeps the stricter fake-IP binding gate", async () => { + const { schemeMatchedProxyFor } = await import("../../src/lib/proxy-env"); + const https = new URL("https://opencode.ai/zen/v1/models"); + const http = new URL("http://ollama.lan:11434/v1/models"); + expect(schemeMatchedProxyFor(https, { HTTPS_PROXY: "http://p:1" })).toBe("http://p:1"); + expect(schemeMatchedProxyFor(http, { HTTP_PROXY: "http://p:2" })).toBe("http://p:2"); + // A SOCKS URL in a scheme-matched variable is a valid explicit binding. + expect(schemeMatchedProxyFor(https, { HTTPS_PROXY: "socks5://p:3" })).toBe("socks5://p:3"); + expect(schemeMatchedProxyFor(https, { ALL_PROXY: "socks5://p:4" })).toBe("socks5://p:4"); + // A non-SOCKS ALL_PROXY never counts for the binding gate, even for the + // http: targets effectiveProxyFor reports it for. + expect(schemeMatchedProxyFor(http, { ALL_PROXY: "http://p:5" })).toBeNull(); + expect(schemeMatchedProxyFor(https, { ALL_PROXY: "http://p:6" })).toBeNull(); + expect(schemeMatchedProxyFor(https, { HTTP_PROXY: "http://p:7" })).toBeNull(); + // An unusable scheme-matched value is not a binding either: admitting a + // fake-IP answer against it would pin-connect to an address nothing resolves. + expect(schemeMatchedProxyFor(https, { HTTPS_PROXY: "not a url" })).toBeNull(); + expect(schemeMatchedProxyFor(https, { HTTPS_PROXY: "ftp://p:8" })).toBeNull(); + }); }); describe("provider outbound default User-Agent", () => { From 42f09837f985f8ff89b3d9620521b177ab82bcb1 Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 20 Sep 2026 14:20:58 +0900 Subject: [PATCH 2/4] fix(integrations): reject symlinked managed write targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Carries the managed-target symlink refusal onto current dev, without the src/config.ts re-export — that file sits exactly at its file-size ratchet cap, and the only consumer imports the leaf directly. A managed client configuration lives in a directory another process can write, and the writer used to resolve a symlink at the final path component both when inspecting the target and when committing the atomic replacement. A symlink swapped in between read and write could redirect the write — and the ownership journal's confidence — onto a file the integration does not manage. Three layers now refuse that. loadTarget probes the named directory entry without following it whenever the IO exposes a no-follow probe, so apply, refresh, disable and restore all classify a symlinked target as unsafe before any write is planned; the Aside profile guard forwards that probe so the per-profile path keeps the same boundary. fileIO.writeText rejects a non-regular entry up front. And the atomic commit replaces the named directory entry itself: a new atomicWriteFileNoFollow resolves only the parent (an OS alias above the configured root stays legitimate) and re-validates the target inside the write immediately before the rename, so a link exchanged after validation is refused rather than followed. A swap past the last check can only replace the named entry, never redirect through it. Regressions pin an omo catalog symlink refused at rest with its target byte-identical, a symlink swapped in during apply's snapshot window refused with no ownership recorded, a Cline pair member exchanged for a symlink at the write boundary unable to redirect the replacement, and disable and restore each refusing a symlinked target while leaving the linked file alone. Refresh shares the apply observation and write path, so it inherits the same refusals. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> --- src/clients/aside-profiles.ts | 4 ++ src/config/atomic-write.ts | 32 ++++++++- src/integrations/config-io.ts | 54 +++++++++++--- structure/clients/integrations.md | 5 ++ structure/config.md | 3 +- structure/runtime.md | 2 + tests/clients/cline-writer.test.ts | 26 ++++++- tests/clients/integrations-writer.test.ts | 86 ++++++++++++++++++++++- 8 files changed, 196 insertions(+), 16 deletions(-) diff --git a/src/clients/aside-profiles.ts b/src/clients/aside-profiles.ts index 770857611af..612e281ff0a 100644 --- a/src/clients/aside-profiles.ts +++ b/src/clients/aside-profiles.ts @@ -201,6 +201,7 @@ export function guardAsideProfileIO(profile: AsideProfile, io: IntegrationIO, pr const selected = { ...profile }; const registered = registeredProfiles(selected, profiles).map(peer => ({ ...peer })); const captured = boundary(selected, registered, false); + const lstatProbe = io.lstatKind; function check(path: string, directory: boolean, mutation: boolean): void { if (path !== (directory ? selected.detectDir : selected.configPath)) { refuse("IO attempted to access a different account path."); @@ -214,6 +215,9 @@ export function guardAsideProfileIO(profile: AsideProfile, io: IntegrationIO, pr return { readText: path => { check(path, false, false); return io.readText(path); }, statKind: path => { check(path, path === selected.detectDir, false); return io.statKind(path); }, + ...(lstatProbe + ? { lstatKind: (path: string) => { check(path, path === selected.detectDir, false); return lstatProbe(path); } } + : {}), writeText: (path, text) => { check(path, false, true); io.writeText(path, text); }, removeFile: path => { check(path, false, true); io.removeFile(path); }, mkdirp: path => { check(path, true, true); io.mkdirp(path); }, diff --git a/src/config/atomic-write.ts b/src/config/atomic-write.ts index 4ad69b1bb9d..27f4471d402 100644 --- a/src/config/atomic-write.ts +++ b/src/config/atomic-write.ts @@ -10,7 +10,7 @@ import { unlinkSync, writeFileSync, } from "node:fs"; -import { dirname } from "node:path"; +import { basename, dirname, join } from "node:path"; import { recordOwnedConfigPath } from "../lib/config-ownership"; import { assertNotRealHomeUnderTest } from "../lib/test-home-guard"; import { @@ -215,14 +215,14 @@ async function writePrivateTempFileAsync( carryHardenAcrossContentWrite(path); } -export function atomicWriteFile( +function atomicWriteFileToTarget( path: string, content: string, + target: string, io?: AtomicWriteIO, hooks: AtomicWriteHooks = {}, ): void { recordOwnedConfigPath(getConfigDir(), path); - const target = resolveWriteTarget(path); assertResolvedTargetAllowed(path, target); const tmp = `${target}.ocx.${process.pid}.${nextAtomicTempSequence()}.tmp`; let hardened = false; @@ -287,6 +287,32 @@ export function atomicWriteFile( } } +export function atomicWriteFile( + path: string, + content: string, + io?: AtomicWriteIO, + hooks: AtomicWriteHooks = {}, +): void { + atomicWriteFileToTarget(path, content, resolveWriteTarget(path), io, hooks); +} + +/** + * Atomically replace the named directory entry without resolving a symlink at + * that entry. This is for files in directories writable by another process: + * a raced symlink is replaced, never followed to a more privileged target. + */ +export function atomicWriteFileNoFollow( + path: string, + content: string, + io?: AtomicWriteIO, + hooks: AtomicWriteHooks = {}, +): void { + // Only the final entry is no-follow: the parent still resolves, because an + // OS alias above the configured root (a home junction, /tmp) is legitimate + // and Windows cannot exclusive-create a temp through a junction. + atomicWriteFileToTarget(path, content, join(resolveWriteTarget(dirname(path)), basename(path)), io, hooks); +} + export interface AtomicWriteAsyncIO { write: (path: string, content: string) => void | Promise; harden: (path: string) => void | Promise; diff --git a/src/integrations/config-io.ts b/src/integrations/config-io.ts index 8a1e1480ec8..df5337a121e 100644 --- a/src/integrations/config-io.ts +++ b/src/integrations/config-io.ts @@ -10,7 +10,7 @@ import { lstatSync, mkdirSync, readFileSync, rmSync, statSync } from "node:fs"; import type { ConfigFormat } from "../clients/config-export"; import { MAX_JSON_NESTING } from "./serialize"; -import { atomicWriteFile } from "../config"; +import { atomicWriteFileNoFollow, isMissingPathError } from "../config/atomic-write"; import type { JournalEntry } from "./journal"; import type { OwnershipRecord } from "./ownership"; import type { IntegrationClientId } from "./registry"; @@ -192,6 +192,15 @@ export type ReadResult = export type StatKind = "file" | "dir" | "other" | "missing" | "failed"; +function lstatKind(path: string): StatKind { + try { + const stats = lstatSync(path); + return stats.isFile() ? "file" : stats.isDirectory() ? "dir" : "other"; + } catch (error) { + return (error as NodeJS.ErrnoException).code === "ENOENT" ? "missing" : "failed"; + } +} + export interface IntegrationIO { /** * ONLY a missing file yields `missing`. Every other failure (EACCES, EPERM, @@ -234,6 +243,18 @@ export type TargetState = * an unreadable config gets clobbered. */ export function loadTarget(io: IntegrationIO, configPath: string): TargetState { + // Managed client paths are a lower-trust boundary. Never inspect through a + // final symlink that can be retargeted between this read and the eventual + // write: when a no-follow probe exists, the named directory entry itself must + // be a regular file or absent. `missing` stays legal so a virtual pair probe + // (such as Cline's pair-aware statKind) can still report one absent member. + const named = io.lstatKind?.(configPath); + // A failed probe is uncertainty, not evidence about the entry's shape: it + // keeps the read-failed classification the follow-probe would have produced. + if (named === "failed") return { ok: false, why: "read-failed" }; + if (named !== undefined && named !== "file" && named !== "missing") { + return { ok: false, why: "not-regular-file" }; + } const kind = io.statKind(configPath); if (kind === "missing") return { ok: true, before: null }; if (kind === "failed") return { ok: false, why: "read-failed" }; @@ -252,14 +273,7 @@ export function loadTarget(io: IntegrationIO, configPath: string): TargetState { */ export function fileIO(): Omit { return { - lstatKind: path => { - try { - const stats = lstatSync(path); - return stats.isFile() ? "file" : stats.isDirectory() ? "dir" : "other"; - } catch (error) { - return (error as NodeJS.ErrnoException).code === "ENOENT" ? "missing" : "failed"; - } - }, + lstatKind, readText: path => { try { return { kind: "text", text: readFileSync(path, "utf8") }; @@ -277,8 +291,28 @@ export function fileIO(): Omit { + const kind = lstatKind(path); + if (kind !== "file" && kind !== "missing") { + throw new Error(`refusing unsafe integration write target: ${path}`); + } assertIntegrationWriteOwnership(path); - atomicWriteFile(path, text); + atomicWriteFileNoFollow(path, text, undefined, { + // The pre-check above rejects a symlink already in place; this one runs + // inside the atomic write immediately before the rename, so a link + // exchanged after validation is refused rather than followed. Even a + // swap past this point can only replace the named entry, never redirect + // the write through it. + validateBeforeRename: target => { + try { + if (lstatSync(target).isSymbolicLink()) { + throw new Error(`refusing to replace symbolic-link integration target: ${target}`); + } + } catch (error) { + if (isMissingPathError(error)) return; + throw error; + } + }, + }); }, removeFile: path => rmSync(path, { force: true }), mkdirp: path => mkdirSync(path, { recursive: true, mode: 0o700 }), diff --git a/structure/clients/integrations.md b/structure/clients/integrations.md index 402a60e07f0..5e674c73ed0 100644 --- a/structure/clients/integrations.md +++ b/structure/clients/integrations.md @@ -6,6 +6,8 @@ The client-integration subsystem writes one generated OpenCodex provider contrib third-party client's existing config without taking ownership of the rest of that file. Its core promise is reversibility: apply snapshots first, writes atomically, records exactly what it owns, and refuses refresh, disable, or restore when the current file cannot be classified safely. +Managed client targets are inspected without following a final symbolic link, and their atomic +replacement addresses the named directory entry rather than resolving that link again at commit. Shared response support has a separate [bounded ingestion contract](../transports/inventory.md#bounded-response-ingestion-and-orcarouter-login): raw-byte callers own their byte and deadline budgets and inherit best-effort cancellation. @@ -280,3 +282,6 @@ existing explicit confirmation. The journal endpoint evaluates Undo against the Recovery reads commit history and ownership through strict store methods. Unreadable or malformed metadata is uncertainty, never evidence that a transaction did not commit. Pending records validate complete ownership, exact Cline paths and result fingerprints before either native file is replaced. +Native pair writes replace the named directory entries without following final symlinks. A symlink +present at validation is refused, and one exchanged into place during a mutation is refused rather +than redirecting OpenCodex's write outside Cline's settings directory. diff --git a/structure/config.md b/structure/config.md index 5d68a5ad766..47e0fb54033 100644 --- a/structure/config.md +++ b/structure/config.md @@ -64,7 +64,8 @@ owner-only state and is validated before a record is returned. `src/config.ts` r symbols for compatibility, but new lifecycle-only callers import the process-state leaf directly. Replacing config and process-state writes use `src/config/atomic-write.ts`. The leaf preserves the shared -process-wide temp sequence, symlink target resolution, real-home test guard, owner manifest, +process-wide temp sequence, symlink target resolution, no-follow directory-entry replacement for +externally writable integration directories, real-home test guard, owner manifest, Windows ACL hardening, scrub-before-unlink failure path, and explicit residual-temp errors. A caller must not replace it with a local temp-and-rename shortcut. diff --git a/structure/runtime.md b/structure/runtime.md index 130ab5d4bfe..5c19599c4f5 100644 --- a/structure/runtime.md +++ b/structure/runtime.md @@ -399,6 +399,8 @@ privately to final dispatch; preliminary route selection does not inject Go-only Private pool credential metadata follows the [quota-history publication identity contract](providers/openai-tiers.md#quota-history-publication-identity); credential-only and account DTO projections omit it. Cline CLI joins the existing export/client integration registries. Explicit CLI sync and POST /api/sync refresh its owned pair; unattended catalog refresh excludes it. See [Cline paired files](clients/integrations.md#cline-paired-files). +Its paired-file writer uses the config atomic-write primitive that replaces the named entry without +following a final symlink, so an exchange during a mutation cannot redirect the write. `claudeCode.stabilizePromptCache` is a default-off operator setting for [translated instruction stabilization](data-planes/inbound-compat.md#opt-in-claude-instruction-stabilization). diff --git a/tests/clients/cline-writer.test.ts b/tests/clients/cline-writer.test.ts index 4247f0abde8..d9275ed9b96 100644 --- a/tests/clients/cline-writer.test.ts +++ b/tests/clients/cline-writer.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdirSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { existsSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, renameSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { clineConfigPath } from "../../src/clients/config-export"; @@ -140,6 +140,30 @@ describe("Cline journaled pair", () => { expect(readFileSync(catalog, "utf8")).toBe(originalCatalog); }); + test.skipIf(process.platform === "win32")("a member exchanged for a symlink at the write boundary cannot redirect replacement", () => { + seed(); + const victim = join(root, "victim.json"); + const displaced = `${settings}.attacker-saved`; + writeFileSync(victim, '{"sentinel":true}'); + const io = input.store!.io(); + let exchanged = false; + const result = applyIntegration({ ...input, io: { + ...io, + writeText: (path, text) => { + if (path === settings && !exchanged) { + exchanged = true; + renameSync(settings, displaced); + symlinkSync(victim, settings); + } + io.writeText(path, text); + }, + } }); + expect(result.ok).toBe(false); + expect(readFileSync(victim, "utf8")).toBe('{"sentinel":true}'); + expect(lstatSync(settings).isSymbolicLink()).toBe(true); + expect(readFileSync(displaced, "utf8")).toBe(originalSettings); + }); + test("occupied custom provider requires explicit overwrite and remains reversible", () => { seed(); const doc = settingsDoc(); diff --git a/tests/clients/integrations-writer.test.ts b/tests/clients/integrations-writer.test.ts index 16d118cd3fd..53d78f9415c 100644 --- a/tests/clients/integrations-writer.test.ts +++ b/tests/clients/integrations-writer.test.ts @@ -1,5 +1,7 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs"; +import { + mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, symlinkSync, unlinkSync, writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { buildClientContribution, type ExportModel } from "../../src/clients/config-export"; @@ -93,6 +95,14 @@ function installOmp(): string { return configPath; } +function installOmo(): string { + const spec = INTEGRATION_CLIENTS.omo; + mkdirSync(spec.detectDir(TEST_ENV, home), { recursive: true }); + const configPath = spec.configPath(TEST_ENV, home); + mkdirSync(dirname(configPath), { recursive: true }); + return configPath; +} + function installDsh(): string { const spec = INTEGRATION_CLIENTS.dsh; mkdirSync(spec.detectDir(TEST_ENV, home), { recursive: true }); @@ -149,6 +159,44 @@ function reverseJsonObjectKeys(value: unknown): unknown { } describe("apply", () => { + test.skipIf(process.platform === "win32")("refuses an omo catalog symlink without changing its target", () => { + const configPath = installOmo(); + const victim = join(dirname(home), "victim.json"); + const original = '{"security":{"mode":"strict"}}\n'; + writeFileSync(victim, original); + symlinkSync(victim, configPath); + + expect(readIntegrationState(input({ clientId: "omo" })).state).toBe("unsafe"); + const result = applyIntegration(input({ clientId: "omo" })); + + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("unsafe"); + expect(readFileSync(victim, "utf8")).toBe(original); + expect(store.listOperations("omo")).toHaveLength(0); + }); + + test.skipIf(process.platform === "win32")("an omo symlink swap before commit cannot replace its target", () => { + const configPath = installOmo(); + const checked = '{"notes":"client-owned"}\n'; + writeFileSync(configPath, checked); + const victim = join(dirname(home), "victim.json"); + const original = '{"security":{"mode":"strict"}}\n'; + writeFileSync(victim, original); + const captureSnapshot = store.captureSnapshot.bind(store); + store.captureSnapshot = (clientId, opId, before) => { + const snapshot = captureSnapshot(clientId, opId, before); + unlinkSync(configPath); + symlinkSync(victim, configPath); + return snapshot; + }; + + const result = applyIntegration(input({ clientId: "omo" })); + + expect(result.ok).toBe(false); + expect(readFileSync(victim, "utf8")).toBe(original); + expect(store.readRecords().omo).toBeUndefined(); + }); + test("refuses Kimi TOML date rewrites without changing the file or ownership store", () => { const spec = INTEGRATION_CLIENTS.kimi; mkdirSync(spec.detectDir(TEST_ENV, home), { recursive: true }); @@ -789,6 +837,24 @@ describe("apply", () => { }); describe("disable", () => { + test.skipIf(process.platform === "win32")("refuses a symlinked managed target and leaves its target alone", () => { + const configPath = installOmo(); + const applied = applyIntegration(input({ clientId: "omo" })); + expect(applied.ok).toBe(true); + const victim = join(dirname(home), "victim.json"); + const original = '{"security":{"mode":"strict"}}\n'; + writeFileSync(victim, original); + unlinkSync(configPath); + symlinkSync(victim, configPath); + + const result = disableIntegration(input({ clientId: "omo" })); + + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("unsafe"); + expect(readFileSync(victim, "utf8")).toBe(original); + expect(store.readRecords().omo).toBeDefined(); + }); + test("removes only our block and leaves the rest byte-identical", () => { const configPath = installHermes(); const original = "providers:\n other:\n api: http://elsewhere\nunknown_top: keep-me\n"; @@ -1078,6 +1144,24 @@ describe("Hermes source preservation", () => { }); describe("restore", () => { + test.skipIf(process.platform === "win32")("refuses a symlinked managed target and leaves its target alone", () => { + const configPath = installOmo(); + const applied = applyIntegration(input({ clientId: "omo" })); + expect(applied.ok).toBe(true); + const opId = store.listOperations("omo")[0]!.opId; + const victim = join(dirname(home), "victim.json"); + const original = '{"security":{"mode":"strict"}}\n'; + writeFileSync(victim, original); + unlinkSync(configPath); + symlinkSync(victim, configPath); + + const result = restoreIntegration({ ...input({ clientId: "omo" }), opId }); + + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("unsafe"); + expect(readFileSync(victim, "utf8")).toBe(original); + }); + test("undoes an apply back to the exact prior bytes", () => { const configPath = installHermes(); const original = "providers:\n other:\n api: http://elsewhere\n"; From de5d554e115a788db513aae18bb56fe96b859e3f Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 20 Sep 2026 14:20:58 +0900 Subject: [PATCH 3/4] docs(devlog): record lane B transport and write-safety progress --- .../030_lane_b_transport_write_safety.md | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 devlog/_plan/260920_meaning_preservation_batch/030_lane_b_transport_write_safety.md diff --git a/devlog/_plan/260920_meaning_preservation_batch/030_lane_b_transport_write_safety.md b/devlog/_plan/260920_meaning_preservation_batch/030_lane_b_transport_write_safety.md new file mode 100644 index 00000000000..2b4bc1d06cf --- /dev/null +++ b/devlog/_plan/260920_meaning_preservation_batch/030_lane_b_transport_write_safety.md @@ -0,0 +1,66 @@ +# Lane B — request-scoped transport and managed-write safety + +Status: branch pushed for hosted CI. One branch, two ordered commits, one PR +against `dev`, per the batch topology. + +## Scope + +- #5087 — DNS pinning and transport selection now follow whether a proxy + actually applies to the request, not whether one is configured. +- #5241 — a managed configuration write never follows a terminal symlink to + another file, including apply/refresh/disable/restore and their races. + +Both items carry the existing contributor pull requests by luvs01 with a +`Co-authored-by` trailer on each branch commit. The original pull requests +stay open for the coordinator. + +## Branch + +`codex/260920-lane-b-transport-write-safety`, cut from `origin/dev` +(`0613aaec17`). Commits in order: + +1. `fix(transport): decide DNS pinning by whether the proxy applies to the request` +2. `fix(integrations): reject symlinked managed write targets` + +## Review findings on current dev (beyond the carried diffs) + +- #5087's carried model counted a non-SOCKS `ALL_PROXY` for `http:` targets on + POSIX only. The repository's own provider-outbound e2e drives that exact + request through the proxy and runs green on the Windows shard too, so the + platform gate was dropped: `ALL_PROXY` counts for `http:` on every CI + platform. A present-but-unusable scheme-matched variable now fails closed + instead of falling through to `ALL_PROXY`. +- The Mihomo IPv6 fake-IP gate keeps its stricter documented condition + (scheme-matched variable or SOCKS5 `ALL_PROXY`, non-SOCKS `ALL_PROXY` + never counts) via a dedicated `schemeMatchedProxyFor`, so the documented + and tested #3462 behaviour is byte-identical. Moving it to the new snapshot + would have contradicted the provider docs in every shipped locale. +- The carried #5087 diff had no regression for the DNS-failure degradation + branch. Added both directions: a mismatched proxy surfaces the DNS error + instead of degrading to an unpinned fetch; a scheme-matched proxy keeps the + degradation. +- #5241's carried diff re-exported the new primitive from `src/config.ts`, + which sits exactly at its file-size ratchet cap. The re-export was dropped; + the only consumer imports the leaf directly. +- #5241's carried tests covered apply (at rest and swap-during-write) and the + Cline pair boundary. Added disable and restore refusing a symlinked target + with the linked file byte-identical. Refresh shares the apply observation + and write path, so it inherits the same refusals. + +## Decision-function reach (#5087) + +Every `providerOutboundGet/Post` caller — provider discovery, the +model-catalog gather modules, quota probes, ollama show, and the management +model-refresh routes — funnels through the single changed decision. The main +inference dispatch (`providerFetch`) and OAuth token exchange +(`src/oauth/*`, bare global fetch) do not use the DNS-pinned transport today +and are unchanged. + +## Verification + +- Local suites, focused tests, typecheck, builds and live runs: NOT RUN (lane + rule). Verification is static source review plus exact-head hosted CI. +- Union-defect sweep: no capped file touched (`src/config.ts` left + byte-identical), no new test files (layout inventories unchanged), nothing + exhaustive over a union restated (locale catalogs, rosters and generated + counts untouched). From c3b3e7d03e5a412bc8a8a138c47056926c022ac2 Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 20 Sep 2026 14:23:41 +0900 Subject: [PATCH 4/4] docs(devlog): link lane B progress to its pull request --- .../030_lane_b_transport_write_safety.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/devlog/_plan/260920_meaning_preservation_batch/030_lane_b_transport_write_safety.md b/devlog/_plan/260920_meaning_preservation_batch/030_lane_b_transport_write_safety.md index 2b4bc1d06cf..c992b81e129 100644 --- a/devlog/_plan/260920_meaning_preservation_batch/030_lane_b_transport_write_safety.md +++ b/devlog/_plan/260920_meaning_preservation_batch/030_lane_b_transport_write_safety.md @@ -1,7 +1,8 @@ # Lane B — request-scoped transport and managed-write safety -Status: branch pushed for hosted CI. One branch, two ordered commits, one PR -against `dev`, per the batch topology. +Status: PR #5264 open against `dev` at exact head `235525b52a`, hosted CI +in flight. One branch, two ordered commits plus this progress record, per the +batch topology. ## Scope @@ -16,8 +17,7 @@ stay open for the coordinator. ## Branch -`codex/260920-lane-b-transport-write-safety`, cut from `origin/dev` -(`0613aaec17`). Commits in order: +`codex/260920-lane-b-transport-write-safety`. Commits in order: 1. `fix(transport): decide DNS pinning by whether the proxy applies to the request` 2. `fix(integrations): reject symlinked managed write targets`