Fix catalog-latest-url-bypass: require tag-pinned catalog download URLs - #4194
Conversation
Reject floating releases/latest URLs in the community catalog agent workflows and require the URL tag to match the submitted version. Refs github#4185 Assisted-by: Cursor Grok 4.6 (supervised) Signed-off-by: shaurya2k06 <shaurya2k06@gmail.com>
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
Separate substring checks for vX.Y.Z and X.Y.Z were not independent. Refs github#4185 Assisted-by: Cursor Grok 4.6 (supervised) Signed-off-by: shaurya2k06 <shaurya2k06@gmail.com>
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/test_github_workflows.py:126
- These independent whole-document checks do not actually guard the required clauses. For example, changing
MUST followtomay followin all three workflows still passes because the tag-match sentence supplies anotherMUST; similarly,rejectcan be satisfied elsewhere in each document. Assert the complete normalized validation statements so weakening either high-severity requirement fails this regression test.
assert "should follow the pattern" not in lowered
assert "releases/latest/" in source_text
assert "reject" in lowered
assert "`vX.Y.Z` or `X.Y.Z`" in source_text
assert "MUST" in source_text or "must" in lowered
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Thanks for tightening these catalog checks—the direction looks good. Before approving, could you please address a few remaining concerns?
Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol). |
|
oh yeah, for sure! I'll make the changes needed asap |
Keep tag-pinned URLs but accept suffixes like aide-v1.0.0, require download_url to match the submitted repository, and treat sha256 as optional follow-up rather than a hard catalog gate. Refs github#4185 Assisted-by: Cursor Grok 4.6 (supervised) Signed-off-by: shaurya2k06 <shaurya2k06@gmail.com>
Skip the HTTP check for a floating releases/latest URL without aborting the rest of Step 2. Refs github#4185 Assisted-by: Cursor Grok 4.6 (supervised) Signed-off-by: shaurya2k06 <shaurya2k06@gmail.com>
|
valid points, updated accordingly. All three workflows now say mark the pinning check failed, skip the HTTP request for that URL, then continue the remaining validations. |
|
Hey @mnriem , could I get a review on this? |
There was a problem hiding this comment.
🟡 Changes recommended
The unconditional SHA verification conflicts with the requirement not to fetch URLs that fail pinning validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
Keep archive fetching and optional hash verification behind successful URL pinning checks. Refs github#4185 Assisted-by: Codex (model: GPT-5, autonomous) Signed-off-by: shaurya2k06 <shaurya2k06@gmail.com>
|
Updated all three catalog workflow prompts in a1a7967 so downloads—including optional SHA verification—run only after every pinning check succeeds. Tightened the shared regression assertion to keep the gate and SHA rule together. Checks: focused workflow tests (6 passed), Ruff, and the CLI smoke test pass. The full local suite completed with 6,957 passed and 9 unrelated script-parity failures on macOS. @mnriem, could you please re-review? Posted on behalf of @Shaurya2k06 by Codex (model: GPT-5). |
|
Thanks @Shaurya2k06 — confirmed the pinning/HTTP-skip and sha256-gating fixes are applied consistently across the extension, preset, and bundle validation workflows, and all threads are resolved. This is now on me: I'll re-review to clear my earlier changes-requested and trigger CI. |
There was a problem hiding this comment.
🔵 Needs a closer look
The regression test does not enforce the mandatory wording central to the security fix.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/test_github_workflows.py:152
- This regression test does not actually enforce the central
MUST follow ... tag-pinned patternrequirement: it only rejects the former exact phrase, so changing the workflow to advisory wording such as “may follow the pattern” would still pass while reintroducing the weakness from #4185. Assert that each source contains the mandatoryThe download URL MUST followclause as well.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Thank you! |
Bug fix — catalog-latest-url-bypass
Proposed fix for issue #4185, applying the remediation from the bug assessment.
Verdict: valid · Severity: high
Summary
Community catalog agent workflows treated tag-pinned
download_urlas advisory and accepted floatingreleases/latest/aliases after an HTTP 200 check. The three workflows now MUST use a tag-pinned URL, rejectreleases/latest/before any HTTP check, and require the URL tag to match the submitted version (vX.Y.ZorX.Y.Z).Changes
.github/workflows/add-community-extension.mdreleases/latest/reject + tag/version match.github/workflows/add-community-preset.md.github/workflows/add-community-bundle.mdtests/test_github_workflows.pyTests Added or Updated
tests/test_github_workflows.py::test_community_submission_workflows_require_tag_pinned_download_urls— asserts each community workflow drops “should follow the pattern”, forbidsreleases/latest/, and requires tag/version match languageLocal Verification
.venv/bin/python -m pytest tests/test_github_workflows.py -q→ 6 passed.venv/bin/specify --help→ CLI help renderedgh aw compilewas not available locally; lock YAML left unchanged because compiled workflows runtime-import the.mdprompt bodiesDeviations from Assessment
None. Scope is the three workflow files plus a static test, as preferred. No shared checklist doc, no Python URL validator, no skill/template changes.
Risks & Review Notes
Refs #4185
Testing
uv run specify --helpuv sync && uv run pytest(focused:tests/test_github_workflows.py)AI Disclosure
Drafted and implemented under human supervision by Cursor Grok 4.6. I reviewed the code, ran tests, and the commit myself. I take full responsibility of the code generated by me.