From 9dc58576f259326eb971d8640a8e6b45ed892535 Mon Sep 17 00:00:00 2001 From: Kevin Cui Date: Mon, 27 Jul 2026 19:57:20 -0400 Subject: [PATCH 1/2] fix(logging): keep credential-bearing URL values out of the CLI log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Presigned download URLs carry their credentials in query values, and the local rolling log captured them through several channels (the default log level is debug, so every request-lifecycle field lands in the file): - the download flow logged the full URL, the raw query string, and the post-redirect `response.url` verbatim; - Bun fetch errors expose the full request URL on an enumerable `path` property, which pino's default `err` serializer writes out — so every failed request leaked the URL even where no field logged it; - the argv line only masked `--api-key`/`--session-token`/`--token`, letting `oo file download `, `oo variables create NAME VALUE`, and payload flags such as `--headers` land verbatim; - `oo skills install pkg#` embedded the temporary share credential in the request pathname, which the shared request logger records as the `path` field; - a hand-edited auth.toml with a syntax error logged the TOML parse error, whose message and codeblock embed the offending lines — raw API keys included. Land one policy in `application/logging/url-sanitizer.ts`: URLs in log fields keep origin, path, and query parameter names while values become `REDACTED`. `sanitizeUrlForLogging` covers explicit fields, a custom `err` serializer (wired into the production logger and the test log capture) covers URL-bearing error properties and params, argv redaction now sanitizes URL arguments and secret positionals, the share download overrides its logged path, and the auth store mirrors the connector store in omitting TOML parse errors. Closes #323 Signed-off-by: Kevin Cui --- __tests__/helpers.ts | 6 + .../logging/create-cli-logger.test.ts | 25 ++++ src/adapters/logging/create-cli-logger.ts | 4 + src/adapters/store/file-auth-store.test.ts | 35 +++++ src/adapters/store/file-auth-store.ts | 6 +- src/application/bootstrap/run-cli.test.ts | 50 +++++++ src/application/bootstrap/run-cli.ts | 67 +++++++++- src/application/commands/connector/logout.ts | 5 +- .../commands/file/download/request.test.ts | 64 +++++++++ .../commands/file/download/request.ts | 10 +- .../skills/registry-skill-source.test.ts | 32 +++++ .../commands/skills/registry-skill-source.ts | 10 +- src/application/logging/url-sanitizer.test.ts | 125 ++++++++++++++++++ src/application/logging/url-sanitizer.ts | 89 +++++++++++++ src/application/self-update/core.ts | 13 +- src/application/update/release-metadata.ts | 17 ++- 16 files changed, 535 insertions(+), 23 deletions(-) create mode 100644 src/application/logging/url-sanitizer.test.ts create mode 100644 src/application/logging/url-sanitizer.ts 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..81579e35 100644 --- a/src/application/bootstrap/run-cli.test.ts +++ b/src/application/bootstrap/run-cli.test.ts @@ -52,6 +52,56 @@ 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("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..3c56a79c 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,24 @@ 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; +// 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 +667,14 @@ 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 positionalCount = 0; - for (const argument of argv) { + for (const [index, argument] of argv.entries()) { if (shouldRedactNextValue) { redactedArguments.push(redactedCliArgumentValue); shouldRedactNextValue = false; @@ -666,7 +688,20 @@ function redactSensitiveCliArguments(argv: readonly string[]): string[] { continue; } - redactedArguments.push(argument); + if ( + positionalRule !== undefined + && index >= positionalRule.commandPath.length + && !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 +711,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.ts b/src/application/commands/connector/logout.ts index 43f165e0..9f58a6d7 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"; @@ -42,7 +43,9 @@ export const connectorLogoutCommand: CliCommandDefinition = { context.logger.info( { - url: selfHosted.url, + // The stored value is normalized at login, but a hand-edited + // file can carry query values or userinfo — sanitize anyway. + url: sanitizeUrlForLogging(selfHosted.url), }, "Self-hosted connector configuration removed.", ); 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.", From 62371e51fad9485f164b117b13a4dc4ec5791d36 Mon Sep 17 00:00:00 2001 From: Kevin Cui Date: Mon, 27 Jul 2026 20:27:22 -0400 Subject: [PATCH 2/2] fix(logging): address review findings on argv and logout sanitization Track a bare `--` in argv redaction so secret positionals passed after the option terminator (`oo variables create NAME -- -value`) are still redacted instead of slipping past the `startsWith("-")` check. Reuse the sanitized connector URL for the logout success output as well as the log field, so a hand-edited connector.toml carrying query values or userinfo cannot leak them to the terminal. Signed-off-by: Kevin Cui --- src/application/bootstrap/run-cli.test.ts | 21 +++++++++++++++++++ src/application/bootstrap/run-cli.ts | 14 ++++++++++--- .../commands/connector/logout.test.ts | 20 ++++++++++++++++++ src/application/commands/connector/logout.ts | 10 +++++---- 4 files changed, 58 insertions(+), 7 deletions(-) diff --git a/src/application/bootstrap/run-cli.test.ts b/src/application/bootstrap/run-cli.test.ts index 81579e35..13f3961f 100644 --- a/src/application/bootstrap/run-cli.test.ts +++ b/src/application/bootstrap/run-cli.test.ts @@ -102,6 +102,27 @@ describe("runCli bootstrap", () => { } }); + 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 3c56a79c..a157ec37 100644 --- a/src/application/bootstrap/run-cli.ts +++ b/src/application/bootstrap/run-cli.ts @@ -126,8 +126,9 @@ const sensitiveCliOptionLongFlags = [ "--token", ] as const; // Commands whose positional arguments carry secret values. Every positional -// beyond `keptPositionals` (counted after the command words) is redacted; -// option values may be over-redacted by this rule, which errs on the safe side. +// 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; @@ -672,6 +673,7 @@ function redactSensitiveCliArguments(argv: readonly string[]): string[] { ); const redactedArguments: string[] = []; let shouldRedactNextValue = false; + let pastOptionTerminator = false; let positionalCount = 0; for (const [index, argument] of argv.entries()) { @@ -688,10 +690,16 @@ function redactSensitiveCliArguments(argv: readonly string[]): string[] { continue; } + if (argument === "--" && !pastOptionTerminator) { + pastOptionTerminator = true; + redactedArguments.push(argument); + continue; + } + if ( positionalRule !== undefined && index >= positionalRule.commandPath.length - && !argument.startsWith("-") + && (pastOptionTerminator || !argument.startsWith("-")) ) { positionalCount += 1; 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 9f58a6d7..852af090 100644 --- a/src/application/commands/connector/logout.ts +++ b/src/application/commands/connector/logout.ts @@ -41,11 +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( { - // The stored value is normalized at login, but a hand-edited - // file can carry query values or userinfo — sanitize anyway. - url: sanitizeUrlForLogging(selfHosted.url), + url: sanitizedUrl, }, "Self-hosted connector configuration removed.", ); @@ -53,7 +55,7 @@ export const connectorLogoutCommand: CliCommandDefinition = { writeLine( context.stdout, context.translator.t("connector.logout.success", { - url: selfHosted.url, + url: sanitizedUrl, }), ); },