Skip to content

fix(github-http): validate release asset metadata URLs - #4438

Open
WOLIKIMCHENG wants to merge 1 commit into
github:mainfrom
WOLIKIMCHENG:fix/validate-release-asset-metadata-urls
Open

fix(github-http): validate release asset metadata URLs#4438
WOLIKIMCHENG wants to merge 1 commit into
github:mainfrom
WOLIKIMCHENG:fix/validate-release-asset-metadata-urls

Conversation

@WOLIKIMCHENG

Copy link
Copy Markdown
Contributor

Description

Harden GitHub release-asset resolution so release metadata can replace a browser download URL only when the metadata URL:

  • uses the derived GitHub or GHES API origin, including the expected scheme and effective port
  • identifies the same owner and repository case-insensitively
  • matches the exact release-asset REST path with a numeric asset ID
  • contains no malformed percent escapes, raw whitespace or control characters, query or fragment delimiters, path parameters, or invalid authority components

This preserves public GitHub and GHES behavior, including custom ports, equivalent IPv6 literals, loopback HTTP, direct API-asset passthrough, and fallback to the original browser download URL.

Testing

  • uvx ruff@0.15.0 check src tests — all checks passed
  • .venv/bin/python -m pytest tests/test_github_http.py -q — 78 passed
  • git diff --check — passed

Require metadata asset URLs to match the derived API origin, repository
identity, and exact numeric release-asset endpoint.

Reject malformed metadata URLs while preserving direct passthrough and
browser-download fallback behavior.
@WOLIKIMCHENG
WOLIKIMCHENG requested a review from mnriem as a code owner September 4, 2026 06:44
@mnriem mnriem added author-needs-disclosure AI assistance not disclosed — disclose AI use per CONTRIBUTING triage-must-have Verdict: high-value, important work for Spec Kit — do first labels Sep 8, 2026
@mnriem
mnriem requested a balanced review from Copilot September 8, 2026 21:40
@mnriem

mnriem commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Thanks — hardening the release-asset download path is worth prioritizing. Two things before review: please disclose any AI assistance per CONTRIBUTING (the body has none), and note that because this is hand-written URL-validation on a security trust boundary, I'm going to put it through a security-focused review rather than a quick pass. Two questions that will help: is there a known way the current metadata-URL handling can be abused (a repro or scenario), or is this defense-in-depth? And have you considered leaning on urllib/a vetted parser for the authority/percent-escape checks rather than bespoke parsing? Marking author-awaiting pending disclosure; I'll trigger the review in parallel.

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.

🟢 Approval recommended

The validation is comprehensively tested, with no unresolved issues.

Pull request overview

Hardens GitHub release-asset resolution by strictly validating metadata URLs while preserving GitHub.com and GHES behavior.

Changes:

  • Validates API origin, repository identity, asset path, and URL syntax.
  • Supports GHES custom ports and equivalent IPv6 hosts.
  • Adds comprehensive validation tests.
File summaries
File Description
tests/test_github_http.py Covers valid and malformed GitHub/GHES metadata URLs.
src/specify_cli/_github_http.py Adds strict release-asset metadata URL validation.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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

Labels

author-needs-disclosure AI assistance not disclosed — disclose AI use per CONTRIBUTING triage-must-have Verdict: high-value, important work for Spec Kit — do first

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants