Merge pull request #2049 from Open-Source-Legal/claude/hopeful-davinci-s5jd4h

Harden authority Phase-4 SSRF guard + address review (issue #2026)

Author: JSv4

changelog.d/2049-cgnat-ssrf.security.md

@@ -0,0 +1,66 @@
+- **Closed a CGNAT SSRF gap in the authority-source fetch guard (issue #2026).**
+  `opencontractserver/utils/safe_http.py::_assert_public_ip` rejected resolved
+  addresses via the `ipaddress` property denylist (`is_private` / `is_loopback`
+  / `is_link_local` / `is_multicast` / `is_reserved` / `is_unspecified`), but
+  RFC 6598 Carrier-Grade NAT shared address space (`100.64.0.0/10`) is
+  classified as **neither** private **nor** reserved **nor** global on current
+  CPython — verified `is_private == is_reserved == is_global == False` on 3.11
+  and 3.12 — so a host resolving into that block would have passed validation
+  and been fetched. It is now rejected explicitly and version-independently via
+  a `_CGNAT_NETWORK` membership check, with the CIDR pinned in
+  `opencontractserver/constants/safe_http.py::CGNAT_SHARED_ADDRESS_SPACE_CIDR`.
+  The `.gov` host allowlist already made exploitation hard in practice (an
+  attacker would need an allowlisted host to resolve into the block), but the
+  IP guard is defence-in-depth and is also reachable by callers that pass a
+  custom `allowlist`. Regression coverage in
+  `opencontractserver/tests/test_safe_http.py` rejects the CGNAT block and
+  proves the adjacent public addresses (`100.63.255.255`, `100.128.0.0`) still
+  pass.
+- **Closed an IPv4-mapped IPv6 bypass of the same guard.** `_assert_public_ip`
+  now unwraps an IPv4-mapped IPv6 address (`::ffff:a.b.c.d`) to its embedded
+  IPv4 before the property/CGNAT checks. On CPython 3.11 the IPv6
+  `is_private` / `_CGNAT_NETWORK` checks do not reflect the mapped IPv4 for the
+  CGNAT-mapped form, so a resolver returning `::ffff:100.64.0.1` would have
+  slipped past every check; unwrapping makes the guard version-independent for
+  the mapped forms of private/loopback/link-local/CGNAT addresses. The CGNAT
+  membership test is additionally guarded by `isinstance(ip, IPv4Address)` so a
+  native IPv6 address is skipped rather than relying on `IPv6Address in
+  IPv4Network` returning `False` (true only on CPython 3.11+; 3.10 raises
+  `TypeError`). Covered by parametrized regressions in `test_safe_http.py`
+  (mapped forms rejected; public native IPv6 passes). Other IPv6-embedded-IPv4
+  forms (NAT64 `64:ff9b::/96` + RFC 8215 `64:ff9b:1::/48`, 6to4 `2002::/16`,
+  Teredo `2001:0::/32`, deprecated IPv4-compatible `::/96`) need no special
+  handling — CPython already flags those whole prefixes `is_private`/
+  `is_reserved` on 3.11 and 3.12 — and `test_ipv6_embedded_ipv4_tunnels_rejected`
+  now pins that coverage so a future Python change couldn't silently open the
+  hole.
+- **Strip per-service credentials on cross-host redirects in `safe_fetch_bytes`.**
+  Request headers are now dropped of `Authorization` / `Cookie` /
+  `Proxy-Authorization` (`CROSS_HOST_STRIPPED_HEADERS`) when a redirect crosses to
+  a different host (RFC 9110 §15.4). httpx — unlike browsers / `requests` —
+  forwards request headers verbatim across origins, so without this a future
+  caller passing a `.gov` API credential could leak it from one allowlisted host
+  to another (e.g. `ecfr.gov` → `federalregister.gov`) while following a redirect.
+  No current caller passes credentials, so this is forward-looking hardening; the
+  default `User-Agent` is preserved across the hop. The cross-origin test compares
+  `netloc` (host **and** port), so a same-host/different-port redirect
+  (`ecfr.gov` → `ecfr.gov:9000`, a different service) also strips. Covered by
+  `TestSafeFetchBytesCredentialStripping` (cross-host, cross-port, same-host).
+- **`_assert_public_ip` now fails CLOSED on an empty DNS result.**
+  `socket.getaddrinfo` can return an empty list **without** raising `gaierror` on
+  some resolver configs; the per-address loop would then be a no-op and the host
+  declared safe (fail-open) while httpx still resolves independently at connect
+  time. It now raises `SSRFValidationError` when no addresses resolve. Covered by
+  `test_empty_getaddrinfo_rejected`.
+- **Redirect-loop robustness in `safe_fetch_bytes`.** The hop check now keys off
+  `r.has_redirect_location` rather than `r.is_redirect` — in httpx `is_redirect`
+  is true for *any* 3xx, so a `304 Not Modified` (or any non-`Location` 3xx) was
+  treated as a redirect, resolved `Location: ""` back to the current URL, and
+  looped to the redirect cap with a misleading "exceeded N redirects". A present-
+  but-empty `Location:` now fails fast with a clear error, and a negative
+  `Content-Length` (e.g. `-1`, which parsed cleanly and slipped the `> max_bytes`
+  guard) is rejected as malformed. None are SSRF bypasses (the loop is bounded
+  and the streamed-bytes cap is the real backstop), but they remove wasted hops
+  and misleading diagnostics under server misbehaviour. Covered by
+  `test_empty_location_header_rejected`, `test_non_location_3xx_not_followed_as_redirect`,
+  and `test_negative_content_length_raises`.

changelog.d/2049-phase4-review.changed.md

@@ -0,0 +1,61 @@
+- **Authority discovery Phase-4 code-review follow-ups (issue #2026).**
+  - `opencontractserver/utils/safe_http.py::safe_fetch_bytes` now sends a
+    default `User-Agent` (`DEFAULT_USER_AGENT`) so the US Code title-ZIP loader
+    and the agentic fetch tool identify OpenContracts to `.gov` servers instead
+    of going out as an anonymous `httpx` client (politeness + fewer rate-limit
+    blocks); a caller-supplied `User-Agent` (the FR/CFR providers) still
+    overrides it. This applies to **every** no-`headers` caller of
+    `safe_fetch_bytes`/`safe_fetch_text`, not only the providers — notably
+    `enrichment/services/popular_name_importer.py`, which now sends
+    `DEFAULT_USER_AGENT` instead of the bare `httpx` default (no test asserts on
+    the old UA string, so nothing regresses). The two byte-identical `_USER_AGENT` literals in
+    `cfr_provider.py` / `federal_register_provider.py` are consolidated into a
+    single `AUTHORITY_PROVIDER_USER_AGENT` constant (same `constants/safe_http.py`
+    neighbourhood) so the contact address can no longer drift between them, and
+    `us_code_provider.py` (the third deterministic authority provider, previously
+    sending the generic default) now sends the same UA so all three present
+    consistently to `.gov` hosts. The default/caller merge uses `httpx.Headers`
+    (case-insensitive), so a caller passing `user-agent` in any casing overrides
+    the default rather than emitting two conflicting `User-Agent` lines.
+    `AUTHORITY_PROVIDER_USER_AGENT` also gains a `+` before its info URL
+    (`(+https://github.com/...)`, the conventional "informational URL" marker)
+    to match `DEFAULT_USER_AGENT` — a cosmetic change to the UA bytes the
+    FR/CFR/US Code providers send to `.gov` hosts, with no behavioural impact.
+  - Replaced the bare `* 4` UTF-8 worst-case byte factor in
+    `opencontractserver/pipeline/authority_source_providers/agentic_web_locator_provider.py`
+    with the named `UTF8_MAX_BYTES_PER_CHAR` constant
+    (`opencontractserver/constants/safe_http.py`).
+  - Removed the unused `source_url` override parameter from
+    `AuthorityGateService.evaluate`
+    (`opencontractserver/enrichment/services/authority_gate_service.py`): no
+    caller passed it and no test exercised it, so it was speculative API surface
+    over a single source of truth (the domain gate reads
+    `sections[0].source_url`).
+  - Moved the agentic provider's private `_sanitize_for_prompt` helper into
+    `opencontractserver/utils/prompt_sanitization.py` as the public
+    `sanitize_for_prompt_strict` (its stricter, ASCII-only companion to the
+    existing `sanitize_plaintext_for_prompt`), so this security helper is
+    discoverable alongside the other prompt-injection utilities rather than
+    buried in a provider file. Tests moved to `test_prompt_sanitization.py`.
+  - Hardened the Federal Register provider's step-1 redirect parsing: the
+    document-number capture in `_LOCATION_DOC_NUMBER_RE` is now `[\d-]+` (real FR
+    numbers are `YYYY-NNNNN`), so a malformed/attacker-influenced `Location`
+    carrying letters, underscores, or URL-special characters (`?`, `#`) fails to
+    match and raises instead of silently being interpolated into the step-2 URL
+    (wrong endpoint, no error). The step-1 `requests.get` magic `timeout=15` is
+    replaced with the shared `(CONNECT_TIMEOUT_SECONDS, READ_TIMEOUT_SECONDS)`
+    constants.
+  - Documented the deliberate trust boundary on the agentic
+    `_tool_web_search` query (LLM-generated, forwarded unsanitized to a
+    text-only search tool — no SSRF surface), why `AuthorityGateService` does
+    not extend `BaseService` (stateless, no user context), and why the redirect
+    loop in `safe_fetch_bytes` does not `r.read()` the redirect body (unbounded
+    read that would bypass the per-hop size cap).
+- **New backend test coverage (issue #2026).**
+  `test_safe_http.py` adds the CGNAT rejection + boundary cases, the
+  `MAX_REDIRECTS` exhaustion path, and the default/overridden `User-Agent`
+  behaviour; `test_federal_register_provider.py` adds the step-3 raw-text
+  size-cap `SSRFValidationError`→abstract fallback regression; `test_authority_gate.py`
+  adds the no-colon `canonical_key` heading-fallback edge case; and
+  `test_agentic_web_locator.py` adds `_tool_web_search` delegation/error-propagation
+  coverage.

opencontractserver/constants/safe_http.py

@@ -18,11 +18,58 @@
     }
 )
 
-ALLOWED_SCHEMES: frozenset[str] = frozenset({"https"})  # gov sources are all TLS
+# HTTPS only. ``http://`` is intentionally excluded even for local/test
+# convenience: a downgraded hop is an SSRF/MITM foothold and every allowlisted
+# .gov source serves TLS. Do NOT add "http" here to make a test pass — mock the
+# transport instead (see test_safe_http.py::TestValidateUrlScheme).
+ALLOWED_SCHEMES: frozenset[str] = frozenset({"https"})
 MAX_REDIRECTS: int = 5
 CONNECT_TIMEOUT_SECONDS: float = 5.0
 READ_TIMEOUT_SECONDS: float = 60.0  # OLRC title ZIPs are large
 
+# RFC 6598 Carrier-Grade NAT / shared address space. ``ipaddress`` does NOT
+# classify this block as private/reserved/global on any current CPython
+# (verified False for is_private AND is_reserved on 3.11 and 3.12) — it is simply
+# absent from CPython's ``ipaddress`` ``_private_networks`` list — so the
+# property-based denylist in ``_assert_public_ip`` would let a host resolving
+# here slip through. Rejected explicitly and version-independently; the behaviour
+# is pinned by ``test_safe_http.py::test_cgnat_shared_address_space_rejected``
+# (re-run it to re-verify the gap after a Python upgrade).
+CGNAT_SHARED_ADDRESS_SPACE_CIDR: str = "100.64.0.0/10"
+
+# Identifies OpenContracts to public-domain .gov servers when a caller does not
+# supply its own User-Agent (the FR/CFR providers pass a more specific one that
+# overrides this). Sending a real UA — rather than the bare ``httpx`` default —
+# is polite and reduces the chance of being rate-limited or outright blocked.
+DEFAULT_USER_AGENT: str = (
+    "OpenContracts/1.0 "
+    "(+https://github.com/Open-Source-Legal/OpenContracts; "
+    "contact: opensource@opencontracts.dev)"
+)
+
+# Specific UA the deterministic authority-source providers (Federal Register,
+# eCFR) send, overriding DEFAULT_USER_AGENT. Single source of truth so the two
+# providers cannot drift the contact address / URL apart.
+AUTHORITY_PROVIDER_USER_AGENT: str = (
+    "OpenContracts-authority-provider/1.0 "
+    "(+https://github.com/Open-Source-Legal/OpenContracts; "
+    "contact: opensource@opencontracts.dev)"
+)
+
+# Per-service credential headers stripped when ``safe_fetch_bytes`` follows a
+# redirect to a DIFFERENT host (RFC 9110 §15.4). httpx — unlike browsers and
+# ``requests`` — forwards request headers verbatim across a cross-origin
+# redirect, so a caller-supplied Authorization/Cookie must not leak from one
+# allowlisted .gov service to another. Lowercase for case-insensitive matching.
+#
+# NOTE: this is the standard credential set, NOT an exhaustive safe-list.
+# Non-standard per-service headers (``X-Api-Key``, ``X-Auth-Token``, …) are NOT
+# stripped, so a caller that sends such a header for a specific service must not
+# rely on this set to protect it across a cross-host redirect.
+CROSS_HOST_STRIPPED_HEADERS: frozenset[str] = frozenset(
+    {"authorization", "cookie", "proxy-authorization"}
+)
+
 # Conservative DEFAULT body cap. Most authority fetches (FR JSON, eCFR/FR raw
 # text bodies) are well under this; a constrained worker should never buffer
 # hundreds of MB by default. Callers that genuinely need a larger body (only the
@@ -33,3 +80,9 @@
 # (Title 26, Tax) ships well under 100 MB, so 200 MB is generous headroom while
 # still bounding a runaway download far below the old 500 MB blanket default.
 OLRC_TITLE_ZIP_MAX_BYTES: int = 200 * 1024 * 1024  # 200 MB
+
+# UTF-8 worst-case encodes one Unicode scalar in up to 4 bytes. Callers that cap
+# *characters* (e.g. the agentic locator's ``max_fetch_chars``) multiply by this
+# to derive the *byte* cap passed to ``safe_fetch_*`` — so streaming aborts at
+# the byte ceiling instead of buffering a huge body before truncating to chars.
+UTF8_MAX_BYTES_PER_CHAR: int = 4

opencontractserver/enrichment/services/authority_gate_service.py

@@ -41,6 +41,11 @@ class AuthorityGateService:
        public-domain allowlist (else GATE_BLOCKED_DOMAIN).
     4. At least one section key or heading must match the canonical_key.
     5. If require_approval_for_agentic, park at pending_approval.
+
+    Unlike the other authority services this does NOT extend ``BaseService``:
+    ``evaluate`` is a pure, stateless classmethod with no user context and no
+    ORM access, so the Tier-0 visibility/permission helpers BaseService provides
+    have nothing to operate on here.
     """
 
     @classmethod
@@ -50,17 +55,15 @@ def evaluate(
         canonical_key: str,
         sections: list[AuthoritySection],
         provider_license: str,
-        source_url: str | None = None,
         require_approval_for_agentic: bool = False,
     ) -> GateDecision:
         """Evaluate whether fetched sections may be ingested.
 
         Args:
             canonical_key: The requested authority key (e.g. "usc-15:78j").
             sections: List of AuthoritySection objects returned by the provider.
+                The domain gate checks ``sections[0].source_url``.
             provider_license: The provider's declared license ClassVar.
-            source_url: Optional override for the URL to check against the
-                allowlist. When None, uses sections[0].source_url if available.
             require_approval_for_agentic: When True, gate returns
                 PENDING_APPROVAL for an otherwise-valid result.
 
@@ -74,11 +77,11 @@ def evaluate(
               (fix the provider's license metadata vs. an allowlist/security
               review), so operators can filter on state alone without parsing
               ``reason``.
-            - A missing source URL (``source_url`` None/"" AND no
-              ``sections[0].source_url``) is treated as ``GATE_UNLOCATED`` when
-              sections are present: a result we cannot attribute to an
-              allowlisted domain must NOT bypass the domain gate on its license
-              alone. See ``test_none_source_url_is_unlocated``.
+            - A missing source URL (``sections[0].source_url`` None/"") is
+              treated as ``GATE_UNLOCATED`` when sections are present: a result
+              we cannot attribute to an allowlisted domain must NOT bypass the
+              domain gate on its license alone. See
+              ``test_none_source_url_is_unlocated``.
         """
         # 1) License gate -------------------------------------------------------
         if provider_license != "public-domain":
@@ -99,10 +102,7 @@ def evaluate(
             )
 
         first = sections[0]
-        effective_url = (
-            source_url if source_url is not None else (first.source_url or "")
-        )
-        domain = urlparse(effective_url).hostname or None
+        domain = urlparse(first.source_url or "").hostname or None
 
         # 3) Source-domain allowlist --------------------------------------------
         # A missing/un-parseable source URL means we cannot attribute the result