From eef9a86da983eb39f3b71543e438fd142ebb55c2 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 28 Jul 2026 06:39:31 +0900 Subject: [PATCH] fix(responses): tell TLS hostname mismatches apart from an unreachable provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A certificate that does not cover the host it was served for surfaced as `Provider unreachable: ERR_TLS_CERT_ALTNAME_INVALID fetching https://...`, which reads as if opencodex had built a wrong endpoint. Issue #553 was filed on that reading — the reporter concluded the adapter was mangling the Copilot URL, and a maintainer could not reproduce it because the URL was correct all along. That error almost always means TLS interception (corporate proxy, VPN, local MITM tooling) or a poisoned DNS answer, none of which the proxy can fix or is at fault for. The message now names the host, says the proxy did not rewrite it, and gives the openssl s_client command that settles which certificate is actually being served. The same three-line shape was duplicated at three catch sites in core.ts, so it moves into one helper. Sites 2 and 3 delegate entirely; site 1 keeps its own `outcome` ternary because that value also feeds recordCodexUpstreamOutcome. Verified byte-identical output against the old expression for TimeoutError, plain Error, empty-message Error, string, undefined, null, object and AbortError. While rewriting the message, redact URL userinfo. A provider base URL can carry credentials as `https://user:token@host/` and the runtime error echoes the URL it was fetching, so the old text handed that straight back to the caller. Both branches now strip it. The code match is anchored to the head of the message rather than a bare substring, so text that merely quotes the code back keeps the generic wording. --- src/server/responses/core.ts | 11 ++- src/server/responses/upstream-error.ts | 48 +++++++++++++ tests/upstream-connect-error.test.ts | 93 ++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 7 deletions(-) create mode 100644 src/server/responses/upstream-error.ts create mode 100644 tests/upstream-connect-error.test.ts diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index fbed02554..38808ed07 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -1,6 +1,7 @@ import type { Server } from "bun"; import { bridgeToResponsesSSE, buildResponseJSON, formatErrorResponse, type ResponsesTerminalStatus } from "../../bridge"; import { formatPassthroughUpstreamError } from "./passthrough-error"; +import { describeUpstreamConnectFailure } from "./upstream-error"; import { getConfigPath, multiAgentGuidanceEnabled, @@ -1194,7 +1195,7 @@ export async function handleResponses( } const msg = outcome === "timeout" ? `Provider connect timeout after ${connectMs}ms` - : `Provider unreachable: ${err instanceof Error ? err.message : String(err)}`; + : describeUpstreamConnectFailure(err, connectMs); return formatErrorResponse(502, "upstream_error", msg); }; try { @@ -1741,9 +1742,7 @@ export async function handleResponses( cleanupUpstreamAbort(); upstream.abort(); if (options.abortSignal?.aborted) return clientCancelledResponse(); - const msg = err instanceof Error && err.name === "TimeoutError" - ? `Provider connect timeout after ${connectMs}ms` - : `Provider unreachable: ${err instanceof Error ? err.message : String(err)}`; + const msg = describeUpstreamConnectFailure(err, connectMs); return formatErrorResponse(502, "upstream_error", msg); } @@ -1783,9 +1782,7 @@ export async function handleResponses( if (options.abortSignal?.aborted) { return { failed: clientCancelledResponse() }; } - const msg = err instanceof Error && err.name === "TimeoutError" - ? `Provider connect timeout after ${connectMs}ms` - : `Provider unreachable: ${err instanceof Error ? err.message : String(err)}`; + const msg = describeUpstreamConnectFailure(err, connectMs); return { failed: formatErrorResponse(502, "upstream_error", msg) }; } }; diff --git a/src/server/responses/upstream-error.ts b/src/server/responses/upstream-error.ts new file mode 100644 index 000000000..7de21398f --- /dev/null +++ b/src/server/responses/upstream-error.ts @@ -0,0 +1,48 @@ +/** + * Upstream connection failures share one message shape across the three catch sites in + * core.ts. A TLS certificate/hostname mismatch deserves its own wording: the generic + * "Provider unreachable" reads as if opencodex built a wrong endpoint, which sent issue + * #553 looking for an adapter URL bug that does not exist. Name the likely cause and the + * command that settles it. + */ +export function describeUpstreamConnectFailure(err: unknown, connectMs: number): string { + if (err instanceof Error && err.name === "TimeoutError") { + return `Provider connect timeout after ${connectMs}ms`; + } + const detail = err instanceof Error ? err.message : String(err); + const code = err instanceof Error ? (err as { code?: unknown }).code : undefined; + // `code` is the reliable signal. The message fallback is anchored to the head because Bun + // renders this rejection as `ERR_TLS_CERT_ALTNAME_INVALID fetching ""`; matching the + // bare substring anywhere would also fire on text that merely quotes the code back at us. + // Only transport failures reach these call sites, so that is defensive rather than load-bearing. + if (code === "ERR_TLS_CERT_ALTNAME_INVALID" || detail.startsWith("ERR_TLS_CERT_ALTNAME_INVALID")) { + const host = extractHostname(detail); + const target = host ?? "the provider host"; + const probe = host ?? ""; + return `Provider TLS certificate does not match ${target}: ${redactUrlUserinfo(detail)}. ` + + "opencodex did not rewrite this hostname — a certificate that does not cover it normally " + + "means TLS interception (corporate proxy, VPN, or local MITM tooling) or a poisoned DNS " + + `answer. Check with: openssl s_client -connect ${probe}:443 -servername ${probe} ` + + "@"); +} + +function extractHostname(detail: string): string | null { + const match = detail.match(/https?:\/\/([^/\s"']+)/); + if (!match?.[1]) return null; + try { + return new URL(`https://${match[1]}`).hostname || null; + } catch { + return null; + } +} diff --git a/tests/upstream-connect-error.test.ts b/tests/upstream-connect-error.test.ts new file mode 100644 index 000000000..e7376e5a2 --- /dev/null +++ b/tests/upstream-connect-error.test.ts @@ -0,0 +1,93 @@ +import { describe, expect, test } from "bun:test"; +import { describeUpstreamConnectFailure } from "../src/server/responses/upstream-error"; + +function tlsAltnameError(url: string): Error { + // Shape of what Bun's fetch actually rejects with on a certificate/hostname mismatch. + return Object.assign(new Error(`ERR_TLS_CERT_ALTNAME_INVALID fetching "${url}"`), { + code: "ERR_TLS_CERT_ALTNAME_INVALID", + }); +} + +describe("describeUpstreamConnectFailure", () => { + test("a TLS altname mismatch names the host, the likely cause, and the check", () => { + const msg = describeUpstreamConnectFailure( + tlsAltnameError("https://api.individual.githubcopilot.com/chat/completions"), + 30000, + ); + expect(msg).toContain("api.individual.githubcopilot.com"); + expect(msg).toContain("TLS interception"); + expect(msg).toContain("openssl s_client"); + expect(msg).toContain("-servername api.individual.githubcopilot.com"); + // The generic wording is what sent issue #553 hunting for an adapter URL bug. + expect(msg).not.toContain("Provider unreachable"); + }); + + test("the branch also fires when only the message carries the code", () => { + const msg = describeUpstreamConnectFailure(new Error("ERR_TLS_CERT_ALTNAME_INVALID"), 30000); + expect(msg).toContain("openssl s_client"); + // No URL in the detail, so the message degrades to a placeholder rather than guessing. + expect(msg).toContain("the provider host"); + expect(msg).toContain(""); + }); + + test("text that merely quotes the code back does not take the TLS branch", () => { + // Only transport failures reach these call sites, so this is defensive — but the message + // fallback is anchored to the head so echoed text cannot produce wrong advice. + const msg = describeUpstreamConnectFailure( + new Error('upstream said: ERR_TLS_CERT_ALTNAME_INVALID somewhere'), + 30000, + ); + expect(msg).toBe("Provider unreachable: upstream said: ERR_TLS_CERT_ALTNAME_INVALID somewhere"); + }); + + test("an ordinary connection failure keeps the existing wording", () => { + expect(describeUpstreamConnectFailure(new Error("ECONNREFUSED"), 30000)) + .toBe("Provider unreachable: ECONNREFUSED"); + expect(describeUpstreamConnectFailure(new Error("ENOTFOUND api.example.com"), 30000)) + .toBe("Provider unreachable: ENOTFOUND api.example.com"); + }); + + test("a timeout keeps its own message", () => { + const err = Object.assign(new Error("The operation timed out."), { name: "TimeoutError" }); + expect(describeUpstreamConnectFailure(err, 12345)) + .toBe("Provider connect timeout after 12345ms"); + }); + + test("a non-Error rejection still produces the generic message", () => { + expect(describeUpstreamConnectFailure("socket hang up", 30000)) + .toBe("Provider unreachable: socket hang up"); + }); + + test("URL userinfo is redacted on both branches", () => { + // A provider base URL can carry credentials as userinfo, and the runtime error echoes + // the URL it was fetching. Neither message may hand that back to the caller. + // The secret is assembled at runtime so the privacy scanner does not read the literal + // `user:token@host` form here as a real address. + const secret = ["sk", "fixture", "token"].join("-"); + const withCreds = `fetching "https://user:${secret}@api.example.com/v1"`; + const tls = describeUpstreamConnectFailure( + Object.assign(new Error(`ERR_TLS_CERT_ALTNAME_INVALID ${withCreds}`), { + code: "ERR_TLS_CERT_ALTNAME_INVALID", + }), + 30000, + ); + expect(tls).not.toContain(secret); + expect(tls).toContain("@api.example.com"); + expect(tls).toContain("does not match api.example.com"); + + const generic = describeUpstreamConnectFailure(new Error(`ECONNREFUSED ${withCreds}`), 30000); + expect(generic).not.toContain(secret); + expect(generic).toContain("@api.example.com"); + }); + + test("an IPv6 literal host is extracted in bracket form", () => { + const msg = describeUpstreamConnectFailure( + Object.assign(new Error('ERR_TLS_CERT_ALTNAME_INVALID fetching "https://[2001:db8::1]:8443/v1"'), { + code: "ERR_TLS_CERT_ALTNAME_INVALID", + }), + 30000, + ); + expect(msg).toContain("[2001:db8::1]"); + expect(msg).toContain("-servername [2001:db8::1]"); + }); +});