Skip to content

harden: bound HTTP reads and enforce strict redirects#3140

Open
PascalThuet wants to merge 3 commits into
github:mainfrom
PascalThuet:split/bounded-http-reads
Open

harden: bound HTTP reads and enforce strict redirects#3140
PascalThuet wants to merge 3 commits into
github:mainfrom
PascalThuet:split/bounded-http-reads

Conversation

@PascalThuet

@PascalThuet PascalThuet commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Part of splitting #2442 into smaller, dedicated PRs. Re-derived against current main.

What

  • New src/specify_cli/_download_security.py: read_response_limited (bounded, chunk-loop read) + is_https_or_localhost_http (shared scheme predicate) + size constants.
  • authentication/http.py: open_url(strict_redirects=...) plus an always-on _validate_strict_redirect inside the redirect handler — rejects redirects whose target isn't HTTPS (except HTTP→localhost), composing with the existing redirect_validator and auth-stripping rather than replacing them.
  • _version.py / _github_http.py: bounded JSON reads (MAX_JSON_METADATA_BYTES) + strict_redirects=True on the release-tag fetch.
  • authentication/azure_devops.py: route the OAuth token POST through the redirect handler (empty host list → scheme check only), so a 307/308 cannot forward the client_secret request body to a non-HTTPS host; bound the token read.
  • Test fakes that exercise GitHub release-resolution downloads now model HTTPResponse.read(size) cursor semantics, so the broader extension/preset/workflow suites cover the bounded-read path.

Why

Unbounded response.read() on remote JSON is a memory-DoS surface; a redirect to plain HTTP can downgrade transport or leak credentials. Both are closed without changing the happy path.

Validation

  • tests/test_download_security.py, test_github_http.py, test_authentication.py, test_upgrade.py, test_self_upgrade_*271 passed, 1 skipped. (The 19 warnings are pre-existing auth-fixture file-permission notes.)
  • tests/test_extensions.py tests/test_presets.py tests/test_workflows.py927 passed.
  • uvx ruff check src tests — clean.
  • git diff --check — clean.
  • HOME=/private/tmp/hermes-home .venv/bin/python -m pytest tests/integrations/test_integration_hermes.py -q34 passed. The full local pytest tests -q run only fails when Hermes writes to /Users/pascalthuet/.hermes, which is outside this sandbox's writable roots.

Foundation note: this PR adds only the read/URL core of _download_security. The ZIP-extraction + checksum helpers (and the tree-wide unbounded-read gate) land in the follow-up extension/preset and catalog PRs, which extend this module. Independent of PR2/PR3.

Disclosure: Updated on behalf of @PascalThuet by Codex (model: GPT-5, autonomous).

Add a shared _download_security module (read_response_limited,
is_https_or_localhost_http, size constants) and route the GitHub release
and Azure DevOps token network reads through bounded reads so an oversized
response can't exhaust memory.

Add a strict_redirects mode to authentication.open_url: the redirect handler
now rejects any redirect whose target isn't HTTPS (or HTTP to localhost),
composing with the existing per-hop redirect_validator and auth-stripping.
The Azure DevOps token POST is routed through that handler so a 307/308
cannot forward the client_secret body to a non-HTTPS host.
@PascalThuet PascalThuet requested a review from mnriem as a code owner June 23, 2026 21:57
Assisted-by: Codex (model: GPT-5, autonomous)

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.

Pull request overview

This PR hardens Spec Kit’s outbound HTTP handling by introducing bounded response reads for in-memory JSON parsing and enforcing strict redirect safety (HTTPS-only, with a narrow localhost HTTP exception) to reduce memory-DoS and credential-leak/downgrade risks.

Changes:

  • Add read_response_limited() and is_https_or_localhost_http() in a new _download_security module, including size ceilings for downloads vs JSON metadata.
  • Enforce strict redirect validation in the auth redirect handler and thread strict_redirects through open_url() for unauthenticated fallback behavior.
  • Update GitHub/Azure DevOps HTTP codepaths to use bounded JSON reads; adjust tests/fakes to model HTTPResponse.read(size) cursor semantics.
Show a summary per file
File Description
src/specify_cli/_download_security.py Introduces shared bounded-read + URL scheme predicate utilities and size constants.
src/specify_cli/authentication/http.py Adds strict redirect validation and a strict_redirects option to open_url().
src/specify_cli/_version.py Uses bounded reads for the “latest release” JSON and enables strict redirects.
src/specify_cli/_github_http.py Uses bounded reads for GitHub release metadata JSON resolution.
src/specify_cli/authentication/azure_devops.py Routes token POST via strict-redirect opener and bounds token JSON reads.
tests/test_download_security.py Adds focused unit tests for the new bounded-read and scheme predicate helpers.
tests/http_helpers.py Makes read() mocks stream-like and adds an autouse fixture to route opener .open() through urlopen() for tests.
tests/test_upgrade.py Adds a regression test pinning _fetch_latest_release_tag() to bounded reads and strict redirects; updates fixtures/imports.
tests/test_authentication.py Updates Azure token tests for opener-based fetching; adds redirect downgrade rejection coverage.
tests/test_github_http.py Updates mocks for bounded reads; adds redirect auth-preservation test across GitHub-owned hosts.
tests/test_extensions.py Updates GitHub release download mocks to stream-like read() behavior.
tests/test_presets.py Updates GitHub release download mocks to stream-like read() behavior.
tests/test_workflows.py Updates GitHub release download fakes to implement read(size) cursor semantics.
tests/self_upgrade_helpers.py Exposes the opener-routing fixture for self-upgrade test modules.
tests/test_self_upgrade_detection.py Imports the opener-routing fixture so existing urlopen patches remain effective.
tests/test_self_upgrade_execution.py Imports the opener-routing fixture so existing urlopen patches remain effective.
tests/test_self_upgrade_verification.py Imports the opener-routing fixture so existing urlopen patches remain effective.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 17/17 changed files
  • Comments generated: 1

Comment thread src/specify_cli/authentication/azure_devops.py
Assisted-by: Codex (model: GPT-5, autonomous)

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.

Copilot's findings

  • Files reviewed: 17/17 changed files
  • Comments generated: 1

Comment on lines +64 to +69
def _validate_strict_redirect(_old_url: str, new_url: str) -> None:
if not is_https_or_localhost_http(new_url):
raise urllib.error.URLError(
"unsafe redirect: target must use HTTPS with a hostname, "
"or HTTP for localhost (127.0.0.1, ::1)"
)

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please address Copilot feedback

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.

3 participants