fix: prevent Squid config injection via --allow-domains and --allow-urls#1517
fix: prevent Squid config injection via --allow-domains and --allow-urls#1517
Conversation
Malicious input to --allow-domains or --allow-urls containing whitespace, newlines, or null bytes could inject arbitrary directives into the generated squid.conf. This adds: 1. Character validation in validateDomainOrPattern() rejects whitespace, null bytes, quotes, semicolons, backslashes 2. assertSafeForSquidConfig() in squid-config.ts validates every value before interpolation into squid.conf 3. URL pattern validation in cli.ts for --allow-urls Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (6 files)
✨ New Files (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
This PR hardens the generation of squid.conf against config-injection attacks by validating/guarding user-supplied allowlist inputs (--allow-domains and --allow-urls) before they are interpolated into Squid directives.
Changes:
- Rejects dangerous characters in
validateDomainOrPattern()to prevent directive/token injection via--allow-domains. - Adds a defense-in-depth
assertSafeForSquidConfig()guard around Squid config interpolation points for domains/patterns/URL patterns. - Adds CLI validation for
--allow-urls(whitespace / null bytes) and extends unit tests to cover injection attempts.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/domain-patterns.ts |
Adds rejection of whitespace/control/meta characters in domain/pattern inputs to prevent Squid config injection. |
src/squid-config.ts |
Introduces assertSafeForSquidConfig() and applies it at domain/pattern/url interpolation points in generated squid.conf. |
src/cli.ts |
Adds URL pattern validation to block whitespace/null bytes before config generation. |
src/domain-patterns.test.ts |
Adds unit tests for rejected injection characters and acceptance of underscore-containing names. |
src/squid-config.test.ts |
Adds defense-in-depth tests ensuring config generation rejects injected newline/space payloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/domain-patterns.ts
Outdated
| const charCode = match[0].charCodeAt(0); | ||
| const charDesc = charCode <= 0x20 || charCode === 0x7f | ||
| ? `U+${charCode.toString(16).padStart(4, '0')}` | ||
| : `'${match[0]}'`; | ||
| throw new Error( | ||
| `Invalid domain '${trimmed}': contains invalid character ${charDesc}. ` + | ||
| `Domain names must not contain whitespace, quotes, semicolons, backslashes, or control characters.` |
There was a problem hiding this comment.
The thrown error message interpolates trimmed directly (e.g., Invalid domain '${trimmed}'), which can include the very control characters you’re rejecting (LF/CR/Unicode whitespace). This can lead to multi-line / forged log output when the CLI logs error.message. Consider rendering the rejected input via JSON.stringify(trimmed) (and/or escaping) and also make the human-readable list of rejected characters accurate (the regex currently also rejects # and backticks).
| const charCode = match[0].charCodeAt(0); | |
| const charDesc = charCode <= 0x20 || charCode === 0x7f | |
| ? `U+${charCode.toString(16).padStart(4, '0')}` | |
| : `'${match[0]}'`; | |
| throw new Error( | |
| `Invalid domain '${trimmed}': contains invalid character ${charDesc}. ` + | |
| `Domain names must not contain whitespace, quotes, semicolons, backslashes, or control characters.` | |
| const safeDomainForMessage = JSON.stringify(trimmed); | |
| const charCode = match[0].charCodeAt(0); | |
| const charDesc = charCode <= 0x20 || charCode === 0x7f | |
| ? `U+${charCode.toString(16).padStart(4, '0')}` | |
| : `'${match[0]}'`; | |
| throw new Error( | |
| `Invalid domain ${safeDomainForMessage}: contains invalid character ${charDesc}. ` + | |
| `Domain names must not contain whitespace, quotes, semicolons, backticks, hash characters, backslashes, or control characters.` |
This comment has been minimized.
This comment has been minimized.
|
Smoke test results for ✅ GitHub MCP — Last 2 merged PRs: Overall: PASS
|
|
Smoke test results (run 23810570009)
Overall: PASS
|
Chroot Version Comparison Results
Result: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Smoke Test Results✅ GitHub MCP: #1508 fix: copy get-claude-key.sh to chroot-accessible path | #1469 feat: add volume mount for ~/.copilot/session-state to persist events.jsonl Overall: PASS
|
Smoke Test Results — Copilot Engine
Overall: PASS PR author:
|
Chroot Version Comparison Results
Overall: Tests did not fully pass — Python and Node.js versions differ between host and chroot environments. Go versions match.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Applied Copilot's suggestion in 86db03b: using |
lpcox
left a comment
lpcox
left a comment
There was a problem hiding this comment.
Review: Squid Config Injection Fix
Nice work on the layered defense approach — validateDomainOrPattern() as primary validation + assertSafeForSquidConfig() as defense-in-depth is the right pattern.
🔴 Issue: Inconsistent character validation between domains and URLs
The DANGEROUS_CHARS regex in validateDomainOrPattern() is comprehensive:
const DANGEROUS_CHARS = /[\s\0"'`;#\\]/;But both the URL validation in cli.ts and the defense-in-depth assertSafeForSquidConfig() only check a subset:
// cli.ts:1643 - URL validation
if (/[\s\0]/.test(url)) {
// squid-config.ts:61 - defense-in-depth
if (/[\s\0]/.test(value)) {This means URL patterns can still contain # (Squid comment character), ;, quotes, backticks, and backslashes. For example, a # in a URL pattern would cause everything after it to be treated as a comment in the generated Squid config.
Fix: Both should use the same DANGEROUS_CHARS set. Consider extracting it as a shared constant:
// Shared constant (export from domain-patterns.ts)
export const SQUID_DANGEROUS_CHARS = /[\s\0"'`;#\\]/;Then use it in all three locations: validateDomainOrPattern(), assertSafeForSquidConfig(), and the cli.ts URL validation.
The whole point of defense-in-depth is catching what the primary validation misses — so assertSafeForSquidConfig() should validate the full dangerous character set.
✅ What looks good
- All interpolation points in squid-config.ts are covered —
formatDomainForSquid()and everyurl_regex/dstdom_regexACL line usesassertSafeForSquidConfig() - DNS servers and ports are validated through separate paths (IP parsing, integer range checks) and are not vulnerable
- Test coverage is thorough — injection chars, underscore DNS names, and defense-in-depth tests all present
- Error messages use
JSON.stringify()to safely display the malicious input without risk of terminal escape sequences
|
@copilot update the PR based on this review #1517 (review) |
Updated in 6c22710 and 441af68. See the reply above for details on the changes made. |
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
|
Smoke test summary:
|
Summary
--allow-domainsor--allow-urlscontaining whitespace, newlines, null bytes, hash characters (#), semicolons, or quotes could inject arbitrary Squid config directives (e.g.,http_access allow all) or truncate ACL lines (via#comment injection) in the generatedsquid.conf, bypassing all domain filteringvalidateDomainOrPattern()that rejects whitespace, null bytes, quotes, semicolons, backslashes, and hash characters — all of which are Squid config delimiters or metacharactersSQUID_DANGEROUS_CHARSfromdomain-patterns.tscovering whitespace, null bytes,#,;, quotes, and backticks — used consistently across all three validation sites. Backslash is intentionally excluded from this shared constant because URL regex patterns passed to--allow-urlslegitimately use\for regex escaping; domain names additionally reject\via a local checkassertSafeForSquidConfig()insquid-config.tsnow usesSQUID_DANGEROUS_CHARS(expanded from the previous[\s\0]subset) and is applied to user-provided values only — internally-generated regex patterns derived from already-validated domain names are trusted and not re-checked--allow-urlsvalidation incli.tsfrom[\s\0]toSQUID_DANGEROUS_CHARS, now catching#,;, and quote injectionChanges
src/domain-patterns.tsSQUID_DANGEROUS_CHARSconstant; added dangerous character rejection (including\) invalidateDomainOrPattern()src/squid-config.tsassertSafeForSquidConfig()helper usingSQUID_DANGEROUS_CHARS; applied to user-provided interpolation points; removed from internally-generated regex patternssrc/cli.ts--allow-urlsvalidation to useSQUID_DANGEROUS_CHARS(catches#,;, quotes)src/domain-patterns.test.tssrc/squid-config.test.ts#, and;injection in config generationTest plan
_dmarc.example.com,_acme-challenge.example.com)#and;injection at squid-config level via URL patterns🤖 Generated with Claude Code