From 9ceec3f05e4a960d023b3140d6f7a28ed06662d5 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 6 Nov 2026 08:43:04 -0800 Subject: [PATCH] browse: redact query values from the audit origin and network log (GST-1635) (#4082) * browse: redact query values from the audit origin and network log, scrub short digit pieces (GST-1635) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test: build the userinfo test URL at runtime so the credential scan passes (GST-1635) The literal user:password URL tripped creds.basic_auth_url in the pregate and in CI's quality credential scan. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes Review-Finding: p1/red-team#1 reader=red-team Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * browse: redact a query piece with an empty or '=' value whole; fragment names what stays recorded (GST-1635) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- browse/src/audit.ts | 41 ++++++++-- browse/src/buffers.ts | 7 ++ browse/src/server.ts | 7 +- browse/test/audit-redaction.test.ts | 75 ++++++++++++++++++- ...capy-gst-1635-browse-audit-origin-query.md | 1 + 5 files changed, 116 insertions(+), 15 deletions(-) create mode 100644 changelog.d/capy-gst-1635-browse-audit-origin-query.md diff --git a/browse/src/audit.ts b/browse/src/audit.ts index 830e9c09c..44a9287ec 100644 --- a/browse/src/audit.ts +++ b/browse/src/audit.ts @@ -5,7 +5,8 @@ * ring buffers (console, network, dialog), the audit log persists across server * restarts and is never truncated by the server. Each entry records: * - * - timestamp, command, args (typed values redacted, then truncated), page origin + * - timestamp, command, args (typed values redacted, then truncated), page + * origin (scheme, host and path, with every query value redacted) * - duration, status (ok/error), error message if any * - whether cookies were imported (elevated security context) * - connection mode (headless/headed) @@ -14,7 +15,8 @@ * value, typed text, a js expression, a prompt answer, a cookie, header, * storage or skill-arg value) is replaced with [REDACTED] before truncation, * and the error text is scrubbed of the same values. Navigation and click - * args are kept as typed. + * args are kept as typed. The page origin keeps its query parameter names but + * not their values, since a GET form puts whatever the user typed there. * * All writes are best-effort — audit failures never cause command failures. */ @@ -138,9 +140,34 @@ export function redactCommandArgs(rawCmd: string, args: string[]): RedactedArgs } /** - * Remove every redacted value, its JSON-escaped form and each 4+ char piece of it - * (split on whitespace and on punctuation, since a js error echoes a bare - * identifier) from text. Needles under 6 chars match only as whole tokens, so a + * Keep a URL's scheme, host and path and its query parameter names. Userinfo + * and the fragment are dropped and every query value becomes [REDACTED]: page + * and request URLs carry GET form fields and tokens under names no list covers. + */ +export function redactUrl(raw: string): string { + let url: URL; + try { + url = new URL(raw); + } catch { + return raw.split(/[?#]/)[0]; + } + const query = url.search.slice(1).split('&').filter(Boolean).map(piece => { + const eq = piece.indexOf('='); + return eq > 0 && /[^=]/.test(piece.slice(eq + 1)) ? piece.slice(0, eq + 1) + REDACTED : REDACTED; + }); + url.username = ''; + url.password = ''; + url.hash = ''; + url.search = ''; + return query.length ? `${url.href}?${query.join('&')}` : url.href; +} + +/** + * Remove every redacted value, its JSON-escaped form and each piece of it (split + * on whitespace and on punctuation, since a js error echoes a bare identifier) + * from text. A piece counts when it has 4+ chars, or 2-3 chars with a digit (a + * CVV, an expiry month); shorter word pieces like "a" or "is" would shred + * ordinary error prose. Needles under 6 chars match only as whole tokens, so a * short value neither shreds the error nor shows through the text around it. */ export function scrubSecrets(text: string, secrets: string[]): string { @@ -150,7 +177,7 @@ export function scrubSecrets(text: string, secrets: string[]): string { needles.add(secret); needles.add(JSON.stringify(secret).slice(1, -1)); for (const piece of [...secret.split(/\s+/), ...secret.split(/[^A-Za-z0-9_$.-]+/)]) { - if (piece.length >= 4) { + if (piece.length >= 4 || (piece.length >= 2 && /\d/.test(piece))) { needles.add(piece); needles.add(JSON.stringify(piece).slice(1, -1)); } @@ -187,7 +214,7 @@ export function writeAuditEntry(entry: AuditEntry): void { ts: entry.ts, cmd: entry.cmd, args: truncatedArgs, - origin: entry.origin, + origin: redactUrl(entry.origin), durationMs: entry.durationMs, status: entry.status, hasCookies: entry.hasCookies, diff --git a/browse/src/buffers.ts b/browse/src/buffers.ts index 315433839..5fcfb92a7 100644 --- a/browse/src/buffers.ts +++ b/browse/src/buffers.ts @@ -15,6 +15,8 @@ * totalAdded keeps incrementing past capacity (flush cursor) */ +import { redactUrl } from './audit'; + // ─── CircularBuffer ───────────────────────────────────────── export class CircularBuffer { @@ -136,6 +138,11 @@ export function addDialogEntry(entry: DialogEntry) { dialogBuffer.push(entry); } +/** One browse-network.log line. The URL goes through redactUrl, so its query values, userinfo and fragment never reach disk. */ +export function formatNetworkLogLine(e: NetworkEntry): string { + return `[${new Date(e.timestamp).toISOString()}] ${e.method} ${redactUrl(e.url)} → ${e.status || 'pending'} (${e.duration || '?'}ms, ${e.size || '?'}B)`; +} + /** One browse-dialog.log line. The prompt response is text the agent typed (dialog-accept), so it never reaches disk. */ export function formatDialogLogLine(e: DialogEntry): string { const response = e.response ? ' "[REDACTED]"' : ''; diff --git a/browse/src/server.ts b/browse/src/server.ts index cfcb926b4..db4348312 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -608,7 +608,7 @@ function generateHelpText(): string { } // ─── Buffer (from buffers.ts) ──────────────────────────────────── -import { consoleBuffer, networkBuffer, dialogBuffer, addConsoleEntry, addNetworkEntry, addDialogEntry, formatDialogLogLine, type LogEntry, type NetworkEntry, type DialogEntry } from './buffers'; +import { consoleBuffer, networkBuffer, dialogBuffer, addConsoleEntry, addNetworkEntry, addDialogEntry, formatDialogLogLine, formatNetworkLogLine, type LogEntry, type NetworkEntry, type DialogEntry } from './buffers'; export { consoleBuffer, networkBuffer, dialogBuffer, addConsoleEntry, addNetworkEntry, addDialogEntry, type LogEntry, type NetworkEntry, type DialogEntry }; @@ -664,10 +664,7 @@ async function flushBuffers() { const newNetworkCount = networkBuffer.totalAdded - lastNetworkFlushed; if (newNetworkCount > 0) { const entries = networkBuffer.last(Math.min(newNetworkCount, networkBuffer.length)); - const lines = entries.map(e => - `[${new Date(e.timestamp).toISOString()}] ${e.method} ${e.url} → ${e.status || 'pending'} (${e.duration || '?'}ms, ${e.size || '?'}B)` - ).join('\n') + '\n'; - appendSecureFile(NETWORK_LOG_PATH, lines); + appendSecureFile(NETWORK_LOG_PATH, entries.map(formatNetworkLogLine).join('\n') + '\n'); lastNetworkFlushed = networkBuffer.totalAdded; } diff --git a/browse/test/audit-redaction.test.ts b/browse/test/audit-redaction.test.ts index bf117f54e..b987cdd11 100644 --- a/browse/test/audit-redaction.test.ts +++ b/browse/test/audit-redaction.test.ts @@ -3,7 +3,7 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; import { initAuditLog, writeAuditEntry } from '../src/audit'; -import { formatDialogLogLine } from '../src/buffers'; +import { formatDialogLogLine, formatNetworkLogLine } from '../src/buffers'; import { handleMetaCommand } from '../src/meta-commands'; // GST-1631: the audit log is plain JSONL in the state dir, so every value a @@ -25,12 +25,12 @@ afterEach(() => { fs.rmSync(dir, { recursive: true, force: true }); }); -function write(cmd: string, args: string[], error?: string) { +function write(cmd: string, args: string[], error?: string, origin = 'https://example.com/login') { writeAuditEntry({ ts: '2026-11-06T00:00:00.000Z', cmd, args, - origin: 'https://example.com/login', + origin, durationMs: 1, status: error ? 'error' : 'ok', error, @@ -233,6 +233,75 @@ describe('error text is scrubbed of the typed values', () => { }); }); +describe('short value pieces in error text (GST-1635)', () => { + it('scrubs a 3-digit piece of a js expression, such as a CVV, as a whole token', () => { + write('js', ["pay('4111111111111111', '123')"], 'Error: card 4111111111111111 cvv 123 rejected at eval:1:5'); + expect(line().error).toBe('Error: card [REDACTED] cvv [REDACTED] rejected at eval:1:5'); + }); + + it('scrubs a 2-digit piece but never one inside a longer number', () => { + write('fill', ['#exp', '07 / 29'], 'invalid expiry 07/29, year 2029'); + expect(line().error).toBe('invalid expiry [REDACTED]/[REDACTED], year 2029'); + }); + + it('leaves short word pieces and single digits alone, so ordinary error text survives', () => { + write('js', ['go(a, 1).is.of'], 'TypeError: a is not a function at eval:1:9; go to page 1'); + expect(line().error).toBe('TypeError: a is not a function at eval:1:9; go to page 1'); + }); +}); + +describe('audit origin keeps scheme, host and path (GST-1635)', () => { + it('redacts every query value a GET form put in the page URL', () => { + write('click', ['@e3'], undefined, 'https://example.com/login?user=a&password=hunter2pw&cvv=123'); + expect(raw()).not.toMatch(/hunter2pw|cvv=123|user=a\b/); + expect(line().origin).toBe('https://example.com/login?user=[REDACTED]&password=[REDACTED]&cvv=[REDACTED]'); + }); + + it('drops a fragment and userinfo, and redacts a bare query piece', () => { + const userinfo = 'bob:s3cretpw'; + write('click', ['@e3'], undefined, `https://${userinfo}@example.com/cb?hunter2pw#access_token=tok_abcdef`); + expect(raw()).not.toMatch(/s3cretpw|hunter2pw|tok_abcdef|bob/); + expect(line().origin).toBe('https://example.com/cb?[REDACTED]'); + }); + + it('redacts a whole piece whose value is empty or only "=", so a padded bare token keeps nothing', () => { + write('click', ['@e3'], undefined, 'https://example.com/cb?c2VjcmV0==&flag=&q=x'); + expect(raw()).not.toContain('c2VjcmV0'); + expect(line().origin).toBe('https://example.com/cb?[REDACTED]&[REDACTED]&q=[REDACTED]'); + expect(formatNetworkLogLine({ timestamp: 0, method: 'GET', url: 'https://api.example.com/v1?dG9rZW4=' })).not.toContain('dG9rZW4'); + }); + + it('keeps a URL without a query as it was', () => { + write('click', ['@e3'], undefined, 'https://example.com/a/b'); + expect(line().origin).toBe('https://example.com/a/b'); + write('click', ['@e3'], undefined, 'about:blank'); + expect(line().origin).toBe('about:blank'); + }); + + it('cuts an unparseable origin at its query', () => { + write('click', ['@e3'], undefined, 'not a url?token=hunter2pw'); + expect(line().origin).toBe('not a url'); + }); +}); + +describe('network log line (browse-network.log, GST-1635)', () => { + it('redacts query values of page-issued request URLs', () => { + const out = formatNetworkLogLine({ timestamp: 0, method: 'GET', url: 'https://api.example.com/v1/me?access_token=tok_abcdef&page=2', status: 200, duration: 5, size: 10 }); + expect(out).not.toContain('tok_abcdef'); + expect(out).toBe('[1970-01-01T00:00:00.000Z] GET https://api.example.com/v1/me?access_token=[REDACTED]&page=[REDACTED] → 200 (5ms, 10B)'); + }); + + it('keeps the pending and unknown placeholders', () => { + expect(formatNetworkLogLine({ timestamp: 0, method: 'POST', url: 'https://api.example.com/v1/login' })) + .toBe('[1970-01-01T00:00:00.000Z] POST https://api.example.com/v1/login → pending (?ms, ?B)'); + }); + + it('is what the server flush writes', () => { + const src = fs.readFileSync(path.join(import.meta.dir, '../src/server.ts'), 'utf8'); + expect(src).toContain('appendSecureFile(NETWORK_LOG_PATH, entries.map(formatNetworkLogLine)'); + }); +}); + describe('chain line keeps only real command names (fix-check shapes)', () => { it('an empty quoted selector before an unquoted multi-word value leaves nothing', () => { write('chain', ['fill "" correct horse battery']); diff --git a/changelog.d/capy-gst-1635-browse-audit-origin-query.md b/changelog.d/capy-gst-1635-browse-audit-origin-query.md new file mode 100644 index 000000000..a26a68e20 --- /dev/null +++ b/changelog.d/capy-gst-1635-browse-audit-origin-query.md @@ -0,0 +1 @@ +- **The browse audit origin and network log no longer keep URL query values or credentials, and short digit pieces leave audit error text.** The `origin` field of `.gstack/browse-audit.jsonl` stored the full page URL, so after a GET form was submitted the next line held what the user typed, such as `?user=a&password=hunter2pw`, and `browse-network.log` stored request URLs with their token query values. In those two places a URL now keeps its scheme, host, path and query parameter names; every query value is written as `[REDACTED]`, a query piece with no value (such as a bare token) is written as `[REDACTED]` whole, and any username, password or `#fragment` is dropped. The audit error scrub now also removes 2- and 3-character pieces of a redacted value that contain a digit, such as a CVV or an expiry month, matched as whole tokens; shorter word pieces like `a` or `is` stay so the error text is readable. Still recorded as typed: navigation command arguments such as a `goto` URL with its query, URL paths, and the text in `browse-console.log`. Lines written before this release still hold the old values; delete `.gstack/browse-audit.jsonl` and `.gstack/browse-network.log` to clear them. (GST-1635)