refactor: decompose validateDomainOrPattern and split domain-patterns.ts by concern#5063
Conversation
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
✨ New Files (2 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
This PR refactors the domain allowlist/blocklist utilities by splitting the previous multi-purpose src/domain-patterns.ts into smaller modules, isolating the security-critical domain validation logic and separating matching/parsing concerns. This improves auditability of Squid-injection prevention checks while keeping matching utilities focused.
Changes:
- Extracted
validateDomainOrPatternandSQUID_DANGEROUS_CHARSinto a dedicatedsrc/domain-validation.tsmodule with helper sub-validators. - Moved domain list parsing and matching utilities (e.g.
parseDomainList,isDomainMatchedByPattern,parseUrlPatterns) intosrc/domain-matchers.ts. - Updated multiple call sites and tests to import from the new canonical modules.
Show a summary per file
| File | Description |
|---|---|
| src/domain-validation.ts | New security-focused module containing validateDomainOrPattern and Squid injection-prevention primitives. |
| src/domain-matchers.ts | New module for domain list parsing, wildcard matching, and URL pattern parsing. |
| src/domain-patterns.ts | Reduced to shared types and low-level wildcard/protocol parsing utilities. |
| src/domain-patterns.test.ts | Updated tests to import validation vs matcher utilities from the new modules. |
| src/squid/domain-acl.ts | Switched imports to domain-matchers / domain-validation after the split. |
| src/squid/acl-generator.ts | Switched parseDomainList import to domain-matchers. |
| src/squid/policy-manifest.ts | Switched parseDomainList import to domain-matchers. |
| src/config-writer.ts | Switched parseUrlPatterns import to domain-matchers. |
| src/config-writer.test.ts | Updated mocks/requireMock references to domain-matchers. |
| src/commands/preflight.ts | Switched validateDomainOrPattern import to domain-validation. |
| src/commands/preflight.test.ts | Updated mocks/imports to domain-validation. |
| src/commands/validators/agent-options.ts | Switched SQUID_DANGEROUS_CHARS import to domain-validation. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 4
| /** | ||
| * Protocol restriction for a domain | ||
| */ | ||
| type DomainProtocol = 'http' | 'https' | 'both'; | ||
| export type DomainProtocol = 'http' | 'https' | 'both'; | ||
|
|
| /** | ||
| * Security validation for domain names and wildcard patterns | ||
| * | ||
| * This module is the Squid-injection prevention path for --allow-domains. | ||
| * All checks here are security-critical: they prevent malicious input from | ||
| * escaping into the generated Squid configuration. | ||
| */ |
| /** | ||
| * Reject structurally invalid domain strings: double dots, lone dot, bare | ||
| * `*.` / `.*` (incomplete wildcards), and patterns with too many wildcard segments. | ||
| */ | ||
| function checkStructuralValidity(trimmed: string): void { | ||
| // Double dots are never valid in domain names | ||
| if (trimmed.includes('..')) { | ||
| throw new Error(`Invalid domain '${trimmed}': contains double dots`); | ||
| } | ||
|
|
||
| // A lone dot is not a valid domain | ||
| if (trimmed === '.') { | ||
| throw new Error('Invalid domain: cannot be just a dot'); | ||
| } | ||
|
|
||
| // `*.` and `.*` are incomplete — they have no concrete label on one side | ||
| if (trimmed === '*.' || trimmed === '.*') { | ||
| throw new Error(`Invalid pattern '${trimmed}': incomplete domain`); | ||
| } |
| export interface ParsedDomainList { | ||
| /** Plain domains without wildcards */ | ||
| plainDomains: PlainDomainEntry[]; | ||
| /** Wildcard patterns with pre-compiled regex */ | ||
| patterns: DomainPattern[]; | ||
| } |
|
@copilot address review feedback |
- Update jest.mock path in validate-options.test.ts from '../domain-patterns' to '../domain-validation' to match the module where SQUID_DANGEROUS_CHARS now lives - Broaden domain-validation.ts module comment to include --allow-urls (SQUID_DANGEROUS_CHARS is used for both --allow-domains and --allow-urls) - Remove unreachable *./ .* dead code in checkStructuralValidity; checkOverBroadPattern already rejects all patterns composed only of '*' and '.' before checkStructuralValidity is ever called - Fix misleading 'pre-compiled regex' comment in domain-matchers.ts; DomainPattern.regex is a regex string compiled at match time
Done in commit
|
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
❌ Contribution Check failed. Please review the logs for details. |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓 Comment posted; smoke test failed, no labels added |
|
✅ Build Test Suite completed successfully! |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
🔌 Smoke Services — All services reachable! ✅ |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓 |
|
❌ Security Guard failed. Please review the logs for details. |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
✅ Smoke Gemini completed. All facets verified. 💎 Smoke test completed with partial failures. File system and bash tests passed, but GitHub-related tests failed. |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
|
❌ Smoke Claude failed |
🔥 Smoke Test Results — Auth mode: PAT (COPILOT_GITHUB_TOKEN)
Overall: PASS CC
|
🔥 Smoke Test Results — PASSPR: refactor: decompose validateDomainOrPattern and split domain-patterns.ts by concern
Overall: ✅ PASS
|
🔍 Smoke Test: API Proxy OpenTelemetry Tracing
All 5 scenarios passed. No unexpected failures detected.
|
Smoke Test: Copilot BYOK (Direct) Mode — ✅ PASSTests:
Status: Running in direct BYOK mode ( CC:
|
|
|
|
refactor: decompose validateDomainOrPattern and split domain-patterns.ts by concern GitHub MCP Testing: ✅ Running in direct BYOK mode (COPILOT_PROVIDER_API_KEY + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw) Overall status: PASS cc
|
Smoke Test: GitHub Actions Services Connectivity
Overall: FAIL —
|
Chroot Version Comparison Results
Overall: ❌ Not all tests passed — Python minor patch and Node.js major versions differ between host and chroot environments.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
|
Smoke test in progress Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
src/domain-patterns.tsconflated three distinct concerns in one 417-line file, with the security-criticalvalidateDomainOrPatternbeing a 106-line monolith that buried each individual injection-prevention check.Split by concern
src/domain-patterns.tsDomainProtocol,DomainPattern,PlainDomainEntry,ParsedDomain) + low-level utilities (parseDomainWithProtocol,isWildcardPattern,wildcardToRegex)src/domain-validation.tsSQUID_DANGEROUS_CHARS,validateDomainOrPattern, private sub-validatorssrc/domain-matchers.tsparseDomainList,isDomainMatchedByPattern,parseUrlPatterns,ParsedDomainListvalidateDomainOrPattern decomposed into named helpers
The 106-line function is now a readable orchestrator delegating to three private helpers:
Callers updated
All 7 import sites updated to the canonical source module:
preflight.ts,agent-options.ts,config-writer.ts,acl-generator.ts,domain-acl.ts,policy-manifest.ts, and the corresponding test mocks.