-
Notifications
You must be signed in to change notification settings - Fork 513
fix(responses): tell TLS hostname mismatches apart from an unreachabl… #798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
batuchek68-ux
wants to merge
1
commit into
lidge-jun:codex/260729-security-md-reporting-path
from
batuchek68-ux:codex/260728-tls-altname-diagnosis
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 "<url>"`; 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 ?? "<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} ` | ||
| + "</dev/null | openssl x509 -noout -subject -ext subjectAltName"; | ||
| } | ||
| return `Provider unreachable: ${redactUrlUserinfo(detail)}`; | ||
| } | ||
|
|
||
| /** | ||
| * A provider base URL may carry credentials as userinfo (`https://user:token@host/`), and the | ||
| * runtime error echoes the URL it was fetching. Strip it before the message reaches a client | ||
| * or a log line. | ||
| */ | ||
| function redactUrlUserinfo(detail: string): string { | ||
| return detail.replace(/(https?:\/\/)[^/\s"'@]*@/g, "$1<redacted>@"); | ||
| } | ||
|
|
||
| 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; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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("<host>"); | ||
| }); | ||
|
|
||
| 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("<redacted>@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("<redacted>@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]"); | ||
| }); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this TLS branch fires for a configured provider URL whose hostname contains shell metacharacters that
new URL()accepts, the diagnostic emits that host unquoted inside a copy-pasteable pipeline. For example a host containing backticks or semicolons is expanded or splits the command beforeopensslruns, so the suggestion can execute unintended shell syntax; quote/escape the host (or avoid a ready-to-run shell pipeline) before including it here.Useful? React with 👍 / 👎.