Skip to content

feat(tools): route OSV and CVE.org adapter recipes through vetted-ops (#1320) - #1326

Merged
potiuk merged 6 commits into
apache:mainfrom
Kaap10:feat/vetted-ops-http-1320
Sep 22, 2026
Merged

potiuk merged 6 commits into
apache:mainfrom
Kaap10:feat/vetted-ops-http-1320

Conversation

@Kaap10

@Kaap10 Kaap10 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Route tools/osv/ (4 operations) and tools/cve-org/ (1 operation) through tools/vetted-ops HTTP read backend, replacing bare curl recipes that are denied by the framework's sandbox baseline (docs/setup/secure-agent-setup.md).
  • Add a stdlib-only (urllib.request) HTTP read backend to the dispatcher alongside the existing gh backend.
  • Close the Bash(curl *) deny-versus-adapter-recipes inconsistency across the entire contract:security-cross-ref surface.

Type of change

  • Tool / bridge contract (tools/<system>/*.md)
  • Python package (tools/*/ with pyproject.toml)
  • Documentation (docs/, README.md, CONTRIBUTING.md)

Test plan

  • validate_tools(), validate_adapter_authoring(), and validate_capability_taxonomy_coverage() pass with 0 errors
  • vetted-ops unit test suite covering HTTP request builders, regex validation, parameter rejection, and proxy handling passes
  • ruff check and ruff format --check pass on src/ and tests/
  • vendor-neutrality-score is unchanged (10/11 contracts, 91%)

RFC-AI-0004 compliance

  • Sandbox — no new unrestricted host access; api.osv.dev and cveawg.mitre.org are already on the egress-gateway allowlist
  • Vendor neutrality — placeholders and canonical endpoint mappings used throughout

Linked issues

Closes #1320
Refs #1297

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread tools/osv/README.md
Comment thread tools/osv/tool.md Outdated
Comment thread tools/osv/tool.md Outdated
Comment thread tools/vetted-ops/README.md
Comment thread tools/vetted-ops/src/vetted_ops/cli.py Outdated
Comment thread tools/vetted-ops/tests/test_vetted_ops.py
Comment thread tools/vetted-ops/tests/test_vetted_ops.py
Comment thread tools/cve-org/README.md Outdated
Comment thread tools/cve-org/tool.md Outdated
Comment thread tools/vetted-ops/src/vetted_ops/ops.py

@onlyarnav onlyarnav left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 the Maven ecosystem are keyed by groupId:artifactId (e.g. org.apache.logging.log4j:log4j-core).
    Since Magpie is an Apache project and Maven is explicitly included in ecosystems = ["PyPI", "Maven", "npm", "Go"], running:
    vetted-op-read --caller security-issue-triage osv-query-package org.apache.logging.log4j:log4j-core Maven 2.14.1
    will fail parameter validation with ParamError: package name did not match ^[A-Za-z0-9@][A-Za-z0-9._@/-]{0,200}$.
  • Required Change:
    Update line 80 in tools/vetted-ops/src/vetted_ops/ops.py to allow colons:
    _PACKAGE_NAME = re.compile(r"^[A-Za-z0-9@][A-Za-z0-9._@/:-]{0,200}$")
    And in tools/vetted-ops/tests/test_vetted_ops.py (lines 261–270), add a Maven coordinate test case to test_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_id rejects all such IDs with ParamError.
  • Required Change:
    Update line 77 in tools/vetted-ops/src/vetted_ops/ops.py to allow colons:
    _VULN_ID = re.compile(r"^[A-Za-z0-9][-A-Za-z0-9:]{2,60}$")
    And in tools/vetted-ops/tests/test_vetted_ops.py (lines 228–238), add distributor advisory test cases to test_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), including HTTPError 4xx/5xx exits, URLError, and proxy behavior, has 0 test coverage.

  • Required Change:
    Add unit tests in tools/vetted-ops/tests/test_vetted_ops.py (after line 1100) mocking urllib.request.urlopen to 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.request sends Python-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_HASH currently rejects.
  • Required Change:
    Update line 86 to accept up to 64 characters:
    _COMMIT_HASH = re.compile(r"^[0-9a-f]{7,64}$")
    And in tools/vetted-ops/tests/test_vetted_ops.py (lines 291–298), add a 64-character hash into test_valid_commits_are_accepted.

@Kaap10

Kaap10 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @potiuk and @onlyarnav for the thorough and helpful reviews!

I've addressed all the feedback in the latest commits:

  • Sandbox & Egress: Added api.osv.dev across .claude/settings.json, tools/sandbox-lint/expected.json, and docs/setup/secure-agent-setup.md.
  • Skill & Recipe Conversions: Converted the inlined check in issue-sync/gather.md and the remaining recipe in tools/osv/tool.md from curl to vetted-op-read (exit 4). Replaced hardcoded callers in the tool docs with the <caller> placeholder, and restored www.cve.org / nvd.nist.gov in the CVE prerequisites.
  • Dispatcher Hardening:
    • Expanded parameter regexes to support Maven coordinates (_PACKAGE_NAME), distributor advisories (_VULN_ID), and 64-char Git SHA-256 hashes (_COMMIT_HASH).
    • Enforced that backend == "http-read" must be read-only in _register().
    • Added timeout=30, TimeoutError handling, default User-Agent, and https:// scheme validation.
    • Replaced assert isinstance with explicit ParamError exceptions.
    • Added an upgrade note in tools/vetted-ops/README.md regarding [values] ecosystems.
  • Tests & Typing:
    • Reverted the local Windows test tweaks to restore the clean Linux CI baseline.
    • Added unit test coverage for the widened regexes, catalogue-level HTTPS URL invariants, and mocked _run_http execution branches (including mypy type annotations for the mock helpers).
    • Synchronized tools/spec-loop/specs/vetted-command-surface.md to document the http-read backend.

Please let me know if anything else needs refinement!

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.dev is 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 of curl.
  • _register() now rejects backend="http-read" with writes=True, so
    read-only is enforced at registration rather than asserted about today's
    catalogue.
  • Endpoint scheme validation landed in config.py at load time, which is the
    right layer — file:// and http:// 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 the gh backend gets
    from argv[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] ecosystems upgrade note, and the assert
    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 on main; 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

Comment thread tools/osv/tool.md
@potiuk
potiuk merged commit fccd1e0 into apache:main Sep 22, 2026
11 of 12 checks passed
potiuk added a commit that referenced this pull request Sep 22, 2026
…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
@Kaap10

Kaap10 commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for the thorough review and for merging this!
I really appreciate the guidance.
I'll open a quick follow-up PR to clarify the vetted-op-read path in the adapter docs as you suggested.

Ah, I just pulled the latest main branch and noticed you already patched the vetted-op-read paths in PR #1339!
Thank you for taking care of that follow-up directly.
Looking forward to contributing more in the future!

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.

feat(tools): route OSV and CVE.org adapter recipes through vetted-ops sandbox dispatcher

3 participants