Release: develop -> main - #38
Merged
Merged
Conversation
* fix(invite): serve the landing from the asset binding, not from the redirect An invite or promo link never reached the landing page. _routes.json hands the request to the Function first, so the 200-rewrite in _redirects never runs, and the asset lookup behind context.next() answers for the path as asked: 404, with the site's own 404 page. The older rewrite injected the campaign's title into that page, which is why a shared link looked roughly right in a browser while being the wrong document with the wrong status — and why every share crawler dropped it before reading a tag. Measured on a preview deploy, three lookups side by side on /invite/AB12CD: context.next() 404, 2693 bytes (the 404 page) context.next(request for index.html) 308 env.ASSETS.fetch(index.html) 200, 9827 bytes (the shell) So the shell is read from the asset binding by name, injected, and answered 200 — for HEAD as well as GET. Without the binding, or if the file under that name is not the shell any more, the platform's own answer stands rather than an invented one. functions/_middleware.js is under coverage at 100%. It was outside it while it decided which page every visitor and crawler sees, which is how the previous attempt reached production. * docs(invite): say where the landing comes from, now that it is not the redirect CONTRIBUTING and README described the Function as a rewrite of bytes that were already the landing. They were not: the 200-rewrites in _redirects never run on these paths, so the answer behind context.next() is the site's 404 page. Both now say that the shell is read from the asset binding by name, checked against the two landing marks, injected and answered 200. * fix(invite): answer only the case the platform gets wrong Reading the shell for every landing path also replaced two answers that were already right: /invite/ and /promo/ are real files the platform serves with 200, and /invite/index.html comes back as the 308 that canonicalises it. The first was still correct because it was rewritten anyway; the second was not — serving the landing under the explicit file name would quietly create a second URL for the same page. The platform is now asked first. A 404 is the one answer that means the 200-rewrite never resolved the path, and only that case is answered from the asset binding. Everything else stands: a redirect is handed back as it came, a non-HTML answer likewise, and an HTML answer that really is the shell keeps its status and is only rewritten in place. * docs(invite): name the four routed paths, the new gate, and what stands Three statements no longer matched the code they sit next to: - the coverage comment still said only browser logic is measured to 100%, while the include beside it now names the Pages Function as well; - the quality-gate table did not mention the 100% gate this adds for functions/_middleware.js, which is the gate whose absence let a change that served the 404 page on every invite link through a full review; - both docs said _routes.json hands /invite/* and /promo/* to the Function, where the file names four paths and check-site.mjs requires all four. The paragraphs also still described the old shape, where the shell was read for every landing path. They now say the platform is asked first and only its 404 is replaced, and that /invite/ is rewritten in place while /invite and /invite/index.html keep their 308. A test carries the request URL through onRequest with ?lang=en. Passing the pathname alone would have produced a landing in the wrong language with nothing in this file to notice. * fix(invite): build the answer's headers instead of copying them Copying the source response's headers carried in the ones public/_headers had already set, and Pages sets them again on the way out. Measured on the deploy, before this change and on both the old code and the new: /invite/ answers with cache-control: public, max-age=60, public, max-age=60 The answer's headers are now built: a content-type, and whatever _headers adds. That removes the duplicate and makes the list of headers to delete unnecessary, since none of them is carried in to begin with. The case that asserted their absence now asserts the whole header set, and its fixture sets a Cache-Control so the duplicate it guards against is reachable. The shell lookup asks for no redirect to be followed. A 3xx was already refused by the ok check; saying so at the call is cheaper than relying on it. Two assertions were weaker than they read. The invite case checked that the string og:title appears, which both shells carry already; it now checks the injected value. The promo case checked for the bare code, which an invitation-shaped injection would also satisfy; it now checks the kind. * fix(invite): drop a redirect mode the runtime will not take Asking the asset binding for redirect: 'manual' was a guard against a 3xx the ok check already refuses. Measured on a preview deploy: with it, the Function fails and Pages falls back to serving assets directly — /invite/<code> answered 404 with the site's 404 page again, and /invite/ came back unrewritten. It is removed, and the case that suggested it stays covered by the ok check. The commit that added it also blamed the duplicated Cache-Control on /invite/ on this pass copying headers. That was wrong: public/_headers has a rule for /invite/ and another for /invite/*, and /invite/ matches both, so Pages appends the value twice. It predates this change and is untouched by it. Building the answer's headers is still right — a stale validator must not be carried into a rewritten document — and the comment now says that instead. * fix(invite): carry the site's headers, drop only what the rewrite invalidates Building the answer's headers from scratch took the site's own headers off every landing. public/_headers is applied to the asset, not to a response this Function constructs, so the copy is what carries them. Measured on a preview deploy: without it, /invite/<code> came back with a content-type and nothing else — no Content-Security-Policy, no X-Frame-Options, no X-Content-Type-Options, no Referrer-Policy, no Cache-Control. The source's headers are carried again, minus the ones that described the bytes before the injection: the length, the content coding, both validators and the integrity digests. The case now asserts both halves — that the stale ones are gone and that a security header and the cache policy survive — and its fixture sets a Content-Security-Policy so the second half can fail. * docs(invite): say which paths the platform gets wrong, not all four The README said context.next() answers those paths with the site's 404 page, where those paths was the four _routes.json hands over. Only the code-bearing ones come back 404; /invite/ and /promo/ are real files answered 200, and /invite and /invite/index.html come back as a 308 — which the next sentence of the same paragraph already said. CONTRIBUTING was already narrow. * test(invite): pin the method the shell is asked with, and catch a rejecting binding The asset mock ignored the request's method and its path. Both mattered: - If the shell were ever asked for with the client's method, a HEAD would come back without a body, the landing marks would not be found, and the answer would fall back to the platform's 404 — the bug this pass exists to remove. The mock now records the method and a case requires GET. - The mock returned the same body for every URL, so the promo case proved the shell was asked for but not that it was used. It is path-aware now, and the promo case reads the promo shell's own copy. The binding is also asked inside a try. A Function that throws makes Pages serve the assets directly, which is precisely the wrong answer here; that was measured the hard way earlier on this branch. A rejecting binding now leaves the platform's answer standing, and a case holds that shut. * fix(invite): let a HEAD answer as the GET does on the codeless landing too A HEAD answer carries no body, and the body is what tells the landing shell from any other page. On /invite/ and /promo/ — real files the platform answers 200 — the empty body failed the shell check, so the platform's own response was handed back with the file's ETag, Content-Length and content coding intact, beside a GET on the same URL that had all three stripped. A client could then be answered 304 against a document it never received, which is the hazard the stripping exists for. A HEAD now takes the same route as a 404: the shell is read from the binding with a GET, and the answer is what the GET gets, without a body. The status branch is explicit about which answers this pass may touch at all — 200 and 404 — so a redirect or any other status is handed back before anything is read, and a 200 that is not HTML likewise. A case holds the two methods together on the codeless landing, and the content-type check on the code path now asserts the value rather than only that GET and HEAD agree. * fix(invite): take the answer's headers from the URL that was asked for A HEAD on the codeless landing built its answer from the shell's own headers, where the GET beside it used the platform's. public/_headers matches on the request path, so the shell's set was matched on /invite/index.html and the platform's on /invite/ — the same values today, but two sources for one URL, and the case that holds the two methods together said so by failing. The platform's headers are used wherever it had an answer of its own, and the shell's only stand in for the 404 it could not answer. * test(invite): the answer with no content-type at all The media-type check reads a missing header as an empty string, and no case reached it, so the branch stood uncovered and the 100% gate said so. * docs(invite): the injection also writes the store hand-off, and crawlers only can Two statements claimed more than they hold. The file header said every crawler snapshots the injected tags, which is a claim about other people's software; README already says can. And the in-place branch said only the metadata goes in, where the same injection also rewrites the Play href and the android-app and ios-app alternates when the URL carries a code. * fix(invite): guard the shell's body, not only the call that fetches it The status can be fine and the stream still fail. Reading the body outside the guard would take the Function down, and a Function that throws makes Pages serve the assets directly — which is the answer this pass exists to replace. The status check and the read are inside the try now, and a case gives back a response whose text() rejects. The mock also declared exactly the content-type the assertion expected, so dropping the line that sets it would not have shown. The fixture declares text/html without a charset and the answer is still required to say UTF-8. * docs(invite): say that a HEAD takes the other route, and drop a contradicted only Five statements said something the code does not do, all of them about the same thing: a HEAD under these paths never takes the in-place route. Its answer carries no body, so the shell cannot be recognised in it, and it reads the shell from the binding exactly as the 404 does. CONTRIBUTING, README and the comment above the branch all said the real files are only rewritten in place, without the GET that makes it true. Two more were narrower. The comment over the header source said the shell's set stands in when the platform had nothing to say, where the platform did answer and its status was simply not 200. The comment over the response said a code-bearing path is answered 200, where the same line also serves a codeless GET keeping its own 200 and a codeless HEAD. And this PR contradicted a rule it left alone: the ground rule said public/js/lib is the only code with a unit-coverage gate, which the quality-gate table stopped being true of the moment this PR added one for the middleware. The case that holds GET and HEAD together now also asserts that they reach the same answer by the two different routes, which is what the corrected sentences describe. * fix(invite): guard the in-place read too, and hold the stale list on both routes The shell route reads its body inside a guard because a Function that throws makes Pages serve the assets directly — the answer this pass exists to replace. The in-place route read the platform's body without one, for no reason other than that it was written first. It has the same guard now, and a case gives it a body that rejects. The stale-header list was only asserted on the route that reads the shell. A case now sets all eight on a real 200 file and requires the in-place rewrite to drop them as well, while the site's own headers survive. * docs(invite): bound the HEAD exception, and say where the hand-off applies Three statements were wider than the code. The comment said every HEAD under these paths takes the shell route; a redirect and a non-HTML answer leave before that branch, whatever the method, and a case now holds both shut for a HEAD. The in-place branch said the store hand-off goes in, where on /invite/ and /promo/ there is no code and the injection is a no-op. And the README's testing section still named only the browser logic as measured, while this PR added the gate for functions/_middleware.js to CONTRIBUTING — the same one-of-two-places inconsistency that was already corrected once in this branch. * test(invite): assert what the injection changes, not what the shell already said Three assertions were true of the fixture before any code ran. The English case looked for en_GB anywhere, which the shell carries as its alternate locale; it now requires the main og:locale tag to have changed and the document language with it. The in-place case looked for the German title and the landing marks, neither of which the injection touches; it asks in English instead, so the locale and the language are what it reads. And the cache policy was only compared between HEAD and GET, so losing it on both would have passed. This is the class of test that let the original defect through a full review: green whether the code works or not. * docs(invite): three comments that claimed a little more than they hold The branch comment said both cases answer as the GET does, where a HEAD answers without a body — which the line two above it states and this one dropped. The JSDoc over the answer said the campaign is written into it, where the campaign metadata and the store hand-off go in only when the URL carries a code, and the URL tags, the locale and the language always. And the coverage comment said the middleware decides what every crawler sees, where it is routed to the invite and promo paths alone. * docs(invite): stop enumerating what the injection writes Both sentences were mine from the previous commit and both were too precise to be true. The locale is written on ?lang=en, not always; the campaign title also appears without a code when English is asked for; and a bare /invite/ can carry a code as a query parameter, which the comment said it could not. Neither sentence needs the enumeration. They now name where each decision lives instead of restating it at a distance, which is what kept going wrong. --------- Co-authored-by: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Co-authored-by: TaprootFreakAI <315477232+TaprootFreakAI@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automatic Release PR
This PR was automatically created after changes were pushed to develop.
Commits: 1 new commit(s)
Checklist