Skip to content

Commit fff8c36

Browse files
authored
fix(responses): tell TLS hostname mismatches apart from an unreachable provider (#575)
Closes nothing on its own — #553 stays open per the maintainer note: this improves attribution, it does not resolve the TLS interception condition.
1 parent c4b05fa commit fff8c36

3 files changed

Lines changed: 145 additions & 7 deletions

File tree

src/server/responses/core.ts

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type { Server } from "bun";
22
import { bridgeToResponsesSSE, buildResponseJSON, formatErrorResponse, type ResponsesTerminalStatus } from "../../bridge";
33
import { formatPassthroughUpstreamError } from "./passthrough-error";
4+
import { describeUpstreamConnectFailure } from "./upstream-error";
45
import {
56
getConfigPath,
67
multiAgentGuidanceEnabled,
@@ -1443,7 +1444,7 @@ export async function handleResponses(
14431444
}
14441445
const msg = outcome === "timeout"
14451446
? `Provider connect timeout after ${connectMs}ms`
1446-
: `Provider unreachable: ${err instanceof Error ? err.message : String(err)}`;
1447+
: describeUpstreamConnectFailure(err, connectMs);
14471448
return formatErrorResponse(502, "upstream_error", msg);
14481449
};
14491450
try {
@@ -2101,9 +2102,7 @@ export async function handleResponses(
21012102
cleanupUpstreamAbort();
21022103
upstream.abort();
21032104
if (options.abortSignal?.aborted) return clientCancelledResponse();
2104-
const msg = err instanceof Error && err.name === "TimeoutError"
2105-
? `Provider connect timeout after ${connectMs}ms`
2106-
: `Provider unreachable: ${err instanceof Error ? err.message : String(err)}`;
2105+
const msg = describeUpstreamConnectFailure(err, connectMs);
21072106
return formatErrorResponse(502, "upstream_error", msg);
21082107
}
21092108

@@ -2143,9 +2142,7 @@ export async function handleResponses(
21432142
if (options.abortSignal?.aborted) {
21442143
return { failed: clientCancelledResponse() };
21452144
}
2146-
const msg = err instanceof Error && err.name === "TimeoutError"
2147-
? `Provider connect timeout after ${connectMs}ms`
2148-
: `Provider unreachable: ${err instanceof Error ? err.message : String(err)}`;
2145+
const msg = describeUpstreamConnectFailure(err, connectMs);
21492146
return { failed: formatErrorResponse(502, "upstream_error", msg) };
21502147
}
21512148
};
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
/**
2+
* Upstream connection failures share one message shape across the three catch sites in
3+
* core.ts. A TLS certificate/hostname mismatch deserves its own wording: the generic
4+
* "Provider unreachable" reads as if opencodex built a wrong endpoint, which sent issue
5+
* #553 looking for an adapter URL bug that does not exist. Name the likely cause and the
6+
* command that settles it.
7+
*/
8+
export function describeUpstreamConnectFailure(err: unknown, connectMs: number): string {
9+
if (err instanceof Error && err.name === "TimeoutError") {
10+
return `Provider connect timeout after ${connectMs}ms`;
11+
}
12+
const detail = err instanceof Error ? err.message : String(err);
13+
const code = err instanceof Error ? (err as { code?: unknown }).code : undefined;
14+
// `code` is the reliable signal. The message fallback is anchored to the head because Bun
15+
// renders this rejection as `ERR_TLS_CERT_ALTNAME_INVALID fetching "<url>"`; matching the
16+
// bare substring anywhere would also fire on text that merely quotes the code back at us.
17+
// Only transport failures reach these call sites, so that is defensive rather than load-bearing.
18+
if (code === "ERR_TLS_CERT_ALTNAME_INVALID" || detail.startsWith("ERR_TLS_CERT_ALTNAME_INVALID")) {
19+
const host = extractHostname(detail);
20+
const target = host ?? "the provider host";
21+
const probe = host ?? "<host>";
22+
return `Provider TLS certificate does not match ${target}: ${redactUrlUserinfo(detail)}. `
23+
+ "opencodex did not rewrite this hostname — a certificate that does not cover it normally "
24+
+ "means TLS interception (corporate proxy, VPN, or local MITM tooling) or a poisoned DNS "
25+
+ `answer. Check with: openssl s_client -connect ${probe}:443 -servername ${probe} `
26+
+ "</dev/null | openssl x509 -noout -subject -ext subjectAltName";
27+
}
28+
return `Provider unreachable: ${redactUrlUserinfo(detail)}`;
29+
}
30+
31+
/**
32+
* A provider base URL may carry credentials as userinfo (`https://user:token@host/`), and the
33+
* runtime error echoes the URL it was fetching. Strip it before the message reaches a client
34+
* or a log line.
35+
*/
36+
function redactUrlUserinfo(detail: string): string {
37+
return detail.replace(/(https?:\/\/)[^/\s"'@]*@/g, "$1<redacted>@");
38+
}
39+
40+
function extractHostname(detail: string): string | null {
41+
const match = detail.match(/https?:\/\/([^/\s"']+)/);
42+
if (!match?.[1]) return null;
43+
try {
44+
return new URL(`https://${match[1]}`).hostname || null;
45+
} catch {
46+
return null;
47+
}
48+
}
Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
import { describe, expect, test } from "bun:test";
2+
import { describeUpstreamConnectFailure } from "../src/server/responses/upstream-error";
3+
4+
function tlsAltnameError(url: string): Error {
5+
// Shape of what Bun's fetch actually rejects with on a certificate/hostname mismatch.
6+
return Object.assign(new Error(`ERR_TLS_CERT_ALTNAME_INVALID fetching "${url}"`), {
7+
code: "ERR_TLS_CERT_ALTNAME_INVALID",
8+
});
9+
}
10+
11+
describe("describeUpstreamConnectFailure", () => {
12+
test("a TLS altname mismatch names the host, the likely cause, and the check", () => {
13+
const msg = describeUpstreamConnectFailure(
14+
tlsAltnameError("https://api.individual.githubcopilot.com/chat/completions"),
15+
30000,
16+
);
17+
expect(msg).toContain("api.individual.githubcopilot.com");
18+
expect(msg).toContain("TLS interception");
19+
expect(msg).toContain("openssl s_client");
20+
expect(msg).toContain("-servername api.individual.githubcopilot.com");
21+
// The generic wording is what sent issue #553 hunting for an adapter URL bug.
22+
expect(msg).not.toContain("Provider unreachable");
23+
});
24+
25+
test("the branch also fires when only the message carries the code", () => {
26+
const msg = describeUpstreamConnectFailure(new Error("ERR_TLS_CERT_ALTNAME_INVALID"), 30000);
27+
expect(msg).toContain("openssl s_client");
28+
// No URL in the detail, so the message degrades to a placeholder rather than guessing.
29+
expect(msg).toContain("the provider host");
30+
expect(msg).toContain("<host>");
31+
});
32+
33+
test("text that merely quotes the code back does not take the TLS branch", () => {
34+
// Only transport failures reach these call sites, so this is defensive — but the message
35+
// fallback is anchored to the head so echoed text cannot produce wrong advice.
36+
const msg = describeUpstreamConnectFailure(
37+
new Error('upstream said: ERR_TLS_CERT_ALTNAME_INVALID somewhere'),
38+
30000,
39+
);
40+
expect(msg).toBe("Provider unreachable: upstream said: ERR_TLS_CERT_ALTNAME_INVALID somewhere");
41+
});
42+
43+
test("an ordinary connection failure keeps the existing wording", () => {
44+
expect(describeUpstreamConnectFailure(new Error("ECONNREFUSED"), 30000))
45+
.toBe("Provider unreachable: ECONNREFUSED");
46+
expect(describeUpstreamConnectFailure(new Error("ENOTFOUND api.example.com"), 30000))
47+
.toBe("Provider unreachable: ENOTFOUND api.example.com");
48+
});
49+
50+
test("a timeout keeps its own message", () => {
51+
const err = Object.assign(new Error("The operation timed out."), { name: "TimeoutError" });
52+
expect(describeUpstreamConnectFailure(err, 12345))
53+
.toBe("Provider connect timeout after 12345ms");
54+
});
55+
56+
test("a non-Error rejection still produces the generic message", () => {
57+
expect(describeUpstreamConnectFailure("socket hang up", 30000))
58+
.toBe("Provider unreachable: socket hang up");
59+
});
60+
61+
test("URL userinfo is redacted on both branches", () => {
62+
// A provider base URL can carry credentials as userinfo, and the runtime error echoes
63+
// the URL it was fetching. Neither message may hand that back to the caller.
64+
// The secret is assembled at runtime so the privacy scanner does not read the literal
65+
// `user:token@host` form here as a real address.
66+
const secret = ["sk", "fixture", "token"].join("-");
67+
const withCreds = `fetching "https://user:${secret}@api.example.com/v1"`;
68+
const tls = describeUpstreamConnectFailure(
69+
Object.assign(new Error(`ERR_TLS_CERT_ALTNAME_INVALID ${withCreds}`), {
70+
code: "ERR_TLS_CERT_ALTNAME_INVALID",
71+
}),
72+
30000,
73+
);
74+
expect(tls).not.toContain(secret);
75+
expect(tls).toContain("<redacted>@api.example.com");
76+
expect(tls).toContain("does not match api.example.com");
77+
78+
const generic = describeUpstreamConnectFailure(new Error(`ECONNREFUSED ${withCreds}`), 30000);
79+
expect(generic).not.toContain(secret);
80+
expect(generic).toContain("<redacted>@api.example.com");
81+
});
82+
83+
test("an IPv6 literal host is extracted in bracket form", () => {
84+
const msg = describeUpstreamConnectFailure(
85+
Object.assign(new Error('ERR_TLS_CERT_ALTNAME_INVALID fetching "https://[2001:db8::1]:8443/v1"'), {
86+
code: "ERR_TLS_CERT_ALTNAME_INVALID",
87+
}),
88+
30000,
89+
);
90+
expect(msg).toContain("[2001:db8::1]");
91+
expect(msg).toContain("-servername [2001:db8::1]");
92+
});
93+
});

0 commit comments

Comments
 (0)