Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/acp-bridge/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@
"types": "./dist/spawnChannel.d.ts",
"import": "./dist/spawnChannel.js"
},
"./logRedaction": {
"types": "./dist/logRedaction.d.ts",
"import": "./dist/logRedaction.js"
},
"./bridgeClient": {
"types": "./dist/bridgeClient.d.ts",
"import": "./dist/bridgeClient.js"
Expand Down
255 changes: 255 additions & 0 deletions packages/acp-bridge/src/logRedaction.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,255 @@
/**
* @license
* Copyright 2025 Qwen Team
* SPDX-License-Identifier: Apache-2.0
*/

import { describe, expect, it, vi } from 'vitest';
import { redactLogCredentials } from './logRedaction.js';

const R = '<redacted>';

describe('redactLogCredentials', () => {
// ── Bearer tokens ──────────────────────────────────────────────────

it('redacts Bearer tokens', () => {
expect(
redactLogCredentials('Authorization: Bearer eyJhbGciOi.xyz.abc'),
).toBe(`Authorization: ${R}`);
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This test asserts toBe('Authorization: ${R}') — the same output the Authorization catch-all test expects. The Bearer pattern's unique contribution (preserving the Bearer keyword as Authorization: Bearer <redacted>) is never asserted. If CREDENTIAL_PATTERNS are accidentally reordered (catch-all before Bearer), both tests still pass silently.

Fix: assert the specific Bearer output format to guard the ordering dependency:

Suggested change
});
expect(
redactLogCredentials('Authorization: Bearer eyJhbGciOi.xyz.abc'),
).toBe(`Authorization: Bearer ${R}`);

— qwen3.7-max via Qwen Code /review


it('redacts bare Bearer (no "Authorization:" prefix)', () => {
expect(redactLogCredentials('header Bearer t-abc123_def.456')).toBe(
`header Bearer ${R}`,
);
});

it('is case-insensitive for Bearer', () => {
expect(redactLogCredentials('bearer TOKEN123')).toBe(`bearer ${R}`);
});

it('redacts Bearer tokens with +/= characters (RFC 6750)', () => {
expect(redactLogCredentials('Bearer abc+/def==')).toBe(`Bearer ${R}`);
});

// ── QQBot tokens ──────────────────────────────────────────────────

it('redacts QQBot tokens', () => {
expect(redactLogCredentials('token: QQBot abcdef123456')).toBe(
`token: QQBot ${R}`,
);
});

// ── Authorization header catch-all ────────────────────────────────

it('redacts Authorization with Basic scheme', () => {
expect(redactLogCredentials('Authorization: Basic dXNlcjpwYXNz')).toBe(
`Authorization: ${R}`,
);
});

it('redacts Authorization with Digest scheme', () => {
expect(redactLogCredentials('Authorization: Digest username="user"')).toBe(
`Authorization: ${R}`,
);
});

// ── DingTalk access token header ──────────────────────────────────

it('redacts DingTalk access token header', () => {
expect(
redactLogCredentials('x-acs-dingtalk-access-token: abc123def456'),
).toBe(`x-acs-dingtalk-access-token: ${R}`);
});

// ── API key prefixes ──────────────────────────────────────────────

it('redacts sk- keys with ≥20 chars', () => {
const key = 'sk-' + 'a'.repeat(20);
expect(redactLogCredentials(`key=${key}`)).toBe(`key=sk-${R}`);
});

it('does NOT redact short sk- strings', () => {
expect(redactLogCredentials('sk-test')).toBe('sk-test');
expect(redactLogCredentials('sk-abc')).toBe('sk-abc');
});

it('redacts GitHub personal access tokens', () => {
const token = 'ghp_' + 'A'.repeat(36);
expect(redactLogCredentials(`GITHUB_TOKEN=${token}`)).toContain(R);
});

it('redacts GitLab personal access tokens', () => {
const token = 'glpat-' + 'x'.repeat(20);
expect(redactLogCredentials(token)).toBe(R);
});

it('redacts Slack bot tokens', () => {
const token = 'xoxb-' + '1'.repeat(20);
expect(redactLogCredentials(token)).toBe(R);
});

it('redacts real-format Slack tokens with hyphens', () => {
const token = 'xoxb-fake-' + 'a'.repeat(30);
expect(redactLogCredentials(token)).toBe(R);
});

it('does NOT redact short ghp_ strings', () => {
expect(redactLogCredentials('ghp_short')).toBe('ghp_short');
});

it('redacts sk- keys with hyphens (sk-proj-, sk-ant-)', () => {
const key = 'sk-proj-' + 'a'.repeat(20);
expect(redactLogCredentials(key)).toBe(`sk-${R}`);
});

it('redacts github_pat_ fine-grained PATs', () => {
const token = 'github_pat_' + 'A'.repeat(40);
expect(redactLogCredentials(token)).toBe(R);
});

it('redacts AWS STS temporary credentials (ASIA prefix)', () => {
expect(redactLogCredentials('ASIAIOSFODNN7EXAMPLE')).toBe(R);
});

// ── AWS access key IDs ────────────────────────────────────────────

it('redacts AWS access key IDs', () => {
expect(redactLogCredentials('AKIAIOSFODNN7EXAMPLE')).toBe(R);
});

// ── Key=value secret assignments ──────────────────────────────────

it('redacts token= assignments with ≥10 char values', () => {
expect(redactLogCredentials('token=abcdef1234567890')).toBe(`token=${R}`);
});

it('redacts API_KEY: assignments', () => {
expect(redactLogCredentials('api_key: sk-longenoughvalue123')).toBe(
`api_key: ${R}`,
);
});

it('redacts password= assignments', () => {
expect(redactLogCredentials('password=mysecretpassword123')).toBe(
`password=${R}`,
);
});

it('redacts secret= assignments', () => {
expect(redactLogCredentials('secret=verylongsecretvalue')).toBe(
`secret=${R}`,
);
});

it('does NOT redact short token values', () => {
expect(redactLogCredentials('token=abc')).toBe('token=abc');
});

it('redacts compound key names like AWS_SECRET_ACCESS_KEY', () => {
expect(
redactLogCredentials(
'AWS_SECRET_ACCESS_KEY=wJalrXUtnFEMI/K7MDENG/bPxRfiCY',
),
).toBe(`AWS_SECRET_ACCESS_KEY=${R}`);
});

it('redacts QWEN_DAEMON_TOKEN assignments', () => {
expect(
redactLogCredentials('QWEN_DAEMON_TOKEN=some-long-token-value-here'),
).toBe(`QWEN_DAEMON_TOKEN=${R}`);
});

it('redacts client_secret assignments', () => {
expect(redactLogCredentials('client_secret: abcdef1234567890xxxx')).toBe(
`client_secret: ${R}`,
);
});

// ── JSON-quoted secret fields ───────────────────────────────────

it('redacts JSON "token":"..." fields', () => {
const line = '{"token":"abcdef1234567890xxxx","event":"login"}';
const result = redactLogCredentials(line);
expect(result).not.toContain('abcdef1234567890xxxx');
expect(result).toContain(`"token":"${R}"`);
expect(result).toContain('"event":"login"');
});

it('redacts JSON "client_secret":"..." fields', () => {
const line = '{"client_secret":"my-very-secret-value-1234"}';
const result = redactLogCredentials(line);
expect(result).not.toContain('my-very-secret-value-1234');
});

it('does NOT redact short JSON values', () => {
expect(redactLogCredentials('{"token":"short"}')).toBe('{"token":"short"}');
});

// ── URL-embedded credentials ──────────────────────────────────────

it('redacts URL credentials', () => {
expect(
redactLogCredentials('proxy: https://user:pass@proxy.local:8080'),
).toBe(`proxy: https://${R}@proxy.local:8080`);
});

it('redacts multiple userinfo segments', () => {
expect(redactLogCredentials('http://a:b@c:d@host/path')).toBe(
`http://${R}@host/path`,
);
});

// ── Mixed patterns ────────────────────────────────────────────────

it('redacts multiple credentials on one line', () => {
const line =
'Authorization: Bearer eyJtoken calling https://user:pass@api.example.com/v1';
const result = redactLogCredentials(line);
expect(result).not.toContain('eyJtoken');
expect(result).not.toContain('user:pass');
expect(result).toContain(R);
});

// ── Edge cases ────────────────────────────────────────────────────

it('returns empty string unchanged', () => {
expect(redactLogCredentials('')).toBe('');
});

it('returns non-matching line unchanged', () => {
const line = '[2025-01-01T00:00:00Z] [INFO] Server started on port 4170';
expect(redactLogCredentials(line)).toBe(line);
});

it('preserves non-ASCII content', () => {
const line = '[INFO] 连接成功 Bearer abc123.xyz.def';
const result = redactLogCredentials(line);
expect(result).toContain('连接成功');
expect(result).toContain(`Bearer ${R}`);
});

it('handles very long lines without error', () => {
const longLine = 'x'.repeat(100_000);
expect(() => redactLogCredentials(longLine)).not.toThrow();
});
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This describe('createStderrForwarder redaction integration') block is a near-duplicate of spawnChannel.test.ts:239-253 ("redacts credentials flushed via onEnd (partial line)"). Both create a createStderrForwarder, feed 'Bearer secrettoken123', call onEnd(), and assert the same things. This test exercises createStderrForwarder from spawnChannel.ts, not redactLogCredentials, so it belongs in spawnChannel.test.ts where it already exists.

Consider removing this block to avoid maintaining the same test in two files.

— qwen3.7-max via Qwen Code /review

describe('createStderrForwarder redaction integration', () => {
it('redacts credentials flushed via onEnd (partial line without newline)', async () => {
const { createStderrForwarder } = await import('./spawnChannel.js');
const captured: Array<{ line: string; level?: string }> = [];
const stderrSpy = vi.spyOn(process.stderr, 'write').mockReturnValue(true);
const forwarder = createStderrForwarder({
prefix: '[p] ',
onDiagnosticLine: (l, lvl) => captured.push({ line: l, level: lvl }),
});
forwarder.onData('Bearer secrettoken123');
expect(captured).toHaveLength(0);
forwarder.onEnd();
expect(captured).toHaveLength(1);
expect(captured[0]!.line).not.toContain('secrettoken123');
expect(captured[0]!.line).toContain('<redacted>');
stderrSpy.mockRestore();
});
});
99 changes: 99 additions & 0 deletions packages/acp-bridge/src/logRedaction.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/**
* @license
* Copyright 2025 Qwen Team
* SPDX-License-Identifier: Apache-2.0
*/

const REDACTED = '<redacted>';

const CREDENTIAL_PATTERNS: Array<{ pattern: RegExp; replacement: string }> = [
// Bearer tokens (Feishu, Weixin, Daemon SDK).
// Charset covers RFC 6750 token68 plus base64 (+, /, =, ~, .).
{
pattern: /(Bearer\s+)[A-Za-z0-9._~+/=-]+/gi,
replacement: `$1${REDACTED}`,
},
// QQ Bot tokens (uses "QQBot" prefix instead of "Bearer")
{
pattern: /(QQBot\s+)[A-Za-z0-9._~+/=-]+/gi,
replacement: `$1${REDACTED}`,
},
// Authorization header catch-all (Basic and single-credential schemes).
// Matches "<scheme> <credential>" (up to 2 tokens). Multi-parameter
// schemes like Digest are only partially redacted — acceptable since
// channel SDKs use Bearer/Basic exclusively.
// Must come after Bearer/QQBot so those get the more specific replacement.
{
pattern: /(Authorization:\s*)\S+(?:\s+\S+)?/gi,
replacement: `$1${REDACTED}`,
},
// DingTalk custom access token header
Comment thread
doudouOUC marked this conversation as resolved.
{
pattern: /(x-acs-dingtalk-access-token:\s*)\S+/gi,
replacement: `$1${REDACTED}`,
},
// API keys with common prefixes (≥20 chars to avoid false positives on
// short test fixtures like "sk-test"). Includes hyphens for compound
// prefixes like sk-proj-, sk-ant-api03-.
{
pattern: /sk-[a-zA-Z0-9-]{20,}/g,
replacement: `sk-${REDACTED}`,
},
// GitHub / GitLab / Slack tokens.
Comment thread
doudouOUC marked this conversation as resolved.
// Includes github_pat_ (fine-grained PATs) and ghu_ (app user tokens).
// Slack tokens use hyphens as separators: xoxb-NNN-NNN-alphanum.
{
pattern: /(?:ghp_|gho_|ghs_|ghu_|github_pat_|glpat-|xoxb-|xoxp-)[a-zA-Z0-9_-]{20,}/g,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The GitHub/GitLab/Slack token pattern covers xoxb- and xoxp- but omits xapp- (Slack app-level tokens used for Socket Mode). The project's own secret-scanner.ts in packages/core already recognizes xapp- as a credential type (slack-app-token). Consider adding xapp- to the prefix list:

Suggested change
pattern: /(?:ghp_|gho_|ghs_|ghu_|github_pat_|glpat-|xoxb-|xoxp-)[a-zA-Z0-9_-]{20,}/g,
pattern: /(?:ghp_|gho_|ghs_|ghu_|github_pat_|glpat-|xoxb-|xoxp-|xapp-)[a-zA-Z0-9_-]{20,}/g,

— qwen3.7-max via Qwen Code /review

replacement: REDACTED,
},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The regex includes gho_, ghs_, ghu_, and xoxp- prefixes, but none of these have positive tests. Only ghp_, github_pat_, glpat-, and xoxb- are tested. A typo in the alternation (e.g., gho missing the underscore) would go undetected.

it('redacts gho_ OAuth tokens', () => {
  expect(redactLogCredentials('gho_' + 'A'.repeat(36))).toBe(R);
});
it('redacts ghs_ server-to-server tokens', () => {
  expect(redactLogCredentials('ghs_' + 'A'.repeat(36))).toBe(R);
});
it('redacts ghu_ app user tokens', () => {
  expect(redactLogCredentials('ghu_' + 'A'.repeat(36))).toBe(R);
});
it('redacts xoxp- Slack user tokens', () => {
  expect(redactLogCredentials('xoxp-' + '1'.repeat(20))).toBe(R);
});

— qwen3.7-max via Qwen Code /review

// AWS access key IDs (permanent AKIA + temporary STS ASIA)
{
pattern: /(?:AKIA|ASIA)[A-Z0-9]{16}/g,
replacement: REDACTED,
Comment thread
doudouOUC marked this conversation as resolved.
},
// Key=value assignments for simple secret names (token=, secret=, etc.)
{
pattern:
/((?:api[_-]?key|token|secret|password|pwd)[_-]?[=:]\s*)\S{10,}/gi,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Two gaps in credential keyword coverage:

  1. The key=value pattern alternation api[_-]?key|token|secret|password|pwd is missing passwd. A log line like DB_PASSWD=wJalrXUtnFEMI/K7MDENG is not caught. The compound env-var pattern also requires _PASSWORD suffix, not _PASSWD.

  2. The AWS access key pattern (?:AKIA|ASIA)[A-Z0-9]{16} has no trailing boundary assertion. Adjacent alphanumeric characters after the 20-char key are left unredacted.

Suggested fixes:

Suggested change
/((?:api[_-]?key|token|secret|password|pwd)[_-]?[=:]\s*)\S{10,}/gi,
pattern:
/((?:api[_-]?key|token|secret|password|passwd|pwd)[_-]?[=:]\s*)\S{10,}/gi,
Suggested change
/((?:api[_-]?key|token|secret|password|pwd)[_-]?[=:]\s*)\S{10,}/gi,
pattern: /(?:AKIA|ASIA)[A-Z0-9]{16}\b/g,

And add PASSWD to the compound env-var suffix:

_(?:KEY|TOKEN|SECRET|PASSWORD|PASSWD)\s*[=:]\s*

— qwen3.7-max via Qwen Code /review

replacement: `$1${REDACTED}`,
},
// Compound env-var keys ending in _KEY, _TOKEN, _SECRET, or _PASSWORD
// (e.g. AWS_SECRET_ACCESS_KEY=, QWEN_DAEMON_TOKEN=).
// Segment lengths are capped to prevent quadratic backtracking on long
// all-uppercase input.
{
pattern:
/([A-Z][A-Z0-9]{0,50}(?:_[A-Z0-9]{1,50}){0,10}_(?:KEY|TOKEN|SECRET|PASSWORD)\s*[=:]\s*)\S{10,}/g,
replacement: `$1${REDACTED}`,
},
// JSON-quoted secret fields: "token":"...", "client_secret":"...", etc.
// Uses an explicit key list to avoid backtracking on quoted content.
{
pattern:
/("(?:api_key|api-key|apikey|token|secret|password|pwd|access_token|client_secret|app_secret|authorization)"\s*:\s*")[^"]{10,}(")/gi,
replacement: `$1${REDACTED}$2`,
},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The URL-embedded credentials regex (?:[^/\s]+@)+ allows @ inside the inner character class [^/\s], creating ambiguity about which @ is the delimiter vs. content. While bounded in practice by the mandatory @ per iteration, excluding @ from the inner class eliminates all backtracking ambiguity:

Suggested change
},
pattern: /\b([a-z][a-z0-9+.-]{0,31}:\/\/)(?:[^/@\s]+@)+/gi,

— qwen3.7-max via Qwen Code /review

// URL-embedded credentials (scheme://user:pass@host)
{
pattern: /\b([a-z][a-z0-9+.-]{0,31}:\/\/)(?:[^/\s]+@)+/gi,
replacement: `$1${REDACTED}@`,
},
];

/**
* Redacts credentials from a single log line. Applied per-line by
* `createStderrForwarder` before writing to the daemon's stderr and log
* file. The patterns cover Bearer/QQBot tokens, Authorization headers,
* common API key prefixes, secret env assignments, URL-embedded
* credentials, and platform-specific headers (DingTalk).
*

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] redactLogCredentials runs 11 sequential .replace() calls on every log line, even for lines with no credential-like content (e.g., [INFO] Server started on port 4170). For high-throughput worker output, this multiplies per-line regex cost by 11.

A fast-path guard would skip the full scan for the common case:

export function redactLogCredentials(line: string): string {
  if (!/[Bb]earer|QQBot|[Aa]uthorization|x-acs-|sk-|gh[psou]_|github_pat_|glpat-|xox[bp]p-|AKIA|ASIA|:\/\//.test(line)
      && !/(?:token|secret|password|pwd|api[_-]?key)[_-]?[=:]/i.test(line)
      && !/_(?:KEY|TOKEN|SECRET|PASSWORD)\s*[=:]/.test(line)) {
    return line;
  }
  let result = line;
  for (const { pattern, replacement } of CREDENTIAL_PATTERNS) {
    result = result.replace(pattern, replacement);
  }
  return result;
}

— qwen3.7-max via Qwen Code /review

* Patterns are applied sequentially — earlier, more-specific patterns
* (Bearer, QQBot) run before the broader Authorization catch-all.
*/
export function redactLogCredentials(line: string): string {
let result = line;
for (const { pattern, replacement } of CREDENTIAL_PATTERNS) {
result = result.replace(pattern, replacement);
}
return result;
}
Loading
Loading