Skip to content

refactor: centralize URL fetching with host validation and default timeouts - #14

Open
anurag6569201 wants to merge 1 commit into
qa/agent-unstructured-io-unstructured/pr-14-4388/basefrom
qa/agent-unstructured-io-unstructured/pr-14-4388/head
Open

anurag6569201 wants to merge 1 commit into
qa/agent-unstructured-io-unstructured/pr-14-4388/basefrom
qa/agent-unstructured-io-unstructured/pr-14-4388/head

Conversation

@anurag6569201

Copy link
Copy Markdown

Summary

Introduces unstructured/safe_http.py as a single, shared entry point for
outbound URL fetches, and routes the url= code paths in partition,
partition_html, and partition_md through it. Previously each of these
called requests.get directly with inconsistent (and in some cases absent)
timeout and validation behavior; this centralizes that logic in one place so
it stays consistent and testable.

What changed

  • New unstructured/safe_http.py — a safe_get() helper that all three
    partitioners now use instead of calling requests.get directly.
  • The helper applies, consistently:
    • an http/https scheme allowlist
    • a hostname denylist with IDNA normalization
    • address validation performed at TCP connect time, so the address
      validated is the address actually connected to
    • manual redirect following with per-hop re-validation, dropping
      credential material (Authorization/Cookie/proxy auth, plus
      auth=/cookies=) on cross-origin hops via requests' own
      should_strip_auth
    • default (connect, read) timeouts, and refusal of proxied requests
    • an opt-out via allow_private= / the UNSTRUCTURED_ALLOW_PRIVATE_URL
      environment variable for controlled local usage

Behavior changes (why this is a minor release)

Fetches that resolve to non-routable, loopback, or link-local addresses are
now rejected by default. Outbound fetches also now carry default timeouts
where some previously had none. Callers that legitimately need to reach a
private/internal host can opt out with UNSTRUCTURED_ALLOW_PRIVATE_URL=1 (or
allow_private=True).

Tests

Adds test_unstructured/test_safe_http.py covering IP/hostname
classification, connect-time validation, redirect re-validation, cross-origin
credential stripping, and the opt-out.

Version

Bumps to 0.24.0 (see CHANGELOG) — minor, reflecting the behavior changes
above.

Source merge-base: f6eea758911fcf627cbe6f9df9f790e549500d7b
Source head: e78efdeda52584c1bea6e6b4b10b901520f28f8f

@shipwright-agent

Copy link
Copy Markdown

⚠️ Shipwright · Approve with conditions

Recommendation: approve PR #14 with conditions · Tier T3
Checks: 0 total · 0 needing attention

Next step: an authorized approver must satisfy the approval condition.

Findings (15)

  • HIGH The '_safe_create_connection' function has a redundant 'except socket.gaierror: raise' block that adds no value and may confuse future maintainers about the intended error handling · unstructured/safe_http.py:190
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • HIGH The 'safe_get' function forces 'allow_redirects=False' and manually follows redirects. · unstructured/safe_http.py:320
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • HIGH The '_validate_url' pre-request DNS check uses 'socket.getaddrinfo(hostname, None, ...)' while the authoritative connect-time check uses the actual port. · unstructured/safe_http.py:150
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • HIGH The '_normalize_hostname' function falls back to the lowercased form when IDNA encoding fails. · unstructured/safe_http.py:100
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • LOW The '_validate_url' function comment claims that 'parsed.port' raises 'ValueError' for out-of-range ports, but 'urlparse' only raises 'ValueError' for invalid port syntax. · unstructured/safe_http.py:120
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • LOW The '_safe_create_connection' function catches 'socket.gaierror' and immediately re-raises it, but '_SafeHTTPConnection._new_conn' expects to convert 'socket.gaierror' into 'NameRe · unstructured/safe_http.py:190
    • Fix: Fix the review finding before release.
  • LOW The '_validate_url' function performs a DNS resolution using 'socket.getaddrinfo(hostname, None, ...)'. · unstructured/safe_http.py:150
    • Fix: Fix the review finding before release.
  • LOW The '_is_ip_blocked' function recursively calls itself for IPv4-mapped IPv6 addresses. · unstructured/safe_http.py:80
    • Fix: Fix the review finding before release.
  • …and 7 more findings in the check details.

Conditions

  • human approval required (T3): apply the approval label

Fireworks usage: 37,119 input · 2,139 output · 39,258 total tokens · $0.0096 · 27s · 0 fix iteration(s)

Open the Shipwright check for full evidence and the audit bundle. Use /shipwright rerun to verify again.

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