fix: add node ecosystem to smoke-codex network allow-list#1533
fix: add node ecosystem to smoke-codex network allow-list#1533
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the Smoke Codex workflow network allow-list to include Node/NPM access and adds defense-in-depth validation to prevent Squid config injection when interpolating domains/patterns/URL patterns into squid.conf.
Changes:
- Add
nodetonetwork.allowedfor Smoke Codex so npm registry access is permitted. - Add Squid config interpolation guards (rejecting whitespace/null bytes) and strengthen domain/pattern validation against injection characters.
- Regenerate the compiled
smoke-codex.lock.ymlworkflow (includes broader workflow/version/config changes beyond the allow-list tweak).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/squid-config.ts |
Adds assertSafeForSquidConfig and applies it to interpolated ACL values. |
src/squid-config.test.ts |
Adds tests ensuring injected whitespace/newlines are rejected in generated Squid config. |
src/domain-patterns.ts |
Hardens validateDomainOrPattern to reject injection-relevant characters. |
src/domain-patterns.test.ts |
Adds test coverage for rejecting injection characters in domains/patterns. |
src/cli.ts |
Adds CLI-side validation rejecting whitespace/null bytes in --allow-urls patterns. |
.github/workflows/smoke-codex.md |
Adds node to the workflow network allow-list. |
.github/workflows/smoke-codex.lock.yml |
Regenerated compiled workflow, including the updated allow-list and other workflow/config/version changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // null bytes, and comment/quote characters are dangerous. | ||
| // This prevents Squid config injection via --allow-domains. | ||
| const DANGEROUS_CHARS = /[\s\0"'`;#\\]/; |
There was a problem hiding this comment.
validateDomainOrPattern says it rejects “control characters”, but DANGEROUS_CHARS only matches whitespace (\s) and \0 (plus a few punctuation chars). Other ASCII control characters like \u0001/\u007f would currently pass validation, which undermines the injection-defense intent and makes the error message misleading. Consider expanding the regex to cover the full control-character range (e.g., \x00-\x1F\x7F) or adjust the wording to match what’s actually rejected.
| // null bytes, and comment/quote characters are dangerous. | |
| // This prevents Squid config injection via --allow-domains. | |
| const DANGEROUS_CHARS = /[\s\0"'`;#\\]/; | |
| // null bytes, all ASCII control characters, and comment/quote characters are dangerous. | |
| // This prevents Squid config injection via --allow-domains. | |
| const DANGEROUS_CHARS = /[\x00-\x1F\x7F\s"'`;#\\]/; |
| GH_AW_INFO_ALLOWED_DOMAINS: '["defaults","github","node","playwright"]' | ||
| GH_AW_INFO_FIREWALL_ENABLED: "true" | ||
| GH_AW_INFO_AWF_VERSION: "v0.24.1" | ||
| GH_AW_INFO_AWF_VERSION: "v0.25.0" | ||
| GH_AW_INFO_AWMG_VERSION: "" | ||
| GH_AW_INFO_FIREWALL_TYPE: "squid" |
There was a problem hiding this comment.
This PR’s title/description only calls out adding the node network allow-list entry, but the compiled workflow lock includes additional behavioral changes (e.g., AWF version bump and other regenerated metadata/config). If the intent is only to extend the allow-list, consider regenerating the lock with the same compiler/settings as main (or splitting these changes into a separate PR) so the review surface matches the stated scope.
8178998 to
d36f439
Compare
The Smoke Codex workflow asks the agent to run `npm ci && npm run build` (step 8), but `registry.npmjs.org` was not in the allowed domains list. All 8 npm requests were blocked by the firewall, causing the agent to fail before it could call `add_comment` (the required safe output). Add `node` to `network.allowed` so npm registry traffic is permitted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d36f439 to
0009c4f
Compare
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
|
Smoke test results (run 23833495697)
Overall: PASS
|
|
Smoke Test Results —
Overall: PASS
|
Smoke Test: GitHub Actions Services Connectivity ✅All checks passed:
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
This comment has been minimized.
This comment has been minimized.
|
Smoke test summary for workflow run 23849149884
|
Summary
nodetonetwork.allowedinsmoke-codex.mdsonpm ci && npm run build(step 8) can reachregistry.npmjs.orgadd_commentRoot cause identified from run 23810579935: firewall logs showed 8 denied requests to
registry.npmjs.org.Test plan
🤖 Generated with Claude Code