Repository navigation
fix: send source headers only to the URL's origin across redirects - #74
Conversation
Remote downloads followed redirects with each platform's defaults, which disagreed and leaked credentials. Android's HttpURLConnection (AOSP's OkHttp) refused http → https and https → http redirects (http-error with the 3xx) and sent every source header but Authorization on to other hosts. iOS's URLSession followed https → http, also sent every header but Authorization to other hosts, and dropped Authorization on every redirect, so token-protected sources failed with 401 after a same-origin redirect. Both platforms now follow one RedirectPolicy (Kotlin and Swift): - at most 10 redirects; more fail with network-error; - http → https is followed, https → http (or a target that is not http(s)) is not: http-error with the redirect's status; - the source's headers, Authorization included, go only while every hop stays on the URL's origin (scheme, host and port); after the first hop to another origin, the rest of the chain is sent without them. If-None-Match and If-Modified-Since go on every hop, so a 304 from the target still validates the stored copy; - a refused redirect and too many redirects never show the stored copy: the server answered. Android follows redirects itself (instanceFollowRedirects = false, one connection per hop, the live one reachable by cancel()). iOS rebuilds each redirected request in a per-task URLSessionTaskDelegate, because the request URLSession proposes has already lost Authorization. The headers TSDoc, the sources, offline, troubleshooting and migration guides, ARCHITECTURE.md and api-design.md describe the policy. The fixture server gains /__redirect, /__redirect-loop and ?require= / ?forbid= header checks, and viewer-redirects covers a same-origin redirect, a cross-origin one (localhost → 127.0.0.1) and a loop on both platforms. Behavior change: an http URL that redirects to https on the same host changes origin, so its headers are not sent to the https URL. Send credentials to the https URL directly. Closes #70
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 19.35% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 10 files. (9 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title accurately describes the redirect-header change and uses the required fix: prefix. It exceeds the recommended 50-character length at 66 characters but remains concise and descriptive. Description check The description directly explains the unified redirect policy, header handling, platform changes, documentation, fixtures, and test results. Linked Issues check The PR satisfies the coding requirements in #70. Android and iOS use a 10-hop RedirectPolicy. Same-origin redirects retain caller headers, including Authorization. Cross-origin redirects remove caller…Out of Scope Changes check The changes stay within #70. The platform implementations, tests, redirect fixtures, e2e flow, API documentation, migration guidance, troubleshooting guidance, and architecture documentation all suppo…Security Check No medium-or-higher security vulnerability was introduced. Android disables automatic redirects and applies RedirectPolicybefore each hop; it permits only HTTP(S), rejects HTTPS-to-HTTP, caps the c…Full details: Linked Issues check
-
Explanation
The PR satisfies the coding requirements in
#70. Android and iOS use a 10-hop RedirectPolicy. Same-origin redirects retain caller headers, including Authorization. Cross-origin redirects remove caller headers for the remaining chain. HTTP-to-HTTPS redirects are followed, HTTPS-to-HTTP redirects return http-error, and redirect-limit failures return network-error without stale fallback. Conditional headers persist across hops. The PR adds JVM redirect tests, cross-platform e2e coverage, fixture support, and the required TSDoc and guide updates.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within
#70. The platform implementations, tests, redirect fixtures, e2e flow, API documentation, migration guidance, troubleshooting guidance, and architecture documentation all support the unified redirect policy. No unrelated product behavior or independent feature is demonstrated.
Full details: Security Check
-
Explanation
No medium-or-higher security vulnerability was introduced. Android disables automatic redirects and applies
RedirectPolicybefore each hop; it permits only HTTP(S), rejects HTTPS-to-HTTP, caps the chain at 10, and permanently disables caller headers after any scheme, host, or port change. iOS rebuilds each redirected request with the same policy. Both implementations retain only conditional cache validators across hops, as documented, and source URL/header validation remains in place. The changed fixture code is test-only. No prior security findings or unresolved threads were supplied.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 19.35% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 10 files. (9 skipped: 9 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 ESLint
-
If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.
ESLint install timed out. The project may have too many dependencies for the sandbox.
Warning
Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Summary
Remote downloads followed redirects with each platform's defaults, which disagreed and sent credentials on to other hosts (#70). Both platforms now follow one
RedirectPolicy(Kotlin and Swift):AuthorizationdroppedAuthorizationsentAuthorizationsenthttp-error3xxhttp-error3xxhttp-errorwith the 3xxnetwork-errorIOException)URLError)The "Before: Android" column is AOSP's OkHttp source as read in #70, not a device run of the old build; the "Before: iOS" column was reproduced on the iOS 26.5 simulator (see Testing).
If-None-Match/If-Modified-Sincego on every hop, so a304from the target still validates the stored copy.instanceFollowRedirects = false), one connection per hop;cancel()disconnects the live one.URLSessionTaskDelegate(iOS 15+): URLSession's proposed request has already lostAuthorization, even on the same origin.headersTSDoc, a "Redirects" section in the sources guide, the offline guide, two troubleshooting rows, the migration guide, the README guide table,docs/api.md, ARCHITECTURE.md and api-design.md./__redirect?to=,/__redirect-loopand?require=/?forbid=header checks (401 / 403). Three example entries and the newviewer-redirectsflow cover a same-origin redirect withAuthorization, a cross-origin one (localhost→127.0.0.1, whichadb reversecovers) and a loop.Behavior change: an
http://URL that redirects tohttps://on the same host changes origin, soheadersare not sent to the https URL (as the Fetch standard does withAuthorization). Send credentials to the https URL directly. No opt-in to keep headers across origins; that can wait for a request.Closes #70
Testing
DocumentCacheTest+ newRedirectPolicyTest): same-origin redirect keepsAuthorizationandX-Api-Key; cross-origin drops both, also after the chain comes back; a redirected revalidation sends the ETag and keeps the copy on304; 10 redirects followed, the 11th fails withnetwork-erroreven withallowStale; a redirect that is not followed ishttp-error302; cancelling mid-chain stores nothing. The https cases are pureRedirectPolicytests (the test server has no TLS). Full suite: 310 passed.HttpURLConnection(which also dropsAuthorizationacross hosts and refuses scheme changes), not AOSP's OkHttp. WithinstanceFollowRedirects = falseboth return the 3xx, so the new tests exercise our loop.DocumentCache/SharedDownload/RedirectPolicywith stand-ins for the Nitro types, run on the iOS 26.5 simulator against a local server with two http ports and one https port (test root certificate added to the simulator's keychain). 12 cases: same origin, cross origin, chain coming back, http → https, https → http (http-error302, nothing sent to http, no stale copy), 10 vs 11 redirects, 304 after a redirect, plus the pure policy cases. Unfixed code: 10 of 12 failed, as described in the issue. Fixed: all pass.viewer-redirectspasses on Android (API 37 emulator) and iOS (26.5 simulator); it ran on the fixed build only (the red runs are the JVM tests and the harness). The flows that download remotely still pass on both:viewer-offline,viewer-errors,viewer-sources,viewer-initial,pdf-document. These device runs came before a last cleanup of Android'sSharedDownload.download()(the connection is now closed in onefinally, no behavior change); the JVM suite was re-run after it.bun run typecheck,bun run lint,bun run test(124) pass.Need help on this PR? Tag
@codesmith-botwith what you need.