Skip to content

Harden initial URL validation in HTTP helpers - #5350

Merged
mnriem merged 8 commits into
github:mainfrom
mnriem:mnriem-http-credential-protection
Nov 10, 2026
Merged

mnriem merged 8 commits into
github:mainfrom
mnriem:mnriem-http-credential-protection

Conversation

@mnriem

@mnriem mnriem commented Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Harden initial URL validation in build_request() and open_url() by applying the existing HTTPS/loopback transport policy before resolving credentials or constructing requests. HTTPS remains supported, along with HTTP for unambiguous loopback hosts.

Workflow and step catalog fetchers now bypass stale-cache fallback when initial URL, redirect, or final URL validation is rejected, including during forced refresh. These failures retain their URL-policy classification so online bundle catalog fetching and reference validation report definitive errors rather than falling back to packaged snapshots or leaving references unchecked. This intentional hardening prevents expired metadata from masking a rejected URL and makes the outcome independent of which validator rejects it first. Ordinary fetch failures retain their existing availability and stale-cache fallbacks.

Validate raw Location and legacy URI headers before urllib processes redirects. Malformed absolute targets are rejected instead of being normalized onto the current host or surfacing as unclassified parsing failures. This keeps malformed-target handling consistent with other URL-policy rejections while preserving valid relative and scheme-relative redirects and existing authentication-header handling.

Add coverage for accepted and rejected URLs across authentication providers, expired-cache behavior with and without forced refresh, and actual urllib redirect-response processing for every supported redirect status. Document the transport policy and the rationale for the catalog fallback changes in design/catalogs.md.

Testing

Set up this checkout's .venv and editable install using the local development guide. Run commands from the checkout root.

  • Tested locally with .venv/bin/specify --help (Windows PowerShell: .\.venv\Scripts\specify.exe --help)
  • Ran relevant existing tests with .venv/bin/python -m pytest tests -q (Windows PowerShell: .\.venv\Scripts\python.exe -m pytest tests -q); use affected test paths for focused checks and LC_ALL=en_US.UTF-8 on macOS
  • Tested with a sample project (if applicable)

Commands and results:

  • .venv/bin/specify --help — passed.
  • .venv/bin/python -m pytest tests/specify_cli/authentication -q --tb=short — 389 passed.
  • .venv/bin/python -m pytest tests/test_download_security.py tests/test_extensions.py tests/specify_cli/bundles tests/specify_cli/extensions tests/specify_cli/presets tests/specify_cli/integrations/test_catalog.py tests/specify_cli/integrations/test_command_list.py tests/specify_cli/workflows -q --tb=short — 4,013 passed, 29 skipped.
  • LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests -q — full suite completed: 15,042 passed, 134 skipped, 1 failed. The failure was tests/specify_cli/test_version_execution.py::TestWindowsUvToolEnvironment::test_non_windows_uv_tool_dry_run_still_previews_installer: terminal wrapping split the long interpreter path in the dry-run output assertion. The version implementation and this test are unchanged by this PR.
  • COLUMNS=240 LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/specify_cli/test_version_execution.py::TestWindowsUvToolEnvironment::test_non_windows_uv_tool_dry_run_still_previews_installer -q --tb=short — 1 passed. Widening the terminal resolves the isolated output-wrapping assertion without source changes; this isolated rerun is not a full-suite pass.

Sample-project testing is not applicable to this HTTP-helper change.

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (fill in the disclosure below)

AI disclosure: GitHub Copilot using GPT-6.1 Sol generated the implementation, tests, documentation, commit message, and PR description and ran the automated checks. Work was performed in autonomous agent mode at the contributor's direction; reasoning effort used the runtime configuration and was not exposed.

Apply the shared HTTPS and loopback transport policy before resolving authentication or constructing requests. Cover accepted and rejected initial URLs and document the policy.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 9, 2026 21:41

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.

🟡 Changes recommended

Initial URL-policy rejections are misclassified as transient outages, causing bundle reference validation to warn and leave references unchecked rather than definitively reject them.

1 open issue
What changed in this PR

Applies the existing HTTPS/loopback transport policy to initial requests in Spec Kit’s authenticated HTTP helpers, before credentials are resolved or requests are constructed.

Changes:

  • Reject unsafe or malformed initial URLs in build_request() and open_url().
  • Add acceptance and rejection tests across authentication providers and unauthenticated requests.
  • Document the initial-request policy.

Validation was limited to source inspection and a focused standard-library check; project tests and before/after regression runs were not performed.

File Description
tests/​specify_cli/​authentication/​test_authentication.py Tests accepted URLs and early rejection before credential resolution.
src/​specify_cli/​authentication/​http.py Validates initial URLs in both shared HTTP helpers.
docs/​reference/​authentication.md Documents HTTPS requirements and the loopback HTTP exception.

Review effort: Max (auto)

Comment thread src/specify_cli/authentication/http.py Resolved
Share a URLPolicyError base between initial URL and redirect policy rejections. Preserve definitive errors in bundle catalog fetches and reference validation instead of treating them as temporary outages.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 9, 2026 21:54

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.

🟡 Changes recommended

The “catalog validation” wording overstates the implemented behavior because ordinary catalog lookups can still skip failed sources or use stale caches.

2 open issues

Review effort: Max (auto)

Comment thread docs/reference/authentication.md Resolved
Name online bundle catalog fetching and reference validation explicitly without implying changes to ordinary catalog lookup or cache policies.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 9, 2026 22:11

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.

🔵 Needs a closer look

At least one reviewer session failed, leaving the review evidence incomplete.

1 open issue
1 resolved since last review

Review effort: Max (auto)

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.

🟡 Changes recommended

Workflow and step catalog fetchers suppress URL-policy errors and return stale metadata even with force_refresh=True, allowing bundle validation to accept cached data from rejected URLs.

1 open issue
1 resolved since last review

Review effort: Max (auto)

Bypass stale-cache fallback for shared URL policy rejections in workflow and step catalog fetchers while retaining the domain error contract and original cause. Cover expired cache, forced refresh, bundle reference rejection, and transport outage fallback.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 9, 2026 22:42

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.

🟡 Changes recommended

Expired-cache fallback differs between redirect validators in both catalog workflows, making rejected-redirect behavior depend on which validator runs first.

2 open issues

Review effort: Max (auto)

@mnriem mnriem added the triage-must-have Verdict: high-value, important work for Spec Kit — do first label Nov 9, 2026
Classify catalog callback and final URL validation rejections as URL policy failures so workflow and step catalogs never return stale data after rejected URLs, regardless of validator order. Preserve domain errors and ordinary fetch fallback, and extend expired-cache coverage.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 9, 2026 23:11

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.

🟡 Changes recommended

The change alters stale-cache fallback behavior for redirect and final-URL rejections without explicit justification, while mocked redirect tests miss malformed targets that can still return expired cache.

2 open issues
1 resolved since last review

Review effort: Max (auto)

Classify malformed Location and URI targets before urllib parses or normalizes them, covering every supported redirect status. Exercise the real redirect response entry point for expired-cache regressions, preserve valid relative redirects and auth handling, and explain the intentional URL-policy fallback change.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 9, 2026 23:48

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.

🟡 Changes recommended

The PR description omits the stale-cache fallback bypass for URL-policy errors and stricter raw redirect-header validation, leaving both behavior changes and their hardening rationale unexplained.

3 open issues

Review effort: Max (auto)

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.

🔵 Needs a closer look

Security-sensitive shared HTTP/redirect changes and the catalog stale-cache policy change require maintainer sign-off, and the full test suite has not been run.

2 open issues
1 resolved since last review

Review effort: Max (auto)

Resolve catalog and design conflicts by retaining upstream's HTTPS-only catalog requirements alongside definitive URL policy failures and stale-cache safeguards. Preserve the updated integration redirect documentation and cover loopback HTTP catalog rejections.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 10, 2026 18:59

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.

🟡 Changes recommended

An unresolved formatting nit leaves urllib’s <urlopen error …> wrapper in user-facing URL-policy rejection errors in both catalog implementations instead of displaying exc.reason.

1 open issue
2 resolved since last review

Review effort: Max (auto)

Format URLPolicyError.reason in workflow and step catalog diagnostics while retaining the original exception as the cause. Assert exact messages across initial URL, redirect, and final URL rejections.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 10, 2026 19:25

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.

🔵 Needs a closer look

Maintainer sign-off is needed for the stale-cache fallback policy change requiring separate approval and the security-sensitive redirect changes in the shared authenticated HTTP client.

No open issues 🎉

1 resolved since last review

Review effort: Max (auto)

@mnriem
mnriem merged commit 0b457af into github:main Nov 10, 2026
16 checks passed
@mnriem
mnriem deleted the mnriem-http-credential-protection branch November 10, 2026 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

2 participants