feat: Status Page Custom CSS/JS/HTML overrides (Fixes #2863) #3076
Conversation
Armaansaxena
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
|
Note
|
| Cohort / File(s) | Change Summary |
|---|---|
Admin UI — Settings form client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsx |
Added MUI multiline TextFields for form.customCSS, form.headerHTML, form.footerHTML, and form.customJavaScript; added riskAccepted checkbox gating JS and an effect that enables it when JS is present; wired fields to existing handleFormChange. |
Status page rendering (client) client/src/Pages/v1/StatusPage/Status/index.jsx |
Added DOMPurify and useEffect to sanitize and render headerHTML/footerHTML; injects a <style> element with customCSS into document.head and injects/executed sanitized customJavaScript inside an IIFE with try/catch into document.body; cleans up injected nodes on unmount. |
Localization client/src/locales/en.json |
Added i18n keys for labels, descriptions, placeholders, and a JavaScript warning for custom CSS, header/footer HTML, and custom JavaScript. |
Client dependencies / build client/package.json |
Added dompurify (^3.3.0) and bumped vite to ^6.4.1. |
Database schema (server) server/src/db/v1/models/StatusPage.js |
Added optional string fields customJavaScript (maxLength 5000), headerHTML (maxLength 10000), and footerHTML (maxLength 10000) with default "". |
Request validation (server) server/src/validation/joi.js |
Extended createStatusPageBodyValidation with optional customCSS (max 10000), customJavaScript (max 5000), headerHTML (max 5000), and footerHTML (max 5000). |
StatusPage module — public API shape server/src/db/v1/modules/statusPageModule.js |
Included customCSS, customJavaScript, headerHTML, and footerHTML in early-return and aggregation projection so public GET returns these fields. |
Sequence Diagram(s)
sequenceDiagram
actor Admin
participant SettingsUI as Settings Form (client)
participant API as Backend API
participant DB as Database
participant Browser as StatusPage Client (browser)
Admin->>SettingsUI: Edit CSS / HeaderHTML / FooterHTML / JS
SettingsUI->>SettingsUI: Update form state
Admin->>SettingsUI: Save
SettingsUI->>API: POST status page payload (includes new fields)
API->>API: Validate with Joi (new fields)
API->>DB: Persist StatusPage document (new fields stored)
DB-->>API: OK
API-->>SettingsUI: Success
Browser->>API: GET statusPage config
API->>DB: Query status page (projection includes new fields)
DB-->>API: Config
API-->>Browser: Config
Browser->>Browser: Sanitize headerHTML/footerHTML (DOMPurify)
Browser->>Browser: Inject <style> with customCSS into document.head
Browser->>Browser: Inject/evaluate customJavaScript in IIFE (try/catch) into document.body
Browser->>Browser: Render sanitized header/footer or fallback components
Browser->>Browser: Cleanup injected nodes on unmount
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
- Review client injection and sanitation in
client/src/Pages/v1/StatusPage/Status/index.jsx(CSS/HTML/JS handling, cleanup, CSP considerations). - Verify Joi length limits in
server/src/validation/joi.jsalign with DBmaxLengthinserver/src/db/v1/models/StatusPage.js. - Confirm
server/src/db/v1/modules/statusPageModule.jsincludes new fields consistently in both early return and aggregation projection. - Check
client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsxfield names and submission payload match server validation keys. - Inspect
client/package.jsondependency bump for build compatibility (vite & dompurify).
Poem
🐰
I nibble styles and tuck a script away,
A header, footer—trimmed and safe today,
I hop through markup, sanitize with care,
Tuck CSS in head, JS in a guarded lair,
Hop—your status page wears a brand-new flair! 🥕✨
Pre-merge checks and finishing touches
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check | ❓ Inconclusive | The implementation provides custom CSS, JavaScript, HTML header/footer fields with sanitization via DOMPurify and risk acceptance warning, addressing core requirements; however, live preview, export/import, dedicated editors with syntax highlighting, comprehensive security measures, and unit/e2e tests from #2863 are not fully implemented. | Verify that DOMPurify sanitization covers all XSS vectors, confirm the risk acceptance workflow is properly gated, ensure size limits are enforced, and plan for live preview, export/import, and comprehensive testing in follow-up work. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately and concisely summarizes the main change: adding custom CSS, JavaScript, and HTML overrides to status pages. |
| Description check | ✅ Passed | The PR description covers key changes, references the issue, and most checklist items are marked complete; however, several template sections remain incomplete (screenshots/videos, dependency changes, and code formatting verification details). |
| Out of Scope Changes check | ✅ Passed | All changes directly support the custom CSS/JS/HTML override feature: schema extensions, validation rules, UI components, i18n strings, and package dependencies (DOMPurify) are all in-scope. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
📜 Recent review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/Pages/v1/StatusPage/Status/index.jsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/Pages/v1/StatusPage/Status/index.jsx (3)
client/src/Pages/v1/StatusPage/Status/Components/ControlsHeader/index.jsx (1)
ControlsHeader(61-125)client/src/Pages/v1/StatusPage/Status/Components/StatusBar/index.jsx (1)
StatusBar(46-68)client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (1)
MonitorsList(14-62)
🪛 Biome (2.1.2)
client/src/Pages/v1/StatusPage/Status/index.jsx
[error] 221-221: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 246-246: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (4)
client/src/Pages/v1/StatusPage/Status/index.jsx (4)
11-11: LGTM: Required imports for sanitization.The DOMPurify and useEffect imports are correctly added to support HTML sanitization and lifecycle management for custom CSS/JS injection.
Also applies to: 21-21
218-232: LGTM: Header HTML properly sanitized with DOMPurify.The custom header HTML is correctly sanitized using
DOMPurify.sanitize()before being rendered withdangerouslySetInnerHTML. This addresses the XSS vulnerabilities flagged in previous reviews.The conditional logic appropriately falls back to the default
ControlsHeaderwhen no custom header is provided.Note: The Biome static analysis warning about
dangerouslySetInnerHTMLis a false positive in this case, as the content is sanitized with DOMPurify before rendering.
243-251: LGTM: Footer HTML properly sanitized with DOMPurify.The custom footer HTML is correctly sanitized using
DOMPurify.sanitize()before rendering, consistent with the header implementation. This properly mitigates XSS risks.Note: The Biome static analysis warning is a false positive here as well—DOMPurify sanitization makes this usage safe.
57-80: Critical: JavaScript injection executes without verified security controls.The custom JavaScript injection at lines 57-80 shows only basic error handling (IIFE wrapper with try/catch). No client-side feature flag check, mandatory acknowledgment verification, or CSP enforcement is visible in this code. While the IIFE wrapper prevents global pollution, it does not prevent malicious code execution, session hijacking, or data exfiltration.
Before merging, verify server-side implementation of:
- Feature flag gating custom JavaScript execution
- Mandatory admin acknowledgment (with server-side validation)
- Content-Security-Policy headers
- Audit logging of custom JavaScript changes
Manual verification of these controls in server code, Settings UI, and configuration files is required to confirm this feature meets security requirements from Issue #2863.
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
server/src/db/v1/models/StatusPage.js (1)
81-92: Inconsistent indentation: use tabs to match the rest of the file.The new field definitions use 4-space indentation while the rest of the file uses tabs.
- customJavaScript: { - type: String, - default: "", - }, - headerHTML: { - type: String, - default: "", - }, - footerHTML: { - type: String, - default: "", - }, + customJavaScript: { + type: String, + default: "", + }, + headerHTML: { + type: String, + default: "", + }, + footerHTML: { + type: String, + default: "", + },server/src/validation/joi.js (1)
452-488: Inconsistent indentation in schema definition.This schema block now uses 4-space indentation while the rest of the file uses tabs.
client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsx (2)
186-196: Addidprop to TextField for accessibility consistency.Other inputs in this file include
idprops for label association. The TextField components should follow the same pattern.<TextField + id="customCSS" name="customCSS" // <--- CRITICAL: Needed for saving label={t("customCSS")} multilineApply similar changes to the other TextField components (headerHTML, footerHTML, customJavaScript).
241-251: Missing placeholder for footer HTML field.The header HTML field includes a placeholder for user guidance, but the footer HTML field does not. Consider adding one for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
client/package-lock.jsonis excluded by!**/package-lock.jsonserver/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsx(2 hunks)client/src/Pages/v1/StatusPage/Status/index.jsx(1 hunks)client/src/locales/en.json(1 hunks)server/src/db/v1/models/StatusPage.js(1 hunks)server/src/validation/joi.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsx (1)
client/src/Pages/v1/StatusPage/Status/index.jsx (1)
theme(26-26)
client/src/Pages/v1/StatusPage/Status/index.jsx (3)
client/src/Hooks/v1/useIsAdmin.js (1)
useIsAdmin(3-9)client/src/Pages/v1/StatusPage/Status/Hooks/useStatusPageFetch.jsx (1)
useStatusPageFetch(8-51)client/src/Pages/v1/StatusPage/Status/Components/AdminLink/index.jsx (1)
AdminLink(9-34)
🪛 Biome (2.1.2)
client/src/Pages/v1/StatusPage/Status/index.jsx
[error] 191-191: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 212-212: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (1)
client/src/Pages/v1/StatusPage/Status/index.jsx (1)
189-198: I'll help you verify the XSS vulnerability concern in this review comment. Let me search the codebase for sanitization approaches and dependencies.
<function_calls>
#!/bin/bashCheck for sanitization library usage and dependencies
echo "=== Searching for sanitization libraries in codebase ==="
rg -n "DOMPurify|sanitize-html|xss|isomorphic-dompurify" --type=js --type=ts --type=jsx --type=tsx 2>/dev/null | head -20echo -e "\n=== Checking for any imports related to sanitization ==="
rg -n "import.from.['"].*purif|import.from.['"].*sanitiz" --type=js --type=ts --type=jsx --type=tsx 2>/dev/nullecho -e "\n=== Looking for package.json files ==="
fd -t f "package.json" | head -5
</function_calls>#!/bin/bash # Check package.json for sanitization dependencies echo "=== Checking package.json for dependencies ===" if [ -f "package.json" ]; then cat package.json | jq '.dependencies, .devDependencies' 2>/dev/null | grep -i -E "purify|sanitize|xss|html-cleaner" || echo "No explicit sanitization packages found" fi echo -e "\n=== Checking if any HTML sanitization is done elsewhere ===" rg -n "dangerouslySetInnerHTML" --type=js --type=ts --type=jsx --type=tsx 2>/dev/null </parameter> </invoke> </function_calls> ```shell #!/bin/bash # Check the specific file and surrounding context echo "=== Examining the target file ===" if [ -f "client/src/Pages/v1/StatusPage/Status/index.jsx" ]; then wc -l client/src/Pages/v1/StatusPage/Status/index.jsx echo -e "\n--- Lines 180-210 context ---" sed -n '180,210p' client/src/Pages/v1/StatusPage/Status/index.jsx else echo "File not found at expected path, searching for it..." fd -t f "index.jsx" -path "*/StatusPage/Status/*" fi </parameter> </invoke> </function_calls> ```shell #!/bin/bash # Search for server-side sanitization or where headerHTML is set echo "=== Searching for headerHTML source and processing ===" rg -n "headerHTML" --type=js --type=ts --type=jsx --type=tsx 2>/dev/null | head -20 echo -e "\n=== Searching for any sanitization functions defined in the project ===" rg -n "function.*sanitiz|const.*sanitiz|export.*sanitiz" --type=js --type=ts --type=jsx --type=tsx 2>/dev/null </parameter> </invoke> </function_calls> </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| useEffect(() => { | ||
| if (!statusPage) return; | ||
|
|
||
| // Inject Custom CSS | ||
| if (statusPage.customCSS) { | ||
| const style = document.createElement('style'); | ||
| style.id = 'custom-status-css'; | ||
| style.innerHTML = statusPage.customCSS; | ||
| document.head.appendChild(style); | ||
| } | ||
|
|
||
| // Inject Custom JS | ||
| if (statusPage.customJavaScript) { | ||
| try { | ||
| const script = document.createElement('script'); | ||
| script.id = 'custom-status-js'; | ||
| // Wrap in IIFE to avoid global scope pollution | ||
| script.text = `(function() { ${statusPage.customJavaScript} })();`; | ||
| document.body.appendChild(script); | ||
| } catch (e) { | ||
| console.error("Custom JS execution failed:", e); | ||
| } | ||
| } | ||
|
|
||
| // Cleanup on unmount | ||
| return () => { | ||
| const css = document.getElementById('custom-status-css'); | ||
| const js = document.getElementById('custom-status-js'); | ||
| if (css) css.remove(); | ||
| if (js) js.remove(); | ||
| }; | ||
| }, [statusPage]); |
There was a problem hiding this comment.
Critical XSS vulnerability: custom JavaScript executed without sanitization or sandboxing.
Injecting user-provided JavaScript directly into the DOM enables arbitrary code execution. Even with admin-only access, this poses significant risks:
- Compromised admin accounts can inject malicious scripts affecting all visitors
- No Content Security Policy (CSP) mentioned to restrict script sources
- The try-catch block won't catch script execution errors (only script creation errors)
The linked issue #2863 specifically requires "strict Content Security Policy" and security controls. Consider:
- Requiring an explicit enable toggle/feature flag (as mentioned in issue objectives)
- Implementing CSP headers that restrict inline scripts
- At minimum, adding a confirmation dialog warning admins about the security implications
🤖 Prompt for AI Agents
In client/src/Pages/v1/StatusPage/Status/index.jsx around lines 34-65, the code
directly injects user-provided JavaScript into the DOM which creates a critical
XSS risk and the existing try/catch only covers creation, not runtime execution;
instead of appending inline scripts, stop direct injection and implement one or
more mitigations: require a server-side feature flag and explicit admin
enablement setting before any JS can be rendered, present a confirmation/warning
dialog to the enabling admin, render custom JS only inside a sandboxed iframe
with a restrictive sandbox attribute (no scripts/origins) or better yet disallow
execution and only allow safe configurable behaviors, and ensure the server
sends a strict Content-Security-Policy that disallows inline scripts and
restricts script-src to trusted origins; update cleanup accordingly and log the
admin consent and feature-flag checks so runtime script execution never occurs
without these controls.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR enables status page customization but introduces critical security vulnerabilities, including XSS risks from unsanitized HTML and arbitrary JavaScript execution that could compromise user sessions.
📄 Documentation Diagram
This diagram documents the enhanced status page rendering flow with custom CSS, JS, and HTML overrides.
sequenceDiagram
participant U as User
participant C as Client
participant S as Server
U->>C: Visit Status Page
C->>S: Fetch Status Page Data
S-->>C: Return Data (incl. custom fields)
note over C: PR #35;3076 adds custom injection logic
C->>C: Inject Custom CSS if present
C->>C: Render Custom Header/Footer if present
C->>C: Execute Custom JS if present
C-->>U: Display Status Page with Overrides
🌟 Strengths
- Implements a highly requested feature for branding status pages without code forking.
Findings Summary
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | client/.../Status/index.jsx | Security | XSS vulnerability via unsanitized HTML injection | |
| P1 | client/.../Status/index.jsx | Security | Arbitrary JS execution risks user session compromise | |
| P2 | client/.../Status/index.jsx | Architecture | Lacks content security controls risking performance | |
| P2 | server/.../joi.js | Maintainability | Missing size limits could degrade database performance | |
| P2 | client/.../Settings.jsx | Documentation | Insufficient UI warnings for high-risk JS feature |
🔍 Notable Themes
- Security Oversights: Multiple instances where user-provided content is executed without proper sanitization or warnings, posing significant risks to public status pages.
- Validation Gaps: Backend validation lacks essential constraints like size limits, which could lead to performance issues.
📈 Risk Diagram
This diagram illustrates the XSS and arbitrary JavaScript execution risks in the custom override implementation.
sequenceDiagram
participant A as Attacker
participant SP as Status Page
participant U as User
A->>SP: Inject Malicious HTML/JS
SP->>SP: Render without Sanitization
SP->>U: Serve Page with Malicious Code
U->>U: Execute Malicious Code
note over U: R1(P1): XSS via unsanitized HTML
note over U: R2(P1): Arbitrary JS execution
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| {statusPage?.headerHTML ? ( | ||
| <div dangerouslySetInnerHTML={{ __html: statusPage.headerHTML }} /> | ||
| ) : ( | ||
| <ControlsHeader | ||
| statusPage={statusPage} | ||
| url={url} | ||
| isPublic={isPublic} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
P1 | Confidence: High
The implementation uses dangerouslySetInnerHTML without any sanitization, creating a direct XSS vulnerability. Attackers can inject malicious scripts that execute in the context of the status page, potentially compromising user sessions or performing unauthorized actions. This affects all public status pages and their visitors.
Code Suggestion:
import DOMPurify from 'dompurify';
// ...
{statusPage?.headerHTML ? (
<div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(statusPage.headerHTML) }} />
) : (
<ControlsHeader statusPage={statusPage} url={url} isPublic={isPublic} />
)}
| } | ||
|
|
||
| // Inject Custom JS | ||
| if (statusPage.customJavaScript) { |
There was a problem hiding this comment.
P1 | Confidence: High
Direct execution of user-provided JavaScript via new Function() or script injection creates severe security risks. Malicious code can access sensitive data, modify page behavior, or perform actions on behalf of users. The IIFE wrapper only prevents global pollution but doesn't mitigate core security risks.
Code Suggestion:
// Consider implementing a strict CSP and sandboxed execution environment
// For immediate mitigation, add strong warnings and consider feature flag
{statusPage.customJavaScript && (
<div className="security-warning">
Custom JavaScript execution is disabled for security reasons
</div>
)}
|
|
||
| // Inject Custom CSS | ||
| if (statusPage.customCSS) { | ||
| const style = document.createElement('style'); |
There was a problem hiding this comment.
P2 | Confidence: Medium
The current implementation lacks proper content security controls. While the feature enables customization, it should be paired with server-side validation, size limits, and potentially a Content Security Policy (CSP). The related context shows this affects public-facing status pages which may have different security requirements than admin interfaces.
| </Typography> | ||
| </Stack> | ||
| <Stack gap={theme.spacing(6)}> | ||
| <TextField |
There was a problem hiding this comment.
P2 | Confidence: Medium
While there's a warning helper text, the UI doesn't sufficiently communicate the security implications to administrators. The feature enables arbitrary code execution on public pages, which should be accompanied by more prominent warnings and potentially an explicit acknowledgment checkbox.
Code Suggestion:
<TextField
name="customJavaScript"
label={t("customJavaScript")}
multiline
rows={4}
value={form.customJavaScript || ""}
onChange={handleFormChange}
fullWidth
margin="normal"
helperText={t("statusPageCreateCustomJavaScriptWarning")}
error={!!form.customJavaScript}
/>
<FormControlLabel
control={<Checkbox checked={acknowledgedRisk} onChange={...} />}
label={t("statusPageAcknowledgeSecurityRisk")}
/>
Co-authored-by: llamapreview[bot] <184758061+llamapreview[bot]@users.noreply.github.com>
| script.id = 'custom-status-js'; | ||
| // Wrap in IIFE to avoid global scope pollution | ||
| script.text = `(function() { ${statusPage.customJavaScript} })();`; | ||
| document.body.appendChild(script); |
There was a problem hiding this comment.
@Armaansaxena can you explain, why you are injecting customJS on component mount and using appendChld? I understand that intention is custom branding but we are essentially allowing anybody in control of the status file to run custom scripts on all user's browser.
There was a problem hiding this comment.
@SajanGhuman Valid question.
-
The "Why" (Use Case): The primary requirement for customJavaScript is to allow Admins to integrate third-party tools like Google Analytics, Intercom, or Chat Widgets on their status pages. These integrations universally require a script tag to be injected into the DOM to fetch their external resources.
-
The Implementation: I am using appendChild because React's dangerouslySetInnerHTML intentionally does not execute <script> tags for security reasons. To actually load a chat widget or analytics tracker, manual DOM manipulation via document.createElement is necessary.
Addressing the Risk: I fully agree this opens a Stored XSS vector if an Admin account is compromised. To mitigate this:
I am adding a prominent "Security Warning" on the settings page next to this input.
I am implementing DOMPurify for the HTML fields (Header/Footer) to strictly sanitize non-script inputs.
If you believe the risk of allowing JS overrides outweighs the benefit of having Analytics/Chat support, I am happy to remove the JS field entirely and stick to just CSS and Sanitized HTML. Please let me know your preference.
|
The headerHTML and footerHTML fields introduce raw HTML injection into the page. As CodeRabbit is warning, this is essentially creating an critical XSS risk. I understand that the feature is admin-only but rendering untrusted HTML and directly injecting JS is a bad practice overall. |
|
@SajanGhuman @ajhollid My plan to fix this: HTML Sanitization: I will implement DOMPurify to sanitize both headerHTML and footerHTML before rendering, ensuring no malicious scripts can be injected via these fields. Backend Limits: I will update the Joi validation to enforce strict character limits (as suggested by CodeRabbit) to prevent database performance issues. Custom JS: I understand the risk here. Since this was a requested feature in issue #2863 (allowing admins to override behavior), I will add a strict "Risk Acceptance" warning in the UI that the admin must check before saving. If you believe customJS is too dangerous even with admin warnings, please let me know, and I can remove that specific field and limit the scope to just CSS and Sanitized HTML. I will push these fixes shortly. |
…tization, and validation limits
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsx (1)
255-283: Insufficient security controls for arbitrary JavaScript execution.In the PR comments, you acknowledged the security concerns and promised to add an explicit "Risk Acceptance" warning with a checkbox that admins must check before saving custom JavaScript. However, this implementation only shows a warning text without requiring explicit acknowledgment.
As noted in past reviews, allowing arbitrary JavaScript execution on public pages is extremely high-risk. At minimum, implement the checkbox confirmation you described in your response to @SajanGhuman.
Consider this implementation:
const [acknowledgedJSRisk, setAcknowledgedJSRisk] = useState(false); // In the Custom JavaScript section: <Stack gap={theme.spacing(6)}> <TextField name="customJavaScript" label={t("customJavaScript")} color="warning" multiline rows={4} value={form.customJavaScript || ""} onChange={handleFormChange} disabled={!acknowledgedJSRisk} fullWidth margin="normal" helperText={t("statusPageCreateCustomJavaScriptWarning")} /> <FormControlLabel control={ <Checkbox checked={acknowledgedJSRisk} onChange={(e) => setAcknowledgedJSRisk(e.target.checked)} color="error" /> } label={t("statusPageAcknowledgeJavaScriptRisk")} /> </Stack>You'll also need to add the
FormControlLabelimport from@mui/material.client/src/Pages/v1/StatusPage/Status/index.jsx (3)
37-44: Custom CSS injection enables CSS-based attacks.While less severe than JavaScript injection, malicious CSS can:
- Exfiltrate data via
url()with encoded selectors (e.g.,input[value^="a"] { background: url(attacker.com?a) })- Create phishing overlays using absolute positioning and high z-index
- Deny service by injecting resource-intensive animations
Consider sanitizing CSS to remove dangerous constructs like
url(),@import,expression(), andbehavior.if (statusPage.customCSS) { styleElement = document.createElement("style"); styleElement.id = "custom-status-css"; - styleElement.innerHTML = statusPage.customCSS; + // Basic CSS sanitization - remove dangerous patterns + const sanitizedCSS = statusPage.customCSS + .replace(/url\s*\(/gi, '') // Remove url() + .replace(/@import/gi, '') // Remove @import + .replace(/expression\s*\(/gi, ''); // Remove IE expression() + styleElement.innerHTML = sanitizedCSS; document.head.appendChild(styleElement); }Alternatively, consider using a CSS sanitization library like
sanitize-cssor restricting custom CSS to a safe subset of properties.
207-216: Critical XSS vulnerability: headerHTML rendered without sanitization.The
headerHTMLfield is rendered usingdangerouslySetInnerHTMLwithout any sanitization, creating a direct stored XSS vulnerability. Malicious HTML can:
- Execute JavaScript via
<script>tags, event handlers (onclick,onerror, etc.), orjavascript:URLs- Steal session tokens and credentials
- Phish users by overlaying fake login forms
- Redirect users to malicious sites
In your PR comment, you acknowledged this issue and stated you would implement DOMPurify sanitization. However, DOMPurify is not imported or used anywhere in this file.
Apply this diff to sanitize the HTML before rendering:
+import DOMPurify from 'dompurify'; {/* --- CHANGE 3: Custom Header Logic --- */} {statusPage?.headerHTML ? ( - <div dangerouslySetInnerHTML={{ __html: statusPage.headerHTML }} /> + <div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(statusPage.headerHTML) }} /> ) : ( <ControlsHeader statusPage={statusPage} url={url} isPublic={isPublic} /> )}
228-232: Critical XSS vulnerability: footerHTML rendered without sanitization.Same issue as
headerHTML—thefooterHTMLfield is rendered without sanitization, creating a stored XSS vulnerability.Apply this diff:
{/* --- CHANGE 4: Custom Footer Logic --- */} {statusPage?.footerHTML && ( - <div dangerouslySetInnerHTML={{ __html: statusPage.footerHTML }} /> + <div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(statusPage.footerHTML) }} /> )}
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
client/package.json(2 hunks)client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsx(2 hunks)client/src/Pages/v1/StatusPage/Status/index.jsx(4 hunks)client/src/locales/en.json(1 hunks)server/src/db/v1/models/StatusPage.js(1 hunks)server/src/db/v1/modules/statusPageModule.js(3 hunks)server/src/validation/joi.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/src/validation/joi.js
- client/src/locales/en.json
🧰 Additional context used
🧬 Code graph analysis (2)
client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsx (1)
client/src/Pages/v1/StatusPage/Status/index.jsx (1)
theme(26-26)
client/src/Pages/v1/StatusPage/Status/index.jsx (3)
client/src/Pages/v1/StatusPage/Status/Components/ControlsHeader/index.jsx (1)
ControlsHeader(61-125)client/src/Pages/v1/StatusPage/Status/Components/StatusBar/index.jsx (1)
StatusBar(46-68)client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (1)
MonitorsList(14-62)
🪛 Biome (2.1.2)
client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsx
[error] 273-274: This JSX property is assigned multiple times.
This attribute is assigned again here.
(lint/suspicious/noDuplicateJsxProps)
client/src/Pages/v1/StatusPage/Status/index.jsx
[error] 209-209: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 230-230: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (4)
server/src/db/v1/modules/statusPageModule.js (3)
100-137: Data retrieval correctly includes new customization fields.The changes properly destructure and return
customCSS,customJavaScript,headerHTML, andfooterHTMLin the early-exit path when a status page has no monitors.
241-244: MongoDB projection correctly includes new customization fields.The aggregation pipeline properly projects the new fields for status pages with monitors.
68-299: Server-side HTML sanitization for sensitive fields must be verified and likely implemented.The code in
statusPageModule.jsretrievesheaderHTML,footerHTML, andcustomJavaScriptfields without visible sanitization. If these are user-controlled, they require server-side HTML/XSS sanitization before being returned to clients—client-side sanitization alone can be bypassed via direct API calls. Verify whether sanitization is implemented in the API route/controller layer or global middleware, and if not, add it to the data retrieval flow or response pipeline.client/package.json (1)
28-28: Add web search: dompurify 3.3.0 is the latest version (released October 2025) with no known security vulnerabilities; prior versions had CVE-2024-45801 and CVE-2024-47875, both fixed in 3.1.3 and later.The dependency version choice is sound, but verify that dompurify is actually imported and used in
Status/index.jsxor other affected components. IfheaderHTMLandfooterHTMLfields are rendered viadangerouslySetInnerHTMLwithout sanitization, this represents an XSS vulnerability that needs to be addressed regardless of whether the dependency is installed.
| // --- 2. Inject Custom JS --- | ||
| let scriptElement = null; | ||
| if (statusPage.customJavaScript) { | ||
| try { | ||
| scriptElement = document.createElement("script"); | ||
| scriptElement.id = "custom-status-js"; | ||
|
|
||
| // Wrap in IIFE (Immediately Invoked Function Expression) with internal try/catch | ||
| // This ensures that if the user writes bad JS, it errors to console but doesn't crash the UI | ||
| scriptElement.textContent = ` | ||
| (function() { | ||
| try { | ||
| ${statusPage.customJavaScript} | ||
| } catch(e) { | ||
| console.error('Custom Status Page Script Error:', e); | ||
| } | ||
| })(); | ||
| `; | ||
|
|
||
| document.body.appendChild(scriptElement); | ||
| } catch (e) { | ||
| console.error("Failed to inject custom JS element:", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Direct JavaScript injection creates arbitrary code execution vulnerability.
Injecting admin-provided JavaScript directly into the DOM enables arbitrary code execution on public status pages. Even with admin-only access, this poses severe risks:
- Compromised admin accounts can inject malicious scripts affecting all visitors
- Session hijacking: Scripts can steal session tokens, cookies, or credentials
- Phishing: Scripts can modify the page to capture user input
- Data exfiltration: Scripts can access and transmit sensitive data
- The IIFE wrapper only prevents global variable pollution—it does not prevent malicious code execution
- The try-catch only handles script creation errors, not runtime errors or malicious behavior
In your PR comment response to @SajanGhuman, you acknowledged these risks and stated you would implement mitigations. However, none of the promised security controls are present:
- ❌ No server-side feature flag
- ❌ No explicit admin enablement toggle
- ❌ No confirmation dialog (the Settings page has a warning but no mandatory checkbox)
- ❌ No Content Security Policy headers
- ❌ No sandboxing or restricted execution environment
Issue #2863 explicitly requires "strict Content Security Policy" and security controls. This implementation does not meet those requirements.
Recommended mitigations (in order of preference):
- Remove the feature entirely until proper security controls are implemented
- Require explicit feature flag: Add a server-side feature flag that must be enabled, with audit logging
- Implement CSP: Configure Content-Security-Policy headers to restrict inline scripts and script sources
- Sandbox execution: Execute custom JS in an isolated iframe with restrictive sandbox attributes
- At absolute minimum: Require the explicit checkbox acknowledgment you described in PR comments before any JS can be saved or executed
Consider whether the use case (analytics/chat widgets) can be served by a safer allowlist-based integration approach instead of arbitrary script execution.
🤖 Prompt for AI Agents
In client/src/Pages/v1/StatusPage/Status/index.jsx around lines 46-69, the
current approach injects admin-provided JavaScript directly into the DOM which
creates an arbitrary code execution vulnerability; remove this direct injection
and do not append untrusted script text to document.body. Instead, disable
execution until server-side controls exist: require a server-side feature flag
that explicitly enables custom-JS for the deployment, require an explicit
per-save admin acknowledgment checkbox (checked at save time and stored
server-side with audit logging) before any script can be stored or executed, and
only allow execution when the server flag is on and the stored script has the
admin acknowledgment. Replace direct injection with a safe execution model:
either refuse to execute arbitrary JS or render it inside an isolated iframe
with strict sandbox attributes (no allow-scripts unless absolutely necessary)
and a controlled src/srcdoc, and ensure the app sets a strict
Content-Security-Policy header disallowing inline scripts and limiting
script-src to trusted origins when the feature is enabled. If you cannot
implement server-side flagging, acknowledgment, CSP, and sandboxing immediately,
remove the execution path entirely so saved customJavaScript is stored but never
injected or run.
|
Hi @Armaansaxena, Perhaps the solution is to gate this feature behind some sort of approval process whereby the admin acknowledges the risks of allowing arbitrary code to be executed. @SajanGhuman @gorkemcetin what do you think? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsx(2 hunks)client/src/Pages/v1/StatusPage/Status/index.jsx(5 hunks)server/src/db/v1/models/StatusPage.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/src/db/v1/models/StatusPage.js
- client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/Pages/v1/StatusPage/Status/index.jsx (3)
client/src/Pages/v1/StatusPage/Status/Components/ControlsHeader/index.jsx (1)
ControlsHeader(61-125)client/src/Pages/v1/StatusPage/Status/Components/StatusBar/index.jsx (1)
StatusBar(46-68)client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (1)
MonitorsList(14-62)
🪛 Biome (2.1.2)
client/src/Pages/v1/StatusPage/Status/index.jsx
[error] 211-211: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 236-236: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (4)
client/src/Pages/v1/StatusPage/Status/index.jsx (4)
11-11: Good addition of DOMPurify for HTML sanitization.The DOMPurify import addresses previous security concerns and is correctly applied to both
headerHTMLandfooterHTMLrendering.Also applies to: 20-21
208-222: Excellent implementation of sanitized header HTML rendering.The use of
DOMPurify.sanitize()beforedangerouslySetInnerHTMLproperly mitigates XSS risks for custom header content, with appropriate fallback to the defaultControlsHeadercomponent.
233-241: Well-implemented sanitized footer HTML rendering.Consistent with the header implementation,
DOMPurify.sanitize()is correctly applied to protect against XSS attacks in custom footer content.
47-70: Critical: Direct JavaScript injection without security controls.The code wraps custom JavaScript in an IIFE with try/catch, but this only prevents global variable pollution—it does not prevent malicious code execution. The review comment references promised security controls (server-side feature flags, admin acknowledgement checks, CSP headers, and size limits) that should exist before this feature is merged, but the provided code snippet does not show these controls implemented.
Before merging, verify that the following security measures are in place:
- Server-side validation with size limits on the
customJavaScriptfield- Admin acknowledgement checkbox (stored and verified server-side before execution)
- Content-Security-Policy headers that restrict inline script execution
- Feature flag with audit logging for the custom JavaScript feature
If these controls were committed to in PR comments but are not present in the codebase, they must be implemented. If they were deferred, update the issue tracking to reflect that this feature remains incomplete from a security perspective.
I agree. There should be some kind of warning explaining the risks that the admin is aware of and acknowledges to. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
client/src/Pages/v1/StatusPage/Status/index.jsx (1)
55-78: Arbitrary JavaScript execution remains a critical security risk.The IIFE wrapper prevents global variable pollution but does not prevent malicious code execution. A compromised admin account can inject scripts that:
- Steal session tokens and cookies from all visitors
- Redirect users to phishing pages
- Modify page content to capture credentials
Issue #2863 requires "strict Content Security Policy" and feature flags, which are not implemented.
This was flagged in past reviews. The PR comments indicate the author proposed mitigations (explicit checkbox, warnings), which are partially implemented in Settings.jsx. However, the core risk remains: once saved, malicious JS executes for all visitors without server-side gating.
If this feature must ship, consider at minimum:
- A server-side feature flag to disable JS injection entirely
- CSP headers that restrict inline scripts
- Audit logging when custom JS is saved
🧹 Nitpick comments (4)
client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsx (3)
54-58: useEffect only enables risk acceptance, never resets it.When an admin clears the JavaScript field completely,
riskAcceptedremainstruebecause this effect only sets it totrueand never resets it. This could lead to confusion if the admin expects the checkbox state to reflect the field content.Consider resetting the state when the field is empty:
useEffect(() => { - if (form.customJavaScript && form.customJavaScript.length > 0) { - setRiskAccepted(true); - } + setRiskAccepted(form.customJavaScript?.length > 0); }, [form.customJavaScript]);Alternatively, if the intent is to require re-acknowledgment when editing, keep the current logic but document this behavior.
262-300: Inconsistent styling compared to other ConfigBox sections.The Custom JavaScript section uses different styling patterns:
gap={2}instead ofgap={theme.spacing(6)}used elsewhereTypography variant="h6"instead ofvariant="h2"used in other section headers<ConfigBox> - <Stack gap={2}> - <Typography variant="h6">{t("customJavaScript")}</Typography> + <Stack gap={theme.spacing(6)}> + <Typography component="h2" variant="h2"> + {t("customJavaScript")} + </Typography> + <Typography component="p"> + {t("statusPageCreateCustomJavaScriptDescription")} + </Typography> + </Stack> + <Stack gap={theme.spacing(6)}> <FormControlLabel
248-258: Footer HTML TextField is missing a placeholder.The Header HTML field has a
placeholderprop (line 230), but the Footer HTML field does not. Consider adding one for consistency.<TextField name="footerHTML" label={t("customFooterHTML")} multiline rows={3} value={form.footerHTML || ""} onChange={handleFormChange} fullWidth margin="normal" + placeholder={t("statusPageCreateCustomFooterHTMLPlaceholder")} />client/src/Pages/v1/StatusPage/Status/index.jsx (1)
217-229: Consider stricter DOMPurify configuration for HTML sanitization.While DOMPurify.sanitize() mitigates the most severe XSS vectors, the default configuration allows certain event handlers and attributes that could be exploited. The static analysis warning is a false positive given DOMPurify usage, but stricter config is recommended.
+// At module level or in the component +const DOMPURIFY_CONFIG = { + ALLOWED_TAGS: ['div', 'span', 'p', 'a', 'img', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', + 'ul', 'ol', 'li', 'strong', 'em', 'br', 'hr'], + ALLOWED_ATTR: ['href', 'src', 'alt', 'class', 'id', 'style'], + ALLOW_DATA_ATTR: false, +}; {statusPage?.headerHTML ? ( <div dangerouslySetInnerHTML={{ - __html: DOMPurify.sanitize(statusPage.headerHTML) + __html: DOMPurify.sanitize(statusPage.headerHTML, DOMPURIFY_CONFIG) }} />Apply the same config to
footerHTMLsanitization on line 245.Also applies to: 242-248
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsx(5 hunks)client/src/Pages/v1/StatusPage/Status/index.jsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsx (1)
client/src/Pages/v1/StatusPage/Status/index.jsx (1)
theme(27-27)
🪛 Biome (2.1.2)
client/src/Pages/v1/StatusPage/Status/index.jsx
[error] 219-219: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 244-244: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (1)
client/src/Pages/v1/StatusPage/Status/index.jsx (1)
80-91: Cleanup logic is correct and handles edge cases well.The cleanup function properly uses direct references to the created elements and checks
contains()before removal, preventing errors if elements were already removed. Good defensive programming.
| {t("Security Risk Warning") || | ||
| "I understand that adding custom JavaScript poses a security risk and I accept responsibility."} |
There was a problem hiding this comment.
Translation fallback pattern will not work as expected.
t("Security Risk Warning") returns the key string itself if the key is not found, not an empty string. The || fallback will therefore never be used.
Either add the translation key to your i18n files, or use t() with a default value:
-{t("Security Risk Warning") ||
- "I understand that adding custom JavaScript poses a security risk and I accept responsibility."}
+{t("securityRiskWarning", { defaultValue: "I understand that adding custom JavaScript poses a security risk and I accept responsibility." })}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {t("Security Risk Warning") || | |
| "I understand that adding custom JavaScript poses a security risk and I accept responsibility."} | |
| {t("securityRiskWarning", { defaultValue: "I understand that adding custom JavaScript poses a security risk and I accept responsibility." })} |
🤖 Prompt for AI Agents
In client/src/Pages/v1/StatusPage/Create/Components/Tabs/Settings.jsx around
lines 281-282, the expression using `t("Security Risk Warning") || "I
understand..."` is incorrect because `t()` returns the key when missing so the
`||` fallback never triggers; either add the "Security Risk Warning" key to your
i18n translation files, or change the call to provide a default value via the
i18n API (e.g. use t("Security Risk Warning", { defaultValue: "I understand that
adding custom JavaScript poses a security risk and I accept responsibility." }))
so the human-readable fallback is used when the key is absent.
| // SECURITY FIX: Remove dangerous CSS patterns (url, import, expression) | ||
| const safeCSS = statusPage.customCSS | ||
| .replace(/url\s*\(/gi, '') // Block external data/images | ||
| .replace(/@import/gi, '') // Block external stylesheets | ||
| .replace(/expression\s*\(/gi, '') // Block IE scripts | ||
| .replace(/behavior:/gi, ''); // Block IE behaviors | ||
|
|
||
| styleElement.textContent = safeCSS; // Use textContent instead of innerHTML for extra safety | ||
| document.head.appendChild(styleElement); |
There was a problem hiding this comment.
CSS sanitization regex is bypassable and incomplete.
The regex-based sanitization can be circumvented with whitespace/comment variations (e.g., url/**/(). Additionally, missing patterns include:
data:URIs inurl()(already blocked, but only partially)-moz-binding(legacy Firefox XBL)- Unicode escapes (
\0075rl()
Consider using a dedicated CSS sanitizer library or a stricter allowlist approach. If regex must be used, a more robust pattern:
const safeCSS = statusPage.customCSS
- .replace(/url\s*\(/gi, '')
- .replace(/@import/gi, '')
- .replace(/expression\s*\(/gi, '')
- .replace(/behavior:/gi, '');
+ .replace(/url\s*\(/gi, 'blocked(')
+ .replace(/@import\b/gi, '/* blocked */')
+ .replace(/expression\s*\(/gi, 'blocked(')
+ .replace(/behavior\s*:/gi, 'blocked:')
+ .replace(/-moz-binding\s*:/gi, 'blocked:')
+ .replace(/javascript\s*:/gi, 'blocked:');Note: This is still defense-in-depth; a proper CSS parser/sanitizer is more robust.
3 participants
Describe your changes
I have implemented the ability for admins to customize their Status Pages with custom CSS, JavaScript, and HTML overrides for the Header and Footer. This allows teams to brand their status pages without forking the source code.
Backend Changes:
StatusPageMongoose schema to includecustomJavaScript,headerHTML, andfooterHTML(added to existingcustomCSS).server/validation/joi.js) to allow these string fields during creation and updates.Frontend Changes:
Settings.jsxto include input fields for these new options, ensuring correct name attributes for form saving.en.jsonto ensure no hardcoded strings are used in the UI.Settings.jsxto inject the custom CSS/JS viauseEffectand conditionally render the custom HTML Header/Footer if they exist in the database.Write your issue number after "Fixes "
Fixes #2863
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>, use):