Revalidate fetch redirects against network policy (#647) - #667
Conversation
|
Warning Review limit reachedNext included review available in 13 minutes. View limit detailsLimit details: You’ve used the included review currently available. This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (53)
Comment |
Reviewer's GuideFetch now follows redirects manually so every outbound hop is resolved, policy-checked before connection, bounded, loop-safe, credential-sanitized, and redacted; cache identity remains tied to the original URL, with fixture and integration tests documenting the security and behavioral guarantees. Sequence diagram for policy-checked fetch redirectssequenceDiagram
participant Fetch as fetch()
participant Adapter as redirect::dispatch_request
participant Policy as NetworkPolicy
participant Server as HTTP server
Fetch->>Adapter: dispatch_request(url, policy, impure)
Adapter->>Policy: evaluate(original_url)
Policy-->>Adapter: allowed
Adapter->>Server: GET original_url
Server-->>Adapter: redirect response with Location
Adapter->>Adapter: Url::join(location)
Adapter->>Adapter: redact_cross_origin_userinfo()
Adapter->>Policy: evaluate(redirect_target)
alt target allowed
Policy-->>Adapter: allowed
Adapter->>Server: GET redirect_target
Server-->>Adapter: final response
Adapter-->>Fetch: response body
else target rejected
Policy-->>Adapter: violation
Adapter-->>Fetch: redirect_disallowed error
end
State diagram for bounded fetch redirect chainsstateDiagram-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 --> [*]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
b18ba9a to
7d7e6df
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
7d7e6df to
aa320d5
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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: aa320d52ef
ℹ️ 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".
| target against `NetworkPolicy`. The adapter accepts at most five redirects and | ||
| rejects a repeated target. |
There was a problem hiding this comment.
The checked docs/users-guide.md network section still describes only the initial fetch() policy, despite this change adding per-hop policy checks, a five-hop limit, loop rejection, credential stripping, and new user-visible diagnostics. Users configuring allowlists cannot determine from the guide why a previously successful redirected fetch now fails, so document this externally observable contract there.
AGENTS.md reference: AGENTS.md:L52-L53
Useful? React with 👍 / 👎.
| forgery opportunity. Issue #647 requires the least-privilege policy to cover | ||
| each outbound hop rather than only the initial URL. | ||
|
|
||
| ## Decision |
There was a problem hiding this comment.
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.md remains 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 👍 / 👎.
| pub fn spawn_http_server_responses( | ||
| responses: impl IntoIterator<Item = HttpResponse>, | ||
| ) -> io::Result<(String, Arc<AtomicUsize>, HttpServer)> { | ||
| spawn_http_server_responses_with_config(responses, HttpServerConfig::from_env()) |
There was a problem hiding this comment.
This introduces a new public test_support helper used across test targets, but a repo-wide documentation search found no architecture, design, or developers-guide entry defining its ownership, permitted call sites, or composition rules. Record that reuse policy in the appropriate indexed document rather than leaving the abstraction contract implicit in its call sites.
AGENTS.md reference: AGENTS.md:L111-L119
Useful? React with 👍 / 👎.
|
|
||
| /// Determine whether a response requires manual redirect handling. | ||
| fn is_redirect(response: &ureq::Response) -> bool { | ||
| (300..400).contains(&response.status()) |
There was a problem hiding this comment.
When an origin returns a non-redirecting 3xx response, such as 300 Multiple Choices without a preferred Location or 304 Not Modified, this broad range sends it through RedirectState::advance() and turns it into a missing-Location error. Those responses were previously returned by the HTTP client rather than followed; limit the manual loop to the redirect statuses it supports (301, 302, 303, 307, and 308) so unrelated 3xx responses do not regress.
Useful? React with 👍 / 👎.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Disable automatic HTTP redirects and validate each resolved destination before opening its connection. Bound redirect chains, redact diagnostics, preserve original-URL cache identity, and cover policy, cache, fixture, and observability paths.
Split fixture request serving from its public API and share direct redirect-rejection setup. Keep observability assertions focused so the security coverage remains readable and passes CodeScene health rules.
aa320d5 to
ccddd6e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Describe per-hop redirect handling and HTTP fixture reuse, and record the state model in the network design. Limit manual redirects to supported HTTP redirect statuses so unrelated 3xx responses are returned unchanged.
ccddd6e to
aa00f2c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
1 participant
Summary
This branch makes
NetworkPolicyan invariant of every outboundfetchhop, preventing an allowed origin from redirecting a manifest request to a
blocked, non-allowlisted, or disallowed-scheme destination. It disables ureq
automatic redirects, validates each resolved target before connection, bounds
chains, detects loops, and redacts redirect diagnostics.
Closes #647.
Review walkthrough
Validation
make check-fmt: passedmake lint: passedmake doc-coverage: passed (99.14%)make test: passed (2,820 tests, 3 skipped, plus doctests)make markdownlint: passedmake nixie: passedReferences
Summary by Sourcery
Enforce NetworkPolicy on every fetch redirect hop before opening the next connection.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: