fix(security): request-scoped transport pinning and managed-write symlink refusal

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: 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
+
+- #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`. 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).

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); },

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<void>;
   harden: (path: string) => void | Promise<void>;

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<IntegrationIO, "appendJournal" | "putRecord" | "dropRecord"> {
   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<IntegrationIO, "appendJournal" | "putRecord" | "d
       }
     },
     writeText: (path, text) => {
+      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 }),

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<RequestInit, "body" | "method" | "redirect">;
@@ -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`,

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 {