Skip to content

Fix Copilot URL approvals from finalized runtime firewall policy - #70750

Merged
pelikhan merged 6 commits into
mainfrom
pelikhan-crispy-fiesta
Oct 21, 2026
Merged

pelikhan merged 6 commits into
mainfrom
pelikhan-crispy-fiesta

Conversation

@pelikhan

Copy link
Copy Markdown
Collaborator

Summary

Fix the URL-approval portion of #70743 without changing shell permissions or addressing the separate browser ABI failure. This replaces only the URL work from closed, unmerged #70697; merged ledger fixes are untouched.

  • Read the finalized AWF config in the CLI harness after runtime network_allowed expansion, rather than copying compile-time domain strings into flags.
  • Emit separate HTTP/HTTPS and apex/subdomain approvals; preserve explicit protocols, ports, IP literals, domain blocks, existing tool permissions and explicit URL denials.
  • Never grant all URLs. Reject malformed rules/config and untranslatable global blocks explicitly. Leave SDK and unsandboxed execution unchanged.
  • Add translation, runtime-updater and actual harness/subprocess regressions, documentation and a patch changeset.

Evidence

The raw session of run 39529502581 contains three URL approval requests for https://github.com, all denied by the unattended fallback. Local date and cat succeeded. The invocation had --allow-tool shell but no URL approvals.

The independent browser-library ABI failure is tracked in github/gh-aw-firewall#10385, not fixed here.

Validation

  • All 496 focused URL-permission, harness and runtime-network tests passed.
  • JavaScript typecheck passed.
  • make fmt-cjs and make lint-cjs passed (repository-wide lint reports existing warnings, zero errors).
  • PATH=/opt/homebrew/bin:$PATH make agent-report-progress passed, including impacted tests. The first attempt used macOS Bash 3 and failed before validation because mapfile was unavailable; reran successfully with installed Bash 5.
  • No workflow source or compiler output changed, so no lock regeneration was required. The setup action already copies all runtime .cjs helpers.
  • Pinned Copilot 1.0.98 help permissions confirms separate protocol approvals and wildcard URL patterns. End-to-end hosted CLI behavior, especially omitted-port/nested-subdomain matching, remains unverified; no workflows were dispatched.
  • Static review found no confirmed exploitable vulnerability. Its wildcard-block correctness concern was addressed by rejecting unsupported global blocks rather than silently dropping them; exact native CLI matching/denial-precedence coverage remains limited to documented behavior and local argument-level regressions. AWF remains the independent network-enforcement boundary.

Branch renaming was blocked by an existing shared packed-refs.lock; that lock was not removed. The original isolated session branch was committed and pushed successfully.

Translate finalized AWF domain rules into scoped CLI URL approvals and denials, including caller additions, without changing shell, SDK, or firewall permissions.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@pelikhan
pelikhan marked this pull request as ready for review October 21, 2026 02:37
Copilot AI max review requested due to automatic review settings October 21, 2026 02:37
@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

✅ Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel

Outcome: reported_no_action

The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible.

Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.

Generated from Test Quality Sentinel · sonnet50 · 13.3 AIC · ⌖ 15.1 AIC · ◇ 6.61 AIC · ⊞ 12.6K · ◷

🧪 Test quality analysis by Test Quality Sentinel

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.

Design Decision Gate 🏗️

Outcome: reported_no_action

The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible.

Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.

Generated from Design Decision Gate 🏗️ · opus55 · 11.3 AIC · ⌖ 31.9 AIC · ◇ 14.9 AIC · ⊞ 1.9K · ◷

🏗️ ADR gate enforced by Design Decision Gate 🏗️

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

✅ Ponytail Reviewer completed successfully!

Ponytail Reviewer

Outcome: reported_no_action

The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible.

Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.

Generated from Ponytail Reviewer · gpt56 · 7.21 AIC · ⌖ 5.29 AIC · ⊞ 14.7K · ◷

Generated by Ponytail Reviewer for #70750

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

✅ PR Code Quality Reviewer completed the code quality review.

🔎 Code quality review by PR Code Quality Reviewer

@github-actions

Copy link
Copy Markdown
Contributor

Comment Memory

Peek at saved memory (pr-code-quality-reviewer)
reviewed_at: 2026-10-21T02:41:13Z
review_event: REQUEST_CHANGES
top_themes: runtime config path mismatch; silent unsupported wildcard policy
files_reviewed: actions/setup/js/copilot_harness.cjs, actions/setup/js/copilot_url_permissions.cjs, actions/setup/js/copilot_url_permissions.test.cjs, actions/setup/js/update_network_allowed.cjs, docs/src/content/docs/engines/copilot.md
comment_count: 2

Note

This comment is managed by comment memory.

Expand the saved memory block to view or edit the persistent context for this thread.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 2.23 AIC · ⌖ 0.625 AIC · ⊞ 1.6K · ◷
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes

The new permission injection has a runtime configuration-path regression and silently changes behavior for global allow rules. The first issue can make the harness read the wrong firewall config or fail before execution; the second leaves valid policies without approvals and no diagnostic. Please resolve both before merging.

Findings
  • Use GH_AW_AWF_CONFIG_PATH (with the existing default) rather than the hard-coded module constant when reading finalized network policy.
  • Reject or explicitly diagnose * in allowDomains, instead of silently dropping it.

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 2.23 AIC · ⌖ 0.625 AIC · ⊞ 1.6K
Comment /review to run again

Comment thread actions/setup/js/copilot_harness.cjs Outdated
Comment thread actions/setup/js/copilot_url_permissions.cjs

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd to the new copilot_url_permissions.cjs module (triaged as a bug fix; triage ran successfully). No blocking issues — 2 minor, non-blocking suggestions posted inline.

📋 Key Themes & Highlights

Key Themes

  • Error diagnostics: addCopilotURLPermissions swallows the real read/parse error in its catch {}, unlike the sibling loadAwfConfigData() in the same file which distinguishes ENOENT from other failures and logs detail. Minor, but will make a future runtime config bug harder to triage.
  • Hostname validation scope: single-label hostnames (no TLD) pass the DNS-label regex with no explicit test documenting whether that's intended — worth a quick confirmation + test so the supported surface is explicit rather than implicit.

Positive Highlights

  • ✅ Explicit fail-over-silent-drop design for global wildcard domain blocks — directly closes the gap described in the PR body and avoids a dangerous silent no-op.
  • ✅ Strong test coverage in copilot_url_permissions.test.cjs: parameterized malformed-rule rejection, protocol/port/IP preservation, SDK/unsandboxed bypass, and a real subprocess integration test against the harness.
  • ✅ Docs update (docs/src/content/docs/engines/copilot.md line 60) accurately reflects the actual code behavior (apex+subdomain, both protocols unless restricted, no shell/firewall bypass).
  • ✅ Insertion before the -- CLI delimiter is correct and covered by a dedicated test.

@copilot please address the review comments above.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 152.5 AIC · ⌖ 15 AIC · ⊞ 10.2K
Comment /matt to run again

Comment thread actions/setup/js/copilot_url_permissions.cjs Outdated
Comment thread actions/setup/js/copilot_url_permissions.cjs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Native CLI permission matching needs human verification, and confirmed startup regressions remain unresolved.

2 open issues
What changed in this PR

Derives sandboxed Copilot CLI URL approvals from finalized AWF policy, including runtime caller additions, without changing shell permissions.

Changes:

  • Adds scoped URL allow/deny translation and validation.
  • Integrates approvals into harness startup with regression coverage.
  • Documents behavior and adds a patch changeset.
File Description
docs/​src/​content/​docs/​engines/​copilot.md Documents sandboxed URL approvals.
actions/​setup/​js/​copilot_url_permissions.test.cjs Tests translation, runtime additions and subprocess arguments.
actions/​setup/​js/​copilot_url_permissions.cjs Translates finalized firewall rules into URL permissions.
actions/​setup/​js/​copilot_harness.test.cjs Adds config fixtures for startup tests.
actions/​setup/​js/​copilot_harness.cjs Applies URL permissions before CLI startup.
.changeset/​copilot-runtime-url-approvals.md Records the patch release change.

Review effort: Max

Comment thread actions/setup/js/copilot_harness.cjs Outdated
Comment thread actions/setup/js/copilot_harness.test.cjs
@pelikhan

Copy link
Copy Markdown
Collaborator Author

@copilot fix ci failures

Copilot AI and others added 2 commits October 21, 2026 03:14
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

@copilot fix ci failures

Fixed the CJS failures in 145cda6: the harness now falls back to the shared RUNNER_TEMP/gh-aw/awf-config.json only when its implicit default is absent, and the endpoint replay creates an isolated config fixture with an explicit path. The focused 490-test validation and CJS formatting/lint passed.

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot address the following outstanding work in one pass:

  1. Update this branch with the latest main using make merge-main, resolving any conflicts and preserving the intended changes.
  2. Review (actions/setup/js/copilot_harness.cjs:1739): This passes the module-load constant AWF_CONFIG_PATH, which is hard-coded to /tmp/gh-aw/awf-config.json, instead of the configured GH_AW_AWF_CONFIG_PATH, so any run using a non-default finalized config (including the new subprocess regression test) reads the wrong file and either omits the intended approvals or aborts with a misleading config error. - Fix Copilot URL approvals from finalized runtime firewall policy #70750 (comment)
  3. Review (actions/setup/js/copilot_url_permissions.cjs:47): Silently discarding a global allow (*) makes a valid AWF policy look like an empty URL policy, so the run will fail later on URL requests without identifying the unsupported translation. - Fix Copilot URL approvals from finalized runtime firewall policy #70750 (comment)
  4. Review (actions/setup/js/copilot_url_permissions.cjs:91): [/diagnosing-bugs] The bare catch {} discards the real error (ENOENT vs malformed JSON vs permission denied), making future failures hard to diagnose. - Fix Copilot URL approvals from finalized runtime firewall policy #70750 (comment)
  5. Review (actions/setup/js/copilot_url_permissions.cjs:48): [/tdd] The hostname label regex accepts single-label hosts (e.g. localhost, foo) with no TLD requirement, unlike validateDomainPattern in pkg/workflow/network_firewall_validation.go which rejects bare wildcard-without-base but doesn't explicitly test single-label hosts either — worth confirming this is intentional parity rather than a gap. - Fix Copilot URL approvals from finalized runtime firewall policy #70750 (comment)
  6. Review (actions/setup/js/copilot_harness.cjs:1739): This newly mandatory read breaks sandboxed CLI startup on runner.topology: arc-dind. AWF_CONFIG_PATH defaults to /tmp/gh-aw/awf-config.json, but the runner's /tmp is not daemon-visible. buildAWFConfigFileSetup writes the finalized config under ${RUNNER_TEMP}/gh-aw, which is mounted into the sandbox, and no generated code sets GH_AW_AWF_CONFIG_PATH. Unlike the previous optional loader, this read throws and exits before Copilot starts. Fall back to the shared runtime config when the default is absent, preserving explicit overrides and malformed-config failures. Add a startup regres... - Fix Copilot URL approvals from finalized runtime firewall policy #70750 (comment)
  7. Review (actions/setup/js/copilot_harness.test.cjs:3783): Please give runHarness in actions/setup/js/copilot_model_endpoint_mismatch_replay.test.cjs:31–59 the same per-test config fixture and GH_AW_AWF_CONFIG_PATH override added here. That helper sets AWF_REFLECT_ENABLED=1 but creates no AWF config. With no ambient config, the new mandatory read fails before applyCopilotWireAPI: the mismatch case loses its expected diagnostic and record, while the matching case exits 1 without spawning the CLI. Both cases now depend on an unrelated config file being present instead of testing endpoint compatibility in isolation. - Fix Copilot URL approvals from finalized runtime firewall policy #70750 (comment)

Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI.

Sous-chef head: 145cda6
Sous-chef work: 22416d776de3745f2f5e7565f4f54a6c4799079a4a1abe6eca6435dd2d4664f2 345a8ec5932c6a1db97c637f6d517088aab291431ec02dbab14f957e8d650bd7 3ac2701e56266c99ea1375ef39e1739cc4367fc855a431c7b11c14e337770707 6bd9a14a134a7486db77ee631645ca753900a043ea1d09a9a7b756b688525889 99059c5dbda71241065e8c457ee2c8be397865b02fd486e99610f37e3717610b a3d8c818beb8a73640a4fabb334022ac27d347cb45e3c6696fcbd5eb973e79d0
Sous-chef state: 622f3e423f4e3c6807977d115ec885e40eaa824a763cd23d416520fc443fe2d4

Generated by 👨‍🍳 PR Sous Chef · pi · haiku45 · 4.87 AIC · ⌖ 6.24 AIC · ⊞ 1.1K · ◷
Comment /souschef to run again

Copilot AI and others added 2 commits October 21, 2026 04:01
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Require published dry-run source and lock revisions, existing review and execution gates, and stop on authorization failures. Keep non-dry-run dispatch prohibited.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment thread AGENTS.md
@pelikhan

Copy link
Copy Markdown
Collaborator Author

/smoke-copilot

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

📰 BREAKING: Smoke Copilot is now investigating this issue comment. Sources say the story is developing...

@github-actions

Copy link
Copy Markdown
Contributor

Smoke partial. Me run few test. PRs ✅, curl ✅, file ✅, build ✅, summarizer ✅. Serena, Playwright, others not run ❌. Overall: FAIL.

📰 BREAKING: Report filed by Smoke Copilot · copilot · auto · 41.8 AIC · ⌖ 0.352 AIC · ⊞ 14.6K · ◷
Comment /smoke-copilot to run again
Add label smoke to run again

@pelikhan
pelikhan merged commit 2e33c30 into main Oct 21, 2026
39 checks passed
@pelikhan
pelikhan deleted the pelikhan-crispy-fiesta branch October 21, 2026 05:17
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.93.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants