-
-
Notifications
You must be signed in to change notification settings - Fork 819
fix(core): inline custom CSS into experience HTML head to prevent style flash #8917
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
base: master
Are you sure you want to change the base?
Changes from all commits
c84e6dc
b6719c9
1971072
6e85b9c
d275df2
0d8e711
849174a
4c3fbdb
465d213
aeabf72
4cc1db3
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,7 @@ | ||
| --- | ||
| "@logto/core": patch | ||
| --- | ||
|
|
||
| fix a flash of built-in styles on the hosted sign-in experience when custom CSS is configured | ||
|
|
||
| Custom CSS was injected on the client via react-helmet, which mutates `<head>` asynchronously after the page had already painted with the built-in styles. The server-rendered experience HTML now inlines the configured custom CSS into `<head>`, so it is part of the cascade on the first paint. The `</style>` sequence in custom CSS is escaped so it cannot terminate the style element early, and the SSR data embedded in the inline `<script>` is now serialized with HTML-significant characters escaped to prevent script breakout. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,24 @@ import { getExperienceLanguage } from '#src/utils/i18n.js'; | |
| import { type WithI18nContext } from './koa-i18next.js'; | ||
| import { isIndexPath } from './koa-serve-static.js'; | ||
|
|
||
| /** | ||
| * Serialize SSR data for safe embedding inside an inline `<script>`. `JSON.stringify` alone is unsafe: | ||
| * a string such as `</script>` in the data (e.g. inside custom CSS or custom content) would close the | ||
| * script element early and enable injection. Escaping the HTML-significant characters keeps the values | ||
| * identical once parsed by JS, while preventing the HTML parser from recognizing any tag delimiters. | ||
| */ | ||
| const serializeSsrData = (data: SsrData): string => | ||
| JSON.stringify(data) | ||
| .replaceAll('<', '\\u003c') | ||
| .replaceAll('>', '\\u003e') | ||
| .replaceAll('&', '\\u0026') | ||
| // U+2028 (LINE SEPARATOR) and U+2029 (PARAGRAPH SEPARATOR) are valid inside JSON strings but are | ||
| // line terminators in a JavaScript string literal (pre-ES2019). Since this payload is embedded as a | ||
| // JS expression (`Object.freeze(...)`), leaving them literal can break parsing in older engines. | ||
| // Escape to their `\uXXXX` form, which JS decodes back to the original characters. | ||
| .replaceAll('\u2028', '\\u2028') // U+2028 LINE SEPARATOR | ||
| .replaceAll('\u2029', '\\u2029'); // U+2029 PARAGRAPH SEPARATOR | ||
|
|
||
| /** | ||
| * Create a middleware to prefetch the experience data and inject it into the HTML response. Some | ||
| * conditions must be met: | ||
|
|
@@ -54,15 +72,32 @@ export default function koaExperienceSsr<StateT, ContextT extends WithI18nContex | |
| const phrases = await libraries.phrases.getPhrases(language); | ||
|
|
||
| ctx.set('Content-Language', language); | ||
| ctx.body = ctx.body.replace( | ||
|
|
||
| // Inline custom CSS into <head> on the first byte so it is in the cascade for the first paint, | ||
| // removing the flash before react-helmet injects the same <style> client-side. Done BEFORE the SSR | ||
| // placeholder substitution so the `</head>` match can only hit the genuine document head. Skipped in | ||
| // preview mode (`?preview=true`), where the console iframe drives styling live via postMessage and | ||
| // inlining the *saved* CSS could leak rules being edited. `</style` is defused so admin CSS can't | ||
| // terminate the element early (parser sees `<\/style`; the CSS engine unescapes `\/`→`/`). See PR #8917. | ||
| const { customCss } = signInExperience; | ||
|
|
||
| const htmlWithCss = | ||
| customCss && ctx.query.preview !== 'true' | ||
| ? ctx.body.replace( | ||
| '</head>', | ||
| `<style data-custom-css>${customCss.replaceAll(/<\/(style)/gi, '<\\/$1')}</style></head>` | ||
| ) | ||
| : ctx.body; | ||
|
Comment on lines
+84
to
+90
Contributor
Author
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. Intentionally leaving this as a literal |
||
|
|
||
| ctx.body = htmlWithCss.replace( | ||
| ssrPlaceholder, | ||
| `Object.freeze(${JSON.stringify({ | ||
| `Object.freeze(${serializeSsrData({ | ||
| signInExperience: { | ||
| ...pick(logtoUiCookie, 'appId', 'organizationId'), | ||
| data: signInExperience, | ||
| }, | ||
| phrases: { lng: language, data: phrases }, | ||
| } satisfies SsrData)})` | ||
| })})` | ||
| ); | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| import { demoAppApplicationId, fullSignInExperienceGuard } from '@logto/schemas'; | ||
| import { z } from 'zod'; | ||
|
|
||
| import { demoAppUrl } from '#src/constants.js'; | ||
| import { updateSignInExperience } from '#src/api/sign-in-experience.js'; | ||
| import { demoAppUrl, logtoUrl } from '#src/constants.js'; | ||
| import { OrganizationApiTest } from '#src/helpers/organization.js'; | ||
| import ExpectExperience from '#src/ui-helpers/expect-experience.js'; | ||
| import { Trace } from '#src/ui-helpers/trace.js'; | ||
|
|
@@ -108,4 +109,48 @@ describe('server-side rendering', () => { | |
| // Check network requests | ||
| await expectTraceNotToHaveWellKnownEndpoints(); | ||
| }); | ||
|
|
||
| describe('custom CSS inlining', () => { | ||
| // A marker the server adds (`<style data-custom-css>`) but the client-side react-helmet `<style>` | ||
| // does not, so its presence in the served HTML proves the CSS was inlined server-side for the first | ||
| // paint rather than injected later by the client. | ||
| const customCss = '.ssr-custom-css-probe { color: rgb(1, 2, 3); }'; | ||
|
|
||
| afterEach(async () => { | ||
| await updateSignInExperience({ customCss: null }); | ||
| }); | ||
|
|
||
| it('should inline custom CSS into the served <head> so it applies on the first paint', async () => { | ||
| await updateSignInExperience({ customCss }); | ||
| const experience = new ExpectExperience(await browser.newPage()); | ||
| try { | ||
| await experience.navigateTo(demoAppUrl.href); | ||
|
|
||
| const html = await experience.page.content(); | ||
| expect(html).toContain('data-custom-css'); | ||
| expect(html).toContain(customCss); | ||
| } finally { | ||
| // Close in `finally` so a failed assertion/navigation does not leak the page across the suite. | ||
| await experience.page.close(); | ||
| } | ||
| }); | ||
|
Comment on lines
+123
to
+136
Contributor
Author
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. Agreed — fixed in 4cc1db3. Both new custom-CSS tests now run their body in |
||
|
|
||
| it('should not inline custom CSS in preview mode', async () => { | ||
| await updateSignInExperience({ customCss }); | ||
| const experience = new ExpectExperience(await browser.newPage()); | ||
| try { | ||
| // Hit the experience entry directly with `?preview=true`, mirroring how the console preview | ||
| // iframe loads it. Going through the demo app instead would OIDC-redirect to `/sign-in` and | ||
| // drop the query param, so the server would never see preview mode. | ||
| await experience.navigateTo(new URL('/sign-in?preview=true', logtoUrl).href); | ||
|
|
||
| // Preview is driven live by the console iframe via postMessage; the server must not inline saved CSS. | ||
| const html = await experience.page.content(); | ||
| expect(html).not.toContain('data-custom-css'); | ||
| } finally { | ||
| // Close in `finally` so a failed assertion/navigation does not leak the page across the suite. | ||
| await experience.page.close(); | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
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.
Fixed in 4cc1db3. Anchored the capture on the trailing
);(/Object\.freeze\((?<json>.+)\);/) so the greedy match stops at the genuineObject.freeze(...)close, and addedexpect(serialized).toBeTruthy()before the non-null assertion so a regex miss fails with a clear message instead of an opaqueJSON.parse(undefined). Applied the same to the other extraction site for consistency.