ADR-0015: how SP42 should fetch citation sources safely#99
Conversation
Settles the source-fetch edge policy asked for in #34 and folds in #60. One guarded fetch edge in a new dep-light sp42-fetch crate over reqwest: SSRF via a custom resolver (resolved-IP is_global check, closes #60 for free), retry/backoff via reqwest-retry, maxlag ruled out (Action-API only), timeouts/UA codified. Net code removed; consolidates two duplicate guarded clients. Proportionality recorded: deploy-layer IMDSv2 + egress are primary, the in-code guard is cheap defense-in-depth. Refs #34, #60, #51. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 161708ec9a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| `reqwest::redirect::Policy::limited(5)`. Per-hop SSRF is enforced by the | ||
| resolver (#3), not by re-checking URL strings, so the bespoke redirect code is |
There was a problem hiding this comment.
When the redirect target is an IP literal such as http://127.0.0.1/ or http://169.254.169.254/, reqwest does not need DNS, and this ADR already notes the resolver may not fire for literal-IP hosts. Replacing the custom redirect policy/manual loop with Policy::limited(5) while relying on the resolver for all per-hop SSRF checks leaves those redirected literals unchecked, reopening the redirect-to-private case that the existing clients explicitly guard. Please keep a per-hop URL/literal check in the redirect path, or otherwise prove literal-IP checks run on every hop.
Useful? React with 👍 / 👎.
| on other 4xx. Exponential backoff with jitter, ~3 attempts, honoring | ||
| `Retry-After` when present (capped to a sane maximum so a hostile/buggy header | ||
| cannot pin the client). This covers both faces, including Citoid's REST |
There was a problem hiding this comment.
The decision promises to honor Retry-After but constrains the implementation to reqwest-retry + reqwest-middleware; in reqwest-retry 0.9, the retry policy only receives request start time and retry count, while the retryable strategy only classifies responses as transient/fatal, so it cannot set the next delay from a response header. If the follow-up implementation uses this ADR as written, 429/503 responses with Retry-After will still use ordinary exponential backoff and may violate Wikimedia pacing. Please name a Retry-After-aware middleware/dependency or record the small custom logic that is still required.
Useful? React with 👍 / 👎.
Building the edge surfaced five corrections, now recorded in an Implementation-notes section: IP-literal hosts bypass the resolver (need a pre-flight + custom redirect policy, not just Policy::limited); reqwest discards the resolver's error detail; the retry strategy must not retry transport errors; reqwest-retry must pin to the 0.7/0.4 line for reqwest 0.12; and the two-face Citoid split is lighter than decision #2 implied. LOC reframed honestly: production is ~break-even (~-45), the win is de-duplication + correctness, not raw line count. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79e346940a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| 5. **The two-face split is lighter than decision #2 implied.** In the server, | ||
| Citoid already rides a *separate* general client, not the source client; in | ||
| the CLI a single guarded client is injected for both, which is correct because | ||
| `en.wikipedia.org` is public (the guard is a no-op there). No `sp42-core` | ||
| verify-signature change was needed. A strict typed two-face split would have | ||
| needed one and was not worth it. |
There was a problem hiding this comment.
In the article-verification path, Citoid does not already use the server’s separate general reqwest::Client: crates/sp42-server/src/citation_routes.rs:245-247 passes the PlainHttpClient source client into verify_page, and crates/sp42-core/src/citation/verify.rs:809-810 uses that same HttpClient for build_citoid_request(...). With this note saying no sp42-core signature change is needed, the follow-up implementation can leave verification Citoid traffic on the guarded source face, contradicting decision #2's trusted Wikimedia face and the retry/guard split this ADR is meant to settle.
Useful? React with 👍 / 👎.
The implementation confirms a net LOC reduction even excluding tests (~-45 production lines), so adding ip_network + reqwest-retry/-middleware still leaves the codebase smaller — and those crates replace hand-rolled security-critical code rather than bloating it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- decision #4: state the custom redirect policy (cap + per-hop literal check), not bare Policy::limited — literal-IP redirect targets skip the resolver and must be checked in the policy (Codex P1). - decision #5: reqwest-retry does NOT honor Retry-After (verified); drop the claim, record a custom-middleware follow-up (Codex P2). - implementation note #5: correct the Citoid wiring — verify_page passes the guarded source client into verify.rs's Citoid call, so Citoid rides the guarded face there; only bare-url repair uses the separate client. Decision #2 is intent, not fully implemented (Codex P2). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Building Retry-After support showed reqwest-retry can't do it, so the edge uses a ~30-line in-execute retry loop instead — which honors Retry-After, drops both retry crates + the reqwest-0.12 pin, and leaves ip_network as the only genuinely new external crate (rand for jitter is already in-tree). Also records the IPv4-mapped-IPv6 guard fix (Codex P1 on #100). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58eb05f59d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| 10. **Dev/test escape hatch preserved, single source of truth.** | ||
| `SP42_FETCH_ALLOW_PRIVATE=1` (for the loopback-serving benchmark harness) | ||
| swaps the guarded resolver for a pass-through one, read once in the factory |
There was a problem hiding this comment.
For SP42_FETCH_ALLOW_PRIVATE=1 cases that use existing loopback/literal URLs such as http://127.0.0.1:..., swapping only the resolver does not affect the pre-flight IP-literal check required by decision #4, so the documented benchmark/test escape hatch would still reject those URLs. Please specify that the same allow-private flag also parameterizes the literal pre-flight and redirect-literal checks, while preserving the non-SSRF limits.
Useful? React with 👍 / 👎.
| way #51 already does — raw loopback servers + reqwest `.resolve()` so a | ||
| floor-passing hostname can reach the test server while the guard stays | ||
| active — asserting redirect-to-private and literal-to-private are both |
There was a problem hiding this comment.
With the resolved-IP guard from decision #3 enabled, resolving a public-looking hostname to a raw loopback server via reqwest .resolve() produces a loopback address that the guarded resolver must reject before the client can reach the stub server. That makes this harness unable to test redirect/size behavior “while the guard stays active”; it needs an injected resolver fixture or an explicitly scoped guard bypass instead.
Useful? React with 👍 / 👎.
schiste
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 390687af5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - **Hand-rolled retry/backoff.** ~100+ lines of security-adjacent state machine | ||
| vs. a maintained crate. Rejected; `backoff` specifically is unmaintained | ||
| (RUSTSEC-2025-0012). |
There was a problem hiding this comment.
Decision #5 and implementation note #4 now require a custom in-execute retry loop specifically so Retry-After is honored, but this alternative still says hand-rolled retry/backoff is rejected in favor of a maintained crate. When the follow-up fetch implementation is driven from the ADR, this contradictory guidance can send it back to the dependency path the ADR just ruled out and lose the required Wikimedia pacing behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4c6f8a727
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| SP42 fetches two kinds of URL while verifying citations: the **arbitrary | ||
| cited-source URL** (any host on the internet, and attacker-influenceable, since | ||
| citation URLs come from wiki content) and the **Wikimedia REST API** (the Citoid | ||
| metadata endpoint). Issue #34 asked for an ADR to settle the robustness + |
There was a problem hiding this comment.
The scoped edge here only covers cited-source URLs and Citoid, but the recovery plan in docs/design-plans/2026-06-25-fetched-but-unusable-source-recovery.md:127-135 says ADR-0015 must settle recovery trigger conditions and guard reuse before Phase 6 Wayback/PDF recovery. If this ADR is accepted as-is, a follow-up can add the Wayback Availability request/snapshot fetches without any decision on live→recover→none ordering, which recoverable reasons are terminal, or whether those archive requests use the source guard; please include that policy in this ADR or keep Piece 2 gated.
Useful? React with 👍 / 👎.
Adds decision #13: the fetched-but-unusable recovery work (Piece 2) is gated on ADR-0015, so fix the policy here — prefer live->recover->none ordering; recoverable (dead link->Wayback #46, PdfBody->extract #52, ViewerShell->per-host adapter #53) vs terminal (NavChromePaywall, generic unusable) keyed off unusable_reason; recovery fetches reuse the guarded sp42-fetch edge (no new stack); and HttpResponse gains a final_url so ViewerShell host-rules match the post-redirect host. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ab26bfc93
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Books snippet, arXiv HTML-twin, …). Terminal (no recovery — abstain): | ||
| `NavChromePaywall` (a paywall/registration wall must not be bypassed) and a | ||
| generic unusable body. Recovery dispatch keys off | ||
| `CitationFinding.unusable_reason`. |
There was a problem hiding this comment.
For dead-link cases, this sentence leaves the follow-up implementation without a key to trigger Wayback recovery: CitationFinding.unusable_reason is intentionally None for Unreachable sources in crates/sp42-citation/src/citation/verify.rs, and only 2xx-but-unusable bodies populate it. If recovery dispatch follows only this field, dead links will skip the Availability API despite the preceding Unreachable → Wayback policy; please specify dispatch as source_unavailable_reason == Unreachable for dead links and unusable_reason only for fetched-unusable bodies.
Useful? React with 👍 / 👎.
A deployment HTTP_PROXY/HTTPS_PROXY would move target-host resolution into the proxy and bypass the SSRF resolver guard. Call no_proxy() on the guarded source client (stays on under the escape hatch — the hatch relaxes only the address guard). Matches schiste's proxy-bypass decision on ADR-0015 (#99). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Decision #13 still requires HttpResponse.final_url for ViewerShell host-matching, but records that the field is added in the Piece 2 change that consumes it — adding a dormant field to the live edge would churn ~60 HttpResponse construction sites workspace-wide for no current reader. Keeps ADR and the #100 implementation (which does not add the field) in agreement. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…#99) PR #99 already claims ADR-0015 (references/adr, rules-compliant fetch edge). ADR numbering is global, so the Wikidata entity-read and statement-write ADRs move to the next free numbers: read -> ADR-0016, write -> ADR-0017. Renames the two files and updates every cross-reference in PRD-0011, the wikidata README, and the platform README index. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012u5MVp3vAN44KhsVg3vroY
2 participants
When SP42 verifies a citation it fetches the cited source URL — any host on the internet, supplied by whoever edited the wiki. This ADR decides how that fetch behaves safely and politely: don't let a wiki-supplied URL reach internal or cloud-metadata addresses (SSRF), time out instead of hanging, retry transient failures, cap response size and redirects, and identify the client to Wikimedia. It also closes #60 (DNS-rebinding) for free, because the chosen SSRF mechanism handles it. ADR only — implementation is the stacked PR #100.
Key decisions:
sp42-fetchcrate, replacing the two near-duplicate guarded clients. Trust is expressed by which DNS resolver is attached, not by separate client types.ip_network::is_global) — which closes SSRF: source fetch doesn't validate resolved IPs (DNS rebinding to private/metadata) #60's DNS-rebinding gap — backed by an IP-literal check and a redirect-hop check (literals skip the resolver).reqwest-retry(5xx/429 +Retry-After);maxlagruled out (Action-API-only, not the REST/Citoid path); timeouts and User-Agent codified.On dependency cost: this adds three crates (
ip_network,reqwest-retry,reqwest-middleware), but the implementation (PR #100) confirms the codebase still gets smaller — a modest net reduction even excluding tests (≈ −45 production lines; ≈ −300 with tests) — and those crates replace hand-rolled security-critical code (CIDR classification, retry/backoff) rather than bloating it.The implementation was built as a ground-truth check and corrected this ADR in five places — see its Implementation-notes section.
Refs #34, #60, #51.
🤖 Generated with Claude Code