diff --git a/__tests__/helpers.ts b/__tests__/helpers.ts index 61743ec6..f907b6cd 100644 --- a/__tests__/helpers.ts +++ b/__tests__/helpers.ts @@ -40,6 +40,7 @@ import { } from "../src/application/commands/shared/oo-request.ts"; import { APP_NAME } from "../src/application/config/app-config.ts"; import { CliUserError } from "../src/application/contracts/cli.ts"; +import { serializeErrorForLogging } from "../src/application/logging/url-sanitizer.ts"; import { defaultSettings, renderSettingsFile } from "../src/application/schemas/settings.ts"; import { isPathMissingError } from "../src/application/shared/fs-errors.ts"; import { createTerminalColors } from "../src/application/terminal-colors.ts"; @@ -345,6 +346,11 @@ export function createLogCapture(): LogCapture { }, }, level: "debug", + // Mirrors the production logger so captured output reflects + // the same err sanitization (create-cli-logger.ts). + serializers: { + err: serializeErrorForLogging, + }, }, stream, ), diff --git a/src/adapters/logging/create-cli-logger.test.ts b/src/adapters/logging/create-cli-logger.test.ts index 7bd27cf6..421c0eb4 100644 --- a/src/adapters/logging/create-cli-logger.test.ts +++ b/src/adapters/logging/create-cli-logger.test.ts @@ -25,6 +25,31 @@ describe("createCliLogger", () => { expect(content).toContain(`"msg":"file-only log"`); }); + test("sanitizes URL-bearing error paths before writing", async () => { + const logDirectoryPath = await createTemporaryDirectory("oo-log-err-url"); + const loggerHandle = createCliLogger({ + appName: APP_NAME, + env: {}, + logDirectoryPath, + }); + + loggerHandle.logger.warn( + { + err: Object.assign(new Error("Unable to connect."), { + code: "ConnectionRefused", + path: "https://download.example.com/file?signature=secret123", + }), + }, + "request failed", + ); + loggerHandle.close(); + + const content = await readFile(loggerHandle.logFilePath, "utf8"); + + expect(content).not.toContain("secret123"); + expect(content).toContain("signature=REDACTED"); + }); + test("exposes the created log file path", async () => { const logDirectoryPath = await createTemporaryDirectory("oo-log-path"); const loggerHandle = createCliLogger({ diff --git a/src/adapters/logging/create-cli-logger.ts b/src/adapters/logging/create-cli-logger.ts index febc562f..50383d80 100644 --- a/src/adapters/logging/create-cli-logger.ts +++ b/src/adapters/logging/create-cli-logger.ts @@ -1,6 +1,7 @@ import type { LevelWithSilentOrString, Logger } from "pino"; import pino from "pino"; +import { serializeErrorForLogging } from "../../application/logging/url-sanitizer.ts"; import { RollingFileDestination } from "./rolling-file-destination.ts"; export interface CliLoggerOptions { @@ -34,6 +35,9 @@ export function createCliLogger(options: CliLoggerOptions): CliLoggerHandle { name: options.appName, level, timestamp: pino.stdTimeFunctions.isoTime, + serializers: { + err: serializeErrorForLogging, + }, formatters: { level(label) { return { level: label }; diff --git a/src/adapters/store/file-auth-store.test.ts b/src/adapters/store/file-auth-store.test.ts index 40d74bf0..2aa5f1a8 100644 --- a/src/adapters/store/file-auth-store.test.ts +++ b/src/adapters/store/file-auth-store.test.ts @@ -288,6 +288,41 @@ describe("FileAuthStore", () => { } satisfies Partial); }); + test("keeps auth file content out of invalid TOML logs", async () => { + const root = await createTemporaryDirectory("auth-store-toml-log"); + const logCapture = createLogCapture(); + const store = new FileAuthStore({ + appName: APP_NAME, + env: { + HOME: root, + XDG_CONFIG_HOME: root, + }, + logger: logCapture.logger, + platform: "linux", + }); + + await mkdir(dirname(store.getFilePath()), { recursive: true }); + await writeFile( + store.getFilePath(), + [ + "[[auth]]", + "api_key = \"top-secret-key\"", + "broken = [", + ].join("\n"), + "utf8", + ); + + await expect(store.read()).rejects.toMatchObject({ + key: "errors.authStore.invalidToml", + } satisfies Partial); + + const logs = logCapture.read(); + + expect(logs).toContain(`"msg":"Auth store file contained invalid TOML."`); + expect(logs).not.toContain("top-secret-key"); + logCapture.close(); + }); + test("rejects unsupported auth schema", async () => { const root = await createTemporaryDirectory("auth-store-invalid-schema"); const logCapture = createLogCapture(); diff --git a/src/adapters/store/file-auth-store.ts b/src/adapters/store/file-auth-store.ts index 550f8675..277a1410 100644 --- a/src/adapters/store/file-auth-store.ts +++ b/src/adapters/store/file-auth-store.ts @@ -267,12 +267,14 @@ export class FileAuthStore implements AuthStore { try { parsedContent = parseToml(content); } - catch (error) { + catch { + // The TOML parse error is deliberately not logged: its message and + // codeblock embed the offending document lines, which may contain + // raw API keys. this.logger?.error( { ...withCategory(logCategory.systemError), contentBytes: content.length, - err: error, ...withStorePath(this.filePath), }, "Auth store file contained invalid TOML.", diff --git a/src/application/bootstrap/run-cli.test.ts b/src/application/bootstrap/run-cli.test.ts index db5a78f0..13f3961f 100644 --- a/src/application/bootstrap/run-cli.test.ts +++ b/src/application/bootstrap/run-cli.test.ts @@ -52,6 +52,77 @@ describe("runCli bootstrap", () => { } }); + test("keeps presigned URL query values out of the argv log", async () => { + const sandbox = await createCliSandbox(); + + try { + await sandbox.run( + [ + "file", + "download", + "https://download.example.com/report.txt?signature=argv-secret", + ], + { + fetcher: async () => { + throw new Error("offline"); + }, + }, + ); + + const logContent = await readLatestLogContent(sandbox); + + expect(logContent).toContain("\"msg\":\"CLI command received.\""); + expect(logContent).not.toContain("argv-secret"); + expect(logContent).toContain("signature=REDACTED"); + } + finally { + await sandbox.cleanup(); + } + }); + + test("keeps secret positional values out of the argv log", async () => { + const sandbox = await createCliSandbox(); + + try { + await sandbox.run( + ["variables", "create", "MY_VAR", "positional-secret-value"], + { + fetcher: async () => new Response("{}", { status: 500 }), + }, + ); + + const logContent = await readLatestLogContent(sandbox); + + expect(logContent).toContain("\"msg\":\"CLI command received.\""); + expect(logContent).not.toContain("positional-secret-value"); + expect(logContent).toContain(""); + } + finally { + await sandbox.cleanup(); + } + }); + + test("redacts secret positionals passed after a double dash", async () => { + const sandbox = await createCliSandbox(); + + try { + await sandbox.run( + ["variables", "create", "MY_VAR", "--", "-dash-secret-value"], + { + fetcher: async () => new Response("{}", { status: 500 }), + }, + ); + + const logContent = await readLatestLogContent(sandbox); + + expect(logContent).toContain("\"msg\":\"CLI command received.\""); + expect(logContent).not.toContain("dash-secret-value"); + } + finally { + await sandbox.cleanup(); + } + }); + test("executes published skill installation", async () => { const sandbox = await createCliSandbox(); const originalCwd = process.cwd; diff --git a/src/application/bootstrap/run-cli.ts b/src/application/bootstrap/run-cli.ts index e153fc1a..a157ec37 100644 --- a/src/application/bootstrap/run-cli.ts +++ b/src/application/bootstrap/run-cli.ts @@ -46,6 +46,7 @@ import { import { CliUserError } from "../contracts/cli.ts"; import { logCategory } from "../logging/log-categories.ts"; import { withCategory, withErrorKey, withStorePath } from "../logging/log-fields.ts"; +import { sanitizeIfHttpUrl } from "../logging/url-sanitizer.ts"; import { initializeCurrentVersionActiveMarker } from "../self-update/core.ts"; import { readEnvBoolean } from "../shared/env-boolean.ts"; import { createRetryingFetcher } from "../shared/retrying-fetcher.ts"; @@ -112,7 +113,25 @@ interface CreateCliExecutionContextOptions { } const redactedCliArgumentValue = ""; -const sensitiveCliOptionLongFlags = ["--api-key", "--session-token", "--token"] as const; +// Credential flags plus free-form request/payload flags whose values can +// embed tokens (proxy headers/body/query, action data, LLM input). +const sensitiveCliOptionLongFlags = [ + "--api-key", + "--body", + "--data", + "--headers", + "--input", + "--query", + "--session-token", + "--token", +] as const; +// Commands whose positional arguments carry secret values. Every positional +// beyond `keptPositionals` (counted after the command words) is redacted, and +// a bare `--` makes every later argument count as positional; option values +// may be over-redacted by this rule, which errs on the safe side. +const sensitiveCliPositionalRules = [ + { commandPath: ["variables", "create"], keptPositionals: 1 }, +] as const; export async function runCli(argv: string[]): Promise { return executeCli({ @@ -649,10 +668,15 @@ function getSystemLocale(): string | undefined { } function redactSensitiveCliArguments(argv: readonly string[]): string[] { + const positionalRule = sensitiveCliPositionalRules.find(rule => + rule.commandPath.every((word, index) => argv[index] === word), + ); const redactedArguments: string[] = []; let shouldRedactNextValue = false; + let pastOptionTerminator = false; + let positionalCount = 0; - for (const argument of argv) { + for (const [index, argument] of argv.entries()) { if (shouldRedactNextValue) { redactedArguments.push(redactedCliArgumentValue); shouldRedactNextValue = false; @@ -666,7 +690,26 @@ function redactSensitiveCliArguments(argv: readonly string[]): string[] { continue; } - redactedArguments.push(argument); + if (argument === "--" && !pastOptionTerminator) { + pastOptionTerminator = true; + redactedArguments.push(argument); + continue; + } + + if ( + positionalRule !== undefined + && index >= positionalRule.commandPath.length + && (pastOptionTerminator || !argument.startsWith("-")) + ) { + positionalCount += 1; + + if (positionalCount > positionalRule.keptPositionals) { + redactedArguments.push(redactedCliArgumentValue); + continue; + } + } + + redactedArguments.push(sanitizeUrlCliArgument(argument)); if (sensitiveCliOptionLongFlags.includes(argument as typeof sensitiveCliOptionLongFlags[number])) { shouldRedactNextValue = true; @@ -676,6 +719,32 @@ function redactSensitiveCliArguments(argv: readonly string[]): string[] { return redactedArguments; } +// Presigned/signed URLs arrive as positional arguments or option values +// (`oo file download `); their query values are credentials and must not +// reach the argv log fields. +function sanitizeUrlCliArgument(argument: string): string { + const sanitizedArgument = sanitizeIfHttpUrl(argument); + + if (sanitizedArgument !== argument) { + return sanitizedArgument; + } + + const assignmentIndex = argument.indexOf("="); + + if (assignmentIndex === -1) { + return argument; + } + + const value = argument.slice(assignmentIndex + 1); + const sanitizedValue = sanitizeIfHttpUrl(value); + + if (sanitizedValue === value) { + return argument; + } + + return `${argument.slice(0, assignmentIndex + 1)}${sanitizedValue}`; +} + function readSensitiveOptionAssignmentFlag(argument: string): string | undefined { for (const optionFlag of sensitiveCliOptionLongFlags) { if (argument.startsWith(`${optionFlag}=`)) { diff --git a/src/application/commands/connector/logout.test.ts b/src/application/commands/connector/logout.test.ts index ce318718..19570641 100644 --- a/src/application/commands/connector/logout.test.ts +++ b/src/application/commands/connector/logout.test.ts @@ -34,6 +34,26 @@ describe("connector logout CLI", () => { } }); + test("keeps hand-edited URL secrets out of the logout output", async () => { + const sandbox = await createCliSandbox(); + + try { + await writeConnectorFile(sandbox, { + token: "oct_test", + url: "http://localhost:3000/?token=hand-edited-secret", + }); + + const result = await sandbox.run(["connector", "logout"]); + + expect(result.exitCode).toBe(0); + expect(result.stdout).not.toContain("hand-edited-secret"); + expect(result.stdout).toContain("token=REDACTED"); + } + finally { + await sandbox.cleanup(); + } + }); + test("clears a corrupt connector file instead of failing", async () => { const sandbox = await createCliSandbox(); diff --git a/src/application/commands/connector/logout.ts b/src/application/commands/connector/logout.ts index 43f165e0..852af090 100644 --- a/src/application/commands/connector/logout.ts +++ b/src/application/commands/connector/logout.ts @@ -3,6 +3,7 @@ import type { ConnectorFile } from "../../schemas/connector.ts"; import { z } from "zod"; import { CliUserError } from "../../contracts/cli.ts"; +import { sanitizeUrlForLogging } from "../../logging/url-sanitizer.ts"; import { getSelfHostedConnectorConfig } from "../../schemas/connector.ts"; import { writeLine } from "../shared/output.ts"; @@ -40,9 +41,13 @@ export const connectorLogoutCommand: CliCommandDefinition = { ...rest }) => rest); + // The stored value is normalized at login, but a hand-edited file can + // carry query values or userinfo — sanitize before logging or printing. + const sanitizedUrl = sanitizeUrlForLogging(selfHosted.url); + context.logger.info( { - url: selfHosted.url, + url: sanitizedUrl, }, "Self-hosted connector configuration removed.", ); @@ -50,7 +55,7 @@ export const connectorLogoutCommand: CliCommandDefinition = { writeLine( context.stdout, context.translator.t("connector.logout.success", { - url: selfHosted.url, + url: sanitizedUrl, }), ); }, diff --git a/src/application/commands/file/download/request.test.ts b/src/application/commands/file/download/request.test.ts index ad561733..6ba0cea9 100644 --- a/src/application/commands/file/download/request.test.ts +++ b/src/application/commands/file/download/request.test.ts @@ -41,6 +41,70 @@ describe("requestFreshDownload", () => { } }); + test("keeps presigned query values out of the request logs", async () => { + const logCapture = createLogCapture(); + const requestUrl = new URL( + "https://download.example.com/files/report.txt?signature=start-secret&expires=1700000000", + ); + + try { + const response = setResponseUrl( + new Response("payload", { + status: 200, + }), + "https://cdn.example.com/files/report.txt?signature=redirect-secret", + ); + + await requestFreshDownload(requestUrl, { + fetcher: async () => response, + logger: logCapture.logger, + translator: createTranslator("en"), + }); + + const logContent = logCapture.read(); + + expect(logContent).not.toContain("start-secret"); + expect(logContent).not.toContain("redirect-secret"); + expect(logContent).toContain( + "\"url\":\"https://download.example.com/files/report.txt?signature=REDACTED&expires=REDACTED\"", + ); + expect(logContent).toContain( + "\"finalUrl\":\"https://cdn.example.com/files/report.txt?signature=REDACTED\"", + ); + } + finally { + logCapture.close(); + } + }); + + test("keeps presigned query values out of transport-failure logs", async () => { + const logCapture = createLogCapture(); + const requestUrl = new URL( + "https://download.example.com/files/report.txt?signature=transport-secret", + ); + + try { + await expectCliUserError(requestFreshDownload(requestUrl, { + fetcher: async () => { + throw Object.assign(new Error("Unable to connect."), { + code: "ConnectionRefused", + path: requestUrl.toString(), + }); + }, + logger: logCapture.logger, + translator: createTranslator("en"), + })); + + const logContent = logCapture.read(); + + expect(logContent).not.toContain("transport-secret"); + expect(logContent).toContain("signature=REDACTED"); + } + finally { + logCapture.close(); + } + }); + test("rejects non-success statuses that are not explicitly allowed", async () => { const logCapture = createLogCapture(); diff --git a/src/application/commands/file/download/request.ts b/src/application/commands/file/download/request.ts index 620b8296..aa860352 100644 --- a/src/application/commands/file/download/request.ts +++ b/src/application/commands/file/download/request.ts @@ -1,6 +1,7 @@ import type { CliExecutionContext } from "../../../contracts/cli.ts"; import type { FileDownloadSessionRecord } from "../../../contracts/file-download-session-store.ts"; +import { sanitizeUrlForLogging } from "../../../logging/url-sanitizer.ts"; import { requestOoResponse } from "../../shared/oo-request.ts"; type DownloadRequestContext = Pick; @@ -43,12 +44,13 @@ async function requestFileDownload( label: "File download", logFields: { start: { - query: requestUrl.searchParams.toString(), - url: urlString, + url: sanitizeUrlForLogging(requestUrl), }, success: response => ({ - finalUrl: response.url === "" ? urlString : response.url, - url: urlString, + finalUrl: sanitizeUrlForLogging( + response.url === "" ? urlString : response.url, + ), + url: sanitizeUrlForLogging(requestUrl), }), }, }); diff --git a/src/application/commands/skills/registry-skill-source.test.ts b/src/application/commands/skills/registry-skill-source.test.ts index 04df4226..4a8d9a01 100644 --- a/src/application/commands/skills/registry-skill-source.test.ts +++ b/src/application/commands/skills/registry-skill-source.test.ts @@ -4,6 +4,7 @@ import { describe, expect, test } from "bun:test"; import pino from "pino"; import { + createLogCapture, toRequest, } from "../../../../__tests__/helpers.ts"; import { createTranslator } from "../../../i18n/translator.ts"; @@ -152,6 +153,37 @@ describe("registry skill source", () => { ); }); + test("keeps the share id out of shared tarball download logs", async () => { + const logCapture = createLogCapture(); + + try { + await downloadRegistryPackageTarball( + "openai", + "0.0.3", + { + apiKey: "secret-1", + endpoint: "oomol.com", + }, + { + fetcher: async () => new Response(new Uint8Array([7])), + logger: logCapture.logger, + translator: createTranslator("en"), + }, + "share-credential-1", + ); + + const logContent = logCapture.read(); + + expect(logContent).not.toContain("share-credential-1"); + expect(logContent).toContain( + "\"path\":\"/-/oomol/package-shares/download-meta/REDACTED\"", + ); + } + finally { + logCapture.close(); + } + }); + test("reports package download count with authorization", async () => { const requests: Request[] = []; const context = createRegistrySkillSourceContext({ diff --git a/src/application/commands/skills/registry-skill-source.ts b/src/application/commands/skills/registry-skill-source.ts index 4cd64030..5e19f61a 100644 --- a/src/application/commands/skills/registry-skill-source.ts +++ b/src/application/commands/skills/registry-skill-source.ts @@ -4,6 +4,7 @@ import type { AuthAccount } from "../../schemas/auth.ts"; import { z } from "zod"; import { CliUserError } from "../../contracts/cli.ts"; import { withPackageIdentity } from "../../logging/log-fields.ts"; +import { redactedLogValue } from "../../logging/url-sanitizer.ts"; import { requestOo, requestOoResponse } from "../shared/oo-request.ts"; const registryPackageNotFoundStatus = 404; @@ -168,7 +169,14 @@ export async function downloadRegistryPackageTarball( host: { endpoint: account.endpoint, service: "registry" }, label: "Skills install package download", logFields: { - common: withPackageIdentity(packageName, packageVersion), + common: { + ...withPackageIdentity(packageName, packageVersion), + // The share id is a download credential embedded in the path; + // override the request-target path field with a redacted form. + ...(packageShareId === undefined + ? {} + : { path: createRegistryPackageShareDownloadMetaPath(redactedLogValue) }), + }, }, path: packageShareId === undefined ? createRegistryPackageTarballPath(packageName, packageVersion) diff --git a/src/application/logging/url-sanitizer.test.ts b/src/application/logging/url-sanitizer.test.ts new file mode 100644 index 00000000..88a40086 --- /dev/null +++ b/src/application/logging/url-sanitizer.test.ts @@ -0,0 +1,125 @@ +import { describe, expect, test } from "bun:test"; + +import { + sanitizeIfHttpUrl, + sanitizeUrlForLogging, + serializeErrorForLogging, +} from "./url-sanitizer.ts"; + +describe("sanitizeUrlForLogging", () => { + test("redacts query values while keeping parameter names", () => { + const sanitized = sanitizeUrlForLogging( + "https://download.example.com/files/report.txt?signature=abc123&expires=1700000000", + ); + + expect(sanitized).toBe( + "https://download.example.com/files/report.txt?signature=REDACTED&expires=REDACTED", + ); + }); + + test("accepts URL instances", () => { + const sanitized = sanitizeUrlForLogging( + new URL("https://example.com/path?token=secret"), + ); + + expect(sanitized).toBe("https://example.com/path?token=REDACTED"); + }); + + test("keeps URLs without a query untouched", () => { + expect(sanitizeUrlForLogging("https://example.com/files/report.txt")).toBe( + "https://example.com/files/report.txt", + ); + }); + + test("drops userinfo credentials and fragments", () => { + const sanitized = sanitizeUrlForLogging( + "https://user:pass@example.com/path?sig=abc#access_token=xyz", + ); + + expect(sanitized).toBe("https://example.com/path?sig=REDACTED"); + }); + + test("collapses duplicate parameters into one redacted entry", () => { + const sanitized = sanitizeUrlForLogging( + "https://example.com/path?tag=one&tag=two&sig=abc", + ); + + expect(sanitized).toBe("https://example.com/path?tag=REDACTED&sig=REDACTED"); + }); + + test("replaces unparseable input with a placeholder", () => { + expect(sanitizeUrlForLogging("not a url ?signature=abc")).toBe( + "", + ); + }); +}); + +describe("sanitizeIfHttpUrl", () => { + test("sanitizes http and https values, including uppercase schemes", () => { + expect(sanitizeIfHttpUrl("HTTPS://Example.com/Path?Sig=abc")).toBe( + "https://example.com/Path?Sig=REDACTED", + ); + expect(sanitizeIfHttpUrl("http://example.com/p?token=abc")).toBe( + "http://example.com/p?token=REDACTED", + ); + }); + + test("leaves non-URL strings unchanged", () => { + expect(sanitizeIfHttpUrl("plain-value")).toBe("plain-value"); + expect(sanitizeIfHttpUrl("/local/file/path")).toBe("/local/file/path"); + }); +}); + +describe("serializeErrorForLogging", () => { + test("sanitizes URL-shaped path properties from fetch errors", () => { + const error = Object.assign(new Error("Unable to connect."), { + code: "ConnectionRefused", + path: "https://download.example.com/file?signature=secret123", + }); + + const serialized = serializeErrorForLogging(error); + + expect(serialized.path).toBe( + "https://download.example.com/file?signature=REDACTED", + ); + expect(serialized.message).toBe("Unable to connect."); + expect(serialized.code).toBe("ConnectionRefused"); + }); + + test("keeps local filesystem path properties untouched", () => { + const error = Object.assign(new Error("ENOENT: no such file"), { + code: "ENOENT", + path: "/tmp/some/local/file.txt", + }); + + const serialized = serializeErrorForLogging(error); + + expect(serialized.path).toBe("/tmp/some/local/file.txt"); + }); + + test("serializes errors without a path property unchanged", () => { + const serialized = serializeErrorForLogging(new Error("Plain failure.")); + + expect(serialized.message).toBe("Plain failure."); + expect(serialized.path).toBeUndefined(); + }); + + test("sanitizes URL strings inside error params without mutating the error", () => { + const params = { + message: "https://h.example.com/p?token=param-secret", + status: 403, + }; + const error = Object.assign(new Error("Request failed."), { params }); + + const serialized = serializeErrorForLogging(error); + + // pino's err serializer decorates message-bearing nested objects with + // type/stack fields; only the sanitized values matter here. + expect(serialized.params).toMatchObject({ + message: "https://h.example.com/p?token=REDACTED", + status: 403, + }); + expect(JSON.stringify(serialized)).not.toContain("param-secret"); + expect(params.message).toBe("https://h.example.com/p?token=param-secret"); + }); +}); diff --git a/src/application/logging/url-sanitizer.ts b/src/application/logging/url-sanitizer.ts new file mode 100644 index 00000000..217bc605 --- /dev/null +++ b/src/application/logging/url-sanitizer.ts @@ -0,0 +1,89 @@ +import type { SerializedError } from "pino"; + +import pino from "pino"; + +// URL log-sanitization policy: query values, userinfo credentials, and +// fragments must never reach a log line — presigned/signed URLs carry +// short-lived secrets there (signatures, tokens). The origin, path, and query +// parameter names stay as diagnostics. Apply sanitizeUrlForLogging to every +// URL that flows into a log field; the logger's err serializer applies the +// same policy to URL-bearing error properties (Bun fetch errors expose the +// full request URL on `path`). + +/** Placeholder for credential-bearing values in log output. */ +export const redactedLogValue = "REDACTED"; + +const unparseableUrlPlaceholder = ""; + +export function sanitizeUrlForLogging(input: string | URL): string { + let url: URL; + + try { + url = new URL(input); + } + catch { + // Never echo the raw input: an unparseable value may still embed secrets. + return unparseableUrlPlaceholder; + } + + url.username = ""; + url.password = ""; + url.hash = ""; + + // Materialized before mutation; set() also collapses duplicate parameters + // into a single redacted entry, which keeps every parameter name visible. + for (const name of new Set(url.searchParams.keys())) { + url.searchParams.set(name, redactedLogValue); + } + + return url.toString(); +} + +/** Sanitizes http(s) URLs and leaves every other string untouched. */ +export function sanitizeIfHttpUrl(value: string): string { + return isHttpUrl(value) ? sanitizeUrlForLogging(value) : value; +} + +export function serializeErrorForLogging(error: Error): SerializedError { + const serialized = pino.stdSerializers.err(error); + + if (typeof serialized !== "object" || serialized === null) { + return serialized; + } + + const path: unknown = serialized.path; + + // Local filesystem paths (fs errors) stay as-is; only URL-shaped values + // can carry query credentials. + if (typeof path === "string" && isHttpUrl(path)) { + serialized.path = sanitizeUrlForLogging(path); + } + + const params: unknown = serialized.params; + + // CliUserError.params is enumerable and reaches the serialized output; + // message params built from URLs must follow the same policy. + if (typeof params === "object" && params !== null) { + serialized.params = sanitizeUrlRecordValues(params as Record); + } + + return serialized; +} + +function sanitizeUrlRecordValues( + record: Record, +): Record { + const sanitized: Record = {}; + + for (const [key, value] of Object.entries(record)) { + sanitized[key] = typeof value === "string" ? sanitizeIfHttpUrl(value) : value; + } + + return sanitized; +} + +function isHttpUrl(value: string): boolean { + const prefix = value.slice(0, "https://".length).toLowerCase(); + + return prefix.startsWith("http://") || prefix === "https://"; +} diff --git a/src/application/self-update/core.ts b/src/application/self-update/core.ts index 9358d456..31aa5b1a 100644 --- a/src/application/self-update/core.ts +++ b/src/application/self-update/core.ts @@ -11,6 +11,7 @@ import { basename, dirname, isAbsolute, join, normalize, relative, sep } from "n import process from "node:process"; import { APP_NAME } from "../config/app-config.ts"; import { CliUserError } from "../contracts/cli.ts"; +import { sanitizeUrlForLogging } from "../logging/url-sanitizer.ts"; import { isSemver } from "../semver.ts"; import { isFileMissingError, isPathMissingError } from "../shared/fs-errors.ts"; import { pathExists, writeChunk } from "../shared/fs-utils.ts"; @@ -536,7 +537,7 @@ async function fetchBinaryResponse(options: { if (attempt === options.maxStallRetries) { options.logger.warn( { - requestUrl: options.url, + requestUrl: sanitizeUrlForLogging(options.url), stallTimeoutMs: options.stallTimeoutMs, totalAttempts: attempt + 1, }, @@ -550,7 +551,7 @@ async function fetchBinaryResponse(options: { options.logger.warn( { - requestUrl: options.url, + requestUrl: sanitizeUrlForLogging(options.url), retryAttempt: attempt + 1, stallTimeoutMs: options.stallTimeoutMs, }, @@ -595,7 +596,7 @@ async function downloadBinaryResponseOnce(options: { if (abortReason === "timeout") { options.logger.warn( { - requestUrl: options.url, + requestUrl: sanitizeUrlForLogging(options.url), timeoutMs: options.timeoutMs, }, "CLI self-update binary download timed out.", @@ -605,7 +606,7 @@ async function downloadBinaryResponseOnce(options: { options.logger.warn( { err: error, - requestUrl: options.url, + requestUrl: sanitizeUrlForLogging(options.url), }, "CLI self-update binary download failed.", ); @@ -617,7 +618,7 @@ async function downloadBinaryResponseOnce(options: { if (!response.ok) { options.logger.warn( { - requestUrl: options.url, + requestUrl: sanitizeUrlForLogging(options.url), status: response.status, }, "CLI self-update binary download returned a non-success status.", @@ -642,7 +643,7 @@ async function downloadBinaryResponseOnce(options: { if (abortReason === "timeout") { options.logger.warn( { - requestUrl: options.url, + requestUrl: sanitizeUrlForLogging(options.url), timeoutMs: options.timeoutMs, }, "CLI self-update binary download timed out.", diff --git a/src/application/update/release-metadata.ts b/src/application/update/release-metadata.ts index 4b9b26c1..78dbb23e 100644 --- a/src/application/update/release-metadata.ts +++ b/src/application/update/release-metadata.ts @@ -3,10 +3,15 @@ import type { Logger } from "pino"; import type { Fetcher } from "../contracts/cli.ts"; import { z } from "zod"; import { APP_NAME } from "../config/app-config.ts"; +import { sanitizeUrlForLogging } from "../logging/url-sanitizer.ts"; import { isSemver } from "../semver.ts"; export const cliReleaseBaseUrl = "https://static.oomol.com/release/apps/oo-cli"; export const cliLatestReleaseMetadataUrl = `${cliReleaseBaseUrl}/latest.json`; + +// Static today, but logged through the shared policy so a future query-bearing +// release URL cannot leak values into the log. +const sanitizedReleaseMetadataUrlLogValue = sanitizeUrlForLogging(cliLatestReleaseMetadataUrl); export const cliReleaseRequestTimeoutMs = 2000; const latestReleaseVersionSchema = z.object({ @@ -30,7 +35,7 @@ export async function fetchLatestCliReleaseVersion(options: { options.logger.debug( { - requestUrl: cliLatestReleaseMetadataUrl, + requestUrl: sanitizedReleaseMetadataUrlLogValue, timeoutMs, }, "CLI update latest-release request started.", @@ -52,7 +57,7 @@ export async function fetchLatestCliReleaseVersion(options: { options.logger.warn( { durationMs: Date.now() - requestStartedAt, - requestUrl: cliLatestReleaseMetadataUrl, + requestUrl: sanitizedReleaseMetadataUrlLogValue, timeoutMs, }, "CLI update latest-release request timed out or failed.", @@ -64,7 +69,7 @@ export async function fetchLatestCliReleaseVersion(options: { options.logger.warn( { durationMs: Date.now() - requestStartedAt, - requestUrl: cliLatestReleaseMetadataUrl, + requestUrl: sanitizedReleaseMetadataUrlLogValue, status: response.status, }, "CLI update latest-release request returned a non-success status.", @@ -81,7 +86,7 @@ export async function fetchLatestCliReleaseVersion(options: { options.logger.warn( { durationMs: Date.now() - requestStartedAt, - requestUrl: cliLatestReleaseMetadataUrl, + requestUrl: sanitizedReleaseMetadataUrlLogValue, status: response.status, }, "CLI update latest-release response did not include a valid version.", @@ -95,7 +100,7 @@ export async function fetchLatestCliReleaseVersion(options: { options.logger.warn( { durationMs: Date.now() - requestStartedAt, - requestUrl: cliLatestReleaseMetadataUrl, + requestUrl: sanitizedReleaseMetadataUrlLogValue, status: response.status, }, "CLI update latest-release response did not include a valid version.", @@ -107,7 +112,7 @@ export async function fetchLatestCliReleaseVersion(options: { { durationMs: Date.now() - requestStartedAt, latestVersion, - requestUrl: cliLatestReleaseMetadataUrl, + requestUrl: sanitizedReleaseMetadataUrlLogValue, status: response.status, }, "CLI update latest-release request completed.",