Skip to content

Commit 66daa2e

Browse files
usualomausualoma
authored
Merge commit from fork
* fix(jsx): validate attribute names to prevent XSS via key injection * fix(jsx/dom): handle invalid attribute names in DOM renderer
1 parent fa2c74f commit 66daa2e

File tree

8 files changed

+287
-12
lines changed

8 files changed

+287
-12
lines changed

‎src/jsx/base.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import type {
1010
JSX as HonoJSX,
1111
IntrinsicElements as IntrinsicElementsDefined,
1212
} from './intrinsic-elements'
13-
import { normalizeIntrinsicElementKey, styleObjectForEach } from './utils'
13+
import { isValidAttributeName, normalizeIntrinsicElementKey, styleObjectForEach } from './utils'
1414

1515
// eslint-disable-next-line @typescript-eslint/no-explicit-any
1616
export type Props = Record<string, any>
@@ -182,6 +182,9 @@ export class JSXNode implements HtmlEscaped {
182182
: (key) => normalizeIntrinsicElementKey(key)
183183
for (let [key, v] of Object.entries(props)) {
184184
key = normalizeKey(key)
185+
if (!isValidAttributeName(key)) {
186+
continue
187+
}
185188
if (key === 'children') {
186189
// skip children
187190
} else if (key === 'style' && typeof v === 'object') {

‎src/jsx/dom/index.test.tsx‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ describe('DOM', () => {
9696
global.HTMLElement = dom.window.HTMLElement
9797
global.SVGElement = dom.window.SVGElement
9898
global.Text = dom.window.Text
99+
global.DOMException = dom.window.DOMException
99100
root = document.getElementById('root') as HTMLElement
100101
})
101102

@@ -194,6 +195,69 @@ describe('DOM', () => {
194195
expect(root.innerHTML).toBe('<div x-value="value"></div>')
195196
})
196197

198+
it('ignores invalid attribute keys without interrupting updates', async () => {
199+
const invalidKey = '" onfocus="alert(1)'
200+
201+
const App = () => {
202+
const [includeInvalid, setIncludeInvalid] = useState(true)
203+
return includeInvalid ? (
204+
<div id='safe' {...{ [invalidKey]: 'x' }} onClick={() => setIncludeInvalid(false)}>
205+
Hello
206+
</div>
207+
) : (
208+
<div class='updated'>Hello</div>
209+
)
210+
}
211+
212+
render(<App />, root)
213+
expect(root.innerHTML).toBe('<div id="safe">Hello</div>')
214+
215+
root.querySelector('div')?.click()
216+
await Promise.resolve()
217+
218+
expect(root.innerHTML).toBe('<div class="updated">Hello</div>')
219+
})
220+
221+
it('rethrows unexpected errors while setting attributes', () => {
222+
const error = new Error('boom')
223+
const originalSetAttribute = dom.window.Element.prototype.setAttribute
224+
const setAttributeSpy = vi
225+
.spyOn(dom.window.Element.prototype, 'setAttribute')
226+
.mockImplementation(function (this: Element, key: string, value: string) {
227+
if (key === 'data-boom') {
228+
throw error
229+
}
230+
return originalSetAttribute.call(this, key, value)
231+
})
232+
233+
try {
234+
expect(() => render(<div data-boom='x'>Hello</div>, root)).toThrow(error)
235+
} finally {
236+
setAttributeSpy.mockRestore()
237+
}
238+
})
239+
240+
it('rethrows unexpected errors while removing attributes', () => {
241+
render(<div data-boom='x'>Hello</div>, root)
242+
243+
const error = new Error('boom')
244+
const originalRemoveAttribute = dom.window.Element.prototype.removeAttribute
245+
const removeAttributeSpy = vi
246+
.spyOn(dom.window.Element.prototype, 'removeAttribute')
247+
.mockImplementation(function (this: Element, key: string) {
248+
if (key === 'data-boom') {
249+
throw error
250+
}
251+
return originalRemoveAttribute.call(this, key)
252+
})
253+
254+
try {
255+
expect(() => render(<div data-boom={undefined}>Hello</div>, root)).toThrow(error)
256+
} finally {
257+
removeAttributeSpy.mockRestore()
258+
}
259+
})
260+
197261
it('ref', () => {
198262
const App = () => {
199263
const ref = useRef<HTMLDivElement>(null)
@@ -2582,6 +2646,18 @@ describe('DOM', () => {
25822646
expect(document.querySelector('svg title')).toBeInstanceOf(dom.window.SVGTitleElement)
25832647
})
25842648

2649+
it('skips invalid attribute keys in SVG while preserving valid ones', () => {
2650+
const App = () => {
2651+
return (
2652+
<svg>
2653+
<g {...{ ['" onload="alert(1)']: 'x', viewBox: '0 0 10 10' }} />
2654+
</svg>
2655+
)
2656+
}
2657+
render(<App />, root)
2658+
expect(root.innerHTML).toBe('<svg><g viewBox="0 0 10 10"></g></svg>')
2659+
})
2660+
25852661
describe('attribute', () => {
25862662
describe('camelCase', () => {
25872663
test.each`

‎src/jsx/dom/render.ts‎

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,12 @@ const applySelectValue = (select: HTMLSelectElement, props: Props): void => {
155155
}
156156
}
157157

158+
// Unlike SSR (which uses isValidAttributeName for upfront validation),
159+
// the DOM renderer relies on try/catch for performance—invalid attribute
160+
// names are rare at runtime, so avoiding the per-attribute regex check is faster.
161+
const isIgnorableAttributeError = (error: unknown): boolean =>
162+
error instanceof DOMException && error.name === 'InvalidCharacterError'
163+
158164
const applyProps = (
159165
container: SupportedElement,
160166
attributes: Props,
@@ -223,14 +229,20 @@ const applyProps = (
223229

224230
const k = toAttributeName(container, key)
225231

226-
if (value === null || value === undefined || value === false) {
227-
container.removeAttribute(k)
228-
} else if (value === true) {
229-
container.setAttribute(k, '')
230-
} else if (typeof value === 'string' || typeof value === 'number') {
231-
container.setAttribute(k, value as string)
232-
} else {
233-
container.setAttribute(k, value.toString())
232+
try {
233+
if (value === null || value === undefined || value === false) {
234+
container.removeAttribute(k)
235+
} else if (value === true) {
236+
container.setAttribute(k, '')
237+
} else if (typeof value === 'string' || typeof value === 'number') {
238+
container.setAttribute(k, value as string)
239+
} else {
240+
container.setAttribute(k, value.toString())
241+
}
242+
} catch (e) {
243+
if (!isIgnorableAttributeError(e)) {
244+
throw e
245+
}
234246
}
235247
}
236248
}
@@ -246,7 +258,13 @@ const applyProps = (
246258
} else if (key === 'ref') {
247259
refCleanupMap.get(container)?.()
248260
} else {
249-
container.removeAttribute(toAttributeName(container, key))
261+
try {
262+
container.removeAttribute(toAttributeName(container, key))
263+
} catch (e) {
264+
if (!isIgnorableAttributeError(e)) {
265+
throw e
266+
}
267+
}
250268
}
251269
}
252270
}

‎src/jsx/index.test.tsx‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -478,6 +478,76 @@ describe('render to string', () => {
478478
})
479479
})
480480

481+
describe('XSS prevention for attribute keys', () => {
482+
it('Should skip attribute keys containing double quotes', () => {
483+
const props: Record<string, string> = { ['" onfocus="alert(1)']: 'x' }
484+
const template = <div {...props}>Hello</div>
485+
expect(template.toString()).toBe('<div>Hello</div>')
486+
})
487+
488+
it('Should skip attribute keys containing >', () => {
489+
const props: Record<string, string> = { ['"><script>alert(1)</script><x x="']: 'x' }
490+
const template = <div {...props}>Hello</div>
491+
expect(template.toString()).toBe('<div>Hello</div>')
492+
})
493+
494+
it('Should skip attribute keys containing <', () => {
495+
const props: Record<string, string> = { ['foo<bar']: 'x' }
496+
const template = <div {...props}>Hello</div>
497+
expect(template.toString()).toBe('<div>Hello</div>')
498+
})
499+
500+
it('Should skip attribute keys containing backslashes', () => {
501+
const props: Record<string, string> = { ['foo\\bar']: 'x' }
502+
const template = <div {...props}>Hello</div>
503+
expect(template.toString()).toBe('<div>Hello</div>')
504+
})
505+
506+
it('Should skip attribute keys containing backticks', () => {
507+
const props: Record<string, string> = { ['foo`bar']: 'x' }
508+
const template = <div {...props}>Hello</div>
509+
expect(template.toString()).toBe('<div>Hello</div>')
510+
})
511+
512+
it('Should skip attribute keys containing spaces', () => {
513+
const props: Record<string, string> = { ['foo bar']: 'x' }
514+
const template = <div {...props}>Hello</div>
515+
expect(template.toString()).toBe('<div>Hello</div>')
516+
})
517+
518+
it('Should still render valid attributes alongside invalid ones', () => {
519+
const props: Record<string, string> = {
520+
id: 'safe',
521+
['" onfocus="alert(1)']: 'x',
522+
class: 'test',
523+
}
524+
const template = <div {...props}>Hello</div>
525+
expect(template.toString()).toBe('<div id="safe" class="test">Hello</div>')
526+
})
527+
528+
it('Should skip invalid attribute keys before style serialization', () => {
529+
const template = <div {...{ ['" onfocus="alert(1)']: { fontSize: 10 } }}>Hello</div>
530+
expect(template.toString()).toBe('<div>Hello</div>')
531+
})
532+
533+
it('Should skip invalid attribute keys before function prop validation', () => {
534+
const template = <div {...{ ['" onfocus="alert(1)']: () => 'x' }}>Hello</div>
535+
expect(template.toString()).toBe('<div>Hello</div>')
536+
})
537+
538+
it('Should skip invalid attribute keys before dangerouslySetInnerHTML handling', () => {
539+
const template = (
540+
<div {...{ ['" onfocus="alert(1)']: { __html: '<strong>Injected</strong>' } }}>Hello</div>
541+
)
542+
expect(template.toString()).toBe('<div>Hello</div>')
543+
})
544+
545+
it('Should skip invalid attribute keys with Promise values', async () => {
546+
const template = <div {...{ ['" onfocus="alert(1)']: Promise.resolve('x') }}>Hello</div>
547+
expect(await template.toString()).toBe('<div>Hello</div>')
548+
})
549+
})
550+
481551
describe('head', () => {
482552
it('Simple head elements should be rendered as is', () => {
483553
const template = (
@@ -667,6 +737,15 @@ describe('SVG', () => {
667737
)
668738
})
669739

740+
it('should skip invalid attribute keys in SVG while preserving valid ones', () => {
741+
const template = (
742+
<svg>
743+
<g {...{ ['" onload="alert(1)']: 'x', viewBox: '0 0 10 10' }} />
744+
</svg>
745+
)
746+
expect(template.toString()).toBe('<svg><g viewBox="0 0 10 10"></g></svg>')
747+
})
748+
670749
describe('attribute', () => {
671750
describe('camelCase', () => {
672751
test.each`

‎src/jsx/jsx-runtime.test.tsx‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/** @jsxRuntime automatic **/
22
/** @jsxImportSource . **/
33
import { Hono } from '../hono'
4+
import { jsxAttr } from './jsx-runtime'
45

56
describe('jsx-runtime', () => {
67
let app: Hono
@@ -19,6 +20,29 @@ describe('jsx-runtime', () => {
1920
expect(await res.text()).toBe('<h1>Hello</h1>')
2021
})
2122

23+
it('Should skip invalid attribute keys in jsxAttr()', () => {
24+
expect(String(jsxAttr('" onfocus="alert(1)', 'x'))).toBe('')
25+
expect(String(jsxAttr('foo<bar', 'x'))).toBe('')
26+
expect(String(jsxAttr('foo\\bar', 'x'))).toBe('')
27+
expect(String(jsxAttr('foo`bar', 'x'))).toBe('')
28+
})
29+
30+
it('Should skip invalid non-string attribute values in jsxAttr()', async () => {
31+
const invalidKey = '" onfocus="alert(1)'
32+
33+
expect(String(jsxAttr(invalidKey, { fontSize: 10 }))).toBe('')
34+
expect(String(await jsxAttr(invalidKey, Promise.resolve('/docs?q=1&lang=en')))).toBe('')
35+
})
36+
37+
it('Should render valid attribute values in jsxAttr()', async () => {
38+
expect(String(jsxAttr('style', { fontSize: 10, color: 'red' }))).toBe(
39+
'style="font-size:10px;color:red"'
40+
)
41+
expect(String(await jsxAttr('href', Promise.resolve('/docs?q=1&lang=en')))).toBe(
42+
'href="/docs?q=1&amp;lang=en"'
43+
)
44+
})
45+
2246
// https://en.reactjs.org/docs/jsx-in-depth.html#booleans-null-and-undefined-are-ignored
2347
describe('Booleans, Null, and Undefined Are Ignored', () => {
2448
it.each([true, false, undefined, null])('%s', (item) => {

‎src/jsx/jsx-runtime.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,17 @@ export type { JSX } from './jsx-dev-runtime'
99
import { html, raw } from '../helper/html'
1010
import type { HtmlEscapedString, StringBuffer, HtmlEscaped } from '../utils/html'
1111
import { escapeToBuffer, stringBufferToString } from '../utils/html'
12-
import { styleObjectForEach } from './utils'
12+
import { isValidAttributeName, styleObjectForEach } from './utils'
1313

1414
export { html as jsxTemplate }
1515

1616
export const jsxAttr = (
1717
key: string,
1818
v: string | Promise<string> | Record<string, string | number | null | undefined | boolean>
1919
): HtmlEscapedString | Promise<HtmlEscapedString> => {
20+
if (!isValidAttributeName(key)) {
21+
return raw('')
22+
}
2023
const buffer: StringBuffer = [`${key}="`] as StringBuffer
2124
if (key === 'style' && typeof v === 'object') {
2225
// object to style strings

‎src/jsx/utils.test.ts‎

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { normalizeIntrinsicElementKey, styleObjectForEach } from './utils'
1+
import { isValidAttributeName, normalizeIntrinsicElementKey, styleObjectForEach } from './utils'
22

33
describe('normalizeIntrinsicElementKey', () => {
44
test.each`
@@ -17,6 +17,51 @@ describe('normalizeIntrinsicElementKey', () => {
1717
})
1818
})
1919

20+
describe('isValidAttributeName', () => {
21+
test.each`
22+
name | expected
23+
${'class'} | ${true}
24+
${'id'} | ${true}
25+
${'href'} | ${true}
26+
${'data-foo'} | ${true}
27+
${'aria-label'} | ${true}
28+
${'onclick'} | ${true}
29+
${'viewBox'} | ${true}
30+
${'xml:lang'} | ${true}
31+
`('should return $expected for valid name "$name"', ({ name, expected }) => {
32+
expect(isValidAttributeName(name)).toBe(expected)
33+
})
34+
35+
test.each`
36+
name | description
37+
${''} | ${'empty string'}
38+
${'" onfocus="alert(1)'} | ${'double quote'}
39+
${"' onfocus='alert(1)"} | ${'single quote'}
40+
${'foo<bar'} | ${'less than'}
41+
${'"><script>alert(1)</script>'} | ${'greater than'}
42+
${'foo bar'} | ${'space'}
43+
${'foo=bar'} | ${'equals sign'}
44+
${'foo/bar'} | ${'slash'}
45+
${'foo\\bar'} | ${'backslash'}
46+
${'foo`bar'} | ${'backtick'}
47+
${'a\x00b'} | ${'null byte'}
48+
${'a\x1fb'} | ${'control character'}
49+
${'a\x7fb'} | ${'DEL character'}
50+
`('should return false for "$description"', ({ name }) => {
51+
expect(isValidAttributeName(name)).toBe(false)
52+
})
53+
54+
test.each`
55+
name | description
56+
${'xlink:href'} | ${'namespace separator'}
57+
${'data.foo'} | ${'dot'}
58+
${'data_foo'} | ${'underscore'}
59+
${'é'} | ${'non-ascii'}
60+
`('should return true for valid "$description" names', ({ name }) => {
61+
expect(isValidAttributeName(name)).toBe(true)
62+
})
63+
})
64+
2065
describe('styleObjectForEach', () => {
2166
describe('Should output the number as it is, when a number type is passed', () => {
2267
test.each`

0 commit comments

Comments
 (0)
⚡