fix(toast): native gea refactor with manual HTML escaping for XSS#9
fix(toast): native gea refactor with manual HTML escaping for XSS#9dgknbtl wants to merge 4 commits intodashersw:mainfrom
Conversation
This change replaces the vulnerable innerHTML assignment in the Toast component with a secure DOM manipulation approach. By using textContent and createElement, user-provided title and description data are rendered as plain text, preventing malicious script injection while maintaining accessibility and layout.
📝 WalkthroughWalkthroughRefactors toast rendering from imperative DOM/innerHTML diffing to declarative templating with escaped content; updates subscription to request a re-render; adds an XSS safety test and a showcase button that triggers a malicious-description toast. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/gea-ui/tests/toast-xss.test.ts (2)
7-39: Guard global cleanup withtry/finallyto avoid leaking test state on failures.If any assertion fails before Line 35, globals remain mutated and can taint later tests.
Suggested change
-test('Toast: _createToastElement prevents XSS via description', () => { +test('Toast: _createToastElement prevents XSS via description', () => { const dom = new JSDOM('<!doctype html><html><body></body></html>') - globalThis.window = dom.window as any - globalThis.document = dom.window.document - globalThis.HTMLElement = dom.window.HTMLElement + const prevWindow = (globalThis as any).window + const prevDocument = (globalThis as any).document + const prevHTMLElement = (globalThis as any).HTMLElement + + try { + globalThis.window = dom.window as any + globalThis.document = dom.window.document + globalThis.HTMLElement = dom.window.HTMLElement @@ - // Cleanup - dom.window.close() - delete (globalThis as any).window - delete (globalThis as any).document - delete (globalThis as any).HTMLElement + } finally { + dom.window.close() + if (prevWindow === undefined) delete (globalThis as any).window + else (globalThis as any).window = prevWindow + if (prevDocument === undefined) delete (globalThis as any).document + else (globalThis as any).document = prevDocument + if (prevHTMLElement === undefined) delete (globalThis as any).HTMLElement + else (globalThis as any).HTMLElement = prevHTMLElement + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gea-ui/tests/toast-xss.test.ts` around lines 7 - 39, Wrap the test body that sets globals and uses Toaster/_createToastElement in a try/finally: after creating dom and assigning globalThis.window, document and HTMLElement, run the assertions inside try, and in the finally block call dom.window.close() and delete the mutated globals (window, document, HTMLElement) to ensure cleanup even if an assertion fails; reference Toaster and _createToastElement to locate the test's core logic to surround with try/finally.
15-20: Consider adding a title-based payload assertion for full regression coverage.This test validates
descriptionescaping; adding the same check fortitlewould cover both escaped fields changed in the component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gea-ui/tests/toast-xss.test.ts` around lines 15 - 20, Add the same malicious payload check for the toast title: set toastData.title to maliciousPayload (the same string used for description) and add an assertion that the rendered title is escaped/contains the literal text rather than executing HTML (e.g., ensure window.xssDetected remains undefined/false and/or the title element's textContent contains the raw payload rather than an <img> tag). Update the test that uses maliciousPayload and toastData so both title and description fields are validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/gea-ui/tests/toast-xss.test.ts`:
- Around line 2-6: The test file imports the default test function from
'node:test' but the repository standard requires named imports; change the
import statement to use { describe, it } from 'node:test' and update the test
block to use describe(...) with an it(...) for the actual assertion (keeping
existing JSDOM and Toaster imports intact), ensuring the test name and body are
preserved when replacing the test(...) call with describe + it so the file uses
describe/it from 'node:test'.
---
Nitpick comments:
In `@packages/gea-ui/tests/toast-xss.test.ts`:
- Around line 7-39: Wrap the test body that sets globals and uses
Toaster/_createToastElement in a try/finally: after creating dom and assigning
globalThis.window, document and HTMLElement, run the assertions inside try, and
in the finally block call dom.window.close() and delete the mutated globals
(window, document, HTMLElement) to ensure cleanup even if an assertion fails;
reference Toaster and _createToastElement to locate the test's core logic to
surround with try/finally.
- Around line 15-20: Add the same malicious payload check for the toast title:
set toastData.title to maliciousPayload (the same string used for description)
and add an assertion that the rendered title is escaped/contains the literal
text rather than executing HTML (e.g., ensure window.xssDetected remains
undefined/false and/or the title element's textContent contains the raw payload
rather than an <img> tag). Update the test that uses maliciousPayload and
toastData so both title and description fields are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4d31900-94a5-480d-94fd-635f041f0448
📒 Files selected for processing (2)
packages/gea-ui/src/components/toast.tsxpackages/gea-ui/tests/toast-xss.test.ts
| import test from 'node:test' | ||
| import { JSDOM } from 'jsdom' | ||
| import { Toaster } from '../src/components/toast' | ||
|
|
||
| test('Toast: _createToastElement prevents XSS via description', () => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/gea-ui/tests/toast-xss.test.tsRepository: dashersw/gea
Length of output: 1745
Use describe/it imports from node:test per repo test standard.
Line 2 currently uses default test import; this test file must use { describe, it } named imports as required by repository testing guidelines.
Suggested change
import assert from 'node:assert/strict'
-import test from 'node:test'
+import { describe, it } from 'node:test'
import { JSDOM } from 'jsdom'
import { Toaster } from '../src/components/toast'
-test('Toast: _createToastElement prevents XSS via description', () => {
+describe('Toast', () => {
+ it('_createToastElement prevents XSS via description', () => {
const dom = new JSDOM('<!doctype html><html><body></body></html>')
globalThis.window = dom.window as any
globalThis.document = dom.window.document
globalThis.HTMLElement = dom.window.HTMLElement
// We can test the Toaster component directly
const toaster = new Toaster()
const maliciousPayload = "<img src=x onerror='window.xssDetected=true'>"
const toastData = {
id: 'test-toast',
title: 'Safe Title',
description: maliciousPayload
}
// `@ts-ignore` - access to internal method
const el = toaster._createToastElement(toastData)
// In JSDOM, when assigning textContent, the string is not parsed as HTML.
// We check that the malicious img tag is NOT present as an element.
const img = el.querySelector('img')
assert.equal(img, null, 'Malicious img tag should NOT be rendered')
const descEl = el.querySelector('[data-part="description"]')
assert.ok(descEl, 'Description element should exist')
assert.equal(descEl.textContent, maliciousPayload, 'Payload should be rendered as plain text')
// Cleanup
dom.window.close()
delete (globalThis as any).window
delete (globalThis as any).document
delete (globalThis as any).HTMLElement
+ })
+})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/gea-ui/tests/toast-xss.test.ts` around lines 2 - 6, The test file
imports the default test function from 'node:test' but the repository standard
requires named imports; change the import statement to use { describe, it } from
'node:test' and update the test block to use describe(...) with an it(...) for
the actual assertion (keeping existing JSDOM and Toaster imports intact),
ensuring the test name and body are preserved when replacing the test(...) call
with describe + it so the file uses describe/it from 'node:test'.
|
Isn't escaping the inputs a better idea? |
@dashersw, I used textContent because it's natively safe. Browser handles the encoding for me, so I don't need to manage it manually. We are only handling plain text here, so it’s a simple and clean way, I think. |
|
Why don't we convert this into a native Gea component? It can manage its own DOM with a .map on toasts, no? |
|
I got it. I've converted it to a native Gea component. Note that I also had to add a manual escapeHTML helper because the Gea JSX engine doesn't automatically sanitize strings, which was necessary to fully fix the XSS. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/gea-ui/src/components/toast.tsx`:
- Around line 25-37: The escapeHTML function can throw if passed a non-string
truthy value (e.g., number); update escapeHTML to coerce the input to a string
before calling replace and still handle null/undefined/empty by returning ''. In
practice, inside escapeHTML (the function shown), first check for null/undefined
and return '', otherwise convert the value to String(str) (or use `${str}`) and
run the replace on that resulting string so numbers/objects with primitive
string representations are safe.
- Around line 201-226: In the toast renderer mapping over this._currentToasts,
remove the escapeHTML(...) wrappers around t.title and t.description so the JSX
renders the raw values (e.g., change escapeHTML(t.title) and
escapeHTML(t.description) to just t.title and t.description) — this ensures
Gea's renderer uses textContent/nodeValue automatic escaping instead of
double-escaping; locate the JSX inside the component where key={t.id} and
data-part="title"/"description" are rendered and update those expressions
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cecb23b6-5170-4e7c-8be6-d94c1262c3ea
📒 Files selected for processing (3)
examples/showcase/app.tsxpackages/gea-ui/src/components/toast.tsxpackages/gea-ui/tests/toast-xss.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/gea-ui/tests/toast-xss.test.ts
| const escapeHTML = (str: string) => { | ||
| if (!str) return '' | ||
| return str.replace(/[&<>"']/g, (m) => { | ||
| switch (m) { | ||
| case '&': return '&' | ||
| case '<': return '<' | ||
| case '>': return '>' | ||
| case '"': return '"' | ||
| case "'": return ''' | ||
| default: return m | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add type coercion to handle non-string values safely.
The falsy check handles null, undefined, and empty strings, but a truthy non-string value (e.g., a number) would cause a runtime error on .replace(). Since toast data may come from external sources, consider coercing to string first.
🛡️ Proposed fix
const escapeHTML = (str: string) => {
- if (!str) return ''
- return str.replace(/[&<>"']/g, (m) => {
+ if (str == null) return ''
+ const s = String(str)
+ return s.replace(/[&<>"']/g, (m) => {
switch (m) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/gea-ui/src/components/toast.tsx` around lines 25 - 37, The
escapeHTML function can throw if passed a non-string truthy value (e.g.,
number); update escapeHTML to coerce the input to a string before calling
replace and still handle null/undefined/empty by returning ''. In practice,
inside escapeHTML (the function shown), first check for null/undefined and
return '', otherwise convert the value to String(str) (or use `${str}`) and run
the replace on that resulting string so numbers/objects with primitive string
representations are safe.
| > | ||
| {this._currentToasts.map((t: any) => ( | ||
| <div | ||
| key={t.id} | ||
| data-part="toast-root" | ||
| data-toast-id={t.id} | ||
| class="toast-root group pointer-events-auto relative flex w-full items-center justify-between space-x-2 overflow-hidden rounded-md border p-4 shadow-lg transition-all bg-background text-foreground" | ||
| > | ||
| <div class="grid gap-1"> | ||
| {t.title && ( | ||
| <div data-part="title" class="toast-title text-sm font-semibold"> | ||
| {escapeHTML(t.title)} | ||
| </div> | ||
| )} | ||
| {t.description && ( | ||
| <div data-part="description" class="toast-description text-sm opacity-90"> | ||
| {escapeHTML(t.description)} | ||
| </div> | ||
| )} | ||
| </div> | ||
| <button data-part="close-trigger" class="toast-close-trigger text-foreground/50 hover:text-foreground"> | ||
| ✕ | ||
| </button> | ||
| </div> | ||
| ))} | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how Gea framework handles text interpolation in JSX templates.
# Look for the renderer implementation that processes JSX children.
# Search for text node creation or innerHTML usage in the Gea core renderer
rg -n -A5 -B2 'textContent|innerHTML|createTextNode' --type=ts packages/gea/src/
# Also check how the compiler handles JSX text children
rg -n -A5 'text.*node|string.*child' --type=ts packages/gea/src/Repository: dashersw/gea
Length of output: 1751
🌐 Web query:
Gea framework JSX text node textContent innerHTML escaping
💡 Result:
In the Gea framework, JSX text nodes (expressions like {store.count}) are automatically escaped for security, preventing XSS attacks by treating dynamic content as plain text rather than HTML. The Vite plugin compiles JSX to HTML template strings at build time and generates surgical DOM updates via observe calls on specific state paths. These updates target individual text nodes using safe methods like textContent or nodeValue, which escape HTML entities by spec (e.g., <script> renders as literal text). Gea docs do not explicitly detail innerHTML usage for text nodes; instead, its no-VDOM, direct DOM patching approach mirrors standard JS best practices where textContent ensures escaping. No evidence of innerHTML for text interpolation, confirming safe-by-default text node handling akin to React/Solid.
Citations:
- 1: https://geajs.com/
- 2: https://github.com/dashersw/gea
- 3: https://github.com/dashersw/gea/blob/main/docs/core-concepts/jsx-syntax.md
- 4: https://geajs.com/docs/tooling/vite-plugin.html
- 5: https://registry.npmjs.org/%40geajs%2Fcore
- 6: https://www.geasas.co
🏁 Script executed:
#!/bin/bash
# Alternative: Try to fetch Gea documentation or package info
# Check if there's a README or documentation in the review context
fd -t f 'README|CHANGELOG|docs' -x grep -l 'textContent\|innerHTML\|escapeHTML' {}
# Also check for any existing tests that might reveal expected behavior
fd -t f 'test|spec' packages/gea-ui/src/ -name '*toast*' | head -5Repository: dashersw/gea
Length of output: 1836
Remove escapeHTML() calls—Gea uses textContent natively with automatic HTML escaping.
Gea's JSX renderer targets text nodes using textContent and nodeValue, which escape HTML entities automatically per HTML spec. Calling escapeHTML() on t.title and t.description causes double-escaping: content like <img> becomes <img> displayed as literal text to users.
This contradicts the PR title's intent to use textContent. Simply remove the escapeHTML() wrapper:
{t.title && (
<div data-part="title" class="toast-title text-sm font-semibold">
{t.title}
</div>
)}
{t.description && (
<div data-part="description" class="toast-description text-sm opacity-90">
{t.description}
</div>
)}Gea handles escaping automatically for safe-by-default text node handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/gea-ui/src/components/toast.tsx` around lines 201 - 226, In the
toast renderer mapping over this._currentToasts, remove the escapeHTML(...)
wrappers around t.title and t.description so the JSX renders the raw values
(e.g., change escapeHTML(t.title) and escapeHTML(t.description) to just t.title
and t.description) — this ensures Gea's renderer uses textContent/nodeValue
automatic escaping instead of double-escaping; locate the JSX inside the
component where key={t.id} and data-part="title"/"description" are rendered and
update those expressions accordingly.
There was a problem hiding this comment.
@dgknbtl why do you think this is still an issue? shouldn't we fix this at the library level?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@dashersw you're right, fixing this at the library/core level would be the best long-term solution. However, this fix for the Toast component solves the issue quickly and effectively without breaking the current architecture.
Technically, Gea's "No vDOM" philosophy provides good performance, but it also brings a security trade-off. I believe a core-level fix would require a major change in Gea's string based JSX runtime. That's why securing it at the component level while keeping the current structure is the most logical and fast step for now.
|
I've confirmed with a live test that Gea JSX doesn't auto-escape these strings (see attached XSS proof). To keep the component secure, I've kept the manual 
|
dgknbtl
changed the title
2 participants
Toast XSS Fix & Native Gea Refactor
Summary
This PR fixes a critical XSS vulnerability in the Toaster component by refactoring it into a Native Gea Component.
Changes
Toast: Native Gea Refactor & XSS Patch
Following the maintainer's recommendation, the Toaster component was refactored to use Gea's declarative template system:
_createToastElement,_syncToastDOM) with a declarative JSX structure using.map().escapeHTMLhelper to properly sanitizetitleanddescriptionprops, as the Gea JSX engine doesn't automatically sanitize strings.this.__geaRequestRender().Verification
Automated Tests
<img onerror="...">) are safely rendered as escaped text.Manual Verification
Confirmed via Showcase application:
Additional Notes