feat(tools): route OSV and CVE.org adapter recipes through vetted-ops (#1320) - #1326
Conversation
potiuk
left a comment
There was a problem hiding this comment.
Good, careful work — the stdlib-only backend is the right call, the per-parameter regexes with hostile-input cases are exactly the idiom this tool wants, and splitting the change into three readable commits made it easy to follow. Requesting changes on one thing: as it stands the PR doesn't yet close the gap #1320 describes, because the OSV host isn't allowlisted and the place the recipe actually runs from still shells out to curl.
Blocking — the OSV host is not allowlisted (tools/osv/README.md)
Details inline. api.osv.dev is absent from .claude/settings.json, tools/sandbox-lint/expected.json, and docs/setup/secure-agent-setup.md:538, so all four OSV operations are refused under the framework's own recommended baseline — a denied curl becomes a denied urllib.
Blocking — the executing copy of the recipe was not converted
This one has no changed line to anchor to, so it's here: plugins/magpie-security/skills/issue-sync/gather.md:770 still carries
curl -sSf https://cveawg.mitre.org/api/cve/<CVE-ID> \
| jq -r '{state: .cveMetadata.state, datePublished: .cveMetadata.datePublished}'
The adapter tool.md is the reference; that inlined copy in security-issue-sync is what an agent actually runs. Converting the reference and leaving the caller behind means the inconsistency survives where it bites. Worth grepping for other inlined curl copies in plugins/ in the same pass.
Spec sync
AGENTS.md § Commit and PR conventions asks for a spec-sync pre-check before pushing a functionality PR:
Spec-sync pre-check before pushing a functionality PR. The specs in
tools/spec-loop/specs/are the source of truth and must not fall behind the code.
tools/spec-loop/specs/vetted-command-surface.md still states "builders return list[str] executed without a shell" under Approach, and "Operations are gh-shaped today" under Future work 3. Both are now false. A new backend is squarely a spec-affecting change.
Evals
AGENTS.md § Keeping evals and mode-economics in sync treats tool-adapter docs that skills load as behavioural surface — the affected suites here are security-issue-sync (via issue-sync/gather.md) and dependency-audit. The test plan lists the validators, the unit suite and ruff, which covers the dispatcher but not the skills reading the changed recipes.
One design question, for a maintainer rather than for you
The spec refuses gh run download on the grounds that writing to local disk "is a different risk class entirely". Outbound HTTP to third-party hosts may be the same kind of widening: vetted-ops was "only what gh needs — github.com / api.github.com", and this gives the forge dispatcher a second egress channel. A separate vetted-http-read entry point with its own allowlist entry is the alternative. Not something to change on this PR without a maintainer weighing in — flagging it so the decision is explicit rather than incidental.
Smaller observations
Ten more points are inline, from a missing urlopen timeout and unvalidated endpoint schemes down to a couple of nits. One more that isn't anchorable: existing adopters need [values] ecosystems added to their policy TOML or osv-query-package refuses at validation. The README sample covers new adopters; a line in the upgrade notes would cover the rest.
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.md
There was a problem hiding this comment.
Thank you for working on this! De-duplicating feedback already covered in previous review (bare curl in tools/osv/tool.md:70, missing urlopen timeout, cve-org/tool.md:85 curl exit reference, and sandbox allowlisting).
Below are the remaining distinct findings, targeted to specific lines and files:
1. _PACKAGE_NAME regex rejects Maven coordinates (groupId:artifactId) [Blocking]
- Target File:
tools/vetted-ops/src/vetted_ops/ops.py - Specific Line: Line 80
- Current Code:
_PACKAGE_NAME = re.compile(r"^[A-Za-z0-9@][A-Za-z0-9._@/-]{0,200}$")
- Problem:
The character set[A-Za-z0-9._@/-]does not allow a colon (:). In OSV.dev, all packages in theMavenecosystem are keyed bygroupId:artifactId(e.g.org.apache.logging.log4j:log4j-core).
Since Magpie is an Apache project andMavenis explicitly included inecosystems = ["PyPI", "Maven", "npm", "Go"], running:will fail parameter validation withvetted-op-read --caller security-issue-triage osv-query-package org.apache.logging.log4j:log4j-core Maven 2.14.1
ParamError: package name did not match ^[A-Za-z0-9@][A-Za-z0-9._@/-]{0,200}$. - Required Change:
Update line 80 intools/vetted-ops/src/vetted_ops/ops.pyto allow colons:And in_PACKAGE_NAME = re.compile(r"^[A-Za-z0-9@][A-Za-z0-9._@/:-]{0,200}$")
tools/vetted-ops/tests/test_vetted_ops.py(lines 261–270), add a Maven coordinate test case totest_valid_package_names_are_accepted:@pytest.mark.parametrize( "valid_pkg", [ "jinja2", "@scope/package", "apache-airflow", "github.com/gin-gonic/gin", "org.apache.logging.log4j:log4j-core", "osv.dev", "pkg_name", ], )
2. _VULN_ID regex rejects distributor advisories with colons (RHSA, SUSE, ALSA, RLSA)
- Target File:
tools/vetted-ops/src/vetted_ops/ops.py - Specific Line: Line 77
- Current Code:
_VULN_ID = re.compile(r"^[A-Za-z0-9][-A-Za-z0-9]{2,60}$")
- Problem:
OSV indexes distributor advisories that contain colons, such as Red Hat (RHSA-2021:4321), SUSE (SUSE-SU-2021:1234-1), AlmaLinux (ALSA-2021:1234), and Rocky Linux (RLSA-2021:1234). Colons are valid URI path characters (RFC 3986) and safe against directory traversal. Currently,ops.vuln_idrejects all such IDs withParamError. - Required Change:
Update line 77 intools/vetted-ops/src/vetted_ops/ops.pyto allow colons:And in_VULN_ID = re.compile(r"^[A-Za-z0-9][-A-Za-z0-9:]{2,60}$")
tools/vetted-ops/tests/test_vetted_ops.py(lines 228–238), add distributor advisory test cases totest_valid_vuln_ids_are_accepted:@pytest.mark.parametrize( "valid_id", [ "GHSA-7rjr-3q55-vv33", "CVE-2021-45046", "RHSA-2021:4321", "SUSE-SU-2021:1234-1", "PYSEC-2021-123", "RUSTSEC-2020-0001", "GO-2022-0123", "OSV-2020-111", ], )
3. Add unit test execution for _run_http and proxy handling
- Target File:
tools/vetted-ops/tests/test_vetted_ops.py - Specific Line: After line 1100
- Problem:
The PR Test Plan claims:vetted-ops unit test suite covering HTTP request builders, regex validation, parameter rejection, and proxy handling passes
However,test_read_dispatcher_runs_http_reads(lines 1064–1100) only tests--dry-run. The actual execution logic in_run_http(tools/vetted-ops/src/vetted_ops/cli.py:248-284), includingHTTPError4xx/5xx exits,URLError, and proxy behavior, has 0 test coverage. - Required Change:
Add unit tests intools/vetted-ops/tests/test_vetted_ops.py(after line 1100) mockingurllib.request.urlopento test the execution branches of_run_http(success,HTTPError,URLError).
4. Set explicit default User-Agent in _run_http
- Target File:
tools/vetted-ops/src/vetted_ops/cli.py - Specific Line: Lines 269–272
- Current Code:
req = urllib.request.Request(url, data=payload, method=str(method)) for k, v in headers.items(): req.add_header(str(k), str(v))
- Problem:
urllib.requestsendsPython-urllib/<version>by default, which is blocked or aggressively rate-limited by certain CDNs and API gateways. - Required Change:
Add an explicit User-Agent header when not already present:req = urllib.request.Request(url, data=payload, method=str(method)) if "User-Agent" not in headers: req.add_header("User-Agent", "apache-magpie-vetted-ops/0.1.0") for k, v in headers.items(): req.add_header(str(k), str(v))
5. Expand _COMMIT_HASH length to support Git SHA-256 object format
- Target File:
tools/vetted-ops/src/vetted_ops/ops.py - Specific Line: Line 86
- Current Code:
_COMMIT_HASH = re.compile(r"^[0-9a-f]{7,40}$")
- Problem:
Git repositories using the SHA-256 object format use 64-character hex hashes, which_COMMIT_HASHcurrently rejects. - Required Change:
Update line 86 to accept up to 64 characters:And in_COMMIT_HASH = re.compile(r"^[0-9a-f]{7,64}$")
tools/vetted-ops/tests/test_vetted_ops.py(lines 291–298), add a 64-character hash intotest_valid_commits_are_accepted.
|
Thanks @potiuk and @onlyarnav for the thorough and helpful reviews! I've addressed all the feedback in the latest commits:
Please let me know if anything else needs refinement! |
potiuk
left a comment
There was a problem hiding this comment.
LGTM — every blocking point from both reviews is addressed in the tree,
the affected eval suites show no regression, and CI is green. Approving.
I re-checked each earlier finding against 18341fda itself rather than
against the summary:
api.osv.devis now in all three allowlist surfaces
(.claude/settings.json,tools/sandbox-lint/expected.json,
docs/setup/secure-agent-setup.md).- The executing copy in
plugins/magpie-security/skills/issue-sync/gather.md
is converted, and the failure-mode wording names the dispatcher's exit 4
instead ofcurl. _register()now rejectsbackend="http-read"withwrites=True, so
read-only is enforced at registration rather than asserted about today's
catalogue.- Endpoint scheme validation landed in
config.pyat load time, which is the
right layer —file://andhttp://are refused before a descriptor is
ever built. test_every_http_builder_produces_https_url_from_configured_endpoints
gives the HTTP backend the catalogue-level invariant theghbackend gets
fromargv[0] == "gh". That was the finding I most wanted closed, and it
came back stronger than I asked for.- The regex widening (Maven coordinates, distributor advisories, 64-char
SHA-256) matches what @onlyarnav specified. - Spec sync, the
[values] ecosystemsupgrade note, and theassert
removal are all in.
On the design question I raised last round — whether outbound HTTP to
third-party hosts belongs behind a separate vetted-http-read entry point
with its own allowlist entry — I am settling it as-is. Read-only enforcement
at registration, https-only endpoints, and the endpoint-prefix invariant
together keep the widening narrow enough to live in the existing dispatcher.
Nothing to change here.
Evals
Both affected suites were run against this branch:
dependency-audit— 8/8 passed.security-issue-sync— 38/45, and none of the failures are attributable
to this PR. Six reproduce identically onmain; the seventh
(step-2b-proposed-changes/case-1-pr-merged-label-update) passes on
re-run — that step swings by up to three cases between runs of unchanged
code. Those are a project-side problem, filed separately, and nothing for
you to act on.
One non-blocking note
The new recipes spell the dispatcher as a bare vetted-op-read …, while
every permission rule and sandbox exclusion in the repo spells it
uv run --project …/tools/vetted-ops vetted-op-read *, and
tools/vetted-ops/README.md writes it with an explicit … prefix. Unless
the console script is assumed to be on PATH, an agent following these
recipes literally would hit the same recipe-versus-baseline mismatch #1320
exists to close. One sentence in the adapter docs settles it either way.
Fine as a follow-up rather than a change here.
Thanks for the careful iteration on this one.
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.md
…er prefix (#1339) The recipes added in #1326 invoke the dispatcher as a bare `vetted-op-read …`, but every permission rule and sandbox exclusion in the repository spells it with the runner prefix, and `tools/vetted-ops/README.md` writes it as `… vetted-op-read` with an explicit ellipsis for the variable part. An agent following these recipes literally therefore hits a permission prompt or a sandboxed run — the same recipe-versus-baseline mismatch #1320 set out to close, in a new spelling. The framework already has a settled convention for addressing its own `tools/<name>/` subtrees: `uv run --project <framework>/tools/<name>`, used by `privacy-llm`, `reproducible-archive` and others, with `<framework>` resolved per the placeholder table in AGENTS.md. These nine recipes were the outlier rather than a new question, so they now match it. Generated-by: Claude Opus 5
|
Thank you for the thorough review and for merging this! Ah, I just pulled the latest |
Summary
tools/osv/(4 operations) andtools/cve-org/(1 operation) throughtools/vetted-opsHTTP read backend, replacing barecurlrecipes that are denied by the framework's sandbox baseline (docs/setup/secure-agent-setup.md).urllib.request) HTTP read backend to the dispatcher alongside the existingghbackend.Bash(curl *)deny-versus-adapter-recipes inconsistency across the entirecontract:security-cross-refsurface.Type of change
tools/<system>/*.md)tools/*/withpyproject.toml)docs/,README.md,CONTRIBUTING.md)Test plan
validate_tools(),validate_adapter_authoring(), andvalidate_capability_taxonomy_coverage()pass with 0 errorsvetted-opsunit test suite covering HTTP request builders, regex validation, parameter rejection, and proxy handling passesruff checkandruff format --checkpass onsrc/andtests/vendor-neutrality-scoreis unchanged (10/11 contracts, 91%)RFC-AI-0004 compliance
api.osv.devandcveawg.mitre.orgare already on the egress-gateway allowlistLinked issues
Closes #1320
Refs #1297