harden: bound HTTP reads and enforce strict redirects by PascalThuet · Pull Request #3140 · github/spec-kit

PascalThuet · 2026-06-23T21:57:46Z

Part of splitting #2442 into smaller, dedicated PRs. Rebased onto current main.

What

Add shared bounded-response and URL-scheme helpers in src/specify_cli/_download_security.py.
Enforce strict redirect safety for both authenticated and unauthenticated requests while preserving the existing redirect validator, GHES handling, auth stripping, and malformed-URL behavior.
Bound GitHub release metadata, self-upgrade metadata, and Azure DevOps token response reads.
Route the Azure DevOps token POST through the guarded redirect opener.
Keep HTTP test fakes cursor-aware and reuse shared helpers where possible.

Why

Unbounded remote JSON reads expose a memory-denial-of-service surface. Unsafe redirect downgrades can also expose credentials or silently weaken transport. The change closes both paths while preserving the existing happy path and current main validation behavior.

Conflict and review resolution

Preserved the configurable GitHub metadata limit and redirect validator added on main while switching its implementation to the shared chunked reader.
Preserved current GHES shape validation and malformed-redirect handling.
Made strict redirect handling unconditional, including the no-token fallback used by catalogs and downloads.
Updated the Azure AD network-error test to intercept the guarded opener rather than bare urlopen.
Reused the shared opener-routing fixture in affected extension and integration-catalog tests.
Removed obsolete duplicate HTTP fake classes rather than carrying parallel helpers.

Validation

uvx ruff check src tests — passed.
HTTP/security and affected-caller tests — 1,675 passed.
Full suite — 4,786 passed, 153 skipped.
git diff --check — passed.

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

Copilot

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

Copilot

Copilot's findings

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

mnriem

Please address Copilot feedback

Copilot

Review details

Files reviewed: 17/17 changed files
Comments generated: 1
Review effort level: Low

Copilot

Review details

Files reviewed: 17/17 changed files
Comments generated: 1
Review effort level: Low

+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

Please address Copilot feedback

Copilot

Review details

Files reviewed: 17/17 changed files
Comments generated: 0 new
Review effort level: Low

mnriem

Please address test & lint errors

PascalThuet · 2026-07-20T15:29:10Z

Review round summary

Rebase and conflict resolution

Rebased the PR onto the current main branch. The conflict resolution preserves:

the newer GHES validation;
the configurable metadata size limit;
redirect-validator composition;
malformed-URL handling.

Duplicate HTTP fakes were omitted in favor of the existing shared, cursor-aware implementations.

Previous CI failure

The historical junie-kilocode failure is obsolete after the integration-registry changes now present on main.

Validation

Ruff on the current CI scope: passed
Targeted tests: 692 passed, 1 skipped
Extension, preset, and workflow tests: 1,391 passed
Full suite: 4,447 passed, 112 skipped

Head commit: 4881d99

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

Copilot

Review details

Files reviewed: 17/17 changed files
Comments generated: 2
Review effort level: Medium

    timeout: int = 10,
    extra_headers: dict[str, str] | None = None,
    redirect_validator: RedirectValidator | None = None,
+    strict_redirects: bool = False,


+        mock_opener = MagicMock()
+        mock_opener.open.return_value = mock_resp
+        with patch("urllib.request.build_opener", return_value=mock_opener):


mnriem · 2026-07-21T17:34:08Z

Please address Copilot feedback

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. Assisted-by: Codex (model: GPT-5, autonomous)

Assisted-by: Codex (model: GPT-5, autonomous)

PascalThuet · 2026-07-21T22:55:37Z

Addressed the latest Copilot review round and re-audited the PR against current main.

Both findings were valid. open_url now always installs the strict redirect handler, including the unauthenticated fallback used by extension, preset, workflow, bundle, and integration-catalog downloads. The newly introduced strict_redirects switch was removed so there is no insecure default path. The Azure AD network-error test now patches build_opener().open() and cannot issue a real request.

The broader audit found four additional tests coupled to bare urlopen (one extension download and three integration-catalog cases). They now reuse the shared opener-routing fixture; no additional production defect was found within the PR scope.

Validation: Ruff passed on src and tests; 1,675 HTTP/security and affected-caller tests passed; full suite: 4,786 passed, 153 skipped; git diff --check passed.

Commit: 30a745439fbac06f50ec7dd04dd87162351467ca
Base: d7699c39f2c3ab7d9c4f3eceff8f29ae1601cc3b

The review threads remain open for the reviewer or PR author to resolve.

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

PascalThuet requested a review from mnriem as a code owner June 23, 2026 21:57

This was referenced Jun 23, 2026

harden: safe ZIP extraction, checksums, and bounded reads for extension/preset downloads #3141

Draft

harden: bound remaining catalog reads + lock in no-unbounded-read invariant #3145

Draft

mnriem requested a review from Copilot June 24, 2026 12:49

Copilot started reviewing on behalf of mnriem June 24, 2026 12:50 View session

Copilot AI reviewed Jun 24, 2026

View reviewed changes

Comment thread src/specify_cli/authentication/azure_devops.py

mnriem requested a review from Copilot June 24, 2026 18:33

Copilot started reviewing on behalf of mnriem June 24, 2026 18:33 View session

Copilot AI reviewed Jun 24, 2026

View reviewed changes

Comment thread src/specify_cli/authentication/http.py

mnriem requested changes Jun 24, 2026

View reviewed changes

PascalThuet force-pushed the split/bounded-http-reads branch from 885f8fc to f2c3bc1 Compare June 26, 2026 20:14

mnriem requested a review from Copilot June 26, 2026 21:58

Copilot started reviewing on behalf of mnriem June 26, 2026 21:59 View session

Copilot AI reviewed Jun 26, 2026

View reviewed changes

Comment thread tests/test_upgrade.py Outdated

PascalThuet requested a review from mnriem June 26, 2026 22:54

mnriem requested a review from Copilot June 29, 2026 12:15

Copilot started reviewing on behalf of mnriem June 29, 2026 12:15 View session

Copilot AI reviewed Jun 29, 2026

View reviewed changes

mnriem requested changes Jun 29, 2026

View reviewed changes

PascalThuet requested a review from mnriem July 5, 2026 18:15

mnriem requested a review from Copilot July 7, 2026 13:46

Copilot started reviewing on behalf of mnriem July 7, 2026 13:46 View session

Copilot AI reviewed Jul 7, 2026

View reviewed changes

mnriem requested changes Jul 7, 2026

View reviewed changes

PascalThuet force-pushed the split/bounded-http-reads branch from 46c8cb8 to 4881d99 Compare July 20, 2026 15:28

PascalThuet requested a review from mnriem July 20, 2026 15:41

mnriem requested a review from Copilot July 21, 2026 14:59

Copilot started reviewing on behalf of mnriem July 21, 2026 15:00 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

PascalThuet added 7 commits July 22, 2026 00:44

test: align HTTP fakes with bounded reads

96f5fd1

Assisted-by: Codex (model: GPT-5, autonomous)

fix: tolerate invalid token response encoding

a444e46

Assisted-by: Codex (model: GPT-5, autonomous)

test: align GHES fakes with bounded reads

c7e3d50

Assisted-by: Codex (model: GPT-5, autonomous)

test: reuse shared upgrade HTTP response helper

54b62cb

Assisted-by: Codex (model: GPT-5, autonomous)

fix: include rejected redirect target in error

676dfca

Assisted-by: Codex (model: GPT-5, autonomous)

fix: enforce strict redirects by default

30a7454

Assisted-by: Codex (model: GPT-5, autonomous)

PascalThuet force-pushed the split/bounded-http-reads branch from 4881d99 to 30a7454 Compare July 21, 2026 22:55

Uh oh!

Conversation

PascalThuet commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Conflict and review resolution

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

mnriem left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

mnriem left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

mnriem left a comment

Choose a reason for hiding this comment

Uh oh!

PascalThuet commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review round summary

Rebase and conflict resolution

Previous CI failure

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

mnriem commented Jul 21, 2026

Uh oh!

PascalThuet commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PascalThuet commented Jun 23, 2026 •

edited

Loading

PascalThuet commented Jul 20, 2026 •

edited

Loading