Skip to content

fix(detection): report the urllib SSRF flow once, at the sink - #94

Merged
bkd-dotcom merged 1 commit into
mainfrom
fix/ssrf-drop-redundant-request-target
Aug 18, 2026
Merged

fix(detection): report the urllib SSRF flow once, at the sink#94
bkd-dotcom merged 1 commit into
mainfrom
fix/ssrf-drop-redundant-request-target

Conversation

@bkd-dotcom

Copy link
Copy Markdown
Member

Follow-up to #89 — the one nit I said I'd handle myself rather than asking @AdvaitVarhade to push again.

urllib.request.Request was in ssrf_targets alongside urllib.request.urlopen. But Request() only builds a request; taint propagates through the assignment, so the sink already catches the flow. Listing the constructor too reported one vulnerability twice — and since dedup keys on (file, line, category), findings on two different lines can't be collapsed.

dest = flask.request.args.get('dest')
req = urllib.request.Request(dest)      # flagged (L4)  <- redundant
urllib.request.urlopen(req).read()      # flagged (L5)  <- the real sink
before after
findings for that flow 2 (L4 + L5) 1 (L5)

No detection lost

Verified every SSRF shape is unchanged, including the urllib case from #89's own test suite:

get_positional     1x    request_pos_url    1x    httpx_patch     1x
get_kwarge_url     1x    request_kwarg_url  1x    aiohttp_pos     1x
urlopen_direct     1x    urllib_Request     1x  (was 2x)

All five constant-URL false-positive cases remain silent. 259 tests pass, ruff clean. Added test_urllib_request_flow_is_reported_once_at_the_sink to pin it.

Also credits @AdvaitVarhade in CONTRIBUTORS.md for #89.

Follow-up to #89, as flagged in review.

urllib.request.Request() only *builds* a request; taint propagates
through `req = Request(tainted)`, so urlopen(req) already flags the flow.
Listing the constructor as an SSRF sink as well reported one
vulnerability twice, on adjacent lines — and because dedup keys on
(file, line, category), two different lines cannot be collapsed.

    dest = flask.request.args.get('dest')
    req = urllib.request.Request(dest)      # was flagged (L4)
    urllib.request.urlopen(req).read()      # already flagged (L5)

Dropping the constructor loses no detection: the sink still catches it.
Verified — the case the contributor added in #89 still passes, and every
other SSRF shape is unchanged (get/post positional + keyword, requests.
request positional + keyword, httpx.patch, aiohttp.request, direct
urlopen).

Also credits @AdvaitVarhade in CONTRIBUTORS.md for #89.
@github-actions

Copy link
Copy Markdown

Signetry Reviewer — 🟡 Needs human review

A human should decide — the required check is pending.

Deterministic gates (the authority)

Gate Status
Required status check ⏳ pending
Secret scan ✅ clean
CI permission / OIDC ✅ no forbidden change
Dependency skew ✅ ok
All green

Findings

No issues found by the deterministic scanners.

Merge

A human should review and merge.

This review is advisory. It never merges on its own judgement — the deterministic gates + a human are the authority. Findings can have false negatives; a green bot verdict is not a guarantee.

@bkd-dotcom
bkd-dotcom merged commit 5365191 into main Aug 18, 2026
8 checks passed
@bkd-dotcom
bkd-dotcom deleted the fix/ssrf-drop-redundant-request-target branch August 18, 2026 19:25
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