Skip to content

fix(url-safety): reject RFC 6598 shared address space in strict mode#5474

Merged
StressTestor merged 2 commits into
odysseus-dev:devfrom
StressTestor:fix/url-safety-strict-nonglobal
Jul 18, 2026
Merged

fix(url-safety): reject RFC 6598 shared address space in strict mode#5474
StressTestor merged 2 commits into
odysseus-dev:devfrom
StressTestor:fix/url-safety-strict-nonglobal

Conversation

@StressTestor

@StressTestor StressTestor commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

strict-mode SSRF hardening for the shared URL validator. check_outbound_url(..., block_private=True) is the full SSRF lockdown used for exposed multi-tenant deployments. it rejected is_private and is_loopback targets, but RFC 6598 shared / CGNAT space (100.64.0.0/10) fell through, because CPython does not classify that range as is_private (it is "shared", not "private"). so a public skills.sh-style redirect into 100.64.0.1 passed the per-hop guard and still issued the request to a potentially internal CGNAT service. this change rejects 100.64.0.0/10 explicitly in strict mode.

an explicit range reject (a pure integer-arithmetic membership test) closes exactly the shared-space gap and avoids coupling strict mode to is_global's broader definition, which has shifted across CPython versions for other special ranges. default local-first mode (block_private=False) is unchanged: loopback, LAN, and CGNAT stay reachable for local embedding servers. link-local metadata (169.254.0.0/16) was already covered, so CGNAT was the residual.

Target branch

  • This PR targets dev, not main.

Linked Issue

no separate issue was filed. the shared-space gap was raised by @RaresKeY in the strict-mode SSRF review on #5261; this splits the validator-level fix out from that importer PR so it can land on its own.

Part of #5261

Type of Change

  • Bug fix (non-breaking — fixes a confirmed issue)
  • New feature (non-breaking — adds new behaviour)
  • Breaking change (changes or removes existing behaviour)
  • Refactor / cleanup (behaviour unchanged)
  • Documentation only
  • CI / tooling / configuration

Checklist

  • I searched open issues and open PRs — this is not a duplicate.
  • This PR targets dev
  • My changes are limited to the scope described above — no unrelated refactors or whitespace changes mixed in.
  • I actually ran the app (docker compose up or uvicorn app:app) and verified the change works end-to-end. Type-checks and unit tests are not enough. (left unchecked on purpose: this is a pure validator function with no runtime surface a full app-run exercises beyond the injected-resolver tests, and the resolver injection is exactly how the DNS path is driven deterministically. verified at that level plus a revert-to-RED proof, see How to Test.)

How to Test

the validator is DNS-dependent, so the tests inject a resolver and never touch real network. conftest.py pulls the full app stack, so run this stdlib-only module directly with --noconftest:

  1. python3 -m pytest tests/test_url_safety.py -q --noconftest -> 13 passed
  2. the fix is load-bearing (revert-to-RED): git checkout dev -- src/url_safety.py && python3 -m pytest tests/test_url_safety.py::test_strict_mode_blocks_cgnat_shared_space -q --noconftest -> fails on assert ok is False. restore the file -> passes again.
  3. manual check:
python3 -c "from src.url_safety import check_outbound_url as c; \
print('strict CGNAT :', c('http://x', block_private=True,  resolver=lambda h: ['100.64.0.1'])); \
print('strict public:', c('http://x', block_private=True,  resolver=lambda h: ['8.8.8.8'])); \
print('default CGNAT:', c('http://x', block_private=False, resolver=lambda h: ['100.64.0.1']))"
# strict CGNAT : (False, 'private/shared/loopback address blocked: 100.64.0.1')
# strict public: (True, 'ok')
# default CGNAT: (True, 'ok')   # local-first mode unchanged

Visual / UI changes

N/A. no UI or rendering changes. touches only src/url_safety.py (validator logic) and tests/test_url_safety.py.

…mode

Strict mode (block_private=True) is a full SSRF lockdown, but it only
rejected is_private and is_loopback targets. CPython does not classify RFC
6598 shared/CGNAT space (100.64.0.0/10) as is_private (it is "shared", not
"private"), so a public redirect into 100.64.0.1 passed the per-hop guard
and still issued the request to a potentially internal CGNAT service.

not is_global would also exclude it, but only on CPython 3.11.10+/3.12.4+/
3.13+; the CI matrix runs 3.11/3.12, so reject the range explicitly to stay
correct across patch levels and the 3.14 runtime image. Default local-first
mode is unchanged. Adds strict-mode coverage for shared, non-global, and
public targets.
@github-actions github-actions Bot added needs work PR description incomplete — please update before review ready for review Description complete — ready for maintainer review and removed needs work PR description incomplete — please update before review labels Jul 12, 2026
@StressTestor StressTestor changed the title security(url-safety): reject RFC 6598 shared address space in strict mode fix(url-safety): reject RFC 6598 shared address space in strict mode Jul 12, 2026
…mment

The prior comment claimed `not is_global` catches 100.64.0.0/10 only on
CPython 3.11.10+/3.12.4+/3.13+. That is inaccurate for CGNAT: is_global
is False for 100.64.0.1 on every supported version (verified 3.10-3.14).
The version-fragility applies to other ranges gh-113171 touched, not CGNAT.
The explicit range reject is still the right choice; restate the reason as
is_private not covering shared space, and not coupling strict mode to
is_global's broader, cross-version definition. No behavior change.
@StressTestor

Copy link
Copy Markdown
Collaborator Author

@RaresKeY If I could get a review of this one as well that'd be great, appreciate it.

@RaresKeY RaresKeY 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.

I checked the latest head's strict URL-classification change and did not find an author-actionable issue.

Validation

  • Ran: focused URL-safety and strict redirect coverage, including RFC 6598 bounds, IPv4-mapped values, and mixed resolver answers.
  • Not run: live DNS/HTTP behavior or a full multi-version runtime matrix.

@StressTestor
StressTestor merged commit 37a831b into odysseus-dev:dev Jul 18, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review Description complete — ready for maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants