feat(detection): add SSRF (CWE-918) rule for Python (#86)

[AdvaitVarhade]

Summary

Adds detection for Server-Side Request Forgery (SSRF, CWE-918) in Python source code across both AST deterministic scanning and line-based language taint analysis.

Changes

• signetry_core/pipeline/findings/deterministic.py:
  • Added AST inspection for requests, urllib.request, httpx, and aiohttp outbound request calls with tainted URL inputs (positional or keyword arguments).
  • Flags findings under rule ID py.ssrf, category ssrf, severity HIGH, CWE-918.
• signetry_core/pipeline/findings/lang_taint.py:
  • Added _PY LangTaintSpec with source tracking for request params/args/body and sink definition py.taint.ssrf targeting HTTP client calls.
• tests/test_findings_engine.py:
  • Added unit test cases for tainted requests, urllib.request, and httpx calls.
  • Added test case verifying zero false positives on constant/allowlisted URLs.

please check and tell if any changes are required

[bkd-dotcom]

Thanks for picking this up, and sorry for the slow turnaround.

I spent a while with this one because the diff is doing more than the title suggests. Short version: nothing here is unsafe — no new dependencies, no workflow/packaging changes, no subprocess/network/eval, and the regexes are all bounded alternations with no catastrophic-backtracking risk. CLA and CodeQL are fine. I have no concerns about running this code.

But I can't squash this as-is, for one substantive reason plus some cleanup. Details are inline; summarising the two that matter:

1. Precision regression (blocker). Adding _PY to _SPECS turns the line-based regex taint engine on for .py, which already gets the full AST pass in _PyVisitor. Because the regex layer only sees "tainted identifier and requests.get( on the same line", it fires HIGH-severity SSRF on constant-URL calls. I checked these against main — all four are clean before this PR and flagged after:

requests.get('https://api.example.com/search', params={'q': user_q})       # py.taint.ssrf
requests.post('https://api.example.com/v1/items', json=user_body)          # py.taint.ssrf
requests.post('https://api.example.com/submit', data=user_form)            # py.taint.ssrf
requests.get('https://api.example.com/me', headers={'Authorization': tok}) # py.taint.ssrf

The URL is hardcoded in every one. That's the single most common shape of HTTP client code in Python, so this would fire constantly on real repos — and precision is the whole pitch of this engine. The test suite's own docstring claims "ZERO FALSE POSITIVES: safe equivalents produce no findings"; CI is green only because nothing covers these shapes.

2. The new tests don't test the new code. I checked out just your tests/test_findings_engine.py onto unmodified main and both new tests pass. urllib.request.urlopen and httpx.get were already in the target tuple before this PR, so neither test reaches any of the ~45 new lines. The kwarg extraction, the .request positional handling, and the entire _PY spec are uncovered.

Worth flagging that this PR expands an existing rule rather than adding one — py.ssrf and test_detects_ssrf are already on main. Not a problem, but the title and #86 read as though SSRF were missing, and that framing is what made the 46 deletions surprising to me on first read.

What I'd need to merge:

1. Drop _PY from _SPECS. Python has an AST pass; the regex layer costs precision without adding recall here. That alone clears the blocker.
2. Fix the URL extraction to check keywords in addition to positionals (see inline) — right now requests.request('GET', url=tainted) is missed by the AST rule.
3. Either make the aiohttp/httpx client-instance patterns actually match, or drop the dead entries.
4. Tests that fail on main. That's the bar for any detection change here.
5. Please split the unrelated refactors and the py.flask_debug message rewrite into their own PR — happy to merge that separately and quickly.

The failing review check is not your fault — it's 403 Resource not accessible by integration; GITHUB_TOKEN is read-only for fork PRs so the advisory reviewer can't post. Ignore it. You will need to rebase on main though, the branch is behind.

Genuinely useful direction and the AST-side instincts are right — it's the regex layer and the test coverage that need another pass.

[AdvaitVarhade]

thank you for the detailed feedback. i have rebased on main and addressed all the review points:

• removed the python regex taint spec completely from lang taint to prevent precision regressions and maintain ast analysis for python
• reverted all unrelated refactors and message changes so the pull request focuses strictly on the python server side request forgery rule
• restored the dispatch table comment
• cleaned up the sink target list by removing unsupported and dead entries
• resolved positional and keyword url arguments independently to handle requests request, httpx request, aiohttp request, and keyword url calls
• added unit tests covering keyword url arguments, dynamic method positions, aiohttp requests, and urllib request objects
• added negative tests verifying zero findings on constant url calls with user supplied query parameters, json bodies, form data, and headers

please check and tell if any changes are required

[bkd-dotcom]

Re-reviewed from scratch against the current head. This is a clean turnaround — you addressed every point, not just the easy subset. Thank you for taking the precision argument seriously rather than arguing it.

Verified rather than read:

The blocker is genuinely gone

lang_taint.py is untouched — gh pr diff --name-only returns only deterministic.py and the tests. The regex layer stays off for .py, and the AST pass remains the single source of truth for Python.

All the constant-URL cases I posted are silent again:

case	main	this PR
requests.get('https://…', params={'q': user_q})	clean	clean ✅
requests.post('https://…', json=user_body)	clean	clean ✅
requests.post('https://…', data=user_form)	clean	clean ✅
requests.get('https://…', headers={'Authorization': tok})	clean	clean ✅
requests.request('GET','https://…', params={'q': user_q})	clean	clean ✅

And it's a real detection win

Five shapes main misses that this catches:

case	main	this PR
requests.get(url=tainted)	miss	caught
requests.request('GET', tainted)	miss	caught
requests.request('GET', url=tainted)	miss	caught
httpx.patch(tainted)	miss	caught
aiohttp.request('GET', tainted)	miss	caught

The requests.request fix is the important one — main was checking args[0], which is the method, so requests.request was effectively dead for positional calls. Resolving keyword and positional URL arguments independently is the right shape for this.

Also confirmed fixed: # noqa: C901 - dispatch table by design restored; the Python-2 urllib.urlopen spelling gone; the py.flask_debug description/remediation rewrite reverted; dead targets removed (I checked every entry in ssrf_targets resolves to a real callable). 252 tests pass, ruff clean, rebased to zero commits behind main.

Merging this

One nit I'm handling myself rather than making you push again.

urllib.request.Request in ssrf_targets adds no detection and one duplicate. Taint already propagates through the assignment, so main catches that flow at the urlopen sink:

d = flask.request.args.get('d')
req = urllib.request.Request(d)      # <- this PR flags here too
urllib.request.urlopen(req).read()   # <- main already flags here

   main:    1 finding   (L5, at the sink)
   this PR: 2 findings  (L4 Request + L5 urlopen)  <- same vuln, reported twice

Reporting the sink once is the behaviour we want, so I'll drop that single entry in a follow-up. Everything else lands exactly as you wrote it.

Credited in CONTRIBUTORS.md. That's your second solid rule for the engine — the Python SSRF coverage is materially better than what was there before.

Separately: I've left a detailed review on #91. Same verdict on safety (nothing unsafe), but it has a structural issue — the deserialisation rule already exists in main, so that PR doubles every finding. Details and a tested patch are over there.

[github-actions]

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]

Merged — thanks @AdvaitVarhade. Credited in CONTRIBUTORS.md.

For the record, what landed:

• requests.request was effectively dead for positional calls on main — it checked args[0], which is the HTTP method, not the URL. Your independent keyword/positional resolution fixed that.
• Five shapes that main missed are now caught: requests.get(url=…), requests.request('GET', …) positional and keyword, httpx.patch, aiohttp.request.
• All five constant-URL cases from the earlier review stay silent, so the precision argument held.

I've opened #94 for the one nit — dropping urllib.request.Request from ssrf_targets, since taint already propagates to the urlopen sink and listing the constructor reported the same SSRF twice on adjacent lines. Your test for that flow still passes unchanged.

Two process notes, both my fault rather than yours:

1. The red review check on your PRs was my bug, not a problem with your code. Fork PRs get a read-only GITHUB_TOKEN, so the reviewer workflow 403'd trying to post its comment — on every outside contribution. Fixed in fix(ci): advisory reviewer could never comment on a fork PR (403) #92 (trusted/untrusted split via workflow_run, deliberately not pull_request_target). Your next PR should get a proper advisory comment instead of a red X.
2. Your PRs also surfaced that the advisory job's exit 0 # never fail the PR had never run — GitHub uses bash -e, so any Block verdict killed the step first. Also fixed in fix(ci): advisory reviewer could never comment on a fork PR (403) #92.

On #91: the review there is substantive but it's a structural issue, not a quality one — three of your four additions (marshal, shelve, and especially Loader=yaml.Loader, which main waves through) are real gaps I'd verified are undetected today. The problem is only that the rule already exists ~30 lines above, so a second block doubles every finding. The offer there stands: push the rewrite, or say the word and I'll apply the patch with you credited.