Feat/customcss status page#3723
Conversation
Buco7854
requested review from
Br0wnHammer,
ajhollid and
karenvicent
as code owners
The customCSS field existed on the model, types, and form schema but was never editable, sent to the API, or rendered. Wire it end to end: - Accept customCSS in the create/update status page body validation and regenerate the OpenAPI spec - Add a Custom CSS editor (multiline field) to the status page form and include the value in the submitted form data - Support multiline mode in the shared TextInput component - Inject the saved CSS via a style tag when rendering the status page
Buco7854
force-pushed
the
feat/customcss-status-page
branch
from
37ffa94 to
2a0ef82
Compare
|
Testing it to local will force push once all is valid. Putting as a Draft for now. |
Bound customCSS to 100000 characters on both the client schema and the server body validation (the server is the enforcement boundary; multer's 1MB field cap was the only limit), regenerate the OpenAPI spec, and add a validation test suite covering acceptance, clearing, the length cap, and type rejection.
The preview previously rendered the themed status page inline in the admin document, so document-global concerns leaked in both directions: custom CSS spilled into the admin UI and the theme body painting never showed. Pointing the BrowserFrame at the real public URL in an iframe isolates the CSS completely and makes the preview pixel-identical to what visitors see. Auth for unpublished pages keeps working because the same-origin iframe shares localStorage and the API client reads the token from the rehydrated store.
Buco7854
force-pushed
the
feat/customcss-status-page
branch
from
2a0ef82 to
25d8839
Compare
|
Tested it everything is fine. |
|
For a quick docker test you can also use : ghcr.io/buco7854/checkmate-backend-mono:customcss |
|
@Buco7854 Do you think we should also document the available CSS selectors, classes and elements that can be customized through CSS overrides? We could surface this in an info box, help section, or it would just go into Checkmate documentation. Without that guidance, it may not be obvious for developers which selectors they should target when customizing the status page. |
|
@gorkem-bwl @Buco7854 I don't think this feature is used at all on the backend at the moment is it? I think the field is vestigial 🤔 Accepting custom css is a dangerous proposition at it opens an attack vector, so we have to mitigate that before we do anything with the css |
I am not sure about the backend side really. When it comes to attack vectors only admins can add custom CSS, the risk is much lower than allowing arbitrary users to do it. In fact anything admin is doing is his/her responsibility so if we add a warning there I think we should be safe (eg something like:
|
Creating, updating and deleting status pages was only guarded by verifyJWT, so any authenticated team member could set fields including the new customCSS via a direct API call. The admin gating existed only in the client UI (hidden buttons), and the create/configure routes had no guard, so the form was reachable by URL. Enforce the role at the API with isAllowed, matching the monitor routes.
Currently status page style isn't really using particular classes etc, someone adding custom CSS will eventually open their developer tool to search for the element they want to apply the style on etc so I'd say it's not required.
There wasn't any UI to set it and it was not rendered, otherwise it was already mostly working.
We could add stricter CSP so img-src, font-src etc aren't loaded from external sources, even stricter there could be some parsing done backend side to remove any dangerous things like |
…into feat/customcss-status-page # Conflicts: # server/src/api/routes/statusPageRoute.ts
|
@gorkem-bwl @ajhollid I was kind of asking for your opinion if you dont mind. |
I don't really have any opinions on the frontend implementation of this as that's not my field of expertise, but the CSS has to be very strictly and carefully sanitized on the backend so we don't allow any malicious CSS to be accepted. The current validation is not sufficient. User submitted CSS must also be correctly scoped to the status page, we don't want them applying styles that are out of scope. To be honest, this feature is a huge can of worms I'm not personally interested in opening, but if it is going to be worked on then it must be done very carefully. |
Could you give me an example of malicious CSS you'd like sanitized? Just so we are on the same page about the potential attack surface: the CSS is injected as a text node in a single <style> tag on the status page, so this isn't script injection, and anything that isn't valid CSS is just ignored by the browser. Nothing other than CSS can be executed. The only thing that I cans see as a "maybe" is On scoping: we only add a <style> tag with the custom CSS to that specific status page (no other page can be affected, it is not global to checkmate or to mulitple page). Editing status page is also now admin/superadmin only on the backend and frontend. |
Anything that triggers a network request should be sanitized, ie Anything There's lots of malicious but not dangerous things to think about too. What if a user tries to set the "down" color to a color one would expect to indicate "up"? What about setting infinite animations? Massive/recursive blurs? I don't really know here, as this is not my domain, perhaps @gorkem-bwl can weigh in here. These are probably less important since this is an admin only feature, and if they want to ruin their own status pages then 🤷 |
Ok, I'll sanitize anything that can trigger a request from the browser. About position: fixed/absolute, I personally wouldn't touch it, I don't see the point and it's typically something you'd use legitimately. |
Reject status page custom CSS containing @import or a url() / image-set() / image() / cross-fade() whose target is http(s) or protocol-relative, while allowing data: and relative URLs. The check normalizes comments and CSS escapes first so obfuscated forms cannot slip past. Enforced in the create/update body validation.
|
Well ended up doing it tonight. Any external url is now rejected, went for a full reject rather than sanitization so we're upfront with the user. |
| const decodeForScan = (css: string): string => | ||
| css.replace(/\/\*[\s\S]*?\*\//g, "").replace(/\\([0-9a-fA-F]{1,6})\s?|\\([\s\S])/g, (_match, hex: string, char: string) => { | ||
| if (!hex) { | ||
| return char; | ||
| } | ||
| const codePoint = parseInt(hex, 16); | ||
| return codePoint > 0 && codePoint <= 0x10ffff ? String.fromCodePoint(codePoint) : "�"; | ||
| }); | ||
|
|
||
| // An external reference is @import, or a resource function (url, image-set, | ||
| // image, cross-fade) whose target starts with an http(s) scheme or is | ||
| // protocol-relative. Anchoring on the start of the target keeps data: and | ||
| // relative URLs allowed, including data: SVG that embeds the SVG namespace URL. | ||
| const EXTERNAL_REFERENCE = /(?:@import\b|(?:url|image-set|-webkit-image-set|image|cross-fade|-webkit-cross-fade)\s*\(\s*['"]?\s*(?:https?:|\/\/))/i; | ||
|
|
||
| export const cssReferencesExternalResource = (css?: string | null): boolean => { | ||
| if (typeof css !== "string" || css === "") { | ||
| return false; | ||
| } | ||
| return EXTERNAL_REFERENCE.test(decodeForScan(css)); | ||
| }; |
There was a problem hiding this comment.
Unless I'm misunderstanding how this works, this hand-rolled solution allows URLs with escape chars to bypass the sanitization:
const decodeForScan = (css) => {
return css
.replace(/\/\*[\s\S]*?\*\//g, "")
.replace(/\\([0-9a-fA-F]{1,6})\s?|\\([\s\S])/g, (_match, hex, char) => {
if (!hex) {
return char;
}
const codePoint = parseInt(hex, 16);
return codePoint > 0 && codePoint <= 0x10ffff
? String.fromCodePoint(codePoint)
: "�";
});
};
const EXTERNAL_REFERENCE =
/(?:@import\b|(?:url|image-set|-webkit-image-set|image|cross-fade|-webkit-cross-fade)\s*\(\s*['"]?\s*(?:https?:|\/\/))/i;
const cssReferencesExternalResource = (css) => {
if (typeof css !== "string" || css === "") {
return false;
}
return EXTERNAL_REFERENCE.test(decodeForScan(css));
};
const baseline = 'body{background:url("https://bypass/malicious")}';
const hexEscape =
'body{background:url("htt\a ps://webhook.site/29d8a9b0-2092-4dfe-8377-51c59f150b3d")}';
const newLine =
'body{background:url("htt\\\nps://webhook.site/29d8a9b0-2092-4dfe-8377-51c59f150b3d")}';
for (const [name, css] of [
["baseline", baseline],
["hex escaped \\a", hexEscape],
["new line", newLine],
]) {
console.log(`${name}`);
console.log(`${JSON.stringify(css)}`);
console.log(JSON.stringify(decodeForScan(css)));
console.log(cssReferencesExternalResource(css));
console.log("--------------------------------------------------");
}
Run this snippet, it doesn't flag either of those cases:
baseline
"body{background:url(\"https://bypass/malicious\")}"
"body{background:url(\"https://bypass/malicious\")}"
true
--------------------------------------------------
hex-escaped \a
"body{background:url(\"htta ps://webhook.site/29d8a9b0-2092-4dfe-8377-51c59f150b3d\")}"
"body{background:url(\"htta ps://webhook.site/29d8a9b0-2092-4dfe-8377-51c59f150b3d\")}"
false
--------------------------------------------------
line continuation
"body{background:url(\"htt\\\nps://webhook.site/29d8a9b0-2092-4dfe-8377-51c59f150b3d\")}"
"body{background:url(\"htt\nps://webhook.site/29d8a9b0-2092-4dfe-8377-51c59f150b3d\")}"
false
The browser will happily make those requests given those values that skip sanitization.
I don't know enough about CSS to feel confident hand rolling a validation here, it's probably best to outsource this to a well tested and maintained library
There was a problem hiding this comment.
I wasn't sure if you would have been fine with an other dependency thats why I avoided it. If you are fine with it I'll use https://github.com/csstools/tokenizer it's actually already pulled by an other dep so it doesn't change anything.
I'll add CSP for status page so the client refuses to load those anyway if a different domain somehow manage to slip by.
Replace the regex/escape-normalizer with @csstools/css-tokenizer (already present transitively). Tokenizing resolves comments, hex escapes, and line continuations into the real value before scanning, so obfuscated forms (a scheme split by a line continuation or hex escape) cannot slip past. Scan url-tokens and string-tokens to cover url(), quoted @import, and string-form image functions. Add regression tests for the bypasses.
Layer a stricter Content-Security-Policy on the /status/public document so custom CSS cannot load external images, fonts, or stylesheets. Browsers enforce the intersection of all CSP headers, so this only narrows the public status page and never loosens the global policy, while keeping the app's Google Fonts. Browser-enforced backstop behind the validator.
|
normlaly escape that are innofensive should still pass but anything that can be "dangerous" doesnt. In addition I updated CSP for status pages so anything that is not google fonts or current domain is blocked. |
ajhollid
left a comment
ajhollid
left a comment
There was a problem hiding this comment.
Couple of changes here and I think we're good to go.
Detect custom domains by comparing the request host to the app host and apply the tightened CSP there too, not just on the public path. Run the middleware before express.static so static-served root documents get the header, and keep helmet after static so its default-src does not block custom domain API calls.
The form only enforced the length cap, so CSS with an external url() or @import passed client validation and was rejected by the server. Add the same tokenizer based check with the same error message so it is caught inline before submit.
The preview framed the page public URL, which for a custom domain is cross origin and blocked by the admin document CSP frame-src. Point the iframe at the same-origin public path while the frame chrome still shows the public URL.
|
Fixed all 2 points. Should be good now. |
3 participants
Describe your changes
Finishes the custom CSS feature for status pages. The
customCSSfield alreadyexisted on the model and types, but there was no way to actually use it. The
form never sent it, the server validator stripped it, and nothing rendered it.
This PR wires it end to end:
customCSSis now accepted by the create/update status pagevalidation, and the OpenAPI spec is regenerated to match.
page form, and the value is included in the submitted form data.
<style>tag inBaseStatusPage, so it applies to all four themes. React sets style childrenvia
textContent, so the string is never HTML parsed and there is no</style>breakout / XSS vector.an iframe inside the browser chrome instead of rendering the page inline in
the admin document. Without this, the injected CSS leaked into the admin UI
around the preview. As a bonus the preview is now pixel-identical to what
visitors see. Auth for previewing unpublished pages keeps working since the
same-origin iframe shares localStorage. The preview was already showing the
live saved page before, so this changes how it renders, not what it shows.
TextInputgot a tiny fix to supportmultiline. It hardcoded a 34px inputheight, which squashed textareas, and this is the first multiline usage.
and non-string values).
To test: create or edit a status page, paste something obvious like
li { outline: 3px dashed red; }into the Custom CSS box on the appearancestep, save, and open the public URL or the admin preview. Heads up that page
backgrounds need
!importantbecause the theme paints its background on awrapper div with an inline style.
Fixes
Fixes #2564
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
<div>Add</div>)npm run formatin server and client directories, which automatically formats your code.Screenshots:
Before:
After: