Repository navigation
fix(security): backport upstream v1.18 hardening for debug config, browser URLs and OAuth pages #1507
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
base: main
Are you sure you want to change the base?
fix(security): backport upstream v1.18 hardening for debug config, browser URLs and OAuth pages #1507
Changes from all commits
0b03c73
c4a03d0
4d7e7ce
c432a17
b1996f2
92aee8d
2abadef
20e6593
e12b3d4
c2d2bc3
d8b84bc
7743682
55ccdc6
f9ab4d0
07816fa
14e5145
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,8 @@ | ||||||||||
| import open from "open" | ||||||||||
|
|
||||||||||
| export function openUrl(input: string) { | ||||||||||
| const url = URL.canParse(input) ? new URL(input) : undefined | ||||||||||
| if (!url || (url.protocol !== "http:" && url.protocol !== "https:")) | ||||||||||
| return Promise.reject(new Error(`Only http and https links can be opened in the browser: ${input}`)) | ||||||||||
|
Comment on lines
+5
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The rejection message echoes the full raw Prompt for AI agents
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving as is for the same reason: |
||||||||||
| return open(url.href) | ||||||||||
| } | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| import { describe, expect, spyOn, test } from "bun:test" | ||
| import type { ChildProcess } from "node:child_process" | ||
| import { EventEmitter } from "node:events" | ||
| import * as browser from "open" | ||
| import { openUrl } from "@opencode-ai/core/open" | ||
|
|
||
| describe("openUrl", () => { | ||
| test("opens normalized http(s) URLs and preserves the subprocess promise", async () => { | ||
| const subprocess = new EventEmitter() as ChildProcess | ||
| const promise = Promise.resolve(subprocess) | ||
| const opener = spyOn(browser, "default").mockReturnValue(promise) | ||
| try { | ||
| for (const input of ["HTTP://Example.COM:80/path", "https://example.com/a b"]) { | ||
| const result = openUrl(input) | ||
| expect(result).toBe(promise) | ||
| expect(await result).toBe(subprocess) | ||
| expect(opener).toHaveBeenLastCalledWith(new URL(input).href) | ||
| } | ||
| } finally { | ||
| opener.mockRestore() | ||
| } | ||
| }) | ||
|
|
||
| test("rejects values that are not URLs", async () => { | ||
| const opener = spyOn(browser, "default").mockResolvedValue(new EventEmitter() as ChildProcess) | ||
| try { | ||
| await expect(openUrl("not a url")).rejects.toThrow("Only http and https links") | ||
| await expect(openUrl("")).rejects.toThrow("Only http and https links") | ||
| expect(opener).not.toHaveBeenCalled() | ||
| } finally { | ||
| opener.mockRestore() | ||
| } | ||
| }) | ||
|
|
||
| test("rejects non-http schemes", async () => { | ||
| const opener = spyOn(browser, "default").mockResolvedValue(new EventEmitter() as ChildProcess) | ||
| try { | ||
| await expect(openUrl("file:///etc/hosts")).rejects.toThrow("Only http and https links") | ||
| await expect(openUrl("javascript:alert(1)")).rejects.toThrow("Only http and https links") | ||
| await expect(openUrl("ms-msdt:/id PCWDiagnostic")).rejects.toThrow("Only http and https links") | ||
| await expect(openUrl("\\\\server\\share\\file.html")).rejects.toThrow("Only http and https links") | ||
| expect(opener).not.toHaveBeenCalled() | ||
| } finally { | ||
| opener.mockRestore() | ||
| } | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,6 +57,14 @@ export function setBrowserOpenerForTests(fn: (url: string) => void): () => void | |
| } | ||
|
|
||
| function launchBrowser(url: string): void { | ||
| // The server/IdP supplies this URL. Refuse non-web protocol handlers; the notice still carries it for manual copy. | ||
| try { | ||
| const protocol = new URL(url).protocol | ||
| if (protocol !== "http:" && protocol !== "https:") return | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: A refused URL launches nothing, but the later Snowflake timeout error still says 'A sign-in page was opened in the default browser' and asks the user to confirm a tab opened. Make that message conditional on whether a launch happened, or at least log the refusal so the failure is diagnosable. Prompt for AI agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point, but P3 and outside this backport. The URL is still in the sign-in notice for manual copy, so the user can recover. I'm noting it for a follow-up on the driver's timeout copy. |
||
| } catch { | ||
| return | ||
| } | ||
|
|
||
| const [command, args] = | ||
| process.platform === "win32" | ||
| ? // No shell, so `&` in the SAML query needs no escaping (unlike `cmd /c start`). | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| // Fork-only file (not in v1.17.9): extends upstream 82d4c89031. | ||
| // Redact only the debug output; the resolved configuration must remain usable by providers. | ||
| const secretName = | ||
| /(?:api.?key|access.?key|secret|password|passwd|token$|authorization$|cookie$|credential|private.?key|bearer|passphrase|(?:^|[_-])pwd$|(?:^|[_-])auth(?:[_-]|$)|dsn$|connection.?string)/i | ||
|
|
||
| function redactURL(value: string): string { | ||
| const trimmed = value.trim() | ||
| // Prose (prompts, descriptions) is not a URL even when it mentions one. A value that starts with | ||
| // `//` or a scheme directly followed by a non-space (`https:/x`, `jdbc:...`) is still a URL, even | ||
| // with spaces later; `Review: check {file}` stays prose. | ||
| if (/\s/.test(trimmed) && !/^(?:\/\/|[a-z][a-z\d+.-]*:\S)/i.test(trimmed)) return value | ||
| // Also covers malformed authorities and opaque schemes such as jdbc:postgresql://. | ||
| const authority = /^(?:[^/?#]*:)?\/\/([^/?#]*)/.exec(trimmed)?.[1] | ||
| if (authority?.includes("@")) return "***" | ||
| // Fail closed for URL-like strings that cannot be safely parsed, regardless of scheme. | ||
| if (!URL.canParse(trimmed)) { | ||
| return authority !== undefined || /^[a-z][a-z\d+.-]*:/i.test(trimmed) ? "***" : value | ||
| } | ||
| const url = new URL(trimmed) | ||
| if (url.username || url.password) return "***" | ||
|
|
||
| const redactParams = (params: string) => | ||
| params | ||
| .split("&") | ||
| .map((param) => (param ? `${param.split("=", 1)[0]}=***` : param)) | ||
| .join("&") | ||
|
|
||
| // Preserve the original URL spelling and fragments that are only anchors. | ||
| return value | ||
| .replace(/^([^?#]*\?)([^#]*)/, (_, prefix, params) => prefix + redactParams(params)) | ||
| .replace(/#(.*)/s, (fragment, params) => (params.includes("=") ? `#${redactParams(params)}` : fragment)) | ||
| } | ||
|
|
||
| // Endpoint URLs (MCP `url`, provider `api`/`baseURL`) can carry credentials in the path | ||
| // (`/mcp/u/<token>/...`, `/v1/sk-...`), so print only the origin. | ||
| const endpointKey = /^(?:url|baseURL|api|endpoint)$/i | ||
|
|
||
| function redactServerURL(value: string): string { | ||
| const trimmed = value.trim() | ||
| // Explicit endpoint fields fail closed: a schemeless value can still carry a path token. | ||
| if (!URL.canParse(trimmed)) return "***" | ||
| const url = new URL(trimmed) | ||
| if (url.username || url.password || url.origin === "null") return "***" | ||
| const rest = url.pathname.replace(/^\/$/, "") + url.search + url.hash | ||
| return rest ? `${url.origin}/***` : url.origin | ||
| } | ||
|
|
||
| // Free-form secret carriers: every environment value is masked, and argv keeps only the | ||
| // executable (`env TOKEN=x`, `--header "Authorization: ..."`, `-pSecret`, JDBC strings). | ||
| const environmentKey = /^(?:environment|env)$/i | ||
| const argvKey = /^(?:command|headersCommand|args)$/i | ||
|
|
||
| function redactArgv(argv: unknown[]): unknown[] { | ||
| return argv.map((item, index) => | ||
| typeof item !== "string" || index === 0 ? item : "***", | ||
| ) | ||
| } | ||
|
|
||
| export function redactConfig(value: unknown, headers = false): unknown { | ||
| if (typeof value === "string") return headers ? "***" : redactURL(value) | ||
| if (Array.isArray(value)) | ||
| return value.map((item, index) => { | ||
| if (typeof item === "string") { | ||
| const flag = /^--?([^=]+)=/.exec(item) | ||
| const previous = value[index - 1] | ||
| const bare = typeof previous === "string" ? /^--?([^=]+)$/.exec(previous) : null | ||
| if ((flag && secretName.test(flag[1])) || (bare && secretName.test(bare[1]))) return "***" | ||
|
anandgupta42 marked this conversation as resolved.
|
||
| } | ||
| return redactConfig(item, headers) | ||
| }) | ||
| if (value === null || typeof value !== "object") return value | ||
|
|
||
| return Object.fromEntries( | ||
| Object.entries(value).map(([key, item]) => { | ||
| if (typeof item === "string" && (headers || secretName.test(key))) return [key, "***"] | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| // Only address-like values (`.`, `/`, `\`, `:`): a permission entry for a tool named `api` holds `allow`/`deny`. | ||
| if (typeof item === "string" && endpointKey.test(key) && /[./\\:]/.test(item) && !headers) | ||
| return [key, redactServerURL(item)] | ||
| if (Array.isArray(item) && argvKey.test(key) && !headers) return [key, redactArgv(item)] | ||
|
anandgupta42 marked this conversation as resolved.
|
||
| // headersCommand is a record of header name -> argv. | ||
| if (item && typeof item === "object" && /^headersCommand$/i.test(key) && !headers) | ||
| return [ | ||
| key, | ||
| Object.fromEntries( | ||
| Object.entries(item).map(([name, argv]) => [name, Array.isArray(argv) ? redactArgv(argv) : "***"]), | ||
| ), | ||
| ] | ||
| const secretScope = headers || /^headers$/i.test(key) || environmentKey.test(key) || secretName.test(key) | ||
| return [key, redactConfig(item, secretScope)] | ||
|
Comment on lines
+88
to
+89
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a provider declares credential lookup variables via the schema-valid Useful? React with 👍 / 👎. |
||
| }), | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -4,7 +4,9 @@ import { cmd } from "./cmd" | |||||
| import { withNetworkOptions, resolveNetworkOptions } from "../network" | ||||||
| import { AppRuntime } from "../../effect/app-runtime" | ||||||
| import { Flag } from "../../flag/flag" | ||||||
| import open from "open" | ||||||
| // altimate_change start — upstream_fix: backport 29f07e0c69 restrict browser URLs to http(s) | ||||||
| import { openUrl } from "@opencode-ai/core/open" | ||||||
| // altimate_change end | ||||||
| import { networkInterfaces } from "os" | ||||||
| // altimate_change start — auto-register Altimate Base before the server (and its provider state) starts | ||||||
| import { FreeTier } from "../../altimate/free/client" | ||||||
|
|
@@ -81,11 +83,15 @@ export const WebCommand = cmd({ | |||||
| } | ||||||
|
|
||||||
| // Open localhost in browser | ||||||
| open(localhostUrl.toString()).catch(() => {}) | ||||||
| // altimate_change start — upstream_fix: backport 29f07e0c69 restrict browser URLs to http(s) | ||||||
| openUrl(localhostUrl.toString()).catch(() => {}) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Redundant
Suggested change
Reply with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving as is: harmless, and it keeps the line identical to upstream's version of this call site. |
||||||
| // altimate_change end | ||||||
| } else { | ||||||
| const displayUrl = server.url.toString() | ||||||
| UI.println(UI.Style.TEXT_INFO_BOLD + " Web interface: ", UI.Style.TEXT_NORMAL, displayUrl) | ||||||
| open(displayUrl).catch(() => {}) | ||||||
| // altimate_change start — upstream_fix: backport 29f07e0c69 restrict browser URLs to http(s) | ||||||
| openUrl(displayUrl).catch(() => {}) | ||||||
| // altimate_change end | ||||||
| } | ||||||
|
|
||||||
| await new Promise(() => {}) | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.