-
Notifications
You must be signed in to change notification settings - Fork 0
Revalidate fetch redirects against network policy (#647) #667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| # Architectural decision record (ADR) 020: Revalidate every fetch redirect | ||
|
|
||
| ## Status | ||
|
|
||
| Accepted. | ||
|
|
||
| ## Date | ||
|
|
||
| 2026-09-02. | ||
|
|
||
| ## Context and issue | ||
|
|
||
| `fetch()` evaluates the caller-supplied URL against `NetworkPolicy`, which | ||
| limits schemes and hosts before opening a connection. The HTTP client formerly | ||
| followed redirects itself, so an allowed origin could redirect a manifest fetch | ||
| to a link-local address, blocked host, or non-allowlisted host without another | ||
| policy decision. That gap turns a permitted request into a server-side request | ||
| forgery opportunity. Issue #647 requires the least-privilege policy to cover | ||
| each outbound hop rather than only the initial URL. | ||
|
|
||
| ## Decision | ||
|
|
||
| Disable ureq's automatic redirects and follow redirects in the fetch adapter. | ||
| Before every redirected connection, resolve `Location` relative to the current | ||
| URL, remove URL credentials when the origin changes, and evaluate the resolved | ||
| target against `NetworkPolicy`. The adapter accepts at most five redirects and | ||
| rejects a repeated target. | ||
|
Comment on lines
+26
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The checked AGENTS.md reference: AGENTS.md:L52-L53 Useful? React with 👍 / 👎. |
||
|
|
||
| The cache identity remains the original caller-supplied URL. A cache miss | ||
| validates every redirect hop before its response body is written under that | ||
| original key. A cache hit opens no outbound connection; the original URL is | ||
| still evaluated before the entry is read. | ||
|
|
||
| ## Rationale | ||
|
|
||
| - **Policy is an outbound-hop invariant.** Checking the target before each | ||
| request ensures a redirect cannot bypass scheme, allowlist, blocklist, or | ||
| missing-host checks. An HTTPS-to-HTTP downgrade is therefore rejected unless | ||
| `http` is explicitly allowed and its host passes the same policy. | ||
| - **Manual handling makes ordering auditable.** Disabling ureq redirects makes | ||
| the policy check visibly precede every redirected network operation. | ||
| - **The loop is finite and deterministic.** Relative `Location` values resolve | ||
| against the preceding URL, five accepted redirects is the upper bound, and a | ||
| repeated resolved URL produces a loop error rather than another request. | ||
| - **Telemetry and diagnostics stay redacted.** Redirect decisions emit only | ||
| operation, outcome, reason, and hop fields. They never emit a location, URL, | ||
| host, or userinfo. Error URLs remove userinfo before localization, following | ||
| ADR-009's bounded-redaction contract. | ||
|
|
||
| ## Consequences | ||
|
|
||
| - Redirect responses without `Location`, invalid locations, policy rejections, | ||
| loops, and over-limit chains now have distinct localized diagnostics. | ||
| - GET remains the request method at every accepted hop. No caller-derived | ||
| headers are configured on redirected requests, and cross-origin URL | ||
| credentials are stripped before the next request. | ||
| - Redirected responses have one cache entry per original fetch URL, not one per | ||
| final destination. Cached and uncached cache-miss paths therefore apply the | ||
| same hop policy before a body can be stored. | ||
|
|
||
| ## Alternatives considered | ||
|
|
||
| - **Retain ureq automatic redirects.** Rejected because the default redirect | ||
| handler has no Netsuke policy callback before each destination connection. | ||
| - **Check only a final response URL.** Rejected because the disallowed request | ||
| has already occurred by the time a final URL is available. | ||
| - **Use redirect destinations as cache keys.** Rejected because callers request | ||
| the original URL and an allowed endpoint can legitimately change its final | ||
| location. Recording the original request as the identity preserves existing | ||
| cache semantics without allowing an unchecked hop. | ||
|
|
||
| ## Implementation references | ||
|
|
||
| - Redirect adapter and redacted diagnostics: | ||
| [`src/stdlib/network/redirect.rs`](../src/stdlib/network/redirect.rs) | ||
| - Policy evaluation: | ||
| [`src/stdlib/network/policy/mod.rs`](../src/stdlib/network/policy/mod.rs) | ||
| - Original-URL cache key: | ||
| [`src/stdlib/network/cache.rs`](../src/stdlib/network/cache.rs) | ||
| - Two-server and cache coverage: | ||
| [`tests/std_filter_tests/network_redirect_tests.rs`](../tests/std_filter_tests/network_redirect_tests.rs) | ||
| and | ||
| [`src/stdlib/network/redirect_tests.rs`](../src/stdlib/network/redirect_tests.rs) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,6 +148,8 @@ operator, user, and contributor references are easier to find. | |
| - [ADR-019](adr-019-structured-command-shell-selection.md): Allow-listed | ||
| structured-command shell selection, trusted configuration authority, | ||
| resolution, lowering, diagnostics, and safety boundaries. | ||
| - [ADR-020](adr-020-revalidate-fetch-redirects.md): Redirect policy decision | ||
| record, making network policy an invariant of every outbound fetch hop. | ||
|
Comment on lines
+151
to
+152
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Assign a unique ADR number to
🤖 Prompt for AI Agents |
||
|
|
||
| ## Proposals | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1864,7 +1864,34 @@ Implementation details: | |
| `--fetch-allow-scheme <SCHEME>`, declare explicit host allowlists via | ||
| `--fetch-allow-host <HOST>` and `--fetch-default-deny`, and block individual | ||
| hosts through `--fetch-block-host <HOST>`. Policy failures abort before a | ||
| network call and leave the template marked pure. | ||
| network call and leave the template marked pure. Redirect handling applies | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Limit the purity statement to the initial URL check. When a redirect target fails policy, the initial hop has already been sent and Cross-file evidence: Clarify initial and redirect policy failures-Policy failures abort before a network call and leave the template marked pure. Redirect handling applies
+Initial-URL policy failures abort before a network call and leave the template marked pure. Redirect handling applies
...
+Redirect-target policy failures occur after the initial hop and leave the template marked impure.🤖 Prompt for AI Agents |
||
| the same policy before every outbound hop; the decision, including bounded | ||
| redirect handling and cache identity, is recorded in | ||
| [ADR-020](adr-020-revalidate-fetch-redirects.md). | ||
|
|
||
| For screen readers: `fetch` dispatches the current hop until it receives a | ||
| non-redirect response. For a redirect, it resolves the location and rejects a | ||
| missing or invalid location. It then rejects a target that fails policy, has | ||
| already appeared in the chain, or would exceed the five-hop limit; only an | ||
| allowed unseen target becomes the next current hop. | ||
|
|
||
| ```mermaid | ||
| stateDiagram-v2 | ||
| [*] --> CurrentHop | ||
| CurrentHop --> FinalResponse: non-redirect response | ||
| CurrentHop --> ResolveLocation: redirect response | ||
| ResolveLocation --> Reject: missing or invalid Location | ||
| ResolveLocation --> CheckTarget: resolved target | ||
| CheckTarget --> Reject: NetworkPolicy rejects | ||
| CheckTarget --> Reject: repeated target | ||
| CheckTarget --> Reject: five-hop limit reached | ||
| CheckTarget --> CurrentHop: allowed unseen target | ||
| FinalResponse --> [*] | ||
| Reject --> [*] | ||
| ``` | ||
|
|
||
| *Figure: Policy-checked `fetch` redirect state transitions.* | ||
|
|
||
| - `manifest::from_path` derives the workspace root from the manifest file's | ||
| directory before registering the stdlib. This keeps caches scoped to the | ||
| manifest tree even when the CLI evaluates a manifest from another working | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new ADR records a substantive architectural decision, but the commit only links it from the documentation index and updates the security audit; the primary
docs/netsuke-design.mdremains unaware of the redirect-handling boundary. Add a reference from the relevant network-design section so readers following the project's design source of truth can discover the decision.AGENTS.md reference: AGENTS.md:L48-L51
Useful? React with 👍 / 👎.