feat: add localhost keyword for playwright testing#427
Conversation
- Automatically maps 'localhost' to 'host.docker.internal' - Enables host access when localhost is specified - Configures common development ports (3000, 4200, 5173, 8080, etc.) - Supports protocol prefixes (http://localhost, https://localhost) - Preserves user-specified --allow-host-ports if provided - Add comprehensive integration tests for localhost functionality Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
- Add localhost keyword section to AGENTS.md - Add Playwright testing examples to README.md - Add comprehensive guide to docs/usage.md - Create dedicated Playwright testing guide in docs-site - Explain automatic configuration and security considerations Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Add localhost to --allow-domains help text explaining it auto-configures for local testing Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 77.98% | 77.23% | 📉 -0.75% |
| Statements | 78.05% | 77.32% | 📉 -0.73% |
| Functions | 77.77% | 77.36% | 📉 -0.41% |
| Branches | 71.70% | 70.64% | 📉 -1.06% |
📁 Per-file Coverage Changes (1 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
40.1% → 37.8% (-2.24%) | 40.3% → 38.0% (-2.24%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Smoke Test ResultsLast 2 merged PRs:
✅ GitHub MCP Status: PASS
|
|
Smoke Test Results (Claude Engine) GitHub MCP:
Tests: Status: PASS
|
Security Review: Localhost Keyword FeatureI've reviewed PR #427 for security implications. The implementation is generally sound with proper validation, but I found one security concern that should be addressed before merge. 🔴 Security Issue: Port 8888 in Default Port ListFile: Issue: The default localhost port list includes port 8888, which is commonly used by Jupyter Notebook/JupyterLab servers: options.allowHostPorts = '3000,3001,4000,4200,5000,5173,8000,8080,8081,8888,9000,9090';Security Risk:
Recommendation:
Suggested fix: // Remove 8888 from this line:
options.allowHostPorts = '3000,3001,4000,4200,5000,5173,8000,8080,8081,9000,9090';Users who genuinely need Jupyter access can explicitly specify ✅ Security Aspects That Are Safe
VerdictConditional approval - Safe to merge after removing port 8888 from the default list. All other aspects of the implementation maintain the firewall's security posture.
|
|
@copilot please merge origin/main |
|
@copilot merge main and bump up the test coverage so https://github.com/github/gh-aw-firewall/actions/runs/21374333813/job/61536955621?pr=427 won't fail |
Extract localhost processing logic into testable function and add comprehensive unit tests covering: - Plain localhost, http://localhost, https://localhost variants - Protocol prefix preservation - Host access auto-enablement - Default port configuration - Edge cases and domain order preservation Coverage improvements: - Overall: 81.58% → 82.03% (+0.45%) - cli.ts lines: 37.82% → 41.47% (+3.65%) - cli.ts branches: 31.03% → 38.01% (+6.98%) Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Mossaka
left a comment
There was a problem hiding this comment.
Review: REQUEST_CHANGES
@copilot Please fix the following security issue:
Issue: Missing Security Warning
When the localhost keyword auto-enables --enable-host-access, there should be a security warning. Currently, users may not realize they've exposed their host machine to the agent.
Required Fix
Add a security warning in src/cli.ts when localhost triggers host access:
if (localhostResult.shouldEnableHostAccess) {
options.enableHostAccess = true;
logger.warn('Security warning: localhost keyword enables host access - agent can reach services on your machine');
logger.info('localhost keyword detected - automatically enabling host access');
}Additional Suggestions
- Handle multiple localhost variants consistently (e.g., if both
localhostandhttp://localhostare specified) - Add an integration test verifying dangerous ports (SSH:22, databases) are still blocked when using localhost keyword
- Consider reducing the default port list to truly common dev ports (3000, 5173, 8080) and require explicit
--allow-host-portsfor others
The feature is well-implemented otherwise - just needs the security warning.
# Conflicts: # src/cli.test.ts
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 82.44% | 82.08% | 📉 -0.36% |
| Statements | 82.47% | 82.12% | 📉 -0.35% |
| Functions | 81.77% | 81.95% | 📈 +0.18% |
| Branches | 75.59% | 75.41% | 📉 -0.18% |
📁 Per-file Coverage Changes (1 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
40.5% → 41.7% (+1.22%) | 40.7% → 41.9% (+1.20%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Smoke Test ResultsLast 2 Merged PRs:
Test Results: Overall Status: PASS cc @Mossaka
|
Smoke Test Results - Claude EngineLast 2 Merged PRs:
Test Results:
Status: PASS
|
Playwright tests against local dev servers required manual configuration of host access, domain mapping, and port allowlists. The
localhostkeyword now auto-configures this setup.Changes
CLI Enhancement (
src/cli.ts)localhostin--allow-domainsautomatically:host.docker.internal(preserves http:// / https:// prefixes)--enable-host-access--allow-host-portsstill respectedContainer Fix (
containers/agent/setup-iptables.sh)multiportmodule from port range rules (not supported by iptables DNAT)Testing (
tests/integration/localhost-access.test.ts)Documentation
Usage
Security warnings maintained for host access. Custom port lists still supported via
--allow-host-ports.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.