diff options
Diffstat (limited to 'workers')
| -rw-r--r-- | workers/.gitignore | 1 | ||||
| -rw-r--r-- | workers/apex/src/index.ts | 438 | ||||
| -rw-r--r-- | workers/apex/src/special.ts | 7 | ||||
| -rw-r--r-- | workers/apex/src/uagate.ts | 40 | ||||
| -rw-r--r-- | workers/apex/test/router.test.ts | 759 | ||||
| -rw-r--r-- | workers/apex/test/uagate.test.ts | 78 | ||||
| -rw-r--r-- | workers/apex/ua-policy.json | 2 |
7 files changed, 1287 insertions, 38 deletions
diff --git a/workers/.gitignore b/workers/.gitignore index ef5b8080..4b240bc1 100644 --- a/workers/.gitignore +++ b/workers/.gitignore @@ -1,3 +1,4 @@ node_modules/ .wrangler/ dist/ +test-results/ diff --git a/workers/apex/src/index.ts b/workers/apex/src/index.ts index 278794e2..f97b4bc5 100644 --- a/workers/apex/src/index.ts +++ b/workers/apex/src/index.ts @@ -47,6 +47,263 @@ function apexHeaders(resp: Response, env: ApexEnv, cacheClass: string = DEFAULT_ return out; } +// R2's `R2Range` is a three-shape union — `{offset, length?}`, `{length}` (offset implicitly 0) +// and `{suffix}` (the trailing N bytes) — so `"offset" in range` is NOT a safe way to read it: +// the two offset-less shapes would fall through to the full-object 200 branch and be served +// with a Content-Length claiming the whole object while the body held only a slice. workerd is +// observed to normalize every shape to `{offset, length}` before it reaches us, but the type +// admits the others, so resolve all three to concrete byte bounds, clamped to the object size. +// Exported for direct unit testing. +export function resolveRange( + range: { offset?: number; length?: number; suffix?: number }, + size: number, +): { start: number; length: number } { + if (typeof range.suffix === "number") { + const suffix = Math.min(Math.max(range.suffix, 0), size); // a suffix past the start is the whole object + return { start: size - suffix, length: suffix }; + } + const start = Math.min(Math.max(range.offset ?? 0, 0), size); + // The trailing Math.max(_, 0) keeps the documented "clamped to the object size" contract + // total: without it a negative `range.length` would pass straight through Math.min and + // yield a negative length (and so a negative Content-Length). A real R2 binding cannot + // produce that — see classifyRangeHeader's note on observed R2 behaviour — but this + // function is exported and unit-tested as a standalone utility over the R2Range union, so + // it should not have a documented invariant its own signature can violate. Deliberately + // NOT guarding non-finite inputs: NaN bounds are unreachable from the binding and the + // guard would be untestable-in-anger dead weight. + const length = Math.max(Math.min(range.length ?? size - start, size - start), 0); + return { start, length }; +} + +// A single `bytes=` range-spec. Anything with a comma is a multi-range and deliberately +// fails to match. The whitespace class is `\s`, which is DELIBERATELY wider than the ` ` +// (ASCII space) that R2's own parser accepts — see classifyRangeHeader's contract note on +// why the two grammars are allowed to disagree. +const SINGLE_BYTE_RANGE = /^\s*bytes\s*=\s*(\d*)\s*-\s*(\d*)\s*$/i; + +/** + * Numeric comparison of two non-empty digit strings, without going through Number(). + * + * A range-spec's positions are unbounded digit strings, and Number() silently rounds + * anything above 2^53: `Number("9007199254740993") === Number("9007199254740992")`, which + * collapsed `bytes=9007199254740993-9007199254740992` — an invalid spec (last < first) + * that §14.1.2 says to IGNORE, so 200 — into an apparently-valid one that then read as + * unsatisfiable and answered 416. Comparing normalised digit strings by length and then + * lexically is exact at every magnitude. + */ +function cmpDigits(a: string, b: string): number { + const x = a.replace(/^0+(?=\d)/, ""); + const y = b.replace(/^0+(?=\d)/, ""); + if (x.length !== y.length) return x.length - y.length; + return x < y ? -1 : x > y ? 1 : 0; +} + +export type RangeIntent = + | { kind: "ignored" } + | { kind: "unsatisfiable" } + | { kind: "single"; start: number; length: number }; + +/** + * What the client's Range header ASKS FOR, judged against the representation length. + * + * This exists because R2 does not tell us. Probed against a real R2 binding under + * vitest-pool-workers, `get(key, {range: <Headers>})` signals "I ignored your Range" by + * returning the WHOLE object with `range = {offset: 0, length: size}` — the byte-for-byte + * same shape it returns for a legitimately-satisfied whole-object range like `bytes=0-`. + * It does this for every unsatisfiable spec (`bytes=10-` / `bytes=99-` / `bytes=-0` on a + * 10-byte object), every malformed one (`bytes=abc`, `bytes=-`, `bytes=5-2`), multi-ranges + * (`bytes=0-1,4-5`) and unknown units (`items=0-5`). It does NOT throw for any of them and + * it never returns a zero/negative length except for a genuinely zero-length object. + * (The object-literal form `get(key, {range: {offset: 99}})` DOES throw + * "The requested range is not satisfiable (10039)" — but this Worker passes Headers, so + * that path is unreachable here.) + * + * Trusting `obj.range` alone therefore answered `Range: bytes=99-` with + * `206 + Content-Range: bytes 0-9/10` and the FULL body — a 206 that does not correspond to + * the request (RFC 9110 §15.3.7). That is actively dangerous for the resuming downloader + * this range forwarding exists to serve: a client resuming at byte 99 would append bytes + * 0-9 to its partial file and silently corrupt it. Re-deriving intent from the client's own + * header is the only way to separate the three cases. + * + * Satisfiability follows RFC 9110 §14.1.2 verbatim: an int-range is satisfiable iff + * first-pos < length; a suffix-range iff suffix-length is non-zero (so on a zero-length + * representation, a non-zero suffix-range is the ONLY satisfiable form). An invalid spec + * (last-pos < first-pos) MUST be ignored rather than rejected, hence "ignored", not + * "unsatisfiable". + * + * The `single` verdict carries the CONCRETE byte bounds the client asked for, clamped the + * way §14.1.2 clamps them. That is what makes this classifier safe to disagree with R2's + * parser. The two grammars are not identical and cannot be kept identical: R2's accepts + * only ASCII space around the tokens (miniflare's `/^ *(\d+)? *- *(\d+)? *$/`) while this + * one accepts `\s`, so `Range: bytes=2<TAB>-<TAB>4` parses here and is ignored there. When + * a caller compares these bounds against the bytes R2 actually handed back, any such + * divergence — this one, or the next one a parser change introduces — degrades to a plain + * 200 instead of a 206 whose Content-Range describes a body the client did not ask for. + * Chasing byte-for-byte grammar parity would put the guarantee back in the hands of two + * regexes staying in sync, which is the coupling that produced the bug. + */ +export function classifyRangeHeader(header: string, size: number): RangeIntent { + const m = SINGLE_BYTE_RANGE.exec(header); + if (!m) return { kind: "ignored" }; // multi-range, unknown unit, or unparseable + const [, firstRaw, lastRaw] = m; + if (firstRaw === "") { + if (lastRaw === "") return { kind: "ignored" }; // bare "bytes=-" is malformed + // §14.1.2: suffix-length 0 is unsatisfiable; a suffix past the start is the whole object. + if (cmpDigits(lastRaw, "0") <= 0) return { kind: "unsatisfiable" }; + const suffix = Math.min(Number(lastRaw), size); + return { kind: "single", start: size - suffix, length: suffix }; + } + // §14.1.2: an invalid spec (last-pos < first-pos) is ignored, not rejected. + if (lastRaw !== "" && cmpDigits(lastRaw, firstRaw) < 0) return { kind: "ignored" }; + if (cmpDigits(firstRaw, String(size)) >= 0) return { kind: "unsatisfiable" }; + const first = Number(firstRaw); // < size, so within safe-integer range + const last = lastRaw === "" ? size - 1 : Math.min(Number(lastRaw), size - 1); + return { kind: "single", start: first, length: last - first + 1 }; +} + +/** A quoted entity-tag list (`"a", W/"b"`) or `*`, compared per RFC 9110 §8.8.3.2. */ +function etagListMatches(list: string, etag: string, compare: "strong" | "weak"): boolean { + const items = list.split(",").map((s) => s.trim()).filter((s) => s !== ""); + if (items.includes("*")) return true; // "*" matches iff a representation exists — one does + const weaken = (t: string) => t.replace(/^W\//, ""); + // Strong comparison: neither side may be weak (§8.8.3.2). + if (compare === "strong" && etag.startsWith("W/")) return false; + return items.some((t) => + compare === "strong" ? t === etag : weaken(t) === weaken(etag), + ); +} + +/** + * `uploaded <= <HTTP-date>`, at seconds granularity, or null when the date is unparseable + * (§13.1.3: an invalid date MUST be ignored, which callers map to "precondition passes"). + * Seconds granularity matches R2's own comparison, which the Headers form of `onlyIf` + * selects — evaluating at millisecond precision here would disagree with the binding that + * produced the failure we are trying to name. + */ +function uploadedAtOrBefore(uploaded: Date | undefined, httpDate: string): boolean | null { + const at = Date.parse(httpDate); + if (Number.isNaN(at) || !uploaded) return null; + return Math.floor(uploaded.getTime() / 1000) <= Math.floor(at / 1000); +} + +/** + * The conditional headers R2 is allowed to see, filtered to those RFC 9110 §13.2.2 says + * actually apply to THIS request. + * + * R2 ANDs together every validator it is handed; §13.2.2 instead defines a precedence in + * which a lower-ranked validator is not evaluated at all. Forwarding `request.headers` + * wholesale therefore let R2 fail a request on a validator the RFC says to ignore — most + * visibly `If-Modified-Since` on a non-GET/HEAD method, which §13.2.2 step 4 does not + * evaluate, but which R2 evaluated anyway and answered with a body-less object that this + * Worker could only turn into a 412. Filtering at the source means a body-less result now + * always corresponds to a precondition that genuinely applies. + * + * The METHOD decides this before any header does. §13.2.1: "a server MUST ignore the + * conditional request header fields defined by this specification when received with a + * request method that does not involve the selection or modification of a selected + * representation, such as CONNECT, OPTIONS, or TRACE." Filtering by validator applicability + * alone still handed those methods' conditionals to R2, so an OPTIONS carrying a stale + * `If-Match` a client had left lying around was refused 412 where the same request without + * it succeeded. Returning an empty set here is what "ignore" means at this layer: R2 is + * given nothing to evaluate, so it cannot answer body-less, so preconditionStatus() — which + * is only ever reached from a body-less result — is unreachable for these methods too. + */ +function applicablePreconditions(h: Headers, method: string): Headers { + const out = new Headers(); + if (method === "OPTIONS" || method === "TRACE" || method === "CONNECT") return out; + const isGetOrHead = method === "GET" || method === "HEAD"; + const ifMatch = h.get("if-match"); + const ifNoneMatch = h.get("if-none-match"); + if (ifMatch !== null) out.set("if-match", ifMatch); + else { + const ius = h.get("if-unmodified-since"); // §13.2.2 step 2: only when If-Match is absent + if (ius !== null) out.set("if-unmodified-since", ius); + } + if (ifNoneMatch !== null) out.set("if-none-match", ifNoneMatch); + else if (isGetOrHead) { + const ims = h.get("if-modified-since"); // step 4: only when If-None-Match is absent, GET/HEAD only + if (ims !== null) out.set("if-modified-since", ims); + } + return out; +} + +/** + * Which status a FAILED `onlyIf` owes the client, decided by re-evaluating the request's + * conditionals against the object's own validators in RFC 9110 §13.2.2 order. + * + * R2 reports THAT a precondition failed and never WHICH one. Inferring from header + * PRESENCE cannot be right in both directions, which is how `If-Match: "x"` + + * `If-None-Match: "x"` on a matching object — If-Match satisfied, If-None-Match failed, + * so §13.1.2 owes a 304 — came back 412 purely because an If-Match header was present. + * Evaluating the validators removes the guess: presence selects which check runs, the + * comparison decides the answer. + */ +export function preconditionStatus( + h: Headers, isGetOrHead: boolean, etag: string, uploaded: Date | undefined, +): 304 | 412 { + const ifMatch = h.get("if-match"); + if (ifMatch !== null) { + if (!etagListMatches(ifMatch, etag, "strong")) return 412; // §13.2.2 step 1 + } else { + const ius = h.get("if-unmodified-since"); // step 2 + if (ius !== null && uploadedAtOrBefore(uploaded, ius) === false) return 412; + } + const ifNoneMatch = h.get("if-none-match"); + if (ifNoneMatch !== null) { + // §13.1.2: a failed If-None-Match is 304 for GET/HEAD and 412 for every other method. + if (etagListMatches(ifNoneMatch, etag, "weak")) return isGetOrHead ? 304 : 412; + } else if (isGetOrHead) { + const ims = h.get("if-modified-since"); // step 4 + if (ims !== null && uploadedAtOrBefore(uploaded, ims) === true) return 304; + } + // R2 refused for a reason this evaluation could not reproduce (a validator comparison + // that differs at the margins, say). 412 is the safe answer: a 304 would assert a cache + // validity we have not established. + return 412; +} + +/** + * RFC 9110 §13.1.5 If-Range: does the client's validator still describe this object? + * + * R2 cannot answer this — its `R2Conditional` carries only etagMatches / + * etagDoesNotMatch / uploadedBefore / uploadedAfter, so an `If-Range` in the forwarded + * Headers is silently dropped and the range is applied unconditionally. For an object + * whose ETag has moved on, that answered `Range: bytes=100-` + `If-Range: "old"` with + * bytes 100+ of the NEW representation under a 206 — a resuming downloader then appends + * the new tail to its old prefix and silently corrupts the file, which is the precise + * failure this range forwarding exists to avoid. + * + * §13.1.5 requires a STRONG validator, so a weak entity-tag never matches. The date form + * likewise never matches here: it must compare against Last-Modified, and this Worker + * does not emit one, so no client can hold a date validator for this resource that we + * could honour — treating it as a mismatch (serve the complete representation) is both + * correct and the safe direction. + */ +function ifRangeMatches(value: string, etag: string): boolean { + const v = value.trim(); + if (!v.startsWith('"')) return false; // weak tag or HTTP-date → not a strong match + return etagListMatches(v, etag, "strong"); +} + +/** + * Abandon a body stream this Worker has decided not to send. + * + * R2 hands back a body on paths whose response carries none. An unsatisfiable Range gets + * the COMPLETE object (29.2 MiB for the 1.3 PDF) and is answered 416 with a null body; a + * stale `If-Range` gets the sliced range and is answered from a re-read. Dropping the + * reference leaves the stream open until GC collects it, holding the connection; cancelling + * releases it now and aborts the transfer rather than draining it. Failures are swallowed — + * this is cleanup on a path whose response is already decided, and a stream that is already + * closed or errored is exactly the state we wanted. + */ +async function discardBody(body: ReadableStream | null | undefined): Promise<void> { + try { + await body?.cancel(); + } catch { + /* already closed or errored — nothing left to release */ + } +} + async function themed(env: ApexEnv, status: 404 | 503): Promise<Response> { const page = await env.ASSETS.fetch(new Request(`https://apex.internal/${status}.html`)); return apexHeaders(new Response(page.body, { status, headers: { "content-type": "text/html; charset=utf-8" } }), env); @@ -78,14 +335,24 @@ export default { console.log(JSON.stringify({ event: "binding-missing", binding: "DOCS_PDFS" })); return themed(env, 503); } + const method = request.method.toUpperCase(); + const isGetOrHead = method === "GET" || method === "HEAD"; + // §14.2: "GET is the only method for which range handling is defined" — a Range on + // any other method MUST be ignored. Reading it as null here suppresses the whole + // partial-content path in one place: R2 is never asked to slice, and the 206/416 + // branches below are unreachable. Gating only the R2 forward would leave the + // response side still seeing a Range header and answering a HEAD or a POST with a + // 416 or a Content-Range. + const rangeHeader = method === "GET" ? request.headers.get("range") : null; + const onlyIf = applicablePreconditions(request.headers, method); + let raw: R2ObjectBody | R2Object | null; try { - // Forward Range + conditional (If-None-Match/If-Match/If-Modified-Since) headers - // straight through to R2 so a resumed download or a client with a fresh cached - // copy doesn't have to re-pull the full 29.2 MiB object. + // Forward Range + the APPLICABLE conditionals so a resumed download or a client + // with a fresh cached copy doesn't have to re-pull the full 29.2 MiB object. raw = await bucket.get(pdfVersion.pdf_r2_key!, { - range: request.headers, - onlyIf: request.headers, + ...(rangeHeader !== null ? { range: request.headers } : {}), + onlyIf, }); } catch (e) { console.log(JSON.stringify({ event: "binding-error", binding: "DOCS_PDFS", error: String(e) })); @@ -102,25 +369,160 @@ export default { "accept-ranges": "bytes", }; - // A satisfied onlyIf precondition (e.g. If-None-Match matched the R2 object's current - // ETag) makes R2 hand back a body-less R2Object — just the validators, no content. + // A FAILED onlyIf precondition makes R2 hand back a body-less R2Object — just the + // validators, no content — and never says which validator failed. Because `onlyIf` + // was filtered to the conditionals §13.2.2 actually applies to this request, a + // body-less result here always means a precondition that genuinely applies failed; + // preconditionStatus() re-evaluates them against the object's own validators, in + // §13.2.2 order, to decide between 304 and 412. if (!("body" in raw) || !raw.body) { - return apexHeaders(new Response(null, { status: 304, headers: pdfHeaders }), env, pdfCacheClass); + const status = preconditionStatus( + request.headers, isGetOrHead, raw.httpEtag, raw.uploaded); + return apexHeaders(new Response(null, { status, headers: pdfHeaders }), env, pdfCacheClass); } - const obj = raw as R2ObjectBody; + let obj = raw as R2ObjectBody; pdfHeaders["content-type"] = "application/pdf"; - // A satisfied Range request — R2 echoes the actually-served byte range on `obj.range`; - // its absence means either no Range header was sent or R2 served the full object. - const range = obj.range; - if (range && "offset" in range) { - const start = range.offset ?? 0; - const length = range.length ?? obj.size - start; - pdfHeaders["content-range"] = `bytes ${start}-${start + length - 1}/${obj.size}`; - pdfHeaders["content-length"] = String(length); - return apexHeaders(new Response(obj.body, { status: 206, headers: pdfHeaders }), env, pdfCacheClass); + // §13.1.5 If-Range, which R2 cannot evaluate (see ifRangeMatches). A failed validator + // means the client's partial copy is stale, so the Range is ignored ENTIRELY and the + // complete representation is served — including for a spec that would otherwise be + // unsatisfiable, since the 416 branch below must not fire on a range we have decided + // not to honour. R2 has already applied the range at this point, so the whole object + // has to be re-read; that costs one extra R2 read on the rare stale-resume path and + // nothing at all on the common one. + // + // The re-read carries the SAME `onlyIf`. §13.2.1 requires preconditions to hold for the + // representation ultimately selected, and the two reads need not see one object: a bare + // re-get answered `Range` + stale `If-Range` + `If-Match: "A"` with a 200 carrying + // object B, whose ETag the client had explicitly excluded, whenever the key was + // rewritten in between. That rewrite is not hypothetical — the legacy snapshot repo's + // deploy workflow re-uploads this exact key on its `force_pdf_refresh` input. Re-sending + // the conditionals ties the verdict to the bytes actually served, because R2 evaluates + // `onlyIf` against the very object it returns; the body-less outcome that produces is + // not a gap in this path but the correct answer, resolved by preconditionStatus() + // exactly as on the first read. When no conditionals were sent, `onlyIf` is empty and a + // body-less result cannot occur, so the common path is untouched. + // + // A head() before the get() would also expose the validators, and would avoid opening + // this slice stream at all — but it would put a second round-trip on the path where + // If-Range MATCHES, which is the normal resumed download, in exchange for tidying the + // rare one where it does not. It would also widen the window this shape keeps narrow: + // the object whose validators decide the verdict is the one R2 returns from the same + // call. So the get-then-re-read stays, and the slice we are abandoning is cancelled + // rather than left to GC. + const ifRange = rangeHeader !== null ? request.headers.get("if-range") : null; + let rangeApplies = rangeHeader !== null; + if (ifRange !== null && !ifRangeMatches(ifRange, obj.httpEtag)) { + rangeApplies = false; + await discardBody(obj.body); + let full: R2ObjectBody | R2Object | null; + try { + full = await bucket.get(pdfVersion.pdf_r2_key!, { onlyIf }); + } catch (e) { + console.log(JSON.stringify({ event: "binding-error", binding: "DOCS_PDFS", error: String(e) })); + return themed(env, 503); + } + if (!full) return themed(env, 404); // deleted between the two reads + if (!("body" in full) || !full.body) { + // The key was rewritten between the two reads and the request's preconditions do + // not hold for the new representation. Same treatment as a first-read failure, on + // the new object's validators — never the old ones, which describe a representation + // this response is not about. + const status = preconditionStatus( + request.headers, isGetOrHead, full.httpEtag, full.uploaded); + return apexHeaders(new Response(null, { + status, headers: { etag: full.httpEtag, "accept-ranges": "bytes" }, + }), env, pdfCacheClass); + } + obj = full as R2ObjectBody; + pdfHeaders.etag = obj.httpEtag; } + // A satisfied Range request. R2 echoes the actually-served byte range on `obj.range` — + // but it does so for FULL gets too: against a real R2 binding under workerd, a get() + // whose forwarded Headers carry NO Range header still comes back with + // `range = {offset: 0, length: obj.size}`. Keying the 206 off `obj.range` alone therefore + // turned every plain GET of the 1.3 PDF into a 206 — which is exactly what the nightly + // canary sweep observed (`/en/1.3/vyos-documentation.pdf: status=206`) — and RFC 9110 + // §15.3.7 only permits a 206 in answer to a request that actually carried a Range header. + // So: gate on the REQUEST first, then normalize whatever shape R2 handed back — and + // then CHECK that the two agree before promising a 206, because "R2 sliced it" and + // "R2 handed back everything" are the same shape on the wire. + // + // What R2 actually handed back, as concrete bounds. `obj.range` is absent only if a + // binding declines to report one, in which case the body is the complete object. + const actual = obj.range + ? resolveRange(obj.range, obj.size) + : { start: 0, length: obj.size }; + const servedWhole = actual.start === 0 && actual.length === obj.size; + + if (rangeApplies && rangeHeader !== null) { + const intent = classifyRangeHeader(rangeHeader, obj.size); + if (intent.kind === "unsatisfiable") { + // §14.2: "the server SHOULD send a 416"; §15.5.17: a 416 to a byte-range request + // SHOULD carry `Content-Range: bytes */<complete-length>`. Deliberately no + // content-type — there is no PDF payload on this response. 416 is >= 400 so + // apexHeaders() forces no-store, which is right: the verdict depends on the + // request's Range header and the cache key does not include it. + // R2 answers an unsatisfiable Range with the COMPLETE object, so the body being + // dropped here is the whole 29.2 MiB one — the largest abandoned stream on any + // path through this handler. + await discardBody(obj.body); + return apexHeaders( + new Response(null, { + status: 416, + headers: { etag: pdfHeaders.etag, "accept-ranges": "bytes", + "content-range": `bytes */${obj.size}` }, + }), + env, + pdfCacheClass, + ); + } + // A 206 is owed only when the bytes R2 selected are the bytes the client asked for. + // `length === 0` means a zero-length representation (the only way R2 yields it) — + // e.g. a non-zero suffix-range, which §14.1.2 calls satisfiable, against an empty + // object. No valid Content-Range exists for an empty selection (§14.4 forbids a + // last-pos below the first-pos), so a 206 is unrepresentable. Fall through to the + // 200: §15.5.17's own note records that servers are free to ignore Range and answer + // with the complete representation, which for an empty object is exactly this body. + if (intent.kind === "single" && intent.length > 0 && + actual.start === intent.start && actual.length === intent.length) { + pdfHeaders["content-range"] = + `bytes ${intent.start}-${intent.start + intent.length - 1}/${obj.size}`; + pdfHeaders["content-length"] = String(intent.length); + return apexHeaders(new Response(obj.body, { status: 206, headers: pdfHeaders }), env, pdfCacheClass); + } + // Otherwise no 206 is owed: either the spec was one §14.1.2 says to ignore + // (multi-range / malformed / unknown unit), or R2 declined a spec that this parser + // accepted — the grammar divergence classifyRangeHeader documents. Both leave R2 + // having returned the complete object, so fall through to the 200 below. + } + + // Serve what R2 actually handed back, described truthfully. `servedWhole` is the + // normal case and the only one a 200 can describe; a partial body under a 200 would + // ship a Content-Length that contradicts it. A partial body reaching HERE — past the + // agreement check above — means R2 sliced to bounds the request did not ask for, and + // there is no honest success response left: a 200 would misstate the length, and a + // 206 would answer with a range the client never requested, which §14.4 does not + // permit (Content-Range on a 206 describes the selected range, and no range was + // selected). This used to ship that illegal 206. + // + // So: drop the slice and fail. Re-reading the object for a clean 200 is the other + // option Codex offered, but it means a second conditional read with its own + // object-rewritten-between-reads handling — a copy of the If-Range block above, or a + // refactor of that live and currently-correct path — bought for a branch that cannot + // execute under today's workerd (round 3 established empirically that the Headers + // form ignores multi-range and returns the whole object). The log line is the part + // that earns its keep: it is the alarm that R2's range semantics have moved, and it + // is what would justify writing that re-read for real. + if (!servedWhole) { + console.log(JSON.stringify({ + event: "r2-range-divergence", path: url.pathname, size: obj.size, + served: `${actual.start}+${actual.length}`, requested: rangeHeader ?? null, + })); + await discardBody(obj.body); + return themed(env, 503); + } pdfHeaders["content-length"] = String(obj.size); return apexHeaders(new Response(obj.body, { status: 200, headers: pdfHeaders }), env, pdfCacheClass); } diff --git a/workers/apex/src/special.ts b/workers/apex/src/special.ts index f1a843f0..15e693b1 100644 --- a/workers/apex/src/special.ts +++ b/workers/apex/src/special.ts @@ -26,8 +26,13 @@ export async function specialPathFor( }); if (p === "/sitemap.xml") { + // Origin comes from the REQUEST, never a hard-coded production hostname: this same Worker + // also serves the canary origin (docs-next.vyos.io, DOCS_ENV=canary), and a canary sitemap + // index whose entries pointed at docs.vyos.io would send any checker that follows it + // straight to production — a candidate tree with broken or missing per-version sitemaps + // would then pass its own sitemap check by silently grading production instead of itself. const entries = m.versions - .map((v) => `<sitemap><loc>https://docs.vyos.io/en/${v.slug}/sitemap.xml</loc></sitemap>`) + .map((v) => `<sitemap><loc>${url.origin}/en/${v.slug}/sitemap.xml</loc></sitemap>`) .join(""); return new Response( `<?xml version="1.0" encoding="UTF-8"?><sitemapindex xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">${entries}</sitemapindex>`, diff --git a/workers/apex/src/uagate.ts b/workers/apex/src/uagate.ts index 822a7277..4a407e51 100644 --- a/workers/apex/src/uagate.ts +++ b/workers/apex/src/uagate.ts @@ -6,13 +6,43 @@ export interface UaPolicy { export type UaVerdict = "allow" | "block" | "log"; +/** + * The LONGEST entry in `list` occurring in the (already-lowercased) UA, lowercased, or null. + * Longest rather than first-hit so the containment test in uaVerdict() compares against the + * most specific entry a multi-token UA matched, not an arbitrary earlier one. + */ +function bestMatch(lowerUa: string, list: string[]): string | null { + return list.reduce<string | null>((best, entry) => { + const needle = entry.toLowerCase(); + if (!lowerUa.includes(needle)) return best; + return best === null || needle.length > best.length ? needle : best; + }, null); +} + export function uaVerdict(ua: string, policy: UaPolicy): UaVerdict { - const hit = (list: string[]) => list.some((n) => ua.toLowerCase().includes(n.toLowerCase())); + const lowerUa = ua.toLowerCase(); // Explicit blocks take precedence — a request-controlled UA string that spoofs an // allow-listed substring (e.g. "Googlebot EvilScraper") must not be able to bypass a // block entry just by also matching the allow list. - if (hit(policy.block)) return "block"; - if (hit(policy.allow)) return "allow"; - if (hit(policy.log)) return "log"; - return "allow"; // fail-open default + if (bestMatch(lowerUa, policy.block) !== null) return "block"; + + // A log match WINS over any competing allow match, unconditionally. `log` is a telemetry + // verdict, not a denial (the request is served either way), so resolving a contest the + // wrong way is asymmetric: choosing `allow` loses the ua-log event permanently, while + // choosing `log` costs one log line. A UA presenting BOTH an allow token and a log token + // (e.g. "GPTBot/1.0 DuckDuckBot") is exactly the shape worth recording. + // + // There used to be a carve-out here: a matched allow entry that strictly CONTAINED the + // matched log entry won, so a policy could express a narrow allow exception inside a + // broader log entry (log "Foo", allow "Foo-Search"). It is gone, for two reasons. It was + // spoofable — containment was tested between the two matched ENTRIES, never against the + // UA's own token structure, so a caller writing "Bytespider/2.0 Bytespider-Search/1.0" + // matched both entries as independent tokens and bought itself `allow`, and the UA + // string is entirely request-controlled. And it bought nothing: no entry pair in + // ua-policy.json takes that branch. The pair the shipped policy does depend on runs the + // OTHER way — Apple ships "Applebot" (search, allow) and "Applebot-Extended" (AI + // training, log), where the log entry is the longer one, so there is no containment and + // log wins regardless. Losing the carve-out costs a future narrow-allow vendor variant + // nothing worse than being logged as well as served. + return bestMatch(lowerUa, policy.log) === null ? "allow" : "log"; // unknown UAs fail open } diff --git a/workers/apex/test/router.test.ts b/workers/apex/test/router.test.ts index 3ba1867c..b5f965fc 100644 --- a/workers/apex/test/router.test.ts +++ b/workers/apex/test/router.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi } from "vitest"; -import worker from "../src/index"; +import worker, { resolveRange, classifyRangeHeader } from "../src/index"; function makeEnv(overrides: Record<string, unknown> = {}) { const html = (body: string, status = 200) => @@ -126,6 +126,21 @@ describe("apex router (§3.2 order)", () => { vi.resetModules(); } }); + it("/sitemap.xml index entries use the REQUEST origin, not a hard-coded docs.vyos.io", async () => { + // This Worker serves the canary origin too. A canary sitemap index pointing at production + // would send any checker that follows it to docs.vyos.io, so a candidate tree with broken + // per-version sitemaps would pass by silently grading production instead of itself. + const canary = await get("/sitemap.xml"); + const canaryBody = await canary.text(); + expect(canaryBody).toContain("<loc>https://docs-next.vyos.io/en/rolling/sitemap.xml</loc>"); + expect(canaryBody).not.toContain("docs.vyos.io"); + + const prod = await worker.fetch( + new Request("https://docs.vyos.io/sitemap.xml", { headers: { "user-agent": "vitest" } }), + makeEnv({ DOCS_ENV: "production" }), + ); + expect(await prod.text()).toContain("<loc>https://docs.vyos.io/en/1.5/sitemap.xml</loc>"); + }); it("/llms.txt with missing default binding → 503, never 404", async () => { const env = makeEnv({ DOCS_ROLLING: undefined }); expect((await get("/llms.txt", env)).status).toBe(503); @@ -188,9 +203,31 @@ describe("apex router (§3.2 order)", () => { // satisfied byte range on `.range`) behavior closely enough to exercise index.ts's // handling of both without needing the real R2 binding. const ETAG = '"pdf-etag-1"'; + const UPLOADED = new Date("2026-01-15T10:00:00Z"); + const BEFORE_UPLOAD = "Wed, 14 Jan 2026 10:00:00 GMT"; + const AFTER_UPLOAD = "Fri, 16 Jan 2026 10:00:00 GMT"; + // R2 hands back a ReadableStream, not a string, and the difference is exactly what makes + // an abandoned body observable: a stream the Worker neither sends nor cancels stays open + // holding its connection. A string-bodied mock cannot see that class of bug at all, so + // bodies here are real streams that record their own cancellation. `highWaterMark: 0` + // keeps `pull` from running until something actually reads, so a stream cancelled before + // any read still reaches its `cancel()` algorithm rather than being already closed. + function bodyStream(text: string, sink?: { cancelled: string[] }) { + return new ReadableStream({ + pull(c) { + c.enqueue(new TextEncoder().encode(text)); + c.close(); + }, + cancel() { + sink?.cancelled.push(text); + }, + }, { highWaterMark: 0 }); + } + function r2Env( - objects: Record<string, { body: string; etag?: string }>, + objects: Record<string, { body: string; etag?: string; uploaded?: Date }>, overrides: Record<string, unknown> = {}, + sink?: { cancelled: string[] }, ) { return makeEnv({ DOCS_PDFS: { @@ -200,25 +237,100 @@ describe("apex router (§3.2 order)", () => { const etag = hit.etag ?? ETAG; const size = hit.body.length; + const uploaded = hit.uploaded ?? UPLOADED; + const secs = (d: number) => Math.floor(d / 1000); // R2 compares at seconds granularity + + // R2 returns a body-less R2Object whenever an onlyIf precondition FAILS, and it + // never says which one did — that ambiguity is exactly what index.ts has to + // resolve by re-evaluating the request's conditionals against these validators. + // R2 ANDs every validator it is handed and knows nothing about the request + // METHOD; index.ts is what filters the set down to the ones RFC 9110 §13.2.2 + // says apply, so this mock deliberately evaluates whatever it is given. + const bodyless = { httpEtag: etag, size, uploaded }; const ifNoneMatch = options?.onlyIf?.get?.("if-none-match"); - if (ifNoneMatch && ifNoneMatch === etag) { - return { httpEtag: etag, size }; // R2Object, no `body` — precondition matched + if (ifNoneMatch && (ifNoneMatch === "*" || ifNoneMatch.split(",").some( + (t) => t.trim().replace(/^W\//, "") === etag.replace(/^W\//, "")))) { + return bodyless; // If-None-Match matched → "not modified" + } + const ifMatch = options?.onlyIf?.get?.("if-match"); + if (ifMatch && ifMatch !== "*" && !ifMatch.split(",").some( + (t) => t.trim() === etag)) { + return bodyless; // If-Match failed → precondition failed + } + const ifUnmodifiedSince = options?.onlyIf?.get?.("if-unmodified-since"); + if (ifUnmodifiedSince && secs(uploaded.getTime()) > secs(Date.parse(ifUnmodifiedSince))) { + return bodyless; // object is newer than the client's copy } + const ifModifiedSince = options?.onlyIf?.get?.("if-modified-since"); + if (ifModifiedSince && secs(uploaded.getTime()) <= secs(Date.parse(ifModifiedSince))) { + return bodyless; // not modified since the client's copy + } + + // The whole-object result. R2 returns this shape for a plain un-ranged get AND + // — critically — for every Range header it declines to honour. Both verified + // against a real R2 binding under @cloudflare/vitest-pool-workers. + const whole = { + httpEtag: etag, size, uploaded, body: bodyStream(hit.body, sink), + range: { offset: 0, length: size }, + }; const rangeHeader = options?.range?.get?.("range"); - const m = rangeHeader ? /^bytes=(\d+)-(\d+)$/.exec(rangeHeader) : null; - if (m) { - const offset = Number(m[1]); - const length = Number(m[2]) - offset + 1; + if (!rangeHeader) return whole; + + // Range parsing mirrors R2's OWN grammar rather than being merely "strict": + // miniflare src/workers/shared/range.ts uses /^ *bytes *=/i for the prefix and + // /^ *(\d+)? *- *(\d+)? *$/ per comma-separated spec — ASCII SPACE ONLY, never + // \s. index.ts's classifier accepts \s, so the two grammars genuinely disagree + // on a tab. Reproducing R2's grammar here is what makes the tab-separated-Range + // test a real divergence rather than an artefact of a lazily-strict mock. + const prefix = / *bytes *=/i.exec(rangeHeader); + if (!prefix || prefix.index !== 0) return whole; // unknown unit → ignored + const specs = rangeHeader.substring(prefix[0].length).split(","); + if (specs.length !== 1) return whole; // multi-range → ignored + const m = /^ *(\d+)? *- *(\d+)? *$/.exec(specs[0]); + if (!m) return whole; // unparseable (a tab lands here, exactly as in R2) + const [, startRaw, endRaw] = m; + + if (startRaw !== undefined && endRaw !== undefined) { + const offset = Number(startRaw); + const last = Number(endRaw); + // Observed R2: an int-range with first-pos >= size, or an invalid spec with + // last < first, is IGNORED — R2 hands back the complete object rather than + // throwing or returning a zero-length range. (`bytes=10-20` and `bytes=5-2` + // on a 10-byte object both returned `{offset: 0, length: 10}` + the full body.) + if (offset >= size || last < offset) return whole; + const length = Math.min(last, size - 1) - offset + 1; // last-pos clamps to EOF return { - httpEtag: etag, - size, - body: hit.body.slice(offset, offset + length), + httpEtag: etag, size, uploaded, + body: bodyStream(hit.body.slice(offset, offset + length), sink), range: { offset, length }, }; } - - return { httpEtag: etag, size, body: hit.body }; + if (startRaw !== undefined) { // open-ended `bytes=5-` + const offset = Number(startRaw); + if (offset >= size) return whole; // unsatisfiable → ignored + return { + httpEtag: etag, size, uploaded, + body: bodyStream(hit.body.slice(offset), sink), + range: { offset, length: size - offset }, + }; + } + if (endRaw !== undefined) { + // Suffix form. R2Range is a three-shape union and this arm deliberately + // returns the RAW `{suffix}` shape rather than pre-normalizing to + // `{offset, length}` — that is what exercises index.ts's resolveRange(). + // (workerd itself normalizes, but the type admits this shape.) + const n = Number(endRaw); + // miniflare: a suffix >= length yields no ranges, and `bytes=-0` is skipped — + // both leave R2 serving the complete object rather than rejecting. + if (n === 0 || n >= size) return whole; + return { + httpEtag: etag, size, uploaded, + body: bodyStream(hit.body.slice(size - n), sink), + range: { suffix: n }, + }; + } + return whole; // bare `bytes=-` }, } as unknown as R2Bucket, ...overrides, @@ -330,5 +442,626 @@ describe("apex router (§3.2 order)", () => { expect(r.status).toBe(200); expect(await r.text()).toBe("legacy:/en/1.2/vyos-documentation.pdf"); }); + + // --- Regression: a plain GET must never answer 206. R2 reports a whole-object `range` + // on un-ranged gets, so keying the 206 off `obj.range` alone made EVERY plain GET of the + // 1.3 PDF a 206 with a Content-Range — which is what the nightly canary sweep observed + // ("/en/1.3/vyos-documentation.pdf: status=206") and what RFC 9110 §15.3.7 forbids. --- + + it("plain GET (no Range header) → 200, never 206, even though R2 echoes a whole-object range", async () => { + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, + ); + const r = await get("/en/1.3/vyos-documentation.pdf", env); + expect(r.status).toBe(200); + expect(r.headers.get("content-range")).toBeNull(); + expect(r.headers.get("content-length")).toBe("9"); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("suffix Range (bytes=-4) → 206 with the last 4 bytes and a matching Content-Range/Length", async () => { + // The `{suffix}` R2Range shape used to miss the `"offset" in range` guard entirely and + // fall through to the 200 branch, where content-length claimed the WHOLE object size + // while the body held only the tail — a corrupt download for any resumed fetch. + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, // length 9 + { DOCS_ENV: "production" }, + ); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", range: "bytes=-4" }, + }), + env, + ); + expect(r.status).toBe(206); + expect(await r.text()).toBe("YTES"); + expect(r.headers.get("content-range")).toBe("bytes 5-8/9"); + expect(r.headers.get("content-length")).toBe("4"); + }); + + it("failed If-Match → 412, not 304 (a 304 would tell the client its stale copy is current)", async () => { + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, + ); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", "if-match": '"some-other-etag"' }, + }), + env, + ); + expect(r.status).toBe(412); + expect(await r.text()).toBe(""); + // 412 is an error response, so the §3.3 precedence forces no-store over the PDF class. + expect(r.headers.get("Cache-Control")).toBe("no-store"); + }); + + // --- RFC 9110 §13.2.2 precondition PRECEDENCE. R2 reports THAT an onlyIf precondition + // failed, never WHICH one, so index.ts re-derives it from the request's own headers. + // Testing the not-modified family first got the ordering backwards. --- + + async function conditional(headers: Record<string, string>, method = "GET") { + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, + ); + return worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + method, + headers: { "user-agent": "vitest", ...headers }, + }), + env, + ); + } + + it("If-None-Match alone (matching) → 304", async () => { + expect((await conditional({ "if-none-match": '"pdf-etag-1"' })).status).toBe(304); + }); + + it("If-Match + If-None-Match together → 412: If-Match takes strict precedence", async () => { + // The failing precondition here is If-Match. Answering 304 (because If-None-Match is + // also present) would tell the client its stale copy is still current, when the + // higher-precedence check it asked for actually failed. §13.2.2 steps 1 and 3. + const r = await conditional({ + "if-match": '"stale-etag"', + "if-none-match": '"pdf-etag-1"', + }); + expect(r.status).toBe(412); + expect(r.headers.get("Cache-Control")).toBe("no-store"); + }); + + it("If-Unmodified-Since → 412, never 304 (§13.2.2 step 2 outranks the 304 family)", async () => { + const r = await conditional({ + "if-unmodified-since": "Wed, 01 Jan 2020 00:00:00 GMT", + "if-none-match": '"pdf-etag-1"', + }); + expect(r.status).toBe(412); + }); + + it("a failed If-None-Match on a non-GET/HEAD method → 412, not 304", async () => { + // §13.1.2: on a false If-None-Match the origin MUST answer "304 ... if the request + // method is GET or HEAD or 412 ... for all other request methods". Nothing upstream + // restricts the method, so this path is reachable and 304 would be an invalid answer. + const r = await conditional({ "if-none-match": '"pdf-etag-1"' }, "POST"); + expect(r.status).toBe(412); + }); + + it("HEAD keeps the 304 (it is one of the two methods §13.1.2 allows it for)", async () => { + expect((await conditional({ "if-none-match": '"pdf-etag-1"' }, "HEAD")).status).toBe(304); + }); + + // --- RFC 9110 §14.1.2 / §15.5.17 range satisfiability. R2 signals "I ignored your + // Range" by returning the WHOLE object — the same shape as a satisfied whole-object + // range — so index.ts re-derives intent from the client's own header. --- + + async function ranged(rangeHeader: string, body = "PDF-BYTES") { + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body } }, + { DOCS_ENV: "production" }, + ); + return worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", range: rangeHeader }, + }), + env, + ); + } + + it("Range past EOF → 416 + Content-Range: bytes */size, NOT a 206 serving the whole body", async () => { + // The pre-fix path trusted obj.range, and R2 answers an unsatisfiable range with the + // complete object — so this returned `206 Content-Range: bytes 0-8/9` plus all 9 + // bytes. A client resuming at byte 99 would have appended bytes 0-8 to its partial + // file and silently corrupted the download. + const r = await ranged("bytes=99-"); // body is 9 bytes + expect(r.status).toBe(416); + expect(r.headers.get("content-range")).toBe("bytes */9"); + expect(await r.text()).toBe(""); + expect(r.headers.get("Cache-Control")).toBe("no-store"); // 416 >= 400 + }); + + it("Range starting exactly at EOF → 416 (§14.1.2: satisfiable iff first-pos < length)", async () => { + const r = await ranged("bytes=9-"); + expect(r.status).toBe(416); + expect(r.headers.get("content-range")).toBe("bytes */9"); + }); + + it("closed Range wholly past EOF → 416", async () => { + const r = await ranged("bytes=20-30"); + expect(r.status).toBe(416); + }); + + it("suffix-length 0 → 416 (§14.1.2 names it unsatisfiable)", async () => { + const r = await ranged("bytes=-0"); + expect(r.status).toBe(416); + expect(r.headers.get("content-range")).toBe("bytes */9"); + }); + + it("multi-range → 200 with the complete body, not a single-range 206 that misdescribes it", async () => { + // R2 ignores multi-ranges and returns the whole object. Stamping + // `Content-Range: bytes 0-8/9` on it would claim a single partial covering + // everything, in answer to a request for two disjoint sub-ranges. + const r = await ranged("bytes=0-1,4-5"); + expect(r.status).toBe(200); + expect(r.headers.get("content-range")).toBeNull(); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("malformed Range → 200 with the complete body (§14.1.2: invalid spec is ignored)", async () => { + for (const bad of ["bytes=abc", "bytes=-", "bytes=5-2", "items=0-5"]) { + const r = await ranged(bad); + expect(r.status, `Range: ${bad}`).toBe(200); + expect(r.headers.get("content-range"), `Range: ${bad}`).toBeNull(); + } + }); + + it("a satisfiable whole-object Range still gets a real 206", async () => { + // The 416/200 guards must not swallow the legitimate case: `bytes=0-` IS satisfiable + // (first-pos 0 < 9), so it keeps its 206 even though the payload is the whole object. + const r = await ranged("bytes=0-"); + expect(r.status).toBe(206); + expect(r.headers.get("content-range")).toBe("bytes 0-8/9"); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + // --- The classifier's grammar and R2's grammar are NOT the same grammar, and the + // Worker no longer assumes they are: it checks the bounds it derived against the bytes + // R2 actually returned before promising a 206. --- + + it("tab-separated Range → 200 with the whole body, never a 206 for bytes nobody sliced", async () => { + // R2 parses ranges with ASCII space only (/^ *bytes *=/i + /^ *(\d+)? *- *(\d+)? *$/); + // the classifier's \s also accepts a tab. So this header says "single, bytes 2-4" + // here and "unparseable, serve everything" to R2 — and trusting the classifier alone + // shipped `206 Content-Range: bytes 0-8/9` carrying all 9 bytes in answer to a + // request for 3. Same lying-206 class as the unsatisfiable case above. + const r = await ranged("bytes=2\t-\t4"); + expect(r.status).toBe(200); + expect(r.headers.get("content-range")).toBeNull(); + expect(r.headers.get("content-length")).toBe("9"); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("LEADING whitespace is stripped before either parser sees it, so only INNER whitespace diverges", async () => { + // Worth pinning because it bounds the divergence surface. `Headers` strips the + // optional whitespace around a field value (RFC 9110 §5.5), so "\tbytes=2-4" arrives + // as "bytes=2-4" and both grammars accept it — the 206 here is correct, not a + // regression. Only whitespace INSIDE the value (the test above) can reach the two + // parsers intact and be read differently by them. + const r = await ranged("\tbytes=2-4"); + expect(r.status).toBe(206); + expect(r.headers.get("content-range")).toBe("bytes 2-4/9"); + }); + + it("space-separated Range stays a 206 — R2 accepts spaces, so the bounds still agree", async () => { + // The degrade must be driven by actual disagreement, not by giving up on whitespace. + const r = await ranged("bytes = 2-4"); + expect(r.status).toBe(206); + expect(r.headers.get("content-range")).toBe("bytes 2-4/9"); + expect(await r.text()).toBe("F-B"); + }); + + it("positions above 2^53: an invalid spec is ignored (200), not read as unsatisfiable (416)", async () => { + // Number() rounds 9007199254740993 down to ...992, so `last < first` read as false and + // this invalid spec was promoted to "unsatisfiable" → 416. §14.1.2 says ignore it. + const r = await ranged("bytes=9007199254740993-9007199254740992"); + expect(r.status).toBe(200); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("a partial body the Worker did not ask for is a 503, never a 206 for bytes nobody requested", async () => { + // Belt and braces for a future R2 whose grammar accepts something this classifier + // calls "ignored". The body in hand is a slice of bounds the request never named: + // a 200 would ship Content-Length: 9 over 3 bytes, and a 206 would carry + // `Content-Range: bytes 2-4/9` in answer to `bytes=0-1,4-5` — a selected range the + // client did not select, which §14.4 does not permit. Neither is honest, so the + // divergence is surfaced as a failure (plus the r2-range-divergence log line) rather + // than dressed up as a success. Unreachable under today's workerd, which ignores + // multi-range and returns the whole object. + const env = makeEnv({ + DOCS_ENV: "production", + DOCS_PDFS: { + get: async () => ({ + httpEtag: ETAG, size: 9, uploaded: UPLOADED, + body: "F-B", range: { offset: 2, length: 3 }, + }), + } as unknown as R2Bucket, + }); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", range: "bytes=0-1,4-5" }, // classifier: ignored + }), + env, + ); + expect(r.status).toBe(503); + expect(r.headers.get("content-range")).toBeNull(); + }); + + // --- §14.2: "GET is the only method for which range handling is defined." --- + + it("HEAD + Range → 200, no Content-Range: Range is ignored on every method but GET", async () => { + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, + ); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + method: "HEAD", + headers: { "user-agent": "vitest", range: "bytes=0-3" }, + }), + env, + ); + expect(r.status).toBe(200); + expect(r.headers.get("content-range")).toBeNull(); + expect(r.headers.get("content-length")).toBe("9"); + }); + + it("POST + an unsatisfiable Range → 200, not 416: the header is ignored, not judged", async () => { + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, + ); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + method: "POST", + headers: { "user-agent": "vitest", range: "bytes=99-" }, + }), + env, + ); + expect(r.status).toBe(200); + expect(r.headers.get("content-range")).toBeNull(); + }); + + // --- §13.2.1: "a server MUST ignore the conditional request header fields defined by + // this specification when received with a request method that does not involve the + // selection or modification of a selected representation, such as CONNECT, OPTIONS, or + // TRACE." Only OPTIONS is testable through worker.fetch() — TRACE and CONNECT are + // forbidden methods in the fetch spec and `new Request` refuses to construct them. --- + + it("OPTIONS ignores conditionals entirely: neither a failing nor a matching one is judged", async () => { + // A stale If-Match reached R2 as an onlyIf, came back body-less, and this Worker had + // no reading of that but 412 — so an OPTIONS carrying a conditional a client had left + // lying around was refused where the same request without it succeeded. + const options = (headers: Record<string, string>) => worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + method: "OPTIONS", + headers: { "user-agent": "vitest", ...headers }, + }), + r2Env({ "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }), + ); + expect((await options({ "if-match": '"stale-etag"' })).status).toBe(200); // not 412 + expect((await options({ "if-unmodified-since": BEFORE_UPLOAD })).status).toBe(200); // not 412 + expect((await options({ "if-none-match": ETAG })).status).toBe(200); // not 304/412 + }); + + // --- RFC 9110 §13.1.5 If-Range. R2's R2Conditional carries only etagMatches / + // etagDoesNotMatch / uploadedBefore / uploadedAfter, so an If-Range in the forwarded + // Headers is silently DROPPED and the range applied unconditionally. --- + + async function withIfRange(ifRange: string, rangeHeader: string) { + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, + ); + return worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", range: rangeHeader, "if-range": ifRange }, + }), + env, + ); + } + + // --- Abandoned body streams. R2 hands back a body on paths whose response carries + // none; a stream that is neither sent nor cancelled holds its connection until GC. --- + + it("an unsatisfiable Range cancels the whole-object body it answers 416 without", async () => { + // The largest abandoned stream on any path here: R2 answers an unsatisfiable Range + // with the COMPLETE object, which for the 1.3 PDF is 29.2 MiB, and the 416 sends none + // of it. Cancelling aborts the transfer rather than draining or leaking it. + const sink = { cancelled: [] as string[] }; + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, sink, + ); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", range: "bytes=99-" }, + }), + env, + ); + expect(r.status).toBe(416); + expect(sink.cancelled).toEqual(["PDF-BYTES"]); + }); + + it("a stale If-Range cancels the sliced body it discards before re-reading", async () => { + const sink = { cancelled: [] as string[] }; + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, sink, + ); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", range: "bytes=4-", "if-range": '"stale-etag"' }, + }), + env, + ); + expect(r.status).toBe(200); + expect(await r.text()).toBe("PDF-BYTES"); + expect(sink.cancelled).toEqual(["BYTES"]); // the abandoned slice, not the served body + }); + + it("a served body is NEVER cancelled — the cleanup must not reach the response path", async () => { + const sink = { cancelled: [] as string[] }; + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, sink, + ); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", range: "bytes=4-" }, // satisfiable, honoured + }), + env, + ); + expect(r.status).toBe(206); + expect(await r.text()).toBe("BYTES"); + expect(sink.cancelled).toEqual([]); + }); + + it("If-Range matching the current ETag → the range is honoured, 206", async () => { + const r = await withIfRange(ETAG, "bytes=4-"); + expect(r.status).toBe(206); + expect(r.headers.get("content-range")).toBe("bytes 4-8/9"); + expect(await r.text()).toBe("BYTES"); + }); + + it("If-Range naming a STALE ETag → 200 with the complete new representation", async () => { + // The corruption case. R2 cannot evaluate If-Range, so it applied the range anyway and + // this returned bytes 4+ of the NEW object under a 206 — a resuming downloader then + // appends the new tail to its old prefix and silently produces a broken PDF. §13.1.5 + // requires the failed validator to yield the complete representation instead. + const r = await withIfRange('"stale-etag"', "bytes=4-"); + expect(r.status).toBe(200); + expect(r.headers.get("content-range")).toBeNull(); + expect(r.headers.get("content-length")).toBe("9"); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("a stale If-Range wins over an unsatisfiable spec → 200, not 416", async () => { + // Ordering matters: a Range being ignored entirely (§13.1.5) is decided before + // satisfiability (§14.1.2) is ever judged, so no 416 may escape here. + const r = await withIfRange('"stale-etag"', "bytes=99-"); + expect(r.status).toBe(200); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("a WEAK If-Range validator never matches (§13.1.5 requires a strong one)", async () => { + const r = await withIfRange('W/"pdf-etag-1"', "bytes=4-"); + expect(r.status).toBe(200); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("an HTTP-date If-Range never matches — this Worker emits no Last-Modified to compare against", async () => { + const r = await withIfRange(AFTER_UPLOAD, "bytes=4-"); + expect(r.status).toBe(200); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("an object rewritten between the two If-Range reads is still judged against the request's preconditions", async () => { + // §13.2.1 requires preconditions to hold for the representation ULTIMATELY SELECTED. + // The re-read after a stale If-Range was a BARE get(), dropping every other + // precondition the request carried, so: If-Match passes on read 1, the key is + // rewritten, and the bare read 2 then answered 200 with the very representation the + // client's If-Match excluded. The key does get rewritten — `force_pdf_refresh: true` + // in the legacy snapshot repo's deploy workflow re-uploads it. + const KEY = "legacy/1.3/vyos-documentation.pdf"; + const objects: Record<string, { body: string; etag?: string }> = { + [KEY]: { body: "PDF-BYTES", etag: ETAG }, + }; + // Borrow the shared mock, then wrap it so the object changes BETWEEN the two reads. + type Bucket = { get: (key: string, options?: unknown) => Promise<unknown> }; + const inner = (r2Env(objects) as unknown as { DOCS_PDFS: Bucket }).DOCS_PDFS; + let reads = 0; + const env = r2Env(objects, { + DOCS_ENV: "production", + DOCS_PDFS: { + get: async (key: string, options?: unknown) => { + const result = await inner.get(key, options); + if (++reads === 1) objects[key].etag = '"pdf-etag-2"'; // rewritten mid-flight + return result; + }, + }, + }); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { + "user-agent": "vitest", + range: "bytes=4-", + "if-range": '"stale-etag"', // fails → forces the whole-object re-read + "if-match": ETAG, // satisfied on read 1, violated by read 2's object + }, + }), + env, + ); + expect(reads).toBe(2); + expect(r.status).toBe(412); + expect(await r.text()).toBe(""); + // The validator reported is the one belonging to the object the verdict was reached on. + expect(r.headers.get("etag")).toBe('"pdf-etag-2"'); + expect(r.headers.get("content-type")).not.toBe("application/pdf"); + }); + + // --- §13.2.2 preconditions, decided by EVALUATING the validators rather than by + // guessing from which headers are present. --- + + it("If-Match satisfied + If-None-Match satisfied on a GET → 304, not 412", async () => { + // The inverse of the If-Match-fails case above, and the one presence-inference got + // wrong: If-Match matches (so step 1 passes) while If-None-Match also matches (so + // step 3 FAILS) — §13.1.2 owes a 304. Seeing an If-Match header at all returned 412. + const r = await conditional({ + "if-match": '"pdf-etag-1"', + "if-none-match": '"pdf-etag-1"', + }); + expect(r.status).toBe(304); + expect(await r.text()).toBe(""); + }); + + it("If-Modified-Since alone on a non-GET/HEAD → 200: §13.2.2 step 4 never evaluates it", async () => { + // R2 ANDs every validator it is handed and knows nothing about the method, so + // forwarding the raw headers made it fail the request on a validator the RFC says to + // ignore — and the only answer left was a 412. Filtering the conditionals down to the + // applicable set means the request simply proceeds. + const r = await conditional({ "if-modified-since": AFTER_UPLOAD }, "POST"); + expect(r.status).toBe(200); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("If-Modified-Since alone on a GET IS evaluated → 304", async () => { + // The other side of the filter: dropping the header for non-GET must not drop it here. + expect((await conditional({ "if-modified-since": AFTER_UPLOAD })).status).toBe(304); + }); + + it("a satisfied If-Unmodified-Since serves the object; a failed one is 412", async () => { + expect((await conditional({ "if-unmodified-since": AFTER_UPLOAD })).status).toBe(200); + expect((await conditional({ "if-unmodified-since": BEFORE_UPLOAD })).status).toBe(412); + }); + + it("If-Match: * matches any existing representation (§13.1.1)", async () => { + expect((await conditional({ "if-match": "*" })).status).toBe(200); + }); + + it("If-None-Match: * on an existing representation fails → 304 on a GET", async () => { + expect((await conditional({ "if-none-match": "*" })).status).toBe(304); + }); + + it("If-None-Match matches WEAKLY (§13.1.2 mandates the weak comparison)", async () => { + // A weak tag from the client must still match a strong stored tag, or every + // revalidation from a cache that weakened the tag re-downloads 29.2 MiB. + expect((await conditional({ "if-none-match": 'W/"pdf-etag-1"' })).status).toBe(304); + }); + + it("If-None-Match honours a comma-separated tag list", async () => { + const r = await conditional({ "if-none-match": '"other", "pdf-etag-1"' }); + expect(r.status).toBe(304); + }); + + it("If-Match compares STRONGLY: a weak tag from the client never satisfies it", async () => { + expect((await conditional({ "if-match": 'W/"pdf-etag-1"' })).status).toBe(412); + }); + }); + + describe("resolveRange (R2Range is a three-shape union)", () => { + it("resolves offset+length, length-only, and suffix forms, clamped to the object size", () => { + expect(resolveRange({ offset: 2, length: 3 }, 10)).toEqual({ start: 2, length: 3 }); + expect(resolveRange({ offset: 4 }, 10)).toEqual({ start: 4, length: 6 }); // to end of object + expect(resolveRange({ length: 4 }, 10)).toEqual({ start: 0, length: 4 }); // offset defaults to 0 + expect(resolveRange({ suffix: 3 }, 10)).toEqual({ start: 7, length: 3 }); // trailing bytes + expect(resolveRange({ suffix: 99 }, 10)).toEqual({ start: 0, length: 10 }); // suffix past start clamps + expect(resolveRange({ offset: 8, length: 99 }, 10)).toEqual({ start: 8, length: 2 }); // length clamps + }); + + it("never returns a negative length, so Content-Length can never go negative", () => { + // A real R2 binding cannot produce these, but resolveRange is exported and its + // contract says "clamped to the object size" — that must hold for every input the + // R2Range type admits, not just the ones observed in practice. + expect(resolveRange({ offset: 0, length: -5 }, 10)).toEqual({ start: 0, length: 0 }); + expect(resolveRange({ offset: 10, length: 5 }, 10)).toEqual({ start: 10, length: 0 }); + expect(resolveRange({ offset: 99 }, 10)).toEqual({ start: 10, length: 0 }); + expect(resolveRange({ suffix: -3 }, 10)).toEqual({ start: 10, length: 0 }); + expect(resolveRange({ offset: 0, length: 0 }, 0)).toEqual({ start: 0, length: 0 }); + }); + }); + + describe("classifyRangeHeader (§14.1.2 satisfiability, re-derived from the client's header)", () => { + // R2 cannot tell us: it answers unsatisfiable, malformed, multi-range and unknown-unit + // Range headers identically — with the complete object — which is also exactly what a + // satisfied whole-object range looks like. Verified against a real R2 binding. + it("single satisfiable byte ranges → single, with the concrete bounds they select", () => { + // The bounds are the point: they are what the Worker compares against the bytes R2 + // actually returned before it will promise a 206. + const cases: Array<[string, number, number]> = [ + ["bytes=0-", 0, 10], ["bytes=5-", 5, 5], ["bytes=0-0", 0, 1], + ["bytes=-3", 7, 3], ["bytes=0-9", 0, 10], + ["bytes=5-99", 5, 5], // last-pos clamps to EOF + ["bytes=9-", 9, 1], + ["bytes=-99", 0, 10], // suffix past the start is the whole object + ]; + for (const [h, start, length] of cases) { + expect(classifyRangeHeader(h, 10), h).toEqual({ kind: "single", start, length }); + } + }); + + it("unsatisfiable ranges → unsatisfiable", () => { + for (const h of ["bytes=10-", "bytes=99-", "bytes=10-20", "bytes=-0"]) { + // first-pos >= length, or suffix-length 0 + expect(classifyRangeHeader(h, 10), h).toEqual({ kind: "unsatisfiable" }); + } + }); + + it("multi-range, malformed and unknown-unit → ignored (§14.1.2: an invalid spec is ignored)", () => { + for (const h of ["bytes=0-1,4-5", "bytes=abc", "bytes=-", "items=0-5", "bytes=5-2", ""]) { + expect(classifyRangeHeader(h, 10), h).toEqual({ kind: "ignored" }); + } + }); + + it("tolerates the case and whitespace variation R2 itself accepts", () => { + // R2 honours all three of these, so misreading them as "ignored" would downgrade a + // legitimate 206 to a 200. + expect(classifyRangeHeader("BYTES=0-5", 10)).toEqual({ kind: "single", start: 0, length: 6 }); + expect(classifyRangeHeader("bytes = 0-5", 10)).toEqual({ kind: "single", start: 0, length: 6 }); + expect(classifyRangeHeader("bytes=0-5 ", 10)).toEqual({ kind: "single", start: 0, length: 6 }); + }); + + it("zero-length representation: only a non-zero suffix-range is satisfiable", () => { + // §14.1.2 states this case explicitly. `bytes=0-` fails first-pos < length (0 < 0). + expect(classifyRangeHeader("bytes=0-", 0)).toEqual({ kind: "unsatisfiable" }); + // Satisfiable, but it selects zero bytes — no Content-Range can describe an empty + // selection (§14.4), so the caller's `length > 0` guard sends it to a 200. + expect(classifyRangeHeader("bytes=-5", 0)).toEqual({ kind: "single", start: 0, length: 0 }); + }); + + it("positions above 2^53 compare exactly — an invalid spec stays ignored, not 416", () => { + // Number() rounds both of these to 9007199254740992, so `last < first` read as false + // and the spec was promoted from "invalid, ignore it" (§14.1.2 → 200) to + // "unsatisfiable" (→ 416). Digit-string comparison is exact at any magnitude. + expect(classifyRangeHeader("bytes=9007199254740993-9007199254740992", 10)) + .toEqual({ kind: "ignored" }); + // ...while a genuinely huge first-pos is still unsatisfiable. + expect(classifyRangeHeader("bytes=9007199254740993-", 10)).toEqual({ kind: "unsatisfiable" }); + // Leading zeros normalise rather than inflating the digit count. + expect(classifyRangeHeader("bytes=00000005-00000002", 10)).toEqual({ kind: "ignored" }); + expect(classifyRangeHeader("bytes=0000000002-0000000005", 10)) + .toEqual({ kind: "single", start: 2, length: 4 }); + }); + + it("accepts whitespace R2's own parser rejects — the divergence the bounds check absorbs", () => { + // R2 parses ranges with `/^ *bytes *=/i` + `/^ *(\d+)? *- *(\d+)? *$/` (ASCII space + // only; miniflare src/workers/shared/range.ts). This classifier's `\s` accepts a tab + // too, so the two grammars genuinely disagree here. That is tolerated by design: the + // Worker checks these bounds against the bytes R2 returned, so a spec R2 declined + // degrades to a 200 rather than to a 206 describing a body nobody asked for. The + // end-to-end proof is the "tab-separated Range" test below. + expect(classifyRangeHeader("bytes=2\t-\t4", 10)).toEqual({ kind: "single", start: 2, length: 3 }); + expect(classifyRangeHeader("\tbytes=2-4", 10)).toEqual({ kind: "single", start: 2, length: 3 }); + }); }); }); diff --git a/workers/apex/test/uagate.test.ts b/workers/apex/test/uagate.test.ts index f4c16c66..1989f847 100644 --- a/workers/apex/test/uagate.test.ts +++ b/workers/apex/test/uagate.test.ts @@ -17,8 +17,86 @@ describe("UA gate (§3.2.1) — ships log-only for AI crawlers", () => { it("unknown UA → allow (fail-open for humans)", () => { expect(uaVerdict("Mozilla/5.0 (X11; Linux x86_64) Firefox/128.0", policy)).toBe("allow"); }); + it("Applebot is allowed but Applebot-Extended is logged — most-specific match wins", () => { + // Apple's AI-training crawler token CONTAINS the search crawler's, so plain + // substring matching with a fixed allow-before-log precedence let the allow entry + // swallow it: the AI crawler was allowed AND never logged, unlike every other AI + // crawler in the log list. + expect(uaVerdict("Mozilla/5.0 (compatible; Applebot/0.1; +http://www.apple.com/go/applebot)", policy)).toBe("allow"); + expect(uaVerdict("Mozilla/5.0 (compatible; Applebot-Extended/0.1)", policy)).toBe("log"); + }); + it("Google-Extended is not a UA token — it must not sit in the UA policy at all", () => { + // Google-Extended is a robots.txt user-agent control token; it never appears in a + // User-Agent header, so an entry for it could only ever be dead weight. + // Compared case-INSENSITIVELY on both sides: bestMatch() lowercases every policy entry + // before matching, so "google-extended" would be functionally identical to the token + // this guard exists to keep out — but toContain() compares primitives by strict + // equality, so a lowercase variant would sail past a case-sensitive assertion and + // quietly restore the entry. Match the matcher's own case semantics. + const entries = [...policy.allow, ...policy.log, ...policy.block].map((e) => e.toLowerCase()); + expect(entries).not.toContain("google-extended"); + }); it("block takes precedence over allow on a UA matching both lists", () => { const dualMatch = { ...policy, allow: ["Googlebot"], block: ["Googlebot EvilScraper"] }; expect(uaVerdict("Mozilla/5.0 (compatible; Googlebot EvilScraper/1.0)", dualMatch)).toBe("block"); }); + + // --- allow-vs-log contests. Pinned verdicts for the four UAs that distinguish every + // candidate rule, so a future tweak to the precedence cannot silently drop telemetry. --- + + it("a UA carrying BOTH a log token and a longer allow token is logged, not allowed", () => { + // "GPTBot/1.0 DuckDuckBot" matches allow "DuckDuckBot" (11 chars) and log "GPTBot" (6). + // Under the longest-match rule the longer ALLOW needle won and the ua-log event never + // fired; under the original allow-before-log rule it also won. A UA presenting two + // different crawlers' tokens is precisely the shape worth recording, and `log` costs + // nothing but a log line — the request is served either way. + expect(uaVerdict("GPTBot/1.0 DuckDuckBot", policy)).toBe("log"); + }); + + it("pinned verdicts for the four discriminating UAs", () => { + expect(uaVerdict("Mozilla/5.0 (compatible; Applebot/0.1)", policy)).toBe("allow"); + expect(uaVerdict("Mozilla/5.0 (compatible; Applebot-Extended/0.1)", policy)).toBe("log"); + expect(uaVerdict("GPTBot/1.0 DuckDuckBot", policy)).toBe("log"); + expect(uaVerdict("Mozilla/5.0 (compatible; Googlebot/2.1)", policy)).toBe("allow"); + }); + + it("a narrow allow entry no longer overrides a matched log entry — log wins outright", () => { + // This branch used to return "allow" when the matched allow entry strictly CONTAINED + // the matched log entry, so a policy could carve a narrow allow out of a broad log + // entry. Removed as spoofable (see the next test). Both rows are now "log", which is + // the safe verdict — the request is still served either way; only telemetry differs. + const carveOut = { allow: ["Bytespider-Search"], log: ["Bytespider"], block: [] }; + expect(uaVerdict("Bytespider-Search/1.0", carveOut)).toBe("log"); + expect(uaVerdict("Bytespider/1.0", carveOut)).toBe("log"); + }); + + it("the removed carve-out was spoofable by quoting both tokens independently", () => { + // The concrete bypass. Containment was tested between the two matched ENTRIES, never + // against the UA's own token structure, so a request-controlled string naming both + // tokens separately matched allow "Bytespider-Search" and log "Bytespider", satisfied + // the containment test, and bought the AI crawler an `allow`. It must be logged. + const carveOut = { allow: ["Bytespider-Search"], log: ["Bytespider"], block: [] }; + expect(uaVerdict("Bytespider/2.0 Bytespider-Search/1.0", carveOut)).toBe("log"); + }); + + it("dropping the carve-out leaves every SHIPPED-policy verdict unchanged", () => { + // The vendor pair the shipped policy actually depends on runs the other way round: log + // "Applebot-Extended" is LONGER than allow "Applebot", so the allow entry never + // contained the log entry and log already won. No pair in ua-policy.json took the + // removed branch, so its removal is behaviour-preserving for what we ship. + expect(uaVerdict("Mozilla/5.0 (compatible; Applebot/0.1)", policy)).toBe("allow"); + expect(uaVerdict("Mozilla/5.0 (compatible; Applebot-Extended/0.1)", policy)).toBe("log"); + }); + + it("an entry present in BOTH lists resolves to log, not allow", () => { + // Equality is not containment. Listing the same token twice is an authoring error, and + // `log` is the resolution that cannot lose data. + const contradictory = { allow: ["CCBot"], log: ["CCBot"], block: [] }; + expect(uaVerdict("CCBot/2.0", contradictory)).toBe("log"); + }); + + it("block still short-circuits ahead of the allow-vs-log contest", () => { + const all3 = { allow: ["DuckDuckBot"], log: ["GPTBot"], block: ["EvilScraper"] }; + expect(uaVerdict("GPTBot/1.0 DuckDuckBot EvilScraper", all3)).toBe("block"); + }); }); diff --git a/workers/apex/ua-policy.json b/workers/apex/ua-policy.json index 4a3534eb..f0021be5 100644 --- a/workers/apex/ua-policy.json +++ b/workers/apex/ua-policy.json @@ -1,5 +1,5 @@ { "allow": ["Googlebot", "bingbot", "DuckDuckBot", "YandexBot", "Applebot", "UptimeRobot"], - "log": ["GPTBot", "CCBot", "ClaudeBot", "Google-Extended", "Bytespider", "PerplexityBot", "meta-externalagent"], + "log": ["GPTBot", "CCBot", "ClaudeBot", "Applebot-Extended", "Bytespider", "PerplexityBot", "meta-externalagent"], "block": [] } |