-
Notifications
You must be signed in to change notification settings - Fork 473
fix(ui): validate cssLayerName before wrapping styles in @layer #9747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@clerk/ui': patch | ||
| --- | ||
|
|
||
| Validate `appearance.cssLayerName` before wrapping component styles in `@layer`. Values that are not a valid CSS layer name (for example ones containing braces, semicolons, or markup) are now ignored with a one-time console warning instead of being interpolated into the generated stylesheet. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { logger } from '@clerk/shared/logger'; | ||
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { createEmotionCache } from '../createEmotionCache'; | ||
|
|
||
| function insertAndRead(cssLayerName: string | undefined, styles: string) { | ||
| const cache = createEmotionCache({ cssLayerName }); | ||
| const insert = vi.spyOn(cache.sheet, 'insert').mockImplementation(() => {}); | ||
| cache.insert('', { name: 'rule', styles, next: undefined } as any, cache.sheet, true); | ||
| return insert.mock.calls.map(([rule]) => rule).join(''); | ||
| } | ||
|
|
||
| describe('createEmotionCache', () => { | ||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it.each(['app.clerk', '--vendor', 'clérk'])('wraps insertions in the configured layer %s', name => { | ||
| expect(insertAndRead(name, 'color:red;')).toContain(`@layer ${name}`); | ||
| }); | ||
|
|
||
| it('drops a cssLayerName that would break out of the @layer rule', () => { | ||
| vi.spyOn(logger, 'warnOnce').mockImplementation(() => {}); | ||
| const payload = 'x} body { filter: blur(2px) } /*'; | ||
| const emitted = insertAndRead(payload, 'color:red;'); | ||
| expect(emitted).not.toContain('@layer'); | ||
| expect(emitted).not.toContain('blur'); | ||
| }); | ||
|
|
||
| it('drops a cssLayerName carrying markup', () => { | ||
| vi.spyOn(logger, 'warnOnce').mockImplementation(() => {}); | ||
| const emitted = insertAndRead('x{}</style><img src=x onerror=alert(1)><style>', 'color:red;'); | ||
| expect(emitted).not.toContain('</style>'); | ||
| expect(emitted).not.toContain('@layer'); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| import { logger } from '@clerk/shared/logger'; | ||
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { isValidCssLayerName, sanitizeCssLayerName } from '../cssLayerName'; | ||
|
|
||
| // https://drafts.csswg.org/css-syntax-3/#non-ascii-ident-code-point | ||
| const NON_ASCII_IDENT_RANGES: Array<[number, number]> = [ | ||
| [0x00b7, 0x00b7], | ||
| [0x00c0, 0x00d6], | ||
| [0x00d8, 0x00f6], | ||
| [0x00f8, 0x037d], | ||
| [0x037f, 0x1fff], | ||
| [0x200c, 0x200d], | ||
| [0x203f, 0x2040], | ||
| [0x2070, 0x218f], | ||
| [0x2c00, 0x2fef], | ||
| [0x3001, 0xd7ff], | ||
| [0xf900, 0xfdcf], | ||
| [0xfdf0, 0xfffd], | ||
| [0x10000, 0x10ffff], | ||
| ]; | ||
| const inNonAsciiIdentRanges = (cp: number) => NON_ASCII_IDENT_RANGES.some(([lo, hi]) => cp >= lo && cp <= hi); | ||
| const hex = (cp: number) => `U+${cp.toString(16).toUpperCase().padStart(4, '0')}`; | ||
| const rangeEdges = [...new Set(NON_ASCII_IDENT_RANGES.flatMap(([lo, hi]) => [lo, hi]))].map(cp => ({ | ||
| cp, | ||
| label: hex(cp), | ||
| })); | ||
| const rangeNeighbours = [...new Set(NON_ASCII_IDENT_RANGES.flatMap(([lo, hi]) => [lo - 1, hi + 1]))] | ||
| .filter(cp => cp <= 0x10ffff && !inNonAsciiIdentRanges(cp)) | ||
| .map(cp => ({ cp, label: hex(cp) })); | ||
|
|
||
| describe('cssLayerName', () => { | ||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it.each([ | ||
| 'components', | ||
| 'clerk', | ||
| 'app.components', | ||
| 'theme_layer-1', | ||
| '-vendor', | ||
| '--vendor', | ||
| '--', | ||
| '---', | ||
| '--1', | ||
| '_x', | ||
| 'a.b.c', | ||
| 'a.--b', | ||
| 'clérk', | ||
| '-é', | ||
| 'レイヤー', | ||
| '\u{1F600}', | ||
| 'inherits', | ||
| 'revert-layers', | ||
| ])('accepts %s', value => { | ||
| const warn = vi.spyOn(logger, 'warnOnce').mockImplementation(() => {}); | ||
| expect(isValidCssLayerName(value)).toBe(true); | ||
| expect(sanitizeCssLayerName(value)).toBe(value); | ||
| expect(warn).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it.each(rangeEdges)('accepts non-ASCII ident code point $label as start and continuation', ({ cp }) => { | ||
| const char = String.fromCodePoint(cp); | ||
| expect(isValidCssLayerName(char)).toBe(true); | ||
| expect(isValidCssLayerName(`a${char}`)).toBe(true); | ||
| }); | ||
|
|
||
| it.each(rangeNeighbours)('rejects excluded code point $label as start and continuation', ({ cp }) => { | ||
| const char = String.fromCodePoint(cp); | ||
| expect(isValidCssLayerName(char)).toBe(false); | ||
| expect(isValidCssLayerName(`a${char}`)).toBe(false); | ||
| }); | ||
|
|
||
| it.each([ | ||
| 'x} body { color: red } /*', | ||
| 'x{}</style><script>alert(1)</script>', | ||
| 'components;@import url(https://attacker.example/x)', | ||
| 'a b', | ||
| 'a.', | ||
| '.a', | ||
| '1abc', | ||
| '-1abc', | ||
| 'a..b', | ||
| '', | ||
| ' clerk', | ||
| 'clerk\n', | ||
| 'a\\}b', | ||
| 'a\u00A0b', | ||
| 'a\u2028b', | ||
| 'a\u00D7b', | ||
| ])('rejects %j', value => { | ||
| expect(isValidCssLayerName(value)).toBe(false); | ||
| }); | ||
|
|
||
| it.each([ | ||
| 'initial', | ||
| 'inherit', | ||
| 'unset', | ||
| 'revert', | ||
| 'revert-layer', | ||
| 'revert-rule', | ||
| 'INITIAL', | ||
| 'app.revert', | ||
| 'App.Revert-Layer', | ||
| ])('rejects the CSS-wide keyword %s', value => { | ||
| expect(isValidCssLayerName(value)).toBe(false); | ||
| }); | ||
|
|
||
| it('returns undefined and warns once for an invalid name', () => { | ||
| const warn = vi.spyOn(logger, 'warnOnce').mockImplementation(() => {}); | ||
| expect(sanitizeCssLayerName('x} body { color: red } /*')).toBeUndefined(); | ||
| expect(warn).toHaveBeenCalledTimes(1); | ||
| expect(warn.mock.calls[0][0]).toContain('cssLayerName'); | ||
| }); | ||
|
|
||
| it('returns undefined without warning for an empty value', () => { | ||
| const warn = vi.spyOn(logger, 'warnOnce').mockImplementation(() => {}); | ||
| expect(sanitizeCssLayerName(undefined)).toBeUndefined(); | ||
| expect(sanitizeCssLayerName('')).toBeUndefined(); | ||
| expect(warn).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import { logger } from '@clerk/shared/logger'; | ||
|
|
||
| // <layer-name> = <ident> [ '.' <ident> ]*, CSS-wide keywords reserved: | ||
| // https://drafts.csswg.org/css-cascade-5/#layer-names | ||
| // <ident> per https://drafts.csswg.org/css-syntax-3/#ident-token-diagram, minus escape sequences. | ||
| const NON_ASCII_IDENT = | ||
| '\\u00B7\\u00C0-\\u00D6\\u00D8-\\u00F6\\u00F8-\\u037D\\u037F-\\u1FFF\\u200C-\\u200D\\u203F-\\u2040\\u2070-\\u218F\\u2C00-\\u2FEF\\u3001-\\uD7FF\\uF900-\\uFDCF\\uFDF0-\\uFFFD\\u{10000}-\\u{10FFFF}'; | ||
| const IDENT_START = `[A-Za-z_${NON_ASCII_IDENT}]`; | ||
| const IDENT_CHAR = `[A-Za-z0-9_\\-${NON_ASCII_IDENT}]`; | ||
| const CSS_IDENT_RE = new RegExp(`^(?:--|-?${IDENT_START})${IDENT_CHAR}*$`, 'u'); | ||
| // https://drafts.csswg.org/css-cascade-5/#defaulting-keywords | ||
| const CSS_WIDE_KEYWORDS = new Set(['initial', 'inherit', 'unset', 'revert', 'revert-layer', 'revert-rule']); | ||
|
|
||
| export function isValidCssLayerName(value: unknown): value is string { | ||
| return ( | ||
| typeof value === 'string' && | ||
| value.split('.').every(segment => CSS_IDENT_RE.test(segment) && !CSS_WIDE_KEYWORDS.has(segment.toLowerCase())) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Document the constrained The linked As per coding guidelines, “Update documentation for API changes.” 🤖 Prompt for AI Agents |
||
| ); | ||
| } | ||
|
|
||
| export function sanitizeCssLayerName(value: string | undefined): string | undefined { | ||
| if (!value) { | ||
| return undefined; | ||
| } | ||
| if (isValidCssLayerName(value)) { | ||
| return value; | ||
| } | ||
| logger.warnOnce( | ||
| `Clerk: ignoring invalid \`cssLayerName\` ${JSON.stringify(value)}. It must be a CSS layer name such as "clerk" or "app.components".`, | ||
| ); | ||
| return undefined; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the public
cssLayerNamedocumentation.Current Clerk documentation says users can name the layer “anything.” This release drops unsupported values and emits a warning. Document the accepted ASCII segment format and the unlayered fallback so upgrades do not silently change cascade ordering. (clerk.com)
As per coding guidelines, “Update documentation for API changes.”
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we really be documenting that we don't accept invalid CSS layer names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dominic-clerk, not in detail.The rejected-character list and one-time warning are implementation details. They do not need prominent user documentation.
However, the current statement that users can name
appearance.cssLayerName“anything” is no longer correct. A minimal documentation correction should describe it as a valid CSS layer name and avoid promising arbitrary strings. We can handle that as a separate documentation change if it is outside this security fix.You are interacting with an AI system.