Plan: amend for the ClientOnly finding, #238, and the sanitiser module - #239
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Updates the Phase 5 dependency-upgrade plan document to correct previously inaccurate conclusions and to reflect outcomes from #237 and #238 (notably the <ClientOnly> negative control and the DOMPurify centralization).
Changes:
- Corrects the plan’s prior claim that
useWeightedRandomSelection/TestimonialSliderwas a latent hydration-mismatch source, documenting the<ClientOnly>finding and the negative-control result. - Updates the v-html / sanitization write-up to reflect
helpers/sanitize.tsas the single DOMPurify policy module (and removes stalehelpers/getPlainText.tspath references). - Adds/updates decision-log entries to capture the above corrections and the
useLoadingScreen→useStatechange context from #238.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b07365cb40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| to `v-html`. It parses instead of pattern-matching, which is the whole point: a regex cannot match a tag | ||
| containing `<` or `>`, so `<img<a> src=x onerror=alert(1)>` survived `/<[^<>]+>/g` as a working tag. | ||
|
|
||
| **`helpers/sanitize.ts` is the only module that touches DOMPurify**, after a review comment on #237 |
There was a problem hiding this comment.
When documenting the consolidation into helpers/sanitize.ts, also update the verification paragraph below: it still directs readers to test/getPlainText.test.ts, which does not exist at this commit because the tests moved to nuxt-app/test/sanitize.test.ts. The stated five-case coverage is also stale relative to the consolidated suite, so following this verification record currently leads to a nonexistent file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct on both counts — fixed in 2c721a5.
The path was stale because the tests moved to test/sanitize.test.ts when the DOMPurify policy was consolidated into helpers/sanitize.ts (the change this very paragraph documents), and the "5 cases" predated that. So the verification record pointed at a file that does not exist — following it would have led nowhere.
Now:
Unit tests cover the bypass payloads, so the regex cannot come back unnoticed:
test/sanitize.test.ts, 18 cases acrosssanitizeHtml,sanitizeInlineHtmlandgetPlainText[…] The danger check the assertions use is itself tested against unsanitised input, so it cannot pass vacuously — the first version did exactly that, matchingonerrorin text where it was inert content.
Counted rather than estimated: vitest reports 91 tests overall, 18 of them in that file.
Four other things had gone stale, not just this one
Your comment sent me through the document properly, and #240 had merged in the meantime. Also folded in:
- The
/_ipxfollow-up is done, and this document described its failure wrongly: it said the build "currently fails" when it fails intermittently — which is exactly why my first reproduction attempt passed in 193s and the next failed. Corrected, with the before/after numbers and a note that thenitro.staticguard is load-bearing even now that thegeneratescript is gone, sincenpx nuxi generatestill exists. - Static generation dropped, with the capacity figures that decided it (6183 image fetches; response times reaching 292s before ipx 500s).
- Two "known issues" entries still describing a full local build as broken.
- Five decision-log rows for Stop logging auth payloads; stop prerendering image URLs #240 — including the two places where checking changed a claim rather than confirming it: the token exposure is unconfirmed rather than confirmed, and the first version of the ignore rule was verified against the wrong mechanism.
All five intra-doc anchors re-verified against github-slugger, and a sweep confirms no getPlainText.test.ts or helpers/getPlainText.ts references remain. The one surviving npm run generate mention is deliberate — it is the past-tense record of why the script was removed.
Owed from #237 and #238. Part of this section had become actively wrong. Retracts the claim that useWeightedRandomSelection was a latent hydration-mismatch source. TestimonialSlider wraps its list in <ClientOnly>, so testimonials are never server-rendered and the hourly seed cannot participate in hydration. Keeps the correction rather than deleting the claim, because the reasoning looked sound and the negative control is what disproved it: with the client clock shifted past an hour boundary the unfixed build produced zero warnings too. Records the useLoadingScreen fix and, explicitly, that it is not claimed as the mismatch fix -- so both leads are now eliminated and the item stays open with the dev-mode-from-clean-.nuxt next step named. Updates the v-html write-up for helpers/sanitize.ts, which is now the only module touching DOMPurify. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JkKMceYAzAYLrSyC42FWTf
Rebased onto main now that #240 has merged, and amended for everything it changed. Addresses the review comment on this PR plus three other items that had gone stale. The reviewer's point: the verification paragraph still pointed at test/getPlainText.test.ts, which no longer exists -- the tests moved to test/sanitize.test.ts when the DOMPurify policy was consolidated -- and the "5 cases" count predated that. Now 18 cases across the three exports, with a note that the danger check is itself tested against unsanitised input, since its first version passed vacuously. Also: - The /_ipx follow-up is done. Corrects this document's own description of the failure: it said "currently fails" when it fails *intermittently*, which is why one reproduction attempt passed and the next did not. Records the guard on nitro.static as load-bearing, including why it stays now that the generate script is gone. - Static generation dropped, with the capacity numbers that decided it. - The two "known issues" entries that still described the local build as broken. - Five decision-log rows for #240, including the two places where checking changed a claim: the token exposure is unconfirmed rather than confirmed, and the first ignore rule was verified against the wrong mechanism. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JkKMceYAzAYLrSyC42FWTf
Documentation only — the amendment I said I'd make once #237 and #238 merged. Part of that section had become actively wrong, so it's worth landing rather than leaving.
The correction that matters
The plan claimed
useWeightedRandomSelectionwas "a genuine latent instance of this bug elsewhere". It isn't.TestimonialSlider.vuewraps its entire list in<ClientOnly>, so testimonials are never server-rendered — confirmed against production, whose SSR HTML contains no testimonial text, only the component's stylesheet link. The hourly seed cannot participate in hydration however far the clocks diverge.I've kept the retraction visible rather than quietly deleting the claim, because the reasoning looked sound and the thing that disproved it is the transferable part: a negative control. With the client clock shifted 61 minutes before any page JS ran, the unfixed build produced zero warnings too — so the test proved nothing, and that's what sent me looking for
<ClientOnly>instead of shipping a fix for a non-bug.Also recorded
useLoadingScreen→useState(Make loading-screen state per-request; and why the testimonial-seed fix was reverted #238), and explicitly not claimed as the mismatch fix:isLoadingisfalseon both sides in normal operation and the leak was never reproduced. So both leads are now eliminated, the hydration item stays open, and the next step is named — dev-mode reproduction from a clean.nuxt, the only tool that identifies the element.helpers/sanitize.tsis now the only module touching DOMPurify, after the Codex review comment on Audit the v-html sites: 9 live bindings → 4, all sanitising #237. Updates the v-html write-up, which still referred tohelpers/getPlainText.ts.useLoadingScreen(8 of 34 pages never call it, so the no-argument reset is load-bearing).Verification
Markdown only — no code touched. All five intra-doc anchors re-verified against
github-slugger, and no stalegetPlainText.tspath references remain.What's left in the programme
stripe20.4.1 → 22.4.0 is the last Phase 5 item, still waiting on a colleague's review of the payment path.🤖 Generated with Claude Code
https://claude.ai/code/session_01JkKMceYAzAYLrSyC42FWTf