Audit the v-html sites: 9 live bindings → 4, all sanitising - #237
Conversation
Five card/search components regex-stripped tags and passed the result to
v-html. `/<[^<>]+>/g` cannot match a tag containing < or >, so removing
an inner tag reassembled a working one: <img<a> src=x onerror=alert(1)>
survived as <img src=x onerror=alert(1)> and executed.
Those five now use helpers/getPlainText.ts and {{ }}, removing the sink.
It sanitises with ALLOWED_TAGS: [] and RETURN_DOM_FRAGMENT, then reads
textContent, so it parses rather than pattern-matches.
A plain {{ }} swap with the old regex would have shipped a visible bug.
These Directus fields carry entities (für, ") that the regex
never decoded -- it did not need to, because v-html let the browser do
it. Interpolating that directly would print für on every umlaut.
ProfileCreationMainInfos and ProfileCreationDone passed CMS rich text to
v-html with no filtering at all, and cannot use {{ }}: intro_text carries
the brand-colour span. They now sanitise with DOMPurify, which preserves
that markup byte-identically.
Two commented-out v-html bindings in PodcastPlayer.vue are deleted. They
called require(), which does not resolve under Vite ESM. This document
had described them as fine build-time SVG inlining; they were dead.
getMetaInfo.ts keeps the same regex on purpose: it writes to a <meta
content> attribute, not innerHTML, and Nuxt escapes attribute values --
verified against a description containing a raw quote.
Verified in a browser, since these lists are client-rendered: 5 pages,
30 descriptions, zero entities in rendered text, zero child elements,
zero hydration warnings. Lint warnings 134 -> 129.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JkKMceYAzAYLrSyC42FWTf
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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: 96a6fb4f32
ℹ️ 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".
| }>() | ||
|
|
||
| // CMS rich text, so it keeps its markup — but it must be sanitised before reaching v-html. | ||
| const sanitizedIntroText = computed(() => DOMPurify.sanitize(props.introText)) |
There was a problem hiding this comment.
When the sanitization policy or dependency needs to change, this component and ProfileCreationDone.vue will not inherit updates made to the existing repository-wide abstraction in InnerHtml.vue; both newly duplicate its exact computed(() => DOMPurify.sanitize(...)) implementation. Render these fields through InnerHtml or extract a shared sanitization helper so the policy remains centralized.
AGENTS.md reference: AGENTS.md:L69-L73
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and acted on in 34f5fc3. This is the strongest comment on the PR.
You're right that I created the duplication, and right that InnerHtml.vue already was the abstraction. I'd considered rendering these fields through InnerHtml and rejected it for a reason that stands — it carries its own .inner-html styling (lime links, font-black on <strong>, whitespace-pre-line) which would visibly change the ProfileCreation design. But I stopped there instead of taking your second option, and the AGENTS.md rule you cite applies regardless of which one I picked:
Never duplicate logic across modules. If a pattern … is used in more than one place, extract it into a shared abstraction with a clear, generic name.
So there is now one module that owns the policy, helpers/sanitize.ts:
| export | for |
|---|---|
sanitizeHtml |
default policy, anything bound to v-html |
sanitizeInlineHtml |
as above but forbids <p>, for the news ticker's single scrolling line |
getPlainText |
real plain text, for {{ }} |
I went further than the comment asked, because the half-measure was worse. Fixing only my two additions would have left InnerHtml.vue and NewsTicker.vue still calling DOMPurify.sanitize inline — two of four policies centralised, which is the appearance of the rule rather than the rule. DOMPurify now appears in exactly one file:
$ grep -rn "DOMPurify" components/ helpers/ pages/ composables/ | grep -v helpers/sanitize.ts
(no matches)
NewsTicker's { FORBID_TAGS: ['p'] } became sanitizeInlineHtml rather than a config parameter — naming the intent keeps DOMPurify options from leaking back out to callers, which is the thing that made this drift in the first place.
The tests now cover the policy rather than only its callers: rich markup survives (including the brand-colour <span>), the ticker still loses <p> while keeping its text, and no payload leaves a live tag. The danger check is itself asserted against unsanitised input, so those assertions can't pass vacuously — my first attempt at it did, by matching onerror in text where it was inert content rather than an attribute.
There was a problem hiding this comment.
Pull request overview
Audits and hardens v-html usage in the Nuxt app by removing unnecessary HTML sinks (switching to plain interpolation with decoded plain text) and ensuring remaining sinks sanitize CMS-authored rich text. Adds a dedicated getPlainText helper plus unit tests to prevent reintroducing regex-based tag stripping.
Changes:
- Added
helpers/getPlainText.ts(DOMPurify-based plain-text extraction with entity decoding) andtest/getPlainText.test.ts. - Replaced regex +
v-htmlexcerpt rendering withgetPlainText+{{ }}in multiple card/list components. - Added DOMPurify sanitization for the remaining rich-text
v-htmlbindings and removed dead commented-outv-htmlcode inPodcastPlayer.vue.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| nuxt-app/helpers/getPlainText.ts | New helper to convert CMS rich text into decoded plain text for safe interpolation. |
| nuxt-app/test/getPlainText.test.ts | New unit tests covering entity decoding and regex-bypass payloads. |
| nuxt-app/components/MeetupCard.vue | Removes v-html excerpt sink; renders getPlainText(...) via interpolation. |
| nuxt-app/components/ConferenceCard.vue | Removes v-html excerpt sink; renders getPlainText(...) via interpolation. |
| nuxt-app/components/SpeakerListItem.vue | Removes v-html excerpt sink; renders getPlainText(...) via interpolation. |
| nuxt-app/components/PickOfTheDayListItem.vue | Removes v-html excerpt sink; renders getPlainText(...) via interpolation. |
| nuxt-app/components/SearchResultCard.vue | Removes v-html excerpt sink and uses getPlainText(...) across type branches. |
| nuxt-app/components/ProfileCreationMainInfos.vue | Keeps v-html but sanitizes CMS rich text before binding. |
| nuxt-app/components/ProfileCreationDone.vue | Keeps v-html but sanitizes CMS rich text before binding. |
| nuxt-app/components/PodcastPlayer.vue | Deletes commented-out dead v-html + require() SVG inlining. |
| docs/dependency-upgrade-plan.md | Documents the audit outcome and rationale in the upgrade plan. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import type { PropType } from 'vue' | ||
| import { computed, defineComponent } from 'vue' | ||
| import { getPlainText } from '~/helpers/getPlainText' | ||
| import type { ConferenceItem, MeetupItem } from '../types'; | ||
| import LinkButton from './LinkButton.vue' |
There was a problem hiding this comment.
Removed in 34f5fc3 — MeetupItem was indeed unused, and the trailing semicolon is gone with it.
One correction to the reasoning though: it does not fail linting here. Verified:
$ npx eslint components/ConferenceCard.vue
36:31 warning 'MeetupItem' is defined but never used @typescript-eslint/no-unused-vars
✖ 1 problem (0 errors, 1 warning)
no-unused-vars is deliberately set to warn in eslint.config.mjs, and CI gates on errors — that was a considered choice when the flat config landed, because switching @nuxt/eslint-config@1 on as-is turned 0 errors into 121 across a pre-existing backlog. So this was already there, already reported, and not blocking.
Worth fixing anyway since the line was in my diff: warnings 129 → 128.
| <script setup lang="ts"> | ||
| import ProfilePicture from '~/components/ProfilePicture.vue' | ||
| import { useProfileCreationStore } from '~/composables/useProfileCreationStore' | ||
| import DOMPurify from 'isomorphic-dompurify' | ||
| import { computed, ref, watch } from 'vue' |
There was a problem hiding this comment.
This one is not correct, and it's worth being precise because it claims a compile/runtime error.
storeToRefs is auto-imported by @pinia/nuxt, which the app has in modules. From the generated types:
.nuxt/imports.d.ts:73
export { defineStore, acceptHMRUpdate, usePinia, storeToRefs } from '@pinia/nuxt/dist/runtime/composables'
.nuxt/types/imports.d.ts:96
const storeToRefs: typeof import('@pinia/nuxt/dist/runtime/composables').storeToRefs
So no import is needed. Corroborated three ways: build exits 0, vue-tsc reports no new errors (ratchet steady at 263, and a missing identifier would be an error not a warning), and the page renders correctly on the deployed preview — mainInfos is populated and the intro text shows with its brand-colour span intact.
Two other things worth noting: the line is pre-existing and untouched by this PR — I added a DOMPurify import above it and changed defineProps to const props = defineProps, neither of which removes anything. And the premise that "other ProfileCreation components import it explicitly" doesn't hold either; the ones that use it rely on the same auto-import.
No change made.
Re-ran the rendered-output check against the Vercel preview, not just a local node .output/server, since that runtime is where isomorphic-dompurify broke in Phase 5. Three of four pages clean. The podcast detail page logs one hydration mismatch there. It is not this change: production, which does not have it, logs the identical warning on the same page. Logged as its own follow-up, with the detail that makes it findable -- it appears on the preview and on production but not on a local build of the same commit, so it tracks the deployment environment rather than the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JkKMceYAzAYLrSyC42FWTf
Verification — including one finding that is not this PRAll checks green. Smoke 18/18 in 17.0s, clean. I then re-ran the rendered-output check against the Vercel preview, not just the local
The hydration warning is pre-existing, and I checked rather than assumedProduction does not have this change and logs the identical warning on the same page:
So it tracks the deployment environment, not the code. That pattern is also the useful part of the finding: something renders differently between the ISR-cached HTML and the client, which points at time- or cache-dependent output rather than markup. Logged as its own follow-up in the plan ( Why I bothered checking the preview separatelyThe local run and the preview run disagreed, and only one of them reflects what users get. Had I stopped at the local |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
nuxt-app/components/ConferenceCard.vue:36
MeetupItemis imported but not used in this component, and the statement also has a trailing semicolon (the repo generally uses no semicolons). Removing the unused type import keeps the module tidy and avoids unused-import lint warnings if/when they’re tightened.
import type { PropType } from 'vue'
import { computed, defineComponent } from 'vue'
import { getPlainText } from '~/helpers/getPlainText'
import type { ConferenceItem, MeetupItem } from '../types';
My first note pointed at useNow.ts and date formatting. Both are wrong:
useNow is not used on that page and exists to prevent this, and ISR
staleness is ruled out because the cached and freshly rendered HTML are
byte-identical.
Records what is ruled out so nobody repeats it, including a
<template><!----></template> that looked conclusive but appears on every
page -- an artefact of comparing innerHTML, since browsers put template
content in .content.
Dev mode did not reproduce cleanly: it failed to serve the page chunk, so
the client rendered the error page against a real server render and every
warning was an artefact of that.
Also logs two real bugs found on the way:
- useWeightedRandomSelection seeds off an hourly bucket and renders on
four ISR-cached pages, so a client in the next hour bucket selects
different testimonials than the cached HTML. A genuine latent instance
of this same class, just not on the page that warns.
- useLoadingScreen keeps isLoading in a module-scope ref, which on the
server is shared across concurrent requests. LoadingScreen is the
first child of <main>, where the divergence appears. Worth fixing
either way -- SSR state belongs in useState.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JkKMceYAzAYLrSyC42FWTf
Codex was right, and it cites a rule this repo already states: AGENTS.md
says never duplicate logic across modules and extract shared abstractions.
The two ProfileCreation components each grew their own
computed(() => DOMPurify.sanitize(...)), duplicating InnerHtml.vue, which
would have made three places to edit for one policy change.
helpers/sanitize.ts is now the only module that touches DOMPurify:
sanitizeHtml default policy, for v-html
sanitizeInlineHtml as above but forbids <p>, for the news ticker
getPlainText real plain text, for {{ }}
getPlainText moved here from its own file, and InnerHtml and NewsTicker
were routed through it too -- otherwise the abstraction would be
half-done, with two of four policies still inline.
Also drops the unused MeetupItem type import Copilot flagged in
ConferenceCard.vue, which was one of the no-unused-vars warnings (129 ->
128). It was pre-existing, but the line is already in this diff.
Tests cover the policy itself now, not just its callers: rich markup
survives (including the brand-colour span), the ticker still loses <p>,
and no payload leaves a live tag. The danger check is itself tested
against unsanitised input so the assertions cannot pass vacuously.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JkKMceYAzAYLrSyC42FWTf
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docs/dependency-upgrade-plan.md:1366
- This section says the bypass payloads are covered by
test/getPlainText.test.ts, but the tests added in this PR are innuxt-app/test/sanitize.test.ts(i.e.test/sanitize.test.tsrelative tonuxt-app). Update the filename so the verification pointer is accurate.
Unit tests cover the bypass payloads, so the regex cannot come back unnoticed: `test/getPlainText.test.ts`,
docs/dependency-upgrade-plan.md:1343
- The document references
helpers/getPlainText.ts, but the implementation added in this PR lives inhelpers/sanitize.ts(thegetPlainTextexport). This makes the audit notes hard to follow and points to a non-existent file.
This issue also appears on line 1366 of the same file.
`helpers/getPlainText.ts` sanitises with `ALLOWED_TAGS: []` and `RETURN_DOM_FRAGMENT`, then reads
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
Came out of the
isomorphic-dompurifyrevert. That prompted "does the server need jsdom at all", and looking at the sinks instead of the dependency found something worth fixing.The defect
Five components stripped tags with a regex, then passed the result to
v-html:That pattern cannot match a tag containing
<or>, so removing an inner tag reassembles a working one:v-html<img<a> src=x onerror=alert(1)><img src=x onerror=alert(1)><svg<a> onload=alert(1)><svg onload=alert(1)>And two components —
ProfileCreationMainInfos,ProfileCreationDone— passed CMS rich text tov-htmlwith no filtering at all. Worse than the regex ones, which at least tried.Reachability, stated plainly: every input is CMS-authored, so exploiting this needs Directus write access or a tampered Algolia index. Defence in depth, not an open door. The strongest argument for fixing it was never the exploit — it's that four components were pushing plain text through an HTML sink for no reason.
Result
InnerHtml.vue,NewsTicker.vueDOMPurify.sanitizeProfileCreationMainInfos.vue,ProfileCreationDone.vuev-htmlDOMPurify.sanitize✅MeetupCard,SpeakerListItem,ConferenceCard,PickOfTheDayListItem,SearchResultCard(5 branches)v-htmlgetPlainText→{{ }}— sink removedPodcastPlayer.vue×2Two corrections to my own earlier plan
The count was wrong. The plan said "eleven bindings" and called the two in
PodcastPlayer.vuefine build-time SVG inlining. They're commented-out dead code callingrequire(), which wouldn't resolve under Vite ESM. Nine were live — which is what ESLint's ninevue/no-v-htmlwarnings had been saying all along."Just use
{{ }}" would have shipped a visible bug. These fields are WYSIWYG HTML containing entities —für,Baukästen,"Moin". The regex never decoded them; it didn't have to, because the value went tov-htmland the browser decoded it. Swapping to{{ }}with the same regex would have printedfüron every German umlaut on the site.&v-html(before){{ }}(naive fix)fürgetPlainText+{{ }}(shipped)für&helpers/getPlainText.tssanitises withALLOWED_TAGS: []+RETURN_DOM_FRAGMENT, then readstextContent— genuine text, every entity decoded. It parses instead of pattern-matching, which is the whole point.Deliberately not in
helpers/index.ts: that barrel is imported by server routes, and pullingisomorphic-dompurifythrough it would instantiate jsdom for consumers that only wanted a date helper.Why the ProfileCreation pair kept
v-htmlThey can't use
{{ }}—intro_textcontains<strong>programmier.<span style="color: #cfff00;">bar</span></strong>, so interpolation destroys the brand colour. DOMPurify's default profile preserves it byte-identically; verified offline and confirmed in the rendered page.Also notable: three sibling components (
ProfileCreationEmojis,ProfileCreationInterests,ProfileCreationDetails) already render the same singleton's fields with{{ }}and identical CSS classes. These two were inconsistent outliers, not a deliberate choice.Verification
Unit tests lock in the bypass so the regex can't return unnoticed —
test/getPlainText.test.ts, 5 cases including<img<a> src=x onerror=…>,<svg<a> onload=…>and the<math><mtext><script>mXSS vector.Rendered output needed a browser, because
SpeakerList,PickOfTheDayListandSearchResultCardare client-rendered — SSR HTML shows nothing for them. Five pages: 30 descriptions rendered, 0 HTML entities in rendered text, 0 child elements inside any description, 0 hydration warnings. That last one matters —getPlainTextruns under jsdom server-side and the real DOM client-side, so a parsing difference would surface as a hydration mismatch.Two of my checks failed before the code did, both the check's fault:
/konferenzforWeb & AI Edition 2026— that's Vue correctly escaping the literal&the helper now produces, and the browser decodes it back. I was reading source HTML where I should have read rendered text./hall-of-fameand/pick-of-the-dayand called it a pass. Those components aren't on those pages at all; they're on the detail routes. A zero count now fails.lintvue/no-v-htmltestbuildDeliberate non-changes
helpers/getMetaInfo.ts:48keeps the regex. It's the only other use of the pattern, and it is not an innerHTML sink — output goes to<meta content="...">. Checked rather than assumed: one CMS description has a raw"in its first 160 chars, and production renders it as", so Nuxt escapes attribute values. A surviving<img onerror=…>is inert there. Fixing it would also dragisomorphic-dompurifyinto thehelpersbarrel for no security gain.Three files left prettier-dirty exactly as they already were on
main(SpeakerListItem,ConferenceCard,SearchResultCard). FormattingSearchResultCardalone would reindent its entire template from 2 to 4 spaces, which has no business in this PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01JkKMceYAzAYLrSyC42FWTf