Fixed Security Vulnerability: Notification HTML reaching Ghost Admin unsanitised - #29746
Fixed Security Vulnerability: Notification HTML reaching Ghost Admin unsanitised#29746pptx704 wants to merge 1 commit into
Conversation
Notification bodies reach Ghost Admin as rendered HTML - the clients mark server notifications htmlSafe so release notes can carry links. Nothing sanitised them, and "Add notifications" was granted to Editor and Super Editor, so an editor-tier account could store script that runs in an Owner's admin session on their next admin page load. - sanitise on write and on read, so installs already holding unsafe HTML in the notifications setting are covered without a data fix - purify the Ember render paths as a second layer, and drop the never-populated dangerouslySetInnerHTML in the React About modal - drop "Add notifications" from Editor and Super Editor; browse and destroy stay so they can still see and dismiss notifications The sanitiser keeps links and formatting and strips script, event handlers and non-http(s) schemes, so release notifications render unchanged. Authored-by: Warden Harness <contact@omukk.dev>
github-actions
Bot
added
the
migration
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
WalkthroughNotification messages are sanitized before persistence, retrieval, email rendering, and client handling. The settings upgrade notification now renders escaped text. The notification sanitizer export is renamed to Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ember-admin/app/services/upgrade-status.js`:
- Around line 18-20: Keep upgradeStatus.message as a plain sanitized string by
removing htmlSafe() from the assignment in the upgrade status service and
storing DOMPurify.sanitize(message ?? '') directly. In
apps/ember-admin/app/services/upgrade-status.js lines 18-20, update the message
assignment; in apps/admin-x-settings/src/components/settings/general/about.tsx
lines 70-74, add a regression test verifying a populated upgrade message renders
in the About modal.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f185be2-edcb-40b4-a722-3bdad52ca834
⛔ Files ignored due to path filters (1)
ghost/core/test/unit/server/services/notifications/__snapshots__/sanitize-notification-html.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
apps/admin-x-settings/src/components/settings/general/about.tsxapps/ember-admin/app/services/notifications.jsapps/ember-admin/app/services/upgrade-status.jsghost/core/core/server/data/migrations/versions/6.56/2026-08-04-10-00-00-remove-add-notifications-permission-from-editors.jsghost/core/core/server/data/schema/fixtures/fixtures.jsonghost/core/core/server/services/notifications/notification-email.tsghost/core/core/server/services/notifications/notifications.jsghost/core/core/server/services/notifications/sanitize-notification-html.tsghost/core/test/unit/server/services/notifications/notifications.test.jsghost/core/test/unit/server/services/notifications/sanitize-notification-html.test.ts
| // Rendered unescaped by the About modal, so purify before trusting it. | ||
| // The server sanitises this too - this is the second layer, not the only one. | ||
| set(this, 'message', htmlSafe(DOMPurify.sanitize(message ?? ''))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'htmlSafe\(DOMPurify\.sanitize|upgradeStatus\.message' \
apps/ember-admin/app/services/upgrade-status.js \
apps/admin-x-settings/src/components/settings/general/about.tsx
fd -t f -i '.*(upgrade-status|about).*(test|spec).*' appsRepository: TryGhost/Ghost
Length of output: 2196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## candidate file outlines"
ast-grep outline apps/ember-admin/app/services/upgrade-status.js || true
ast-grep outline apps/admin-x-settings/src/components/settings/general/about.tsx || true
echo "## relevant service/service tests"
for f in $(git ls-files | rg 'upgrade-status|notification.*upgrade|upgrade.*notification'); do
if [[ "$f" =~ (test|spec|fixture) ]]; then
echo "--- $f"
sed -n '1,220p' "$f" | nl -ba
fi
done
echo "## upgrade status imports and message usages"
rg -n -C 3 "UpgradeStatusService|upgrade-status|Upgrade status|message:|message\??" apps/ember-admin apps/admin-x-settings packages/admin-x-shared || true
echo "## check package ember deps"
fd -a 'package.json|ember-cli-build.js|config/targets.js|index.ts|index.html' apps/ember-admin . | sed 's#^\./##' | head -100
rg -n '"([^"]*)ember[^"]*"|htmlSafe|SafeString|react-dom|react' package.json apps/ember-admin package.json apps/admin-x-settings/package.json 2>/dev/null || trueRepository: TryGhost/Ghost
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## upgrade-status.js"
sed -n '1,120p' apps/ember-admin/app/services/upgrade-status.js | nl -ba
echo "## exact usages of UpgradeStatusService/message"
rg -n -C 5 "UpgradeStatusService|`@service`.*UpgradeStatus|service\\.UpgradeStatus|upgradeStatus|\\.message" apps/ember-admin/app apps/admin-x-settings/src 2>/dev/null | sed -n '1,240p'
echo "## files with upgrade-status in acceptance/unit tests"
rg -l -C 4 "UpgradeStatusService|upgrade/status|upgradeStatus|handleUpgradeNotification|available upgrade|message:" apps/ember-admin/tests | sed -n '1,120p'
echo "## inspect admin-x about component"
sed -n '1,130p' apps/admin-x-settings/src/components/settings/general/about.tsx | nl -ba
echo "## package deps (focused)"
rg -n '"(ember-source|ember-cli-htmlbars|`@ember/ember-test-selectors`|dompurify|react|react-dom)"|htmlSafe|SafeString' apps/ember-admin/package.json apps/admin-x-settings/package.json package.json apps/ember-admin/ember-cli-build.js 2>/dev/null || trueRepository: TryGhost/Ghost
Length of output: 213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## upgrade-status.js"
sed -n '1,120p' apps/ember-admin/app/services/upgrade-status.js | awk '{printf "%5d\t%s\n", NR, $0}'
echo "## exact usages of UpgradeStatusService/message"
rg -n -C 5 "UpgradeStatusService|`@service`.*UpgradeStatus|service\\.UpgradeStatus|upgradeStatus|\\.message" apps/ember-admin/app apps/admin-x-settings/src 2>/dev/null | sed -n '1,240p'
echo "## files with upgrade-status in acceptance/unit tests"
rg -l -C 4 "UpgradeStatusService|upgrade/status|upgradeStatus|handleUpgradeNotification|available upgrade|message:" apps/ember-admin/tests | sed -n '1,120p'
echo "## inspect admin-x about component"
sed -n '1,130p' apps/admin-x-settings/src/components/settings/general/about.tsx | awk '{printf "%5d\t%s\n", NR, $0}'
echo "## package deps (focused)"
rg -n '"(ember-source|ember-cli-htmlbars|`@ember/ember-test-selectors`|dompurify|react|react-dom)"|htmlSafe|SafeString' apps/ember-admin/package.json apps/admin-x-settings/package.json package.json apps/ember-admin/ember-cli-build.js 2>/dev/null || trueRepository: TryGhost/Ghost
Length of output: 30203
🌐 Web query:
Ember SafeString htmlSafe constructor string property @ember/template SafeString
💡 Result:
In Ember.js, the SafeString class is not intended to be user-constructible [1][2][3]. The only public and supported API for creating a SafeString is by using the htmlSafe function, which is imported from @ember/template [4][1][2]. Attempting to instantiate SafeString directly is discouraged as it is an internal implementation detail, not a public API [1][2]. Key points regarding htmlSafe and SafeString: 1. Purpose: htmlSafe is used to mark a string as trusted, indicating to Ember's rendering layer that the string should not be HTML-escaped when rendered in a template [4][1][2]. 2. Security Warning: Marking a string as "safe" does not actually sanitize it [1][2]. If the string contains user-provided input, you must manually sanitize it before passing it to htmlSafe to prevent Cross-Site Scripting (XSS) vulnerabilities [1][2]. 3. Usage: import { htmlSafe } from '@ember/template'; const safeString = htmlSafe('
@ember/template [5]. Historically, htmlSafe was associated with @ember/string in older versions of Ember, but it has long been part of @ember/template [6][7][8]. Always rely on the current official API documentation for your version of Ember [4][2].
Citations:
- 1: https://github.com/emberjs/ember.js/blob/v6.10.0-ember-source/packages/@ember/-internals/glimmer/lib/utils/string.ts
- 2: https://api.emberjs.com/ember/release/classes/safestring/
- 3: https://api.emberjs.com/ember/6.9/classes/safestring/
- 4: https://api.emberjs.com/ember/release/functions/@ember%2Ftemplate/htmlSafe
- 5: https://github.com/emberjs/ember.js/blob/v3.25.1/packages/@ember/-internals/glimmer/lib/utils/string.ts
- 6: https://api.emberjs.com/ember/2.17/classes/@ember%2Fstring/methods/htmlSafe?anchor=htmlSafe
- 7: https://api.emberjs.com/ember/3.0/functions/@ember%2Ftemplate/htmlSafe
- 8: https://api.emberjs.com/ember/3.6/functions/@ember%2Ftemplate/htmlSafe
🌐 Web query:
Ebay nice-modal React SafeString object as JSX child React TypeError warning
💡 Result:
The issue you are encountering with a "SafeString" object (or similar unexpected object types) being passed as a JSX child in nice-modal-react typically stems from an incompatibility between how React renders children and how the modal state or props are being managed [1][2]. In React, JSX children must be valid React nodes (strings, numbers, elements, or arrays of these); passing a complex object—such as a internal library SafeString or a state object—directly into the JSX tree triggers the "Objects are not valid as a React child" TypeError [1][3]. While nice-modal-react is designed to manage modal state and props globally via context, it does not automatically sanitize or serialize objects passed as props or children [2][3]. If you are experiencing this error, consider the following troubleshooting steps: 1. Check your Modal Component definition: Ensure that the modal component created with NiceModal.create or used as a functional component does not attempt to render an object property directly [3][4]. For example, if you pass a complex object (like a SafeString object from a third-party library) as a prop, ensure it is being accessed via a primitive property (e.g., props.myObj.toString() or props.myObj.value) before being rendered [3][4]. 2. Validate NiceModal.show arguments: If you are calling NiceModal.show(id, args), ensure the args object contains only serializable data [2][3]. If an object is passed as an argument, it becomes part of the modal's internal state [3]. If your modal component attempts to render that object as a child, React will throw the error [3]. 3. Verify Component Props: Ensure the modal component is not accidentally receiving the SafeString object as a direct child or as a prop that is subsequently injected into the DOM/JSX tree without conversion to a primitive [3]. If the SafeString is coming from an external utility (often used for HTML-safe strings), you must explicitly convert it to a string or a valid React-renderable format before passing it into your modal or using it inside the modal's return JSX [2][3]. For further debugging, inspect the component receiving the SafeString and locate where the object is being used in the render path. If this is a type-related issue during development, ensure your TypeScript definitions correctly reflect that the prop is an object rather than a renderable React node [5][6][7].
Citations:
- 1: https://github.com/eBay/nice-modal-react/blob/main/src/index.tsx
- 2: https://github.com/ebay/nice-modal-react
- 3: https://github.com/eBay/nice-modal-react?tab=readme-ov-file
- 4: https://github.com/eBay/nice-modal-react/blob/main/README.md
- 5: Add type safety to modal.show() eBay/nice-modal-react#9
- 6: Type regression in 1.2.9 eBay/nice-modal-react#127
- 7: Support optional component props generic in
NiceModalArgs? eBay/nice-modal-react#80
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg 'admin-x-settings/app|admin-x-admin|lib/monolith|ember-admin/ember-cli-build|package.json' | sed -n '1,160p'
echo "## inspect admin-x settings provider files"
for f in $(git ls-files apps/admin-x-settings/src/components/providers apps/admin-x-settings/src/app.tsx | rg 'settings-app-provider|global-data-provider|app.tsx|index.tsx'); do
echo "--- $f"
sed -n '1,180p' "$f"
done
echo "## inspect admin-x package react/dom deps"
sed -n '1,120p' apps/admin-x-settings/package.json | awk '{printf "%5d\t%s\n", NR, $0}'
echo "## search migration/embed code for upgradeStatus prop"
rg -n -C 5 "upgradeStatus|UpgradeStatusType|settings-app-provider|`@tryghost/admin-x-settings`|Embed|admin-x" apps ember-admin app lib packages 2>/dev/null | sed -n '1,260p'Repository: TryGhost/Ghost
Length of output: 36640
Keep upgradeStatus.message as a plain string.
htmlSafe() returns an Ember SafeString, and upgradeStatus.message is consumed directly by React as a JSX child. React does not render SafeString objects as text, so a populated upgrade message stops the About modal from rendering.
apps/ember-admin/app/services/upgrade-status.js#L20: storeDOMPurify.sanitize(message ?? '')directly.apps/admin-x-settings/src/components/settings/general/about.tsx#L70-74: add a regression test with a populated upgrade message.
📍 Affects 2 files
apps/ember-admin/app/services/upgrade-status.js#L18-L20(this comment)apps/admin-x-settings/src/components/settings/general/about.tsx#L70-L74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ember-admin/app/services/upgrade-status.js` around lines 18 - 20, Keep
upgradeStatus.message as a plain sanitized string by removing htmlSafe() from
the assignment in the upgrade status service and storing
DOMPurify.sanitize(message ?? '') directly. In
apps/ember-admin/app/services/upgrade-status.js lines 18-20, update the message
assignment; in apps/admin-x-settings/src/components/settings/general/about.tsx
lines 70-74, add a regression test verifying a populated upgrade message renders
in the About modal.
Source: MCP tools
|
@pptx704 Hey, thanks for the submission and report. In the future, these are ideally submitted through security@ghost.org so we can handle them securely, though this one only impacts Admin users. While your PR is pretty close, I want to separate out the concerns - the migration and unnecessary permissions + the sanitization - so I'm going to PR this and credit you in those commits. |
ref #29746 - removed `notification:add` from the Editor and Super Editor roles - retained `notification:browse` and `notification:destroy` so those roles can still view and dismiss notifications - added a migration for existing installs and aligned production/test fixtures and integrity expectations - added API regression coverage for Administrator, Editor, and Super Editor notification creation permissions Notifications are a system-wide administrative channel. Editor-tier roles need to view and dismiss them, but do not need to create them. Thanks to @pptx704 for reporting this permission gap in #29746.
ref #29746 Notification messages are rendered as trusted HTML in Admin. Sanitising on read and write protects both new and previously stored records while preserving semantic update-service content and links.
2 participants
Fixed notification HTML reaching Ghost Admin unsanitised
Classification
htmlSafe,dangerouslySetInnerHTML)Add notificationsis granted to the Editor and Super Editor roles, making the sink reachable below the administrator boundarystatusfield defaults to'alert', which is precisely the value that triggers the HTML-trusted render pathAV:N/AC:L/PR:L/UI:R/S:C/C:H/I:H/A:N). Scoring availability asA:H, on the basis that a compromised Owner session can destroy site content, yields 9.0 (Critical). Unofficial, self-assessed.mainas of this branch. No claim is made about the full affected range — the pattern predates 6.21.Why this is an escalation, not self-XSS
Editors are a semi-trusted role on multi-author publications. The primitive lets an Editor execute JavaScript in the admin origin under the session of every Owner, Administrator and Editor who opens Ghost Admin — crossing a role boundary Ghost otherwise enforces server-side. From there the attacker inherits the victim's ambient session against the Admin API.
Aggravating factor: no in-product remediation
DELETE /notifications/:iddoes not delete.destroy()only marks the notification seen for the calling user;destroyAllis not exposed over HTTP, andnotificationsis acore-group setting with no settings-API write path. An injected payload therefore cannot be removed through the admin UI, and deleting the attacker's account does not clear it. Eviction otherwise requires direct database access — which is why the fix sanitises on read as well as on write, so upgrading alone neutralises already-stored payloads.Summary
Ghost stores notification bodies without sanitising them, and both admin clients render those bodies as trusted HTML. Because the
Add notificationspermission is granted to the Editor and Super Editor roles, a user holding an editor-tier account can persist arbitrary HTML — including script — that executes in the admin-origin session of every Owner, Administrator and Editor who loads Ghost Admin.This is a privilege escalation across a staff role boundary, not merely self-XSS.
The vulnerability
The chain has four links, all in current
main:services/notifications/notifications.jsadd()merges caller input straight into thenotificationssetting.messageis never sanitised. Thestatusfield defaults to'alert'.fixtures.jsongrantsnotification: "all"toEditorandSuper Editor, which includes theAdd notificationspermission.POST /ghost/api/admin/notifications/therefore succeeds for an Editor.apps/ember-admin/app/services/session.js→loadServerNotifications():apps/ember-admin/app/services/notifications.js→handleNotification():gh-alert.hbsthen renders{{@message.message}}. The double-stache would normally escape, buthtmlSafe()overrides that.apps/ember-admin/app/services/upgrade-status.jsapplieshtmlSafe()on the same data for the other branch, andadmin-x-settings' About modal rendered it viadangerouslySetInnerHTML.There is no
Content-Security-Policyon/ghost/, so nothing contains the payload once it lands.Impact
An Editor obtains code execution in the Owner's admin origin. The alert bar renders on every admin page load, so no interaction beyond opening Ghost Admin is required, and the payload persists in the
notificationssetting until explicitly deleted.Verified end-to-end against a live Ghost 6.21 instance with an account whose only role was
Editor.Proof of concept
Minimal, non-destructive. Steps 1–2 as an Editor; step 3 observed as Owner.
Swapping the
<b>for<img src=x onerror=...>yields script execution in the Owner's session; that variant was confirmed but is omitted here deliberately.Cleanup is not straightforward, which is part of the finding.
DELETE /ghost/api/admin/notifications/<id>/does not delete —destroy()only setsseenand appends the caller toseenBy:The record stays in the
notificationssetting and continues to render for every staff user who has not individually dismissed it.destroyAllexists but is explicitly not exposed over HTTP, andnotificationsis acore-group setting, so there is no supported API path to purge an injected payload. Removing one requires dismissing as each affected user, or editing the setting directly in the database.This materially raises severity: the payload is not just persistent, it is not removable through the admin UI, and deleting the attacker's account does not clear it. Note that the read-side sanitisation in this PR neutralises already-stored payloads on upgrade, without operator intervention — that is the main reason it is not write-side only.
Higher-severity variants
Not demonstrated, but reachable from the same primitive once script runs in an Owner session:
destroy()being dismiss-only anddestroyAllnot being exposed over HTTP, an injected notification cannot be removed through the admin UI at all — eviction requires direct database access.updates.ghost.orgthroughupdate-check-service.js(message: message.content). A compromise of that feed would deliver HTML into the admin of every install that polls it. Notably the email rendering of this exact content was already sanitised, with a code comment naming a compromised feed as the threat — the in-app path simply lacked the equivalent guard.<a>to an external site. That is why this PR also removes the permission rather than relying on sanitisation alone.The fix
notifications.js). Write-side stops new unsafe HTML being persisted. Read-side matters independently: installs upgrading from an affected version may already hold unsafe HTML in thenotificationssetting, and this covers them without a data migration.sanitize-email-html.ts→sanitize-notification-html.tssince it is no longer email-specific. The allowlist is unchanged: links and formatting are preserved;script, event handlers and non-http(s) schemes are stripped. Release notifications render exactly as before.DOMPurify.sanitize()beforehtmlSafe().dompurifywas already a dependency there. The React About modal'sdangerouslySetInnerHTMLis replaced with text rendering — nothing in the repo currently supplies that prop, so there is no markup to preserve and no new dependency was added.Add notificationsis dropped fromEditorandSuper Editorinfixtures.json, with a migration for existing installs.browseanddestroyare retained so editors can still see and dismiss notifications. Nothing in Ghost Admin posts notifications — the endpoint exists for the update-check service and integrations — so this removes no working functionality.Testing
test/unit/server/services/notifications/notifications.test.js— 12 passed, including three new cases covering write-side sanitisation, read-side sanitisation of already-stored HTML, and non-string message passthrough.sanitize-notification-html.test.ts— 1 passed (snapshot renamed and updated).notification-email.test.ts— 3 passed.test/unit/server/services/update-check— 14 passed.ghost/core,apps/ember-adminandapps/admin-x-settings.Not run: the Ember acceptance suite, and a live migration against a seeded database. Both are worth exercising in CI.
Notes for reviewers
Content-Security-Policyon/ghost/, and the admin session cookie isSameSite=None(deliberate, for the comments-ui auth iframe), which leaves the Origin check as the sole CSRF barrier on these endpoints.Disclosure
This issue was automatically identified, tested, and fixed by Warden (in development), an autonomous penetration testing agent using DeepSeek V4 Flash.