Repository navigation
fix(sandbox): scope ASF network grants to ASF projects - #1978
Conversation
github-actions
Bot
added
family:setup
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
fe2ba14 to
e1d4f5f
Compare
github-actions
Bot
added
the
substrate:framework-dev
|
Hi @potuik, The changes now cover historical ASF permission migration, stale checker handling, Markdown parsing edge cases, manual installation, and linked worktrees. All 35 organization-network tests pass, the five behavioural evals have passed individually, and the latest CI checks, including CodeQL, are green. The earlier CodeQL findings are also resolved. Thanks! |
Kaap10
left a comment
Kaap10
left a comment
There was a problem hiding this comment.
LGTM, verified locally with all 35 organization network tests and CI passing green.
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
The sandbox half of #1803 is careful work and the selector fails closed, but the gateway half removes the ASF hosts for everyone without giving ASF projects a way back, so the split is not finished yet.
Same egress policy as the shell (tools/egress-gateway/tool.md:102)
Containers get the egress gateway as their HTTP proxy, so tools inside them that honour proxy variables are bound by the same host allow-list as sandboxed commands.
Requests to ASF hosts remain blocked by the default gateway even for
ASF projects.
This solves the gateway half of #1803 by removing the hosts for everyone, and ASF projects get nothing back. #1803 asks to "split the ASF hosts into a fragment that setup adds only when organization: ASF", for both the sandbox template and the egress allowlist. After this PR an ASF project's shell can reach lists.apache.org, while its containers get a 403 for every *.apache.org host, and the docs here rule out EGRESS_ALLOW_EXTRA as the way back. That also contradicts the quoted line, which this PR leaves in place. The issue names whimsy too, which was reachable through the gateway's .apache.org suffix and is in neither the fragment nor the allowlist now. Please give ASF projects a supported gateway path, such as having setup emit the ASF entries for the gateway the project starts, and decide which hosts belong in that set. If a gateway really is shared across projects and cannot be scoped, say what an ASF adopter should do instead, and update the quoted line.
Flagged by both the primary and adversarial reviewers.
Smaller observations
general—tools/skill-evals/evals/setup/README.mdcounts the five new cases but has no table row forasf-network-reconciliation. #1982 also moves this count from 157 to 162, so whichever merges second needs 167.- See inline comments on
docs/adapters/antigravity.md:97,tools/egress-gateway/src/egress_gateway/allowlist.py:26.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
699618b to
c49e2ef
Compare
|
I rebased this onto |
Removing the ASF hosts from the default allowlist left ASF projects with no supported way to reach them from containers. A gateway's allowlist belongs to its process, so an ASF project runs its own gateway on its own port with the ASF hosts, and no other project receives them. Also correct the lines that still said the gateway mirrors the sandbox, state that Antigravity has no project-level grant yet, and add the missing asf-network-reconciliation row to the setup eval table. Generated-by: Claude Opus 5
c49e2ef to
4c1cc2d
Compare
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks for the careful work here, the selector and the migration are solid. I've pushed a fixup on top of your commit so this can land now.
tools/egress-gateway: a gateway's allowlist belongs to its process, so an ASF project now runs its own gateway on its own port with the ASF hosts, and no other project receives them. The README has an ASF projects section with the command and host list, includingwhimsy.apache.organdissues.apache.org.docs/adapters/antigravity.md: states that there is no project-level grant yet, with a follow-up issue.allowlist.pyand the docs that still said the gateway mirrors the sandbox are corrected, and the setup eval table has itsasf-network-reconciliationrow.
Please pull before pushing again.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Magpie handles maintainer review:
Contributing guide.
A PMC member answered Q1-Q11, each checked against the code first. Where the proposed answer described something the code does not do, the claim is narrowed to what ships instead of being ratified as written. - Adopt strict triage and a closed symptom vocabulary. - Keep the unreviewed-change adversary in the model, since required review on main is lifted, and narrow C-D6 to match. - Scope C-K3 and C-I3 to the components that meet them. - Record patch pinning as a design requirement, not a property. - Add claims for the backtest gaps, including egress scoping now that #1978 has landed. - Bring scope and trust boundaries up to date with the shipped tools. Point the project config at the private tracker and the security model, and correct two mailing-list addresses that do not exist. Generated-by: Claude Opus 5
Describe the ASF network scoping from #1978, including the claim it made false that containers share the sandbox's host allow-list. Record the run cost and pointer proof from #2000, the verify-rc pane from #2007, and the nine-skill pr-management family. Recheck every Known gaps bullet in eleven specs: delete those now closed, rewrite the stale ones, and annotate the rest with gap-closes-when. Generated-by: Claude Opus 5
Summary
organization: ASF, with fail-closed validation.Type of change
.claude/skills/<name>/) — eval fixtures updated belowtools/<system>/*.md)tools/*/withpyproject.toml)docs/,README.md,CONTRIBUTING.md)projects/_template/)prek, workflows, validators)Test plan
prek run --all-filespassesuv run pytest/ruff check/mypypasses(
PYTHONPATH=tools/skill-evals/src python3 -m skill_evals.runner tools/skill-evals/evals/<skill>/)(a regression test for the bug fixed / the behaviour added — see CONTRIBUTING.md)
RFC-AI-0004 compliance
Linked issues
Closes #1803
Notes for reviewers (optional)
project.mdresolution and rejects ambiguous organization declarations or modified ASF fragments.setup_preflightso it remains available in marketplace and snapshot installations..claude/settings.local.json, preserving user-owned settings and the approval process.EGRESS_ALLOW_EXTRAremains process-wide; the project-local ASF fragment does not automatically widen the shared gateway allowlist.