fix(head): preserve dangerous html during client sync#1180
Conversation
Client-managed next/head tags currently drop dangerouslySetInnerHTML when hydration or navigation rebuilds vinext-owned head nodes. That diverges from Next.js and can leave SSR-injected script, style, or noscript content empty after the client projection runs. The client path treated raw HTML as unsafe even though the SSR path already accepts the developer opt-in. Reuse one typed prop application helper for client head nodes and set innerHTML from dangerouslySetInnerHTML with the same precedence as Next.js. Add regression coverage for client raw HTML application, empty __html fallback, and precedence over children.
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good fix — the old code was silently dropping dangerouslySetInnerHTML content on client sync, which broke inline <style> and <script> in next/head after hydration. The extracted _applyHeadPropsToElement is clean, the test double approach is sensible, and the Next.js references check out.
Two minor observations below, one of which is worth addressing.
| if (dangerouslySetInnerHTML) { | ||
| domEl.innerHTML = getDangerouslySetInnerHTML(dangerouslySetInnerHTML) ?? ""; | ||
| } else if (typeof props.children === "string") { | ||
| domEl.textContent = props.children; |
There was a problem hiding this comment.
Next.js's reactElementToDOM also handles array children by joining them:
el.textContent =
typeof children === 'string'
? children
: Array.isArray(children)
? children.join('')
: ''vinext only handles the string case here (and in the old inlined code). This isn't a regression from this PR — the old code had the same gap — but since you're extracting this into a named helper it's a good time to close it for parity.
| domEl.textContent = props.children; | |
| } else if (typeof props.children === "string") { | |
| domEl.textContent = props.children; | |
| } else if (Array.isArray(props.children)) { | |
| domEl.textContent = props.children.join(""); | |
| } |
| ): void { | ||
| const dangerouslySetInnerHTML = props.dangerouslySetInnerHTML; | ||
|
|
||
| if (dangerouslySetInnerHTML) { |
There was a problem hiding this comment.
Truthiness check: if (dangerouslySetInnerHTML) is falsy for 0, "", NaN, etc., but the real concern here is null and undefined (the only realistic "absent" values for this prop). Since getDangerouslySetInnerHTML already validates the shape, the truthiness gate is fine in practice. Just noting it for posterity — no change needed.
|
|
||
| expect(element.innerHTML).toBe("raw content"); | ||
| expect(element.textContent).toBe(""); | ||
| }); |
There was a problem hiding this comment.
Nice coverage. One more edge case worth adding: a test where _applyHeadPropsToElement receives only children (no dangerouslySetInnerHTML) to confirm the textContent path works and that the extracted helper didn't regress the common case. Something like:
it("sets textContent from children when dangerouslySetInnerHTML is absent", () => {
const element = createElementDouble();
_applyHeadPropsToElement(element, { children: "hello" });
expect(element.textContent).toBe("hello");
expect(element.innerHTML).toBe("");
});
|
Review posted. Here's a summary of the feedback: Overall: Good fix. The old code silently dropped Comments posted:
|
Address review feedback: - Handle Array.isArray(children) in _applyHeadPropsToElement by joining array elements, matching Next.js reactElementToDOM behavior - Add test for children-only path (no dangerouslySetInnerHTML) to ensure the extracted helper didn't regress the common case - Add test for array children joining
|
/bigbonk review |
There was a problem hiding this comment.
Good PR — correctly addresses the silent dropping of dangerouslySetInnerHTML during client head sync. The extracted helper is clean, test coverage is solid, the previous review feedback has been addressed (array children, children-only test), and all CI is green.
One real issue: there is an SSR/client behavior divergence for dangerouslySetInnerHTML: { __html: "" } (empty string). See inline comment.
| // SECURITY NOTE: This injects raw HTML. Developers must never pass | ||
| // unsanitized user input here — it is a stored XSS vector. | ||
| const html = getDangerouslySetInnerHTML(value); | ||
| if (html) innerHTML = html; |
There was a problem hiding this comment.
SSR/client parity gap with empty __html.
SSR (this line): if (html) innerHTML = html — when __html is "", getDangerouslySetInnerHTML returns "", but if ("") is falsy, so SSR skips the assignment and falls through to children.
Client (line 296): if (dangerouslySetInnerHTML) — the prop value { __html: "" } is an object (truthy), so the client enters the branch and sets innerHTML = "", which takes precedence over children.
This means <style dangerouslySetInnerHTML={{ __html: '' }}>fallback</style> would render fallback on SSR but empty on the client.
Next.js treats the presence of dangerouslySetInnerHTML as taking precedence regardless of __html value. This should be:
| if (html) innerHTML = html; | |
| const html = getDangerouslySetInnerHTML(value); | |
| if (html != null) innerHTML = html; |
This makes both paths consistent: if dangerouslySetInnerHTML is present with a valid __html (including empty string), it wins over children.
| const dangerouslySetInnerHTML = props.dangerouslySetInnerHTML; | ||
|
|
||
| if (dangerouslySetInnerHTML) { | ||
| domEl.innerHTML = getDangerouslySetInnerHTML(dangerouslySetInnerHTML) ?? ""; |
There was a problem hiding this comment.
Nit: this line is subtly correct but worth a comment. getDangerouslySetInnerHTML returns undefined for non-object values or missing __html, and the ?? "" fallback ensures innerHTML is set to empty string even for malformed props like dangerouslySetInnerHTML: { __html: 42 }. That's the right defensive behavior. No change needed, just noting it.
| /** Tags whose content is raw text — closing-tag sequences must be escaped during SSR. */ | ||
| const RAW_CONTENT_TAGS = new Set(["script", "style"]); | ||
|
|
||
| type HeadDOMElement = Pick<HTMLElement, "innerHTML" | "setAttribute" | "textContent">; |
There was a problem hiding this comment.
Minor: the Pick<HTMLElement, ...> type is a nice lightweight contract for testability. Worth noting this is intentionally narrow — it doesn't include removeAttribute or style, which is fine for head elements but would need expanding if this helper were reused elsewhere.
|
Review posted. Here's a summary: Overall: Good fix. The old code silently dropped One real issue found:
Two informational notes:
|
…rHTML
Address second review: dangerouslySetInnerHTML: { __html: '' } was
handled inconsistently between SSR and client. The SSR path used a
truthiness check (if (html)) which skipped empty strings, falling
through to children. The client path checked the prop object (truthy),
so empty __html correctly took precedence.
Fix: restructure headChildToHTML to check dangerouslySetInnerHTML before
children using != null, making both paths consistent with Next.js
semantics where dangerouslySetInnerHTML always wins when present.
|
/bigbonk review |
There was a problem hiding this comment.
Good fix. The core change is correct — the old code explicitly skipped dangerouslySetInnerHTML on the client path with a // skip for safety comment, which silently broke inline <style> and <script> content in next/head after hydration. The PR correctly aligns with Next.js's reactElementToDOM() behavior.
The extracted _applyHeadPropsToElement helper is clean, the test double approach avoids a DOM dependency, and the SSR/client parity fix for empty __html (using != null on SSR, object truthiness on client) looks correct. Tests pass.
One remaining issue to address below.
| // SECURITY NOTE: This injects raw HTML. Developers must never pass | ||
| // unsanitized user input here — it is a stored XSS vector. | ||
| innerHTML = rawHtml; | ||
| } else if (typeof props.children === "string") { |
There was a problem hiding this comment.
SSR/client parity gap: the SSR path only handles string children here, but the client path (_applyHeadPropsToElement at line 306) also handles Array.isArray(children) by joining them. If a <Head> child has array children (e.g. React.createElement("title", null, "Hello ", name)), the SSR path silently drops the content while the client path renders it.
This isn't a regression from this PR (the old SSR code had the same gap), but since you're already fixing SSR/client parity in this PR, it's worth closing the loop:
| } else if (typeof props.children === "string") { | |
| } else if (typeof props.children === "string") { | |
| innerHTML = escapeHTML(props.children); | |
| } else if (Array.isArray(props.children)) { | |
| innerHTML = escapeHTML(props.children.join("")); | |
| } |
| ): void { | ||
| const dangerouslySetInnerHTML = props.dangerouslySetInnerHTML; | ||
|
|
||
| if (dangerouslySetInnerHTML) { |
There was a problem hiding this comment.
This truthiness check works correctly for the { __html: "" } case (the object is truthy even when __html is empty), but it would incorrectly enter this branch for any truthy non-object value like dangerouslySetInnerHTML: "not an object" or dangerouslySetInnerHTML: 42. In that case getDangerouslySetInnerHTML returns undefined, the ?? "" kicks in, and the element's innerHTML is wiped — silently discarding both the malformed prop and any children.
Using getDangerouslySetInnerHTML as the gatekeeper (matching the SSR path's pattern) would be more robust:
| if (dangerouslySetInnerHTML) { | |
| const rawHtml = getDangerouslySetInnerHTML(props.dangerouslySetInnerHTML); | |
| if (rawHtml != null) { | |
| domEl.innerHTML = rawHtml; |
This makes SSR and client use the exact same entry point and condition shape, and avoids the silent content wipe for malformed props.
| expect(element.innerHTML).toBe("body { color: red; }"); | ||
| }); | ||
|
|
||
| it("uses an empty string for missing dangerouslySetInnerHTML.__html on the client", () => { |
There was a problem hiding this comment.
Good defensive test. Worth noting: this test documents that dangerouslySetInnerHTML: {} (missing __html key entirely) wipes innerHTML to empty. That's a reasonable choice, but it's different from what the SSR path does — SSR's getDangerouslySetInnerHTML({}) returns undefined, so rawHtml != null is false, and SSR falls through to children.
So <style dangerouslySetInnerHTML={{}} children="fallback" /> would render fallback on SSR but empty on client. This is the same class of SSR/client divergence the previous review flagged — using getDangerouslySetInnerHTML as the gatekeeper in _applyHeadPropsToElement (per my other comment) would fix this edge case too.
|
Review posted. Here's a summary: Overall: Good fix. The old code explicitly skipped Three comments posted:
|
…erHTML
Address third review round:
- SSR: add Array.isArray(children) handling to headChildToHTML, matching
the client path for cases like createElement('title', null, 'A', 'B')
- Client: replace truthiness check with getDangerouslySetInnerHTML as
the gatekeeper (matching SSR pattern). This fixes silent content wipes
for malformed props like dangerouslySetInnerHTML: 'string' or
dangerouslySetInnerHTML: {} (missing __html key)
- Both paths now use the exact same entry point and condition shape:
getDangerouslySetInnerHTML() != null
- Update tests: malformed dangerouslySetInnerHTML: {} now correctly
falls through to children on both SSR and client
|
/bigbonk review |
There was a problem hiding this comment.
Clean fix. The old code explicitly dropped dangerouslySetInnerHTML on the client path with // skip for safety, which silently broke inline <style> and <script> content in next/head after hydration. The new code correctly aligns with Next.js's reactElementToDOM() behavior.
Previous review feedback has been addressed: array children, children-only tests, SSR/client parity for empty __html, and using getDangerouslySetInnerHTML as the gatekeeper on both paths.
The extracted _applyHeadPropsToElement helper mirrors the structure of Next.js's reactElementToDOM closely — same precedence logic (dangerouslySetInnerHTML > string children > array children), same content-setting approach (innerHTML vs textContent). The vinext version is slightly more robust than Next.js's since getDangerouslySetInnerHTML validates the __html shape rather than using a bare truthiness check on the object.
Tests are thorough and cover the key edge cases. All 38 head tests and 858 shims tests pass.
Two minor observations below — neither is blocking.
| /** Tags whose content is raw text — closing-tag sequences must be escaped during SSR. */ | ||
| const RAW_CONTENT_TAGS = new Set(["script", "style"]); | ||
|
|
||
| type HeadDOMElement = Pick<HTMLElement, "innerHTML" | "setAttribute" | "textContent">; |
There was a problem hiding this comment.
Nit: The type name HeadDOMElement is a bit generic — someone reading it might expect it to represent a <head> element rather than "an element that lives inside <head>". Something like HeadChildDOMElement or ManagedHeadElement would communicate the intent more clearly. Not blocking.
| function getDangerouslySetInnerHTML(value: unknown): string | undefined { | ||
| if (typeof value !== "object" || value === null) return undefined; | ||
|
|
||
| const html = Reflect.get(value, "__html"); |
There was a problem hiding this comment.
Micro-nit: Reflect.get(value, "__html") works but is an unusual choice when a plain property access would do. The Reflect API is typically used when you need to forward a receiver or when working with Proxy traps. Since value is already validated as a non-null object, a simple (value as Record<string, unknown>).__html would be more conventional and marginally faster (no Reflect dispatch). Not blocking — the current code is correct.
|
Review submitted. Here's a summary: Approved. The fix correctly addresses the bug where Two non-blocking nits posted:
|
2 participants
Overview
next/headraw HTML content stable after vinext client head sync runs.dangerouslySetInnerHTML.__htmlto client-created head nodes instead of silently dropping it.next/headclient DOM projection. SSR escaping and tag filtering are unchanged.packages/vinext/src/shims/head.ts,tests/head.test.ts<style>,<script>, or<noscript>content innext/headno longer lose that content after hydration or client navigation.Why
next/headowns a client projection of the current head state. For parity, that projection needs to preserve the same developer-authored raw content that SSR accepted, subject to the same explicitdangerouslySetInnerHTMLopt-in.<Head>elements, including raw content.innerHTMLfromdangerouslySetInnerHTML.__htmlwhen the prop is present.dangerouslySetInnerHTMLas taking precedence overchildren.What changed
<Head><style dangerouslySetInnerHTML={{ __html }} /></Head>after client sync<style>node was recreated empty.<style>node receivesinnerHTML = __html.dangerouslySetInnerHTML={{}}innerHTML, matching Next.js.childrenanddangerouslySetInnerHTMLMaintainer review path
packages/vinext/src/shims/head.tsvalidates the new helper and its use fromsyncClientHead().tests/head.test.tscovers the client projection behavior without adding a DOM test dependency.Validation
vp test run tests/head.test.tsvp test run tests/head.test.ts tests/shims.test.tsvp checkThe first head test run was red before the implementation because
_applyHeadPropsToElementdid not exist. After implementation, the focused regression tests and broader shim coverage passed.Risk / compatibility
_applyHeadPropsToElementis exported only to keep the client projection unit-testable in this Node test environment.next/headnodes.Non-goals
<Head>.next/scriptbehavior tonext/head.References
reactElementToDOM()setsel.innerHTMLfromdangerouslySetInnerHTML.__htmldangerouslySetInnerHTMLin the generic attribute setter