Skip to content

docs(sentinel): remove false urllib SSRF record - #1633

Merged
seonghobae merged 1 commit into
mainfrom
fix/remove-false-urllib-ssrf-record
Sep 1, 2026
Merged

docs(sentinel): remove false urllib SSRF record#1633
seonghobae merged 1 commit into
mainfrom
fix/remove-false-urllib-ssrf-record

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Outcome

Remove the stale Sentinel entry introduced by #1631 that still described HTTPRedirectHandler.redirect_request(...)->None as an active SSRF vulnerability after #1631's own corrected RCA established that the original opener composition already blocked redirects.

Why

The merged #1631 source hardening remains valid: explicitly raising HTTPError states the no-redirect invariant without relying on handler composition. The Sentinel record, however, contradicted the corrected review and incorrectly preserved a HIGH-vulnerability narrative that #1631's PR body explicitly disclaimed.

This PR restores documentation/evidence consistency only. It does not revert the explicit HTTPError hardening, change URL validation, scanner thresholds, permissions, or branch protection.

Exact head: 79bfcd8e764d44597fbe15e39b0d8f69863c2c1b.


Devin Review

Copy link
Copy Markdown
Contributor Author

QUEUE_SATURATION_CHICKEN_EGG verified for exact head 79bfcd8e764d44597fbe15e39b0d8f69863c2c1b. This PR is a one-file, four-line deletion restoring the corrected RCA already established by #1631's resolved Devin review and merged PR body: the old opener already blocked redirects, while explicit HTTPError is retained only as defensive contract hardening. GitHub reports this head mechanically mergeable with no review findings; Devin is pending and all generated Actions lanes are queued while the repository queue has grown to 884 runs. No product code, redirect hardening, test, security policy, provenance, permission, branch protection, or scanner threshold changes. The only current admission blocker is saturated review/check capacity; merge must bind this exact head.

@devin-ai-integration devin-ai-integration Bot 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.

Devin Review found 1 potential issue.

Devin Review

Comment thread .jules/sentinel.md
Comment on lines -46 to -49
## 2026-09-01 - Prevent SSRF in urllib by raising HTTPError on redirects
**Vulnerability:** Subclassing `urllib.request.HTTPRedirectHandler` and returning `None` to disable redirects leaves the handler vulnerable, as `None` simply passes the request back up the fallback chain, potentially resulting in returning a 301/302 response to the caller rather than preventing execution.
**Learning:** Returning `None` from `redirect_request` relies on default behavior to handle the response, not raising a true failure condition which is expected to halt SSRF bypasses via 301/302.
**Prevention:** To securely prevent redirects in `urllib` and avoid SSRF vulnerabilities, explicitly raise an `urllib.error.HTTPError` inside `redirect_request` instead of returning `None`.

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.

📝 Info: Deleted redirect claim was false

build_opener(NoRedirectHandler()) replaced the default redirect handler, so returning None did not enable fallback redirect following. Explicit rejection and valid prevention guidance remain.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@seonghobae
seonghobae merged commit 09090e9 into main Sep 1, 2026
16 of 32 checks passed
@seonghobae
seonghobae deleted the fix/remove-false-urllib-ssrf-record branch September 1, 2026 18:34
seonghobae added a commit that referenced this pull request Sep 1, 2026
Preserve the current protected-main explicit urllib redirect hardening and its corrected Sentinel record while retaining #1628's workflow-backed Pages reconciliation, fail-before-write, stale-run cancellation, and documentation changes. This is a normal two-parent non-force reconciliation.
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.

1 participant