refactor: pre-validate host port specs once in TypeScript, eliminate duplicate shell parser#6067
Conversation
Parse and validate --allow-host-ports / --allow-host-service-ports once in TypeScript (parseValidPortSpecs in api-proxy-environment.ts), store the result in new env vars AWF_VALID_ALLOW_HOST_PORTS and AWF_VALID_HOST_SERVICE_PORTS, and pass them through to the iptables init container. Replace the three parse_port_specs() call sites in setup-iptables.sh with a new split_valid_port_specs() helper that consumes the pre-validated specs and applies is_valid_port_spec() only as a fail-closed assertion. Remove the former parse_port_specs() function entirely. Update setup-iptables-port-spec.test.sh to test split_valid_port_specs() and add TypeScript tests verifying AWF_VALID_* vars are set on both the agent and iptables-init container environments. Closes #6057
There was a problem hiding this comment.
Pull request overview
This PR refactors host-access port specification handling to reduce validator drift in a security-sensitive path by validating/normalizing port specs once in TypeScript and passing the results to the init-container via new AWF_VALID_* environment variables. The init-container script is updated to consume the pre-validated list via a thin splitter + defense-in-depth assertion, and tests are adjusted accordingly.
Changes:
- Pre-validate and normalize
allowHostPorts/allowHostServicePortsin TypeScript and export asAWF_VALID_ALLOW_HOST_PORTS/AWF_VALID_HOST_SERVICE_PORTS. - Update
setup-iptables.shto replace the full shell parser withsplit_valid_port_specs()and consumeAWF_VALID_*at call sites. - Update/add tests to reflect the new env vars and split behavior.
Show a summary per file
| File | Description |
|---|---|
| tests/setup-iptables-port-spec.test.sh | Updates shell unit tests to exercise split_valid_port_specs() instead of the removed full parser. |
| src/services/agent-service.ts | Forwards the new AWF_VALID_* vars into the iptables-init container environment. |
| src/services/agent-service-build.test.ts | Adds assertions for AWF_VALID_ALLOW_HOST_PORTS on the agent + forwarding into iptables-init. |
| src/services/agent-environment/api-proxy-environment.ts | Performs TypeScript pre-validation and sets AWF_VALID_* env vars. |
| src/services/agent-environment-options.test.ts | Adds assertion for AWF_VALID_HOST_SERVICE_PORTS on the agent environment. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 5
- Review effort level: Low
| local -a _svps_entries=() | ||
| IFS=',' read -ra _svps_entries <<< "$_svps_input" | ||
| local _svps_spec="" | ||
| for _svps_spec in "${_svps_entries[@]}"; do | ||
| if [ -z "$_svps_spec" ]; then |
| # FILTER: also allow user-specified ports from --allow-host-ports (consume pre-validated list) | ||
| if [ -n "$AWF_VALID_ALLOW_HOST_PORTS" ]; then | ||
| local -a gw_ports=() | ||
| parse_port_specs gw_ports "$AWF_ALLOW_HOST_PORTS" "port spec" | ||
| split_valid_port_specs gw_ports "$AWF_VALID_ALLOW_HOST_PORTS" "port spec" | ||
| local port_spec="" |
| if [ -n "$AWF_VALID_HOST_SERVICE_PORTS" ] && [ -n "$AWF_ENABLE_HOST_ACCESS" ]; then | ||
| # Consume pre-validated port list (validated once by TypeScript parseValidPortSpecs()). | ||
| split_valid_port_specs HSP_PORTS "$AWF_VALID_HOST_SERVICE_PORTS" "host service port" | ||
|
|
||
| # Resolve host gateway IP (with AWF_HOST_GATEWAY_IP fallback, same as host access block) |
| if [ -n "$AWF_VALID_ALLOW_HOST_PORTS" ]; then | ||
| echo "[iptables] Redirect user-specified ports to Squid..." | ||
| local -a dnat_ports=() | ||
| parse_port_specs dnat_ports "$AWF_ALLOW_HOST_PORTS" "port spec" | ||
| split_valid_port_specs dnat_ports "$AWF_VALID_ALLOW_HOST_PORTS" "port spec" | ||
| local port_spec="" |
| }); | ||
|
|
||
| describe('toolchain var fallback to GITHUB_ENV', () => { |
|
@copilot address review feedback |
Addressed the review feedback in 4274ecd. Included:
|
|
✅ Copilot review passed with no inline comments. @copilot Add the |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓 |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 99.15% | 99.19% | 📈 +0.04% |
| Statements | 99.11% | 99.15% | 📈 +0.04% |
| Functions | 99.45% | 99.45% | ➡️ +0.00% |
| Branches | 95.77% | 95.72% | 📉 -0.05% |
📁 Per-file Coverage Changes (1 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
✅ Smoke Gemini completed. All facets verified. 💎 |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓 |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🔌 Smoke Services — All services reachable! ✅ |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
❌ Security Guard failed. Please review the logs for details. |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
|
✅ Contribution Check completed successfully! PR #6067 follows the applicable CONTRIBUTING.md guidelines: it includes focused tests for the refactor, changes are organized in the appropriate source/container/test locations, documentation is not needed for this internal refactor, and the description clearly explains the change. No related issue is identified, so no issue reference is required. |
|
✅ Smoke Claude passed |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
✅ Build Test Suite completed successfully! |
Smoke Test: Claude Engine Validation
Overall result: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
Smoke Test: BYOK Direct Mode ✅
Status: PASS — Running in direct BYOK mode via api-proxy sidecar Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test: Copilot PAT Auth Validation ✅ PASS
Overall: PASS | Auth mode: PAT (COPILOT_GITHUB_TOKEN) | CC: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🔍 OTEL Tracing Smoke Test Results
All scenarios pass. 🎉 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
GitHub MCP Testing: ✅ Running in direct BYOK mode (AWF_AUTH_TYPE=github-oidc + AWF_AUTH_AZURE_* + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw) authenticated via Microsoft Entra Overall: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test: Gemini Engine ValidationTest Results:
Overall Status: FAIL 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.
|
|
Smoke test results:
Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"
- "registry.npmjs.org"See Network Configuration for more information.
|
🤖 Copilot Smoke Test Results
Overall: PASS
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test Results
Overall: FAIL — Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Chroot Version Comparison Results
Result: Not all tests passed — Python and Node.js versions differ between host and chroot environments. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test Summary
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 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Host-access port-spec parsing was duplicated between
parseValidPortSpecs()in TypeScript andparse_port_specs()in the agent init-container shell script, with three separate shell call sites each re-validating the same raw user input. This created drift risk in a security-critical firewall path.Changes
src/services/agent-environment/api-proxy-environment.ts— callsparseValidPortSpecs()at environment-build time and stores the normalized results in two new env vars:AWF_VALID_ALLOW_HOST_PORTSandAWF_VALID_HOST_SERVICE_PORTSsrc/services/agent-service.ts— forwards the newAWF_VALID_*vars into the iptables init container's environment blockcontainers/agent/setup-iptables.sh— replacesparse_port_specs()(full duplicate validator with whitespace trimming) withsplit_valid_port_specs(), a thin helper that splits the pre-validated comma-separated string and appliesis_valid_port_spec()only as a fail-closed assertion (defence-in-depth). All three formerparse_port_specscall sites now consume theAWF_VALID_*vars.tests/setup-iptables-port-spec.test.sh— updated to testsplit_valid_port_specsinstead ofparse_port_specs; fixture-drivenis_valid_port_speccoverage retained unchangedTypeScript tests — added assertions that
AWF_VALID_ALLOW_HOST_PORTS/AWF_VALID_HOST_SERVICE_PORTSare set on both the agent and iptables-init container environmentsBefore / After