✨ Added card plugin system for custom editor cards#28642
Open
danielperez9430 wants to merge 2 commits into
Open
✨ Added card plugin system for custom editor cards#28642danielperez9430 wants to merge 2 commits into
danielperez9430 wants to merge 2 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces an end-to-end custom card plugin system for Ghost. Two new lab feature flags ( Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 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/admin-x-settings/src/components/settings/advanced/labs/plugins.tsx`:
- Line 2: In the plugins.tsx file, the Button component import needs to be
separated from the SettingGroupHeader import. Remove Button from the existing
import statement that sources from `@tryghost/admin-x-design-system`, leaving only
SettingGroupHeader from that package. Then add a new separate import statement
to import Button from `@tryghost/shade/components` instead, aligning with the
pattern used in other admin-x-settings features like
welcome-email-customize-modal.tsx.
- Around line 31-34: The div element using dangerouslySetInnerHTML with
card.icon creates an XSS vulnerability since card.icon comes from untrusted
user-uploaded plugin files. Sanitize the card.icon value before passing it to
dangerouslySetInnerHTML by either adding DOMPurify as a dependency and using it
to sanitize the HTML string, or by implementing a safe SVG rendering approach
that restricts icon content to safe SVG elements only. Additionally, review the
component imports to ensure it uses the shade design system utilities instead of
`@tryghost/admin-x-design-system` per coding guidelines.
In `@ghost/admin/app/components/koenig-lexical-editor.js`:
- Around line 151-183: The pluginCards getter only caches successful responses
by setting _pluginCardsLoaded to true inside the if (xhr.status === 200) block.
When requests fail or return non-200 status codes, _pluginCardsLoaded remains
false, causing the getter to retry the synchronous XHR request on every
invocation instead of caching the failure result. To fix this, move the line
that sets _pluginCardsLoaded = true outside the HTTP 200 check so that the cache
flag is set regardless of whether the request succeeds or fails. This ensures
that failed requests are also cached and subsequent calls will return the cached
empty array instead of retrying the blocking synchronous XHR call. Apply this
same fix to the other affected location mentioned in the comment.
In `@ghost/core/content/plugins/infobox/card.css`:
- Line 8: Remove the quotes around the font family name 'Inter' in the
font-family property declaration. The Stylelint rule font-family-name-quotes
expects unquoted font family names in this context. Change 'Inter' to Inter
while keeping the rest of the font-family value intact (system-ui,
-apple-system, sans-serif).
In `@ghost/core/content/plugins/review/card.css`:
- Line 14: The font-family property declaration violates the stylelint
`font-family-name-quotes` rule because the font name `Inter` is quoted with
single quotes. Remove the single quotes around `Inter` in the font-family
declaration to comply with the rule, leaving only the generic font families like
system-ui and sans-serif unquoted as appropriate.
In `@ghost/core/content/plugins/review/preprocess.js`:
- Around line 26-31: The percent calculation in the return object currently only
clamps the upper bound to 100 using Math.min, but negative scores can produce
negative percent values which cause invalid bar widths in rendering. Fix the
percent property by wrapping the existing Math.min call with Math.max(0, ...) to
clamp the result to the range between 0 and 100, ensuring both lower and upper
bounds are properly constrained for the (score / 10) * 100 calculation.
In `@ghost/core/content/plugins/review/template.html`:
- Around line 9-30: The outer `{{`#if` pros}}` condition starting at line 9
incorrectly wraps both the pros section and the cons section, preventing the
cons section from rendering when pros is empty. To fix this, restructure the
template so that the `{{`#if` pros}}` block only wraps the pros content div
(closing the condition after the pros section ends), while the cons content div
remains as a separate block with its own existing `{{`#if` cons}}` condition. Both
sections should remain as children of the `review-card__pros-cons` outer div,
allowing each section to render independently based on its own data
availability.
In `@ghost/core/core/server/api/endpoints/card-plugins.js`:
- Around line 231-237: The deletion conversion logic currently always uses
plugin.cards[0] template for all plugin-card nodes with the same pluginName,
which causes incorrect template application for plugins with multiple cards.
Instead of hardcoding plugin.cards[0], match each plugin-card node to its
corresponding card object from the plugin.cards array (likely by a card ID or
name attribute on the node), and then use that specific card's template when
compiling with Handlebars.compile(). This matching logic needs to be applied at
both the initial conversion block and the deletion conversion block to ensure
each node is converted with its correct template.
- Around line 63-100: The extractZipEntry function is vulnerable to ZIP Slip
attacks because it uses entry.name directly in path.join without validating that
the resulting targetPath remains contained within the targetDir. To fix this,
after constructing targetPath using path.join(targetDir, entry.name), normalize
both the targetPath and targetDir to absolute paths using path.resolve(), then
verify that the normalized targetPath starts with the normalized targetDir
(ensuring it is truly contained). If the check fails, throw an error indicating
a path traversal attempt. This validation must occur before any file system
operations like mkdirSync or writeFileSync are performed on targetPath.
- Around line 326-329: The issue is that fs.remove(zipBasePath) in the
.finally() block executes immediately after pipe(res) is initiated, not after
streaming completes, creating a race condition where the temporary ZIP file gets
deleted while still being streamed. To fix this, remove the fs.remove() call
from .finally() and instead attach a listener to the response stream's finish
event to handle the cleanup after streaming is complete. This ensures the
temporary file is only deleted after the entire response has been sent to the
client.
In
`@ghost/core/core/server/api/endpoints/utils/serializers/output/card-plugins.js`:
- Around line 21-25: The conditional logic in the card-plugins.js file has both
branches assigning the same value to frame.response.plugins, creating a nested
array when response is already an array. When response is an array (checked by
Array.isArray(response)), assign it directly to frame.response.plugins without
wrapping it in brackets. When response is not an array, wrap it in brackets as
currently done. This ensures the plugins property always contains Plugin[]
rather than Plugin[][] when the response is already an array.
In `@ghost/core/core/server/services/card-plugins/index.js`:
- Line 1: The require statement in the getPluginLoader constant imports from an
incorrect path. Change the require path from './plugin-loader' to './loader' to
match the actual module filename used elsewhere in the codebase. This will
prevent a module resolution failure at runtime when this file is loaded.
In `@ghost/core/core/server/services/card-plugins/loader.js`:
- Around line 106-110: The code at lines 106-110 is vulnerable to path traversal
attacks because it constructs the preprocessPath using user-controlled input
from card.preprocess without validating that the resolved path stays within
pluginDir. To fix this, after constructing preprocessPath with path.join(),
resolve both preprocessPath and pluginDir to their absolute normalized paths
using path.resolve() and then verify that preprocessPath starts with pluginDir
(ensuring the traversal path does not escape the plugin directory). Only proceed
with reading the file if this validation passes, otherwise skip preprocessing or
throw an error to prevent arbitrary local file reads.
In `@ghost/core/core/server/services/koenig/node-renderers/plugin-renderer.js`:
- Around line 107-115: The issue is that `renderPayload` is assigned as an alias
to `rawPayload` (both reference the same object), so when the preprocess
function mutates `renderPayload` in-place, it corrupts the original `rawPayload`
that should remain unchanged for later use in `data-ghost-payload`. Fix this by
creating a shallow copy of `rawPayload` instead of aliasing it, so that
mutations during preprocessing do not affect the original payload. This applies
to the preprocess assignment at the start of the function where `renderPayload =
rawPayload` is initialized, and also at another similar location around line
145-150 where the same pattern occurs.
- Around line 52-59: The payloadStr variable in the plugin-renderer.js file is
only escaping double quotes with " but the data-ghost-payload attribute
uses single quotes to wrap the value. If the payload contains single quotes,
they can break out of the attribute and enable injection attacks. Modify the
JSON.stringify(payload).replace() chain to also escape single quotes (convert '
to &`#x27`; or ') in addition to the existing double quote escaping to ensure
the attribute value is fully safe regardless of payload content.
In `@pnpm-workspace.yaml`:
- Line 77: The adm-zip dependency in the catalog section of pnpm-workspace.yaml
uses a range specifier (^0.5.17) instead of an exact version pin, which weakens
reproducibility. Change the adm-zip entry from ^0.5.17 to 0.5.17 (removing the
caret) to pin it to an exact catalog version, consistent with strict catalog
mode requirements.
🪄 Autofix (Beta)
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
Run ID: d1e5e40e-fd60-49b2-a4d9-58b500d481da
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
apps/admin-x-settings/src/components/settings/advanced/labs.tsxapps/admin-x-settings/src/components/settings/advanced/labs/plugins.tsxapps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsxghost/admin/app/components/koenig-lexical-editor.jsghost/core/content/plugins/infobox/card.cssghost/core/content/plugins/infobox/plugin.jsonghost/core/content/plugins/infobox/template.htmlghost/core/content/plugins/review/card.cssghost/core/content/plugins/review/plugin.jsonghost/core/content/plugins/review/preprocess.jsghost/core/content/plugins/review/template.htmlghost/core/core/server/api/endpoints/card-plugins.jsghost/core/core/server/api/endpoints/index.jsghost/core/core/server/api/endpoints/utils/serializers/output/card-plugins.jsghost/core/core/server/api/endpoints/utils/serializers/output/index.jsghost/core/core/server/services/card-plugins/index.jsghost/core/core/server/services/card-plugins/loader.jsghost/core/core/server/services/koenig/node-renderers/index.jsghost/core/core/server/services/koenig/node-renderers/plugin-renderer.jsghost/core/core/server/web/api/endpoints/admin/routes.jsghost/core/core/shared/labs.jsghost/core/package.jsonpnpm-workspace.yaml
| '@vitejs/plugin-react': 4.7.0 | ||
| '@vitest/coverage-v8': 4.1.8 | ||
| '@vitest/coverage-v8': 4.1.7 | ||
| adm-zip: ^0.5.17 |
Contributor
There was a problem hiding this comment.
Pin adm-zip to an exact catalog version.
adm-zip: ^0.5.17 is a range rather than a pin, which weakens reproducibility for strict catalog usage.
Proposed fix
- adm-zip: ^0.5.17
+ adm-zip: 0.5.17As per coding guidelines, pnpm-workspace.yaml should pin shared dependency versions in catalog: with strict catalog mode.
📝 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.
Suggested change
| adm-zip: ^0.5.17 | |
| adm-zip: 0.5.17 |
🤖 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 `@pnpm-workspace.yaml` at line 77, The adm-zip dependency in the catalog
section of pnpm-workspace.yaml uses a range specifier (^0.5.17) instead of an
exact version pin, which weakens reproducibility. Change the adm-zip entry from
^0.5.17 to 0.5.17 (removing the caret) to pin it to an exact catalog version,
consistent with strict catalog mode requirements.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/admin-x-settings/src/components/settings/advanced/labs/plugins.tsx (2)
11-18: 💤 Low valueAdd TypeScript type annotation for
rawIconparameter.The function parameter lacks type information, which reduces type safety.
-function sanitizeIcon(rawIcon) { +function sanitizeIcon(rawIcon: string | null | undefined): string { if (!rawIcon) return DEFAULT_ICON;🤖 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/admin-x-settings/src/components/settings/advanced/labs/plugins.tsx` around lines 11 - 18, Add a TypeScript type annotation to the rawIcon parameter in the sanitizeIcon function to improve type safety. The parameter should be annotated with an appropriate type that represents the input being sanitized (likely string, since it is passed to DOMPurify.sanitize which expects a string).
159-162: ⚡ Quick winConsider using Ghost's modal system instead of browser
confirm().The native browser
confirm()dialog provides a jarring user experience that's inconsistent with Ghost admin's design patterns, which typically use custom confirmation modals.🤖 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/admin-x-settings/src/components/settings/advanced/labs/plugins.tsx` around lines 159 - 162, The handleDelete function currently uses the native browser confirm() dialog, which is inconsistent with Ghost admin's design patterns. Replace the confirm() call with Ghost's modal system (typically accessed through a modal hook or context available in the application) to display a consistent confirmation dialog that matches the admin interface design. Update the async logic to handle the modal's promise-based or callback-based response pattern instead of the synchronous return value from confirm().
🤖 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.
Nitpick comments:
In `@apps/admin-x-settings/src/components/settings/advanced/labs/plugins.tsx`:
- Around line 11-18: Add a TypeScript type annotation to the rawIcon parameter
in the sanitizeIcon function to improve type safety. The parameter should be
annotated with an appropriate type that represents the input being sanitized
(likely string, since it is passed to DOMPurify.sanitize which expects a
string).
- Around line 159-162: The handleDelete function currently uses the native
browser confirm() dialog, which is inconsistent with Ghost admin's design
patterns. Replace the confirm() call with Ghost's modal system (typically
accessed through a modal hook or context available in the application) to
display a consistent confirmation dialog that matches the admin interface
design. Update the async logic to handle the modal's promise-based or
callback-based response pattern instead of the synchronous return value from
confirm().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8101c4d-7afa-4adb-a660-92a632418a0c
📒 Files selected for processing (2)
apps/admin-x-settings/src/components/settings/advanced/labs/plugins.tsxghost/admin/app/components/koenig-lexical-editor.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/admin/app/components/koenig-lexical-editor.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
ghost/core/core/server/api/endpoints/card-plugins.js (5)
128-143:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftApply the card preprocess step before rendering static HTML.
This delete-conversion path renders
compiledTemplate(payload)directly, so cards whose templates depend oncard.preprocesswill be frozen with different or broken markup compared with normal rendering. Reuse the same preprocess path as the live plugin renderer, while keeping the raw payload fordata-ghost-payload.🤖 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 `@ghost/core/core/server/api/endpoints/card-plugins.js` around lines 128 - 143, The card rendering in this delete-conversion path is missing the preprocessing step that the live plugin renderer applies. Before calling compiledTemplate with the payload, apply the same card.preprocess logic that would normally be used when rendering cards. Extract or reuse the preprocessing function from the live plugin renderer, apply it to the payload before passing it to compiledTemplate, while preserving the raw unpreprocessed payload separately for use in the data-ghost-payload attribute. This ensures cards that depend on preprocessing will render consistently between the delete-conversion path and normal rendering.
428-438:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClean up partial plugin installs on failure.
If extraction or loader validation fails, the partially created
pluginDirremains and blocks a retry as “already exists.” Remove it before returning the failure.Suggested cleanup
} catch (err) { + fs.rmSync(pluginDir, {recursive: true, force: true}); throw new errors.InternalServerError({message: 'Failed to extract plugin: ' + err.message}); } @@ const installedPlugin = loader.getPlugin(pluginData.name); if (!installedPlugin) { + fs.rmSync(pluginDir, {recursive: true, force: true}); + loader.invalidate(); throw new errors.InternalServerError({message: 'Plugin installed but failed to load'}); }🤖 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 `@ghost/core/core/server/api/endpoints/card-plugins.js` around lines 428 - 438, When extraction fails in the catch block or when the plugin fails to load after invalidating and reloading the loader (the condition checking if !installedPlugin), the partially created pluginDir directory remains on disk and blocks retry attempts. Add cleanup code to remove the pluginDir before throwing errors in both failure scenarios: within the catch block that handles extraction errors, and before throwing the InternalServerError when the plugin fails to load after the loader.load() call. This ensures partial installations don't block subsequent installation retries.
149-154:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winEscape all dynamic values before injecting data attributes.
data-ghost-payloadis single-quoted but only"is escaped, so a payload value containing'can break the attribute and inject markup into the persisted HTML. Escape&,",',<, and>for the payload, plugin name, and card name.Suggested escaping helper
+function escapeHtmlAttribute(value = '') { + return String(value) + .replace(/&/g, '&') + .replace(/"/g, '"') + .replace(/'/g, '&`#39`;') + .replace(/</g, '<') + .replace(/>/g, '>'); +} + function _convertPluginCardNodes(node, pluginName, cards) { @@ - const payloadStr = JSON.stringify(payload).replace(/"/g, '"'); + const payloadStr = escapeHtmlAttribute(JSON.stringify(payload)); @@ - const dataAttrs = ` data-ghost-plugin="${pluginName}" data-card-name="${node.cardName || ''}" data-ghost-payload='${payloadStr}'`; + const dataAttrs = ` data-ghost-plugin="${escapeHtmlAttribute(pluginName)}" data-card-name="${escapeHtmlAttribute(node.cardName || '')}" data-ghost-payload="${payloadStr}"`;🤖 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 `@ghost/core/core/server/api/endpoints/card-plugins.js` around lines 149 - 154, The data attributes being injected in the dataAttrs string contain unescaped dynamic values that can break out of their quoted context and inject malicious HTML. Create an HTML escaping function that escapes the five characters `&`, `"`, `'`, `<`, and `>` to their HTML entity equivalents. Apply this escaping function to all three dynamic values before they are interpolated into the dataAttrs string: escape `payloadStr`, `pluginName`, and `node.cardName`. This ensures that single quotes, double quotes, and other special characters in these values cannot break attribute boundaries or inject markup into the persisted HTML.
397-425:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate the plugin directory name before deriving filesystem paths.
pluginData.namecontrolspluginDir; a traversal-style name can move the extraction root outsidecontent/plugins, anddestPathcreates directories beforeextractZipEntryvalidates containment. Enforce a safe plugin name and let the validated helper create parent directories.Suggested containment hardening
+const SAFE_PLUGIN_NAME = /^[a-z0-9][a-z0-9_-]*$/i; + @@ - if (!pluginData.name) { - throw new errors.ValidationError({message: 'Plugin name is required in plugin.json'}); + if (!pluginData.name || !SAFE_PLUGIN_NAME.test(pluginData.name)) { + throw new errors.ValidationError({message: 'Plugin name must contain only letters, numbers, underscores, and hyphens'}); } - const pluginDir = path.join(pluginsPath, pluginData.name); + const pluginDir = path.resolve(pluginsPath, pluginData.name); + const resolvedPluginsPath = path.resolve(pluginsPath); + if (!pluginDir.startsWith(resolvedPluginsPath + path.sep)) { + throw new errors.ValidationError({message: 'Invalid plugin name'}); + } @@ if (strippedName) { - const destPath = path.join(pluginDir, strippedName); - fs.mkdirSync(path.dirname(destPath), {recursive: true}); const zipEntry = {...entry, name: strippedName}; extractZipEntry(zip, zipEntry, pluginDir); }🤖 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 `@ghost/core/core/server/api/endpoints/card-plugins.js` around lines 397 - 425, The pluginData.name input is used directly to construct pluginDir and destPath without validation, creating a path traversal vulnerability where a malicious plugin name could extract files outside the intended plugins directory. Validate pluginData.name before using it in path.join by rejecting names containing path traversal characters (such as "..", "/", or "\\") and ensure it is a safe directory name. Apply this validation immediately after checking that pluginData.name exists and before deriving pluginDir, then rely on the extractZipEntry helper to safely create parent directories with proper containment checks.
287-290:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not delete the plugin after conversion fails.
Continuing after a conversion/edit failure can leave unconverted
plugin-cardnodes in posts while removing the plugin needed to render them. Abort deletion so content remains recoverable.Suggested fail-closed behavior
} catch (err) { logging.error(`Failed to convert posts for plugin "${name}": ${err.message}`); - // Continue with deletion even if conversion fails + throw new errors.InternalServerError({ + message: `Failed to convert posts for plugin "${name}"; plugin was not deleted` + }); }🤖 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 `@ghost/core/core/server/api/endpoints/card-plugins.js` around lines 287 - 290, The current code logs a conversion failure but continues with plugin deletion anyway, which can leave unconverted plugin-card nodes in posts without the plugin available to render them. In the catch block where the error from converting posts is logged (in the failed conversion attempt for the plugin named "name"), abort the deletion process instead of continuing. This should be done by using continue to skip to the next plugin in the loop or by restructuring the logic so the deletion code is only reached if conversion succeeds, preventing the plugin from being removed when its posts could not be properly converted.
🤖 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 `@ghost/core/core/server/api/endpoints/card-plugins.js`:
- Around line 124-126: The current card assignment logic at lines 124-126 uses
cards[0] as a fallback whenever the cardName doesn't match, which is unsafe for
multi-card plugins as it may persist the wrong template. Replace the fallback
behavior so that cards[0] is only used when cards.length is exactly 1; for
multi-card scenarios where no cardName match is found, return false instead to
abort the conversion and prevent content corruption.
- Around line 338-343: The current code only cleans up the temp ZIP directory
when the response finish event fires, but does not handle read stream errors or
client aborts. Add an error handler to the read stream (stream.on('error', ...))
to catch file read failures and pass them to next(), and add a close handler to
the response (res.on('close', ...)) to detect aborted downloads. Consolidate the
cleanup logic so that fs.remove(zipBasePath) is called in all scenarios (finish,
stream error, and response close/abort) to prevent leaking temporary directories
or leaving unhandled errors.
---
Outside diff comments:
In `@ghost/core/core/server/api/endpoints/card-plugins.js`:
- Around line 128-143: The card rendering in this delete-conversion path is
missing the preprocessing step that the live plugin renderer applies. Before
calling compiledTemplate with the payload, apply the same card.preprocess logic
that would normally be used when rendering cards. Extract or reuse the
preprocessing function from the live plugin renderer, apply it to the payload
before passing it to compiledTemplate, while preserving the raw unpreprocessed
payload separately for use in the data-ghost-payload attribute. This ensures
cards that depend on preprocessing will render consistently between the
delete-conversion path and normal rendering.
- Around line 428-438: When extraction fails in the catch block or when the
plugin fails to load after invalidating and reloading the loader (the condition
checking if !installedPlugin), the partially created pluginDir directory remains
on disk and blocks retry attempts. Add cleanup code to remove the pluginDir
before throwing errors in both failure scenarios: within the catch block that
handles extraction errors, and before throwing the InternalServerError when the
plugin fails to load after the loader.load() call. This ensures partial
installations don't block subsequent installation retries.
- Around line 149-154: The data attributes being injected in the dataAttrs
string contain unescaped dynamic values that can break out of their quoted
context and inject malicious HTML. Create an HTML escaping function that escapes
the five characters `&`, `"`, `'`, `<`, and `>` to their HTML entity
equivalents. Apply this escaping function to all three dynamic values before
they are interpolated into the dataAttrs string: escape `payloadStr`,
`pluginName`, and `node.cardName`. This ensures that single quotes, double
quotes, and other special characters in these values cannot break attribute
boundaries or inject markup into the persisted HTML.
- Around line 397-425: The pluginData.name input is used directly to construct
pluginDir and destPath without validation, creating a path traversal
vulnerability where a malicious plugin name could extract files outside the
intended plugins directory. Validate pluginData.name before using it in
path.join by rejecting names containing path traversal characters (such as "..",
"/", or "\\") and ensure it is a safe directory name. Apply this validation
immediately after checking that pluginData.name exists and before deriving
pluginDir, then rely on the extractZipEntry helper to safely create parent
directories with proper containment checks.
- Around line 287-290: The current code logs a conversion failure but continues
with plugin deletion anyway, which can leave unconverted plugin-card nodes in
posts without the plugin available to render them. In the catch block where the
error from converting posts is logged (in the failed conversion attempt for the
plugin named "name"), abort the deletion process instead of continuing. This
should be done by using continue to skip to the next plugin in the loop or by
restructuring the logic so the deletion code is only reached if conversion
succeeds, preventing the plugin from being removed when its posts could not be
properly converted.
🪄 Autofix (Beta)
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
Run ID: a17d9eaa-741c-463e-bca1-1d02b5d62b8d
📒 Files selected for processing (2)
ghost/core/core/server/api/endpoints/card-plugins.jsghost/core/core/server/api/endpoints/utils/serializers/output/card-plugins.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/api/endpoints/utils/serializers/output/card-plugins.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/services/koenig/node-renderers/plugin-renderer.js (1)
58-64:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape
pluginNameandcardNameto prevent attribute injection.While
payloadStris properly escaped (lines 52-54),pluginNameandcardNameare inserted directly into double-quoted HTML attributes. If a plugin name contains"characters, it could break out of the attribute boundary.🛡️ Proposed fix
function addDataAttributes(html, pluginName, cardName, payload) { + const escapeAttr = value => String(value) + .replace(/&/g, '&') + .replace(/"/g, '"') + .replace(/'/g, '&`#39`;') + .replace(/</g, '<') + .replace(/>/g, '>'); const payloadStr = JSON.stringify(payload) .replace(/"/g, '"') .replace(/'/g, '&`#39`;'); const tagMatch = html.match(/<(\w+)([^>]*)>/); if (tagMatch) { const tagName = tagMatch[1]; const existingAttrs = tagMatch[2] || ''; - const dataAttrs = ` data-ghost-plugin="${pluginName}" data-card-name="${cardName}" data-ghost-payload='${payloadStr}'`; + const dataAttrs = ` data-ghost-plugin="${escapeAttr(pluginName)}" data-card-name="${escapeAttr(cardName)}" data-ghost-payload='${payloadStr}'`;🤖 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 `@ghost/core/core/server/services/koenig/node-renderers/plugin-renderer.js` around lines 58 - 64, The pluginName and cardName variables are being inserted directly into HTML attributes without escaping, which could allow attribute injection if these values contain double quote characters. While payloadStr is already properly escaped, pluginName and cardName in the dataAttrs string construction (where data-ghost-plugin and data-card-name attributes are defined) need the same HTML escaping treatment. Apply proper HTML attribute escaping to both pluginName and cardName before they are concatenated into the dataAttrs string to prevent breaking out of the attribute boundary.
🧹 Nitpick comments (1)
apps/admin-x-settings/src/components/settings/advanced/labs/plugins.tsx (1)
176-179: 💤 Low valueAdd
.catch()for JSON parsing consistency withhandleDownload.If the server returns a non-JSON error response (e.g., nginx 502),
response.json()will throw, and the original HTTP status context is lost. ThehandleDownloadfunction (line 201) handles this gracefully with.catch(() => ({})).Same pattern should be applied here and in
handleUpload(line 144) for consistency:♻️ Proposed fix
if (!response.ok) { - const data = await response.json(); + const data = await response.json().catch(() => ({})); throw new Error(data.errors?.[0]?.message || `HTTP ${response.status}`); }Similarly for
handleUploadat line 144:if (!response.ok) { - const data = await response.json(); + const data = await response.json().catch(() => ({})); throw new Error(data.errors?.[0]?.message || `HTTP ${response.status}`); }🤖 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/admin-x-settings/src/components/settings/advanced/labs/plugins.tsx` around lines 176 - 179, The response.json() call can throw an error if the server returns non-JSON responses (such as nginx 502 errors), causing loss of the original HTTP status context. Apply the same error handling pattern already used in the handleDownload function by adding .catch(() => ({})) to the response.json() call in the current location (non-JSON error response handling block), and also apply the same fix to the response.json() call in the handleUpload function to ensure consistent error handling across all three locations where JSON parsing occurs.
🤖 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 `@ghost/core/core/server/services/koenig/node-renderers/plugin-renderer.js`:
- Around line 109-110: The shallow spread operator used to create renderPayload
from rawPayload only copies top-level properties. If the preprocess function
mutates nested objects or arrays within the payload, those mutations will still
corrupt the original rawPayload. Replace the shallow copy at the renderPayload
assignment with a deep copy approach such as
JSON.parse(JSON.stringify(rawPayload)) or structuredClone(rawPayload) to ensure
nested structures are fully protected from mutations during preprocessing.
---
Outside diff comments:
In `@ghost/core/core/server/services/koenig/node-renderers/plugin-renderer.js`:
- Around line 58-64: The pluginName and cardName variables are being inserted
directly into HTML attributes without escaping, which could allow attribute
injection if these values contain double quote characters. While payloadStr is
already properly escaped, pluginName and cardName in the dataAttrs string
construction (where data-ghost-plugin and data-card-name attributes are defined)
need the same HTML escaping treatment. Apply proper HTML attribute escaping to
both pluginName and cardName before they are concatenated into the dataAttrs
string to prevent breaking out of the attribute boundary.
---
Nitpick comments:
In `@apps/admin-x-settings/src/components/settings/advanced/labs/plugins.tsx`:
- Around line 176-179: The response.json() call can throw an error if the server
returns non-JSON responses (such as nginx 502 errors), causing loss of the
original HTTP status context. Apply the same error handling pattern already used
in the handleDownload function by adding .catch(() => ({})) to the
response.json() call in the current location (non-JSON error response handling
block), and also apply the same fix to the response.json() call in the
handleUpload function to ensure consistent error handling across all three
locations where JSON parsing occurs.
🪄 Autofix (Beta)
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
Run ID: f9062072-59f8-4938-91cf-faa08f8ff200
📒 Files selected for processing (3)
apps/admin-x-settings/src/components/settings/advanced/labs/plugins.tsxghost/core/core/server/services/card-plugins/index.jsghost/core/core/server/services/koenig/node-renderers/plugin-renderer.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/services/card-plugins/index.js
fa6f216 to
e3dead4
Compare
refs TryGhost#28634 Ghost admins can now install, manage, and delete custom editor cards via the Admin API. The system includes: - cardPlugins API endpoint (CRUD, download, install) with ZIP upload - CardPluginLoader service scanning content/plugins/ directories - Server-side card rendering using shared renderTemplate engine - Handlebars-based template preprocessing with sandboxed preprocess hook - Plugin management UI in admin-x-settings Labs section - Delete-to-HTML conversion: cards become static HtmlNodes preserving content - ZIP Slip protection in plugin extraction and file loading - Path traversal protection in preprocess file resolution - HTML attribute escaping for data-ghost-payload attributes - DOMPurify sanitization for plugin icons in admin UI - Ghost ConfirmationModal for plugin deletion dialogs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a plugin card system to Ghost's admin API and server-side rendering. Admins can upload, manage, download, and delete plugin cards from the Labs settings. Cards are rendered server-side using the same template engine as the editor.
This PR depends on TryGhost/Koenig#1989 being merged and published to npm first. The following are imported from
@tryghost/kg-default-nodes:renderTemplate— template engine for server-side card renderingcreatePreprocessor— sandboxed preprocess hook executorNew Files
API & Services
endpoints/card-plugins.js— Full CRUD API:browse— List all installed card pluginsread— Get a single plugindestroy— Delete a plugin and convert its card nodes to static HTMLdownload— Download a plugin as ZIP (uses@tryghost/zip)install— Upload and extract a plugin ZIPservices/card-plugins/loader.js— Scanscontent/plugins/forplugin.json, loadstemplate.html,card.css, and optionalpreprocess.jsservices/card-plugins/index.js— Singleton plugin loaderserializers/output/card-plugins.js— Output serializer (passes download function through unmodified)node-renderers/plugin-renderer.js— Server-side renderer using the samerenderTemplateengine andscopeCssutility as the editor. Supportspreprocesshook.Admin UI
labs/plugins.tsx— Plugin management UI in Settings → Labs:Example Plugins
content/plugins/review/— Product review card (score, pros/cons, rating bars with color coding)content/plugins/infobox/— Info box card (4 types: info, warning, success, danger)Modified Files
routes.js— RegisteredcardPluginsAPI routes (/plugins/cards,/plugins/:name,/plugins/:name/download,/plugins/install)endpoints/index.js— AddedcardPluginsendpointserializers/output/index.js— AddedcardPluginsserializernode-renderers/index.js— Registeredplugin-cardrendererlabs.js/private-features.tsx— AddedcustomCardPluginsfeature flagpackage.json— Addedhandlebars,@tryghost/zip, andadm-zipdependencieskoenig-lexical-editor.js— Fetches plugin cards and passes them to the editorFeatures
renderTemplatefrom@tryghost/kg-default-nodes(same engine as the editor) andscopeCssfor namespace isolationconfig.pluginCardsto the Koenig editorpreprocess.jsand executes it via the sandboxedcreatePreprocessorutility before renderingdata-ghost-payloadfor re-import correctnessScreenshots
Enabled the feature flag in
Admin > Labs > Private Features:Admin -> Labs -> Card PluginsIn the editor:
Testing examples plugins
Example plugins: ghost_plugins_examples