fix(security): Close remaining credential leaks found in review

intent(security): a Codex review of the first commit surfaced seven more paths
where a credentialed URL survived, three of them outside the logging sites the
advisory named
decision(skill-generation): redact skillSourceUrl at its source in remoteAction —
it is only rendered as a link in the generated SKILL.md and never used to reach
the network, so leaving it raw persisted the credential into a file the user is
likely to commit, which outlives any log
constraint(mcp-prompts): packRemoteRepositoryPrompts echoes the repository into a
literal `repository: "..."` tool-call instruction, so redacting there would break
the call it is telling the model to make; only the tool *result* is redacted
decision(scheme-detection): left isExplicitRemoteUrl case-sensitive and redacted
the "Target path does not exist" message instead — making URL detection
case-insensitive is a behavior change that belongs in its own PR
learned(userinfo-regex): terminating the userinfo search at '?' and '#' let a
password containing either survive untouched; only '/' and whitespace may end it,
and the resulting over-redaction of a path-less URL with '@' in its query is the
safer failure
learned(scp-regex): stopping at the first '@' left the tail of a password that
itself contains '@'; matching to the last '@' plus a `host:path` lookahead fixes
that and also stops `failed at 12:30@example.com` from being mangled

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Kazuki Yamada
2026-08-04 23:06:42 +09:00
parent 4bab13c8ce
commit d5e5614e4d
6 changed files with 116 additions and 19 deletions
+53 -1
View File
@@ -1,5 +1,5 @@
import { describe, expect, test } from 'vitest';
import { redactErrorMessage, redactUrl } from '../../src/shared/urlRedact.js';
import { redactErrorMessage, redactOptionsForLog, redactUrl } from '../../src/shared/urlRedact.js';
// Fixture credentials are assembled from parts rather than written inline, so
// the URLs below do not read as real basic-auth literals to secret scanners.
@@ -50,6 +50,23 @@ describe('urlRedact', () => {
expect(redactUrl(`${USER}:${PASSWORD}@github.com:owner/repo.git`)).toBe('***@github.com:owner/repo.git');
});
test('should redact a scp-style password containing an @ character', () => {
// Stopping at the first '@' would leave the rest of the password in the clear.
expect(redactUrl(`${USER}:abc@${PASSWORD}@github.com:owner/repo.git`)).toBe('***@github.com:owner/repo.git');
});
test('should redact a userinfo containing an unencoded ? or #', () => {
// Malformed, but still a real secret: a password typed with these characters
// unencoded must not survive just because the URL does not parse.
expect(redactUrl(`https://oauth2:se?cret@github.com/o/r.git`)).toBe('https://***@github.com/o/r.git');
expect(redactUrl(`https://oauth2:se#cret@github.com/o/r.git`)).toBe('https://***@github.com/o/r.git');
});
test('should not mistake a time or port for scp-style credentials', () => {
// No `host:path` follows the '@', so this is not a remote at all.
expect(redactUrl('Build failed at 12:30@example.com')).toBe('Build failed at 12:30@example.com');
});
test('should leave ordinary SSH remotes readable', () => {
// The username is a fixed literal here, not a secret: authentication
// happens out of band via the SSH key.
@@ -85,6 +102,13 @@ describe('urlRedact', () => {
);
});
test('should not swallow the diagnostic text that surrounds a redacted query value', () => {
// The quote and the trailing status are part of git's message, not the token.
expect(redactUrl("fatal: unable to access 'https://example.com/r?token=s3cr3t': HTTP 401")).toBe(
"fatal: unable to access 'https://example.com/r?token=***': HTTP 401",
);
});
test('should redact every URL when text embeds more than one', () => {
const text = `tried https://a:${PASSWORD}@one.example.com/x.git then https://c:${PASSWORD}@two.example.com/y.git`;
expect(redactUrl(text)).toBe('tried https://***@one.example.com/x.git then https://***@two.example.com/y.git');
@@ -121,4 +145,32 @@ describe('urlRedact', () => {
expect(redactErrorMessage(undefined)).toBe('undefined');
});
});
describe('redactOptionsForLog', () => {
test('should redact every URL-bearing field', () => {
const url = `https://${TOKEN}@github.com/o/r.git`;
const options = { remote: url, skillSourceUrl: url, quiet: true };
const redacted = redactOptionsForLog(options);
expect(redacted.remote).toBe('https://***@github.com/o/r.git');
expect(redacted.skillSourceUrl).toBe('https://***@github.com/o/r.git');
expect(redacted.quiet).toBe(true);
});
test('should not mutate the options it was given', () => {
// The caller keeps using this object to actually reach the remote.
const options = { remote: `https://${TOKEN}@github.com/o/r.git` };
redactOptionsForLog(options);
expect(options.remote).toBe(`https://${TOKEN}@github.com/o/r.git`);
});
test('should pass through options that carry no URL', () => {
const options: { remote?: string; quiet: boolean } = { quiet: true };
expect(redactOptionsForLog(options)).toEqual({ quiet: true });
});
});
});