From 90ce94ccd4deeccc0d67c82bc815465d46ca4ee5 Mon Sep 17 00:00:00 2001 From: Jakub Kasprzyk Date: Wed, 21 Oct 2026 11:30:34 +0200 Subject: [PATCH] fix: send source headers only to the URL's origin across redirects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ARCHITECTURE.md | 18 ++- README.md | 2 +- benchmarks/serve-fixtures.mjs | 22 +++ docs/api-design.md | 21 ++- docs/api.md | 3 + docs/guides/offline-and-caching.md | 4 +- docs/guides/sources-and-loading.md | 14 ++ docs/migrating-from-react-native-pdf.md | 2 +- docs/troubleshooting.md | 2 + example/documents.ts | 32 +++++ example/e2e/run.sh | 2 +- example/e2e/viewer-redirects.yaml | 18 +++ .../margelo/nitro/betterpdf/RedirectPolicy.kt | 51 +++++++ .../margelo/nitro/betterpdf/SharedDownload.kt | 126 +++++++++++------- .../nitro/betterpdf/DocumentCacheTest.kt | 108 ++++++++++++++- .../nitro/betterpdf/RedirectPolicyTest.kt | 64 +++++++++ package/ios/RedirectPolicy.swift | 48 +++++++ package/ios/SharedDownload.swift | 99 +++++++++++--- package/src/source.ts | 4 + 19 files changed, 555 insertions(+), 85 deletions(-) create mode 100644 example/e2e/viewer-redirects.yaml create mode 100644 package/android/src/main/java/com/margelo/nitro/betterpdf/RedirectPolicy.kt create mode 100644 package/android/src/test/java/com/margelo/nitro/betterpdf/RedirectPolicyTest.kt create mode 100644 package/ios/RedirectPolicy.swift diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 4dd43c2..df2489e 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -438,7 +438,8 @@ fetch(source) ──► stored copy fresh for source.maxAgeMs? ──yes── SharedDownload for the key in flight? ──yes──► join it │ no ▼ - GET (conditional with the copy's ETag / Last-Modified) + GET (conditional with the copy's ETag / Last-Modified), + following redirects by RedirectPolicy 304 → mark validated · 200 + %PDF- → rename into place unreachable / 5xx → view: stored copy as stale · PdfCache: error ``` @@ -457,10 +458,21 @@ fetch(source) ──► stored copy fresh for source.maxAgeMs? ──yes── old ones and removal unlinks, so a displayed document keeps reading its file. iOS views open a pin (hard link in `BetterPdf/open`) because they re-open the displayed file by path for outline, page sizes and images. +- **Redirects:** `RedirectPolicy` (Kotlin and Swift, same rules): at most + 10 (then `network-error`), never https → http (`http-error` with the + 3xx), caller headers only until the first hop to another origin; the + conditional headers go on every hop. Neither platform's own handling + fits: AOSP's OkHttp refuses scheme changes and sends every header but + `Authorization` to other hosts, so Android follows redirects itself + (`instanceFollowRedirects = false`, one connection per hop, each one + reachable by `cancel()`); CFNetwork drops `Authorization` on every + redirect and keeps the other headers on every host, so iOS rebuilds each + redirected request in a task delegate. - **Waiting:** one subscriber per view load or `PdfCache.download()`. Cancelling removes the subscriber; the request stops when none remain. -- **Tests:** `DocumentCacheTest` (JVM, local HTTP server) and the - `viewer-offline` e2e flow on both platforms. +- **Tests:** `DocumentCacheTest` (JVM, local HTTP server), + `RedirectPolicyTest`, and the `viewer-offline` and `viewer-redirects` e2e + flows on both platforms. ## Known platform constraints diff --git a/README.md b/README.md index 7082a5e..8bef339 100644 --- a/README.md +++ b/README.md @@ -95,7 +95,7 @@ all props and ref methods; the [guides](#guides) cover each feature. | Guide | Covers | | --- | --- | -| [Sources and loading](docs/guides/sources-and-loading.md) | Remote, local and bundled files, request headers, base64 data, several files as one document, loading state | +| [Sources and loading](docs/guides/sources-and-loading.md) | Remote, local and bundled files, request headers and redirects, base64 data, several files as one document, loading state | | [Navigation and reading position](docs/guides/navigation.md) | Links and page taps; saving and restoring where the reader left off | | [Offline reading and caching](docs/guides/offline-and-caching.md) | Cache modes, revalidation, offline fallback, `PdfCache` | | [Layout and appearance](docs/guides/layout-and-appearance.md) | Facing pages, translucent bars, dark pages | diff --git a/benchmarks/serve-fixtures.mjs b/benchmarks/serve-fixtures.mjs index c833a37..2ca956e 100644 --- a/benchmarks/serve-fixtures.mjs +++ b/benchmarks/serve-fixtures.mjs @@ -13,6 +13,10 @@ // `GET /__control/offline` makes the server drop every other connection // without answering (a device without network, as far as the app can tell) // until `GET /__control/online`. +// +// Redirects: `/__redirect?to=` answers 302 to ``, and +// `/__redirect-loop` redirects to itself. `?require=a,b` answers 401 unless +// the request has all of these headers, `?forbid=a,b` 403 if it has any. import { createReadStream, statSync } from 'node:fs'; import { createServer } from 'node:http'; @@ -61,6 +65,24 @@ createServer((request, response) => { request.socket.destroy(); return; } + if (url.pathname === '/__redirect' || url.pathname === '/__redirect-loop') { + const location = url.searchParams.get('to') ?? url.pathname; + response.writeHead(302, { Location: location }).end(); + console.log(`${request.method} ${url.pathname}${url.search} → ${location}`); + return; + } + const names = (key) => + (url.searchParams.get(key) ?? '').split(',').filter(Boolean); + const status = names('require').some((n) => !(n in request.headers)) + ? 401 + : names('forbid').some((n) => n in request.headers) + ? 403 + : null; + if (status !== null) { + response.writeHead(status).end(); + console.log(`${request.method} ${url.pathname}${url.search} → ${status}`); + return; + } const name = basename(decodeURIComponent(url.pathname)); const file = join(root, name); try { diff --git a/docs/api-design.md b/docs/api-design.md index 4966d2a..5db6b8f 100644 --- a/docs/api-design.md +++ b/docs/api-design.md @@ -1163,6 +1163,20 @@ invalidation, prefetch and offline reading (#952, #964, #572, #566, #689, - **Only PDFs are stored.** A `200` that does not start with `%PDF-` within 1024 bytes (login page, captive portal) fails with `invalid-pdf` instead of being kept — persistently — as the document. +- **Redirects: one policy, ours.** The platforms disagreed and both were + unsafe: Android (AOSP's OkHttp) refused http → https and sent every + header but `Authorization` on to other hosts; iOS (CFNetwork) followed + https → http and also forwarded every header but `Authorization`, which + it dropped even on the same origin. Now both follow at most 10 redirects + (`network-error` after that), follow http → https but refuse https → http + (`http-error` with the 3xx: the server answered), and send the source's + headers only while every hop stays on the URL's origin (scheme, host, + port), as the Fetch standard does with `Authorization`. A token in + `X-Api-Key` or `Cookie` is a credential too, so all headers are dropped, + for the rest of the chain. The conditional headers are ours and always + go, so a `304` from the target still validates the copy. Neither a refused + redirect nor too many redirects shows the stored copy: the server + answered. Not included: an allowlist of hosts that keep the headers. - **One download per copy.** A list prefetching a document and a viewer opening it share one request (`SharedDownload`); cancelling one waiter leaves the others, and the request stops when nobody waits. @@ -1185,9 +1199,10 @@ invalidation, prefetch and offline reading (#952, #964, #572, #566, #689, - **Tested:** JVM tests run the Android cache against a local HTTP server (reuse, 304, changed document, stale fallback, 5xx vs 4xx, non-PDF responses, invalid headers, shared and cancelled downloads, `cacheKey`, - promotion, restart, cleanup). `viewer-offline` runs the same story on both - platforms with the fixture server's simulated outage - (`/__control/offline`), including an app restart. + promotion, restart, cleanup, redirects). `viewer-offline` runs the same + story on both platforms with the fixture server's simulated outage + (`/__control/offline`), including an app restart; `viewer-redirects` + checks the redirect policy on both. ## Short documents, rounded corners, safe area diff --git a/docs/api.md b/docs/api.md index b080992..9f600d6 100644 --- a/docs/api.md +++ b/docs/api.md @@ -129,6 +129,9 @@ pdf.current?.goToViewport(saved); // a position from onViewportChan `invalid-password` · `unsupported-pdf` · `page-render-failed` (`error.page`) · `out-of-memory` (`error.page` while rendering) +A redirect that is not followed is an `http-error` with the redirect's +status; see [Redirects](guides/sources-and-loading.md#redirects). + `error.message` is human-readable and may change; `error.nativeDescription` carries the platform error for logging. Load failures carry `error.sourceIndex`, the source that could not be loaded (`0` for a single diff --git a/docs/guides/offline-and-caching.md b/docs/guides/offline-and-caching.md index 70f300a..e104087 100644 --- a/docs/guides/offline-and-caching.md +++ b/docs/guides/offline-and-caching.md @@ -26,7 +26,9 @@ const source: PdfUriSource = { - **Offline**: when the server cannot be reached (no connection, no answer within 10 seconds, or a 5xx response), the view shows the stored copy and `onLoad` reports `isStale: true`. Without a copy it fails with - `network-error`. A 4xx response is an error even with a copy. + `network-error`. A 4xx response is an error even with a copy, and so are + a redirect that is not followed and more than 10 redirects; see + [Redirects](sources-and-loading.md#redirects). - **`cacheKey`** identifies the copy instead of `uri` and `headers`. Set it when they change between opens — signed URLs, rotating tokens — or the copy is not found again. Sources with the same `cacheKey` are the same diff --git a/docs/guides/sources-and-loading.md b/docs/guides/sources-and-loading.md index aa13543..8dcfe9d 100644 --- a/docs/guides/sources-and-loading.md +++ b/docs/guides/sources-and-loading.md @@ -27,6 +27,20 @@ before its `password` option. break or another control character (tab is fine): such a source fails with `invalid-source` on both platforms, before anything is sent. +### Redirects + +Both platforms follow redirects the same way: + +- At most 10; more fail with `network-error`. +- From http to https, never from https to http or to anything but + http(s): that fails with `http-error`, and `httpStatus` is the + redirect's status (301, 302, 303, 307 or 308). +- `headers` go to the URL's origin (scheme, host and port) only, + `Authorization` included. After a redirect to another origin, the rest of + the download is sent without them, even if a later redirect comes back. + http to https on the same host is another origin too: when you send + credentials, use the https URL. + ## Base64 documents A document you have as a base64 string — made with pdfmake or pdf-lib, or diff --git a/docs/migrating-from-react-native-pdf.md b/docs/migrating-from-react-native-pdf.md index 61e5141..0f623ef 100644 --- a/docs/migrating-from-react-native-pdf.md +++ b/docs/migrating-from-react-native-pdf.md @@ -90,7 +90,7 @@ users see. | --- | --- | --- | | `source={{ uri }}` | `source={{ uri }}` | `http(s)://`, `file://`, `content://` (Android). | | `source={require('./x.pdf')}` | `source={require('./x.pdf')}` | Same; no Metro config needed. | -| `source.headers` | `source.headers` | Same. | +| `source.headers` | `source.headers` | Same, but sent to the URL's origin only: a redirect to another origin drops them. See [Redirects](guides/sources-and-loading.md#redirects). | | `source.cache: true` | `source.cache: 'persistent'` | Kept across launches, never purged by the OS. See [Caching](#caching). | | `source.cacheFileName` | `source.cacheKey` | Identifies the copy instead of the URL and headers. | | `source.expiration` (seconds) | `source.maxAgeMs` (milliseconds) | An older copy is revalidated with the server (304 keeps it) instead of deleted, and still shown offline. | diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index b97f985..76028e0 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -10,6 +10,8 @@ | On Android, `onError` arrives while the document is shown | It has `error.page`: one page could not be rendered after `onLoad` (`page-render-failed`, or `out-of-memory`), and the document stays open. Keep the viewer on screen; only `onError` without `page` ends the load. See [Errors](api.md#errors). | | A remote document fails with `invalid-pdf` | The server answered with something else, often a login page: check the URL and `headers`. | | A remote document fails with `invalid-source` | Check that the URL is `http(s)://` and the `headers`: often a token read with a trailing newline. `error.message` names the header. | +| A remote document fails with `http-error` and a 3xx `httpStatus` | The server redirects from https to http, which is not followed. Use a URL that stays on https. See [Redirects](guides/sources-and-loading.md#redirects). | +| A remote document fails with `http-error` 401 or 403 after a redirect | The server redirects to another origin (another host or port, or http to https), and `headers` are not sent there. Use the final URL, or one the target accepts without your headers, such as a signed URL. | | A signed URL or rotating token downloads the document on every open | Set `cacheKey` on the source to identify the stored copy. | | The document is a base64 string | Pass `source={{ base64 }}` (1.1+); for large documents write it to a file. See [Base64 documents](guides/sources-and-loading.md#base64-documents). | | A base64 source fails with `invalid-source` | `error.message` says why: a `data:` URI prefix to remove, URL-safe base64 (`-`, `_`) to convert, or another character that is not base64. | diff --git a/example/documents.ts b/example/documents.ts index 262a8ed..d44b4a1 100644 --- a/example/documents.ts +++ b/example/documents.ts @@ -7,6 +7,12 @@ import type { /** Served by `node benchmarks/serve-fixtures.mjs` (Android: `adb reverse tcp:8090 tcp:8090`). */ export const FIXTURE_SERVER = 'http://localhost:8090'; +/** The same server as another origin (another host name). */ +const FIXTURE_SERVER_OTHER_ORIGIN = 'http://127.0.0.1:8090'; + +function redirectTo(url: string): string { + return `${FIXTURE_SERVER}/__redirect?to=${encodeURIComponent(url)}`; +} export const FIXTURES = [ 'small', @@ -115,6 +121,32 @@ export const DOCUMENTS: DocumentEntry[] = [ subtitle: 'Expect http-error', resolve: async () => ({ uri: `${FIXTURE_SERVER}/does-not-exist.pdf` }), }, + { + id: 'remote-redirect', + title: 'Redirect, same origin', + subtitle: 'Authorization must reach the target', + resolve: async () => ({ + uri: redirectTo('/medium.pdf?require=authorization,x-api-key'), + headers: { Authorization: 'Bearer e2e', 'X-Api-Key': 'e2e' }, + }), + }, + { + id: 'remote-redirect-other-origin', + title: 'Redirect, other origin', + subtitle: 'Headers must not reach the target', + resolve: async () => ({ + uri: redirectTo( + `${FIXTURE_SERVER_OTHER_ORIGIN}/medium.pdf?forbid=authorization,x-api-key`, + ), + headers: { Authorization: 'Bearer e2e', 'X-Api-Key': 'e2e' }, + }), + }, + { + id: 'remote-redirect-loop', + title: 'Redirect loop', + subtitle: 'Expect network-error', + resolve: async () => ({ uri: `${FIXTURE_SERVER}/__redirect-loop` }), + }, { id: 'remote-forms', title: 'Remote form', diff --git a/example/e2e/run.sh b/example/e2e/run.sh index cda24b1..e6f22db 100755 --- a/example/e2e/run.sh +++ b/example/e2e/run.sh @@ -10,7 +10,7 @@ cd "$(dirname "$0")" device="$1"; shift flows=("$@") if [ ${#flows[@]} -eq 0 ]; then - flows=(viewer-bundled viewer-scroll-zoom viewer-errors viewer-password viewer-layout-modes viewer-fit-policy viewer-single-page viewer-rtl viewer-outline viewer-scroll-enabled viewer-links viewer-double-tap viewer-scale-change viewer-initial viewer-text-selection viewer-accessibility viewer-search viewer-viewport viewer-page-sizes viewer-page-image viewer-offline viewer-short-document viewer-print viewer-spreads viewer-dark viewer-overlays viewer-forms viewer-inset viewer-inset-last-page viewer-base64 viewer-sources pdf-document) + flows=(viewer-bundled viewer-scroll-zoom viewer-errors viewer-password viewer-layout-modes viewer-fit-policy viewer-single-page viewer-rtl viewer-outline viewer-scroll-enabled viewer-links viewer-double-tap viewer-scale-change viewer-initial viewer-text-selection viewer-accessibility viewer-search viewer-viewport viewer-page-sizes viewer-page-image viewer-offline viewer-short-document viewer-print viewer-spreads viewer-dark viewer-overlays viewer-forms viewer-inset viewer-inset-last-page viewer-base64 viewer-sources viewer-redirects pdf-document) fi failed=0 is_android=0 diff --git a/example/e2e/viewer-redirects.yaml b/example/e2e/viewer-redirects.yaml new file mode 100644 index 0000000..f9b7761 --- /dev/null +++ b/example/e2e/viewer-redirects.yaml @@ -0,0 +1,18 @@ +# Redirects follow one policy on both platforms: the source's headers reach +# a target on the same origin (Authorization included) and never one on +# another origin (localhost → 127.0.0.1), and a loop ends with network-error. +# The fixture server answers 401 / 403 when the target sees the wrong headers. +appId: software.gmi.betterpdf.example +--- +- runFlow: _start.yaml +- tapOn: "Viewer" +- runFlow: { file: _open-list-entry.yaml, env: { ENTRY: "Redirect, same origin.*" } } +- extendedWaitUntil: { visible: "1 / 100", timeout: 60000 } +- tapOn: "‹ Back" +- runFlow: { file: _open-list-entry.yaml, env: { ENTRY: "Redirect, other origin.*" } } +- extendedWaitUntil: { visible: "1 / 100", timeout: 60000 } +- tapOn: "‹ Back" +- runFlow: { file: _open-list-entry.yaml, env: { ENTRY: "Redirect loop.*" } } +- extendedWaitUntil: { visible: "network-error", timeout: 30000 } +- assertVisible: ".*more than 10 redirects.*" +- tapOn: "‹ Back" diff --git a/package/android/src/main/java/com/margelo/nitro/betterpdf/RedirectPolicy.kt b/package/android/src/main/java/com/margelo/nitro/betterpdf/RedirectPolicy.kt new file mode 100644 index 0000000..f4e2107 --- /dev/null +++ b/package/android/src/main/java/com/margelo/nitro/betterpdf/RedirectPolicy.kt @@ -0,0 +1,51 @@ +package com.margelo.nitro.betterpdf + +import java.net.MalformedURLException +import java.net.URL + +/** + * The redirects one download follows, the same as iOS's `RedirectPolicy`: at + * most [MAX_REDIRECTS], to `http(s)` only, never from https to http, and the + * caller's headers only until the first hop to another origin. + */ +internal class RedirectPolicy(first: URL) { + sealed interface Next { + data class Follow(val url: URL) : Next + + /** The redirect response is the answer. */ + data object Refuse : Next + + data object TooMany : Next + } + + private val origin = Origin(first) + private var followed = 0 + + var sendsHeaders = true + private set + + fun next(from: URL, location: String?): Next { + val to = try { + URL(from, location ?: return Next.Refuse) + } catch (_: MalformedURLException) { + return Next.Refuse + } + if (to.protocol != "http" && to.protocol != "https") return Next.Refuse + if (from.protocol == "https" && to.protocol == "http") return Next.Refuse + if (followed == MAX_REDIRECTS) return Next.TooMany + followed++ + if (Origin(to) != origin) sendsHeaders = false + return Next.Follow(to) + } + + private data class Origin(val scheme: String, val host: String, val port: Int) { + constructor(url: URL) : this(url.protocol, url.host.lowercase(), if (url.port == -1) url.defaultPort else url.port) + } + + companion object { + const val MAX_REDIRECTS = 10 + + /** The statuses that redirect, as in URLSession. */ + val STATUSES = setOf(301, 302, 303, 307, 308) + } +} diff --git a/package/android/src/main/java/com/margelo/nitro/betterpdf/SharedDownload.kt b/package/android/src/main/java/com/margelo/nitro/betterpdf/SharedDownload.kt index a30e149..2b636ef 100644 --- a/package/android/src/main/java/com/margelo/nitro/betterpdf/SharedDownload.kt +++ b/package/android/src/main/java/com/margelo/nitro/betterpdf/SharedDownload.kt @@ -62,60 +62,42 @@ internal class SharedDownload( private fun download(): DownloadOutcome { val partial = store.newPartialFile(key) try { - val connection = (URL(url).openConnection() as HttpURLConnection).apply { - val timeoutMs = if (existing != null) REVALIDATION_TIMEOUT_MS else TIMEOUT_MS - connectTimeout = timeoutMs - readTimeout = timeoutMs - instanceFollowRedirects = true - // The copies in the store are the cache; an app-installed - // HttpResponseCache must not answer instead of the server. - useCaches = false - headers.forEach { (name, value) -> setRequestProperty(name, value) } - existing?.metadata?.etag?.let { setRequestProperty("If-None-Match", it) } - existing?.metadata?.lastModified?.let { setRequestProperty("If-Modified-Since", it) } + val connection = connect() + val status = connection.responseCode + if (status == HttpURLConnection.HTTP_NOT_MODIFIED && existing != null) { + val validated = CacheMetadata( + validatedAtMs = clock(), + etag = connection.getHeaderField("ETag") ?: existing.metadata.etag, + lastModified = connection.getHeaderField("Last-Modified") ?: existing.metadata.lastModified, + ) + return DownloadOutcome.Stored(store.markValidated(existing, validated)) } - this.connection = connection - if (cancelled) throw InterruptedException() - val metadata: CacheMetadata - try { - val status = connection.responseCode - if (status == HttpURLConnection.HTTP_NOT_MODIFIED && existing != null) { - val validated = CacheMetadata( - validatedAtMs = clock(), - etag = connection.getHeaderField("ETag") ?: existing.metadata.etag, - lastModified = connection.getHeaderField("Last-Modified") ?: existing.metadata.lastModified, - ) - return DownloadOutcome.Stored(store.markValidated(existing, validated)) - } - if (status !in 200..299) { - return DownloadOutcome.Failed( - PdfFailure(PdfErrorCode.HTTP_ERROR, "Downloading $url failed with HTTP $status", httpStatus = status), - isServerUnreachable = status >= 500, - ) - } - // -1 when unknown; also cleared when the response is transparently - // decompressed, so the byte count never exceeds a reported total. - val totalBytes = connection.contentLengthLong.takeIf { it >= 0 } - connection.inputStream.use { input -> - partial.outputStream().use { output -> - val buffer = ByteArray(BUFFER_BYTES) - var loadedBytes = 0L - while (true) { - if (cancelled) throw InterruptedException() - val read = input.read(buffer) - if (read < 0) break - output.write(buffer, 0, read) - loadedBytes += read - onProgress(this, loadedBytes, totalBytes?.takeIf { it >= loadedBytes }, false) - } - onProgress(this, loadedBytes, totalBytes?.takeIf { it >= loadedBytes }, true) + if (status !in 200..299) { + val reason = if (status in RedirectPolicy.STATUSES) ": the redirect is not followed" else "" + return DownloadOutcome.Failed( + PdfFailure(PdfErrorCode.HTTP_ERROR, "Downloading $url failed with HTTP $status$reason", httpStatus = status), + isServerUnreachable = status >= 500, + ) + } + // -1 when unknown; also cleared when the response is transparently + // decompressed, so the byte count never exceeds a reported total. + val totalBytes = connection.contentLengthLong.takeIf { it >= 0 } + connection.inputStream.use { input -> + partial.outputStream().use { output -> + val buffer = ByteArray(BUFFER_BYTES) + var loadedBytes = 0L + while (true) { + if (cancelled) throw InterruptedException() + val read = input.read(buffer) + if (read < 0) break + output.write(buffer, 0, read) + loadedBytes += read + onProgress(this, loadedBytes, totalBytes?.takeIf { it >= loadedBytes }, false) } + onProgress(this, loadedBytes, totalBytes?.takeIf { it >= loadedBytes }, true) } - metadata = CacheMetadata(clock(), connection.getHeaderField("ETag"), connection.getHeaderField("Last-Modified")) - } finally { - connection.disconnect() - this.connection = null } + val metadata = CacheMetadata(clock(), connection.getHeaderField("ETag"), connection.getHeaderField("Last-Modified")) if (cancelled) throw InterruptedException() if (!partial.hasPdfHeader()) { return DownloadOutcome.Failed( @@ -124,6 +106,8 @@ internal class SharedDownload( ) } return DownloadOutcome.Stored(store.commit(key, partial, mode, metadata)) + } catch (e: PdfFailure) { + return DownloadOutcome.Failed(e, isServerUnreachable = false) } catch (e: MalformedURLException) { return DownloadOutcome.Failed( PdfFailure(PdfErrorCode.INVALID_SOURCE, "Unsupported PDF source: \"$url\"", cause = e), @@ -136,11 +120,53 @@ internal class SharedDownload( isServerUnreachable = true, ) } finally { + this.connection?.disconnect() + this.connection = null // Never leave a partial file behind; a no-op once committed. partial.delete() } } + /** + * Requests [url] and the redirects [RedirectPolicy] follows: the connection + * with the answer. Throws [PdfFailure] after too many redirects. + */ + private fun connect(): HttpURLConnection { + var target = URL(url) + val redirects = RedirectPolicy(target) + while (true) { + val connection = open(target, withHeaders = redirects.sendsHeaders) + if (connection.responseCode !in RedirectPolicy.STATUSES) return connection + when (val next = redirects.next(target, connection.getHeaderField("Location"))) { + RedirectPolicy.Next.Refuse -> return connection + RedirectPolicy.Next.TooMany -> + throw PdfFailure(PdfErrorCode.NETWORK_ERROR, "Downloading $url failed: more than ${RedirectPolicy.MAX_REDIRECTS} redirects") + is RedirectPolicy.Next.Follow -> { + connection.disconnect() + target = next.url + } + } + } + } + + private fun open(target: URL, withHeaders: Boolean): HttpURLConnection { + val connection = (target.openConnection() as HttpURLConnection).apply { + val timeoutMs = if (existing != null) REVALIDATION_TIMEOUT_MS else TIMEOUT_MS + connectTimeout = timeoutMs + readTimeout = timeoutMs + instanceFollowRedirects = false + // The copies in the store are the cache; an app-installed + // HttpResponseCache must not answer instead of the server. + useCaches = false + if (withHeaders) headers.forEach { (name, value) -> setRequestProperty(name, value) } + existing?.metadata?.etag?.let { setRequestProperty("If-None-Match", it) } + existing?.metadata?.lastModified?.let { setRequestProperty("If-Modified-Since", it) } + } + this.connection = connection + if (cancelled) throw InterruptedException() + return connection + } + private companion object { const val BUFFER_BYTES = 64 * 1024 const val TIMEOUT_MS = 30_000 diff --git a/package/android/src/test/java/com/margelo/nitro/betterpdf/DocumentCacheTest.kt b/package/android/src/test/java/com/margelo/nitro/betterpdf/DocumentCacheTest.kt index 2991eed..a4f6cca 100644 --- a/package/android/src/test/java/com/margelo/nitro/betterpdf/DocumentCacheTest.kt +++ b/package/android/src/test/java/com/margelo/nitro/betterpdf/DocumentCacheTest.kt @@ -7,6 +7,7 @@ import java.net.ServerSocket import java.net.Socket import java.nio.file.Files import java.util.concurrent.CompletableFuture +import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.CopyOnWriteArrayList import java.util.concurrent.CountDownLatch import java.util.concurrent.ExecutorService @@ -30,6 +31,9 @@ class DocumentCacheTest { val temp = TemporaryFolder() private lateinit var server: TestServer + + /** Another origin, for redirects. */ + private lateinit var other: TestServer private lateinit var executor: ExecutorService private lateinit var cache: DocumentCache private val clock = AtomicLong(1_000_000) @@ -42,6 +46,7 @@ class DocumentCacheTest { @Before fun setUp() { server = TestServer() + other = TestServer() executor = Executors.newCachedThreadPool() cache = newCache() } @@ -49,6 +54,7 @@ class DocumentCacheTest { @After fun tearDown() { server.stop() + other.stop() executor.shutdownNow() } @@ -224,6 +230,89 @@ class DocumentCacheTest { // endregion + // region Redirects + + @Test + fun `a redirect within the origin keeps the caller's headers`() { + server.redirects["/old.pdf"] = "/doc.pdf" + fetch(source(server.url("/old.pdf"), headers = credentials)).ready() + + assertEquals(listOf("/old.pdf", "/doc.pdf"), server.requests.map { it.path }) + assertEquals("Bearer t", server.requests[1].headers["authorization"]) + assertEquals("secret", server.requests[1].headers["x-api-key"]) + } + + @Test + fun `a redirect to another origin drops the caller's headers for the rest of the chain`() { + server.redirects["/away.pdf"] = other.url("/back.pdf") + other.redirects["/back.pdf"] = server.url("/doc.pdf") + fetch(source(server.url("/away.pdf"), headers = credentials)).ready() + + val sent = listOf(other.requests.single(), server.requests[1]) + assertEquals(listOf("/back.pdf", "/doc.pdf"), sent.map { it.path }) + for (request in sent) { + assertNull(request.headers["authorization"]) + assertNull(request.headers["x-api-key"]) + } + } + + @Test + fun `a redirected revalidation still sends the copy's ETag`() { + server.redirects["/away.pdf"] = other.url("/doc.pdf") + val first = fetch(source(server.url("/away.pdf"), headers = credentials)).ready() + clock.addAndGet(5_000) + val second = fetch(source(server.url("/away.pdf"), headers = credentials, maxAgeMs = 0.0)).ready() + + assertEquals("\"v1\"", other.requests[1].ifNoneMatch) + assertEquals(first.copy.file, second.copy.file) + assertEquals(clock.get(), second.copy.metadata.validatedAtMs) + } + + @Test + fun `ten redirects are followed, the eleventh fails without falling back to the stored copy`() { + redirectChain(hops = 10) + fetch(source(server.url("/hop0.pdf"))).ready() + + redirectChain(hops = 11) + clock.addAndGet(5_000) + val failure = fetch(source(server.url("/hop0.pdf"), maxAgeMs = 0.0), allowStale = true).failure() + assertEquals(PdfErrorCode.NETWORK_ERROR, failure.code) + assertEquals(11 + 11, server.requests.size) + } + + @Test + fun `a redirect that is not followed fails with http-error and its status`() { + fetch(source()).ready() + server.redirects["/doc.pdf"] = "ftp://127.0.0.1/doc.pdf" + + val failure = fetch(source(maxAgeMs = 0.0), allowStale = true).failure() + assertEquals(PdfErrorCode.HTTP_ERROR, failure.code) + assertEquals(302, failure.httpStatus) + } + + @Test + fun `cancelling during a redirect stops the request in progress`() { + server.redirects["/away.pdf"] = other.url("/doc.pdf") + val gate = other.hold() + val recorder = fetch(source(server.url("/away.pdf"))) + other.awaitRequests(1) + recorder.subscription.cancel() + gate.countDown() + awaitIdle() + + assertFalse(recorder.result.isDone) + assertNull(cache.find(source(server.url("/away.pdf")))) + assertEquals(0, partialDirectory.listFiles()?.size ?: 0) + } + + /** `/hop0.pdf` → … → `/hop.pdf` → `/doc.pdf`. */ + private fun redirectChain(hops: Int) { + server.redirects.clear() + for (hop in 0 until hops) server.redirects["/hop$hop.pdf"] = if (hop + 1 < hops) "/hop${hop + 1}.pdf" else "/doc.pdf" + } + + // endregion + // region Sharing and cancellation @Test @@ -361,7 +450,10 @@ class DocumentCacheTest { private class Ready(val copy: CachedCopy, val isStale: Boolean) - private class Request(val ifNoneMatch: String?) + /** [headers] by lowercase name. */ + private class Request(val path: String, val headers: Map) { + val ifNoneMatch get() = headers["if-none-match"] + } private class Recorder : DocumentCache.Listener { val result = CompletableFuture() @@ -403,6 +495,9 @@ class DocumentCacheTest { /** Answer every request with this status and no body. */ @Volatile var status: Int? = null + /** Paths answered with `302` and this `Location`. */ + val redirects = ConcurrentHashMap() + @Volatile private var gate: CountDownLatch? = null @Volatile private var refusing = false val requests = CopyOnWriteArrayList() @@ -428,17 +523,19 @@ class DocumentCacheTest { private fun serve(client: Socket) = client.use { try { val reader = client.getInputStream().bufferedReader(Charsets.ISO_8859_1) - reader.readLine() ?: return + val path = reader.readLine()?.split(' ')?.getOrNull(1) ?: return val headers = generateSequence { reader.readLine()?.takeIf { it.isNotEmpty() } } .associate { it.substringBefore(':').trim().lowercase() to it.substringAfter(':').trim() } val ifNoneMatch = headers["if-none-match"] - requests += Request(ifNoneMatch) + requests += Request(path, headers) gate?.await(5, TimeUnit.SECONDS) val forced = status val currentEtag = etag + val location = redirects[path] val output = client.getOutputStream() when { forced != null -> output.write(head(forced, null, 0)) + location != null -> output.write(head(302, null, 0, location)) currentEtag != null && ifNoneMatch == currentEtag -> output.write(head(304, currentEtag, null)) else -> { val bytes = body @@ -452,10 +549,11 @@ class DocumentCacheTest { } } - private fun head(status: Int, etag: String?, contentLength: Int?): ByteArray = + private fun head(status: Int, etag: String?, contentLength: Int?, location: String? = null): ByteArray = buildString { append("HTTP/1.1 $status Status\r\n") etag?.let { append("ETag: $it\r\n") } + location?.let { append("Location: $it\r\n") } contentLength?.let { append("Content-Length: $it\r\n") } append("Connection: close\r\n\r\n") }.toByteArray(Charsets.ISO_8859_1) @@ -489,6 +587,8 @@ class DocumentCacheTest { } private companion object { + val credentials = mapOf("Authorization" to "Bearer t", "X-Api-Key" to "secret") + fun pdf(version: String) = "%PDF-1.4\n% $version\n%%EOF\n".toByteArray() } } diff --git a/package/android/src/test/java/com/margelo/nitro/betterpdf/RedirectPolicyTest.kt b/package/android/src/test/java/com/margelo/nitro/betterpdf/RedirectPolicyTest.kt new file mode 100644 index 0000000..8fee544 --- /dev/null +++ b/package/android/src/test/java/com/margelo/nitro/betterpdf/RedirectPolicyTest.kt @@ -0,0 +1,64 @@ +package com.margelo.nitro.betterpdf + +import java.net.URL +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class RedirectPolicyTest { + private val start = URL("http://example.com/a/doc.pdf") + + private fun RedirectPolicy.follow(from: URL, location: String): URL = + (next(from, location) as? RedirectPolicy.Next.Follow)?.url ?: throw AssertionError("Expected $location to be followed") + + @Test + fun `resolves relative locations against the redirecting URL`() { + val policy = RedirectPolicy(start) + assertEquals(URL("http://example.com/a/b.pdf"), policy.follow(start, "b.pdf")) + assertEquals(URL("http://example.com/b.pdf"), policy.follow(start, "/b.pdf")) + assertEquals(URL("http://cdn.example.com/b.pdf"), policy.follow(start, "//cdn.example.com/b.pdf")) + } + + @Test + fun `keeps the headers while the origin stays the same`() { + val policy = RedirectPolicy(start) + policy.follow(start, "http://EXAMPLE.com:80/b.pdf") + policy.follow(URL("http://example.com/b.pdf"), "/c.pdf") + assertTrue(policy.sendsHeaders) + + val https = URL("https://example.com/doc.pdf") + val secure = RedirectPolicy(https) + secure.follow(https, "https://example.com:443/b.pdf") + assertTrue(secure.sendsHeaders) + } + + @Test + fun `drops the headers for the rest of the chain after another host, port or scheme`() { + for (location in listOf("http://cdn.example.com/b.pdf", "http://example.com:8080/b.pdf", "https://example.com/b.pdf")) { + val policy = RedirectPolicy(start) + policy.follow(start, location) + assertFalse(location, policy.sendsHeaders) + } + val policy = RedirectPolicy(start) + val away = policy.follow(start, "http://cdn.example.com/b.pdf") + policy.follow(away, start.toString()) + assertFalse(policy.sendsHeaders) + } + + @Test + fun `refuses https to http, other schemes and unusable locations`() { + val https = URL("https://example.com/doc.pdf") + assertEquals(RedirectPolicy.Next.Refuse, RedirectPolicy(https).next(https, "http://example.com/doc.pdf")) + for (location in listOf(null, "ftp://example.com/doc.pdf", "file:///doc.pdf", "unknown:doc", "javascript:alert(1)")) { + assertEquals(location, RedirectPolicy.Next.Refuse, RedirectPolicy(start).next(start, location)) + } + } + + @Test + fun `follows ten redirects, not eleven`() { + val policy = RedirectPolicy(start) + repeat(RedirectPolicy.MAX_REDIRECTS) { policy.follow(start, "/doc.pdf") } + assertEquals(RedirectPolicy.Next.TooMany, policy.next(start, "/doc.pdf")) + } +} diff --git a/package/ios/RedirectPolicy.swift b/package/ios/RedirectPolicy.swift new file mode 100644 index 0000000..27b5283 --- /dev/null +++ b/package/ios/RedirectPolicy.swift @@ -0,0 +1,48 @@ +import Foundation + +/// The redirects one download follows, the same as Android's +/// `RedirectPolicy`: at most `maxRedirects`, to `http(s)` only, never from +/// https to http, and the caller's headers only until the first hop to +/// another origin. +struct RedirectPolicy { + enum Next { + case follow + /// The redirect response is the answer. + case refuse + case tooMany + } + + static let maxRedirects = 10 + /// The statuses that redirect in URLSession. + static let statuses: Set = [301, 302, 303, 307, 308] + + private let origin: Origin + private var followed = 0 + private(set) var sendsHeaders = true + + init(first: URL) { + origin = Origin(first) + } + + mutating func next(from: URL, to: URL) -> Next { + let scheme = to.scheme?.lowercased() + guard scheme == "http" || scheme == "https" else { return .refuse } + if from.scheme?.lowercased() == "https", scheme == "http" { return .refuse } + if followed == Self.maxRedirects { return .tooMany } + followed += 1 + if Origin(to) != origin { sendsHeaders = false } + return .follow + } + + private struct Origin: Equatable { + let scheme: String + let host: String + let port: Int + + init(_ url: URL) { + scheme = url.scheme?.lowercased() ?? "" + host = url.host?.lowercased() ?? "" + port = url.port ?? (scheme == "https" ? 443 : 80) + } + } +} diff --git a/package/ios/SharedDownload.swift b/package/ios/SharedDownload.swift index 275e5ea..560b2df 100644 --- a/package/ios/SharedDownload.swift +++ b/package/ios/SharedDownload.swift @@ -4,14 +4,16 @@ import Foundation /// copy (`DocumentCache` owns `subscribers`). With an `existing` copy it is a /// conditional request: `304` keeps the copy, `200` replaces it. /// -/// Progress and completion arrive on URLSession's delegate queue; `cancel()` -/// is safe from any thread and ends the download without an outcome. +/// Redirects, progress and completion arrive on URLSession's delegate queue; +/// `cancel()` is safe from any thread and ends the download without an +/// outcome. final class SharedDownload { let key: String /// Guarded by the `DocumentCache`'s lock. var subscribers: [DocumentCache.Subscriber] = [] - private let request: URLRequest + private let url: URL + private let headers: [String: String] private let existing: CachedCopy? /// Where a new copy is stored; joiners that want it persistent promote it. private let mode: PdfCacheMode @@ -24,6 +26,9 @@ final class SharedDownload { private var isCancelled = false private var task: URLSessionDownloadTask? private var progressObservation: NSKeyValueObservation? + // Used on URLSession's delegate queue only. + private var redirects: RedirectPolicy + private var hasTooManyRedirects = false /// Shorter than URLSession's default, so a dead network shows the /// existing copy soon. @@ -40,22 +45,10 @@ final class SharedDownload { onProgress: @escaping (SharedDownload, Int64, Int64?, Bool) -> Void, onFinished: @escaping (SharedDownload, DownloadOutcome) -> Void ) { - var request = URLRequest(url: url) - // The copies in the store are the cache; URLCache must neither answer - // instead of the server nor turn a 304 into a cached 200. - request.cachePolicy = .reloadIgnoringLocalCacheData - for (name, value) in headers { - request.setValue(value, forHTTPHeaderField: name) - } - if let existing { - request.timeoutInterval = Self.revalidationTimeout - if let etag = existing.metadata.etag { request.setValue(etag, forHTTPHeaderField: "If-None-Match") } - if let lastModified = existing.metadata.lastModified { - request.setValue(lastModified, forHTTPHeaderField: "If-Modified-Since") - } - } self.key = key - self.request = request + self.url = url + self.headers = headers + self.redirects = RedirectPolicy(first: url) self.existing = existing self.mode = mode self.store = store @@ -65,12 +58,13 @@ final class SharedDownload { } func start() { - let task = URLSession.shared.downloadTask(with: request) { [self] tempURL, response, error in + let task = URLSession.shared.downloadTask(with: request(for: url, withHeaders: true)) { [self] tempURL, response, error in // The temp file is deleted when this handler returns: finish here. progressObservation?.invalidate() if cancelled { return } onFinished(self, outcome(tempURL: tempURL, response: response, error: error)) } + task.delegate = RedirectHandler { [weak self] response, proposed in self?.redirect(response, to: proposed) } progressObservation = task.observe(\.countOfBytesReceived, options: []) { [weak self] observed, _ in guard let self else { return } let loadedBytes = observed.countOfBytesReceived @@ -98,8 +92,50 @@ final class SharedDownload { private var cancelled: Bool { lock.withLock { isCancelled } } + private func request(for url: URL, withHeaders: Bool) -> URLRequest { + var request = URLRequest(url: url) + // The copies in the store are the cache; URLCache must neither answer + // instead of the server nor turn a 304 into a cached 200. + request.cachePolicy = .reloadIgnoringLocalCacheData + if withHeaders { + for (name, value) in headers { + request.setValue(value, forHTTPHeaderField: name) + } + } + if let existing { + request.timeoutInterval = Self.revalidationTimeout + if let etag = existing.metadata.etag { request.setValue(etag, forHTTPHeaderField: "If-None-Match") } + if let lastModified = existing.metadata.lastModified { + request.setValue(lastModified, forHTTPHeaderField: "If-Modified-Since") + } + } + return request + } + + /// The request that follows `response`, built anew: URLSession's proposal + /// keeps the caller's headers on every origin but drops `Authorization` + /// even on the same one. + private func redirect(_ response: HTTPURLResponse, to proposed: URLRequest) -> URLRequest? { + guard let from = response.url, let to = proposed.url else { return nil } + switch redirects.next(from: from, to: to) { + case .follow: + return request(for: to, withHeaders: redirects.sendsHeaders) + case .refuse: + return nil + case .tooMany: + hasTooManyRedirects = true + return nil + } + } + private func outcome(tempURL: URL?, response: URLResponse?, error: Error?) -> DownloadOutcome { - let url = request.url?.absoluteString ?? "" + let url = self.url.absoluteString + if hasTooManyRedirects { + return .failed(PdfLoadFailure( + code: .networkError, + message: "Downloading \(url) failed: more than \(RedirectPolicy.maxRedirects) redirects" + ), isServerUnreachable: false) + } if let error { return .failed(PdfLoadFailure( code: .networkError, @@ -119,9 +155,10 @@ final class SharedDownload { return storing { try store.markValidated(existing, metadata: metadata) } } guard (200..<300).contains(http.statusCode) else { + let reason = RedirectPolicy.statuses.contains(http.statusCode) ? ": the redirect is not followed" : "" return .failed(PdfLoadFailure( code: .httpError, - message: "Downloading \(url) failed with HTTP \(http.statusCode)", + message: "Downloading \(url) failed with HTTP \(http.statusCode)\(reason)", httpStatus: http.statusCode ), isServerUnreachable: http.statusCode >= 500) } @@ -164,3 +201,23 @@ final class SharedDownload { return expected >= 0 && expected >= loaded ? expected : nil } } + +/// Hands each redirect of a task to `decide`; nil ends the task with the +/// redirect response. +private final class RedirectHandler: NSObject, URLSessionTaskDelegate { + private let decide: (HTTPURLResponse, URLRequest) -> URLRequest? + + init(_ decide: @escaping (HTTPURLResponse, URLRequest) -> URLRequest?) { + self.decide = decide + } + + func urlSession( + _ session: URLSession, + task: URLSessionTask, + willPerformHTTPRedirection response: HTTPURLResponse, + newRequest request: URLRequest, + completionHandler: @escaping (URLRequest?) -> Void + ) { + completionHandler(decide(response, request)) + } +} diff --git a/package/src/source.ts b/package/src/source.ts index b4a54cc..97f1498 100644 --- a/package/src/source.ts +++ b/package/src/source.ts @@ -31,6 +31,10 @@ export interface PdfUriSource { * ASCII (encode anything else yourself): Android sends other characters * as UTF-8, iOS as Latin-1 and cuts the value at the first character * outside it. + * + * They go to the URL's origin only: after a redirect to another scheme, + * host or port (http to https included), the rest of the download is + * sent without them. */ headers?: Readonly>; /**