Fix silent importer failures by surfacing validation errors#28546
Merged
9larsons merged 5 commits intoJun 13, 2026
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR collects validation errors in the data importer and throws a DataImportError that includes the full errors array as errorDetails. The import manager now extracts err.errorDetails (or falls back to the raw error) when building importResult.data.errors. The unsuccessful-import email template was updated to escape and list up to five error messages and show an “and N more — check the server logs” note. Unit tests were added for the template and importer error propagation. Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/data/importer/email-template.ts`:
- Line 145: The HTML email template injects error text directly in the map
expression (result!.data!.errors!.map(...`${e?.message || e?.toString?.() ||
'Unknown error'}`...)), which is unsafe; add an HTML-escaping step and use it
when building the list items. Implement or import a small escapeHtml helper
(e.g. escapeHtml(s: string): string) and replace the interpolation with escaped
text (escapeHtml(e?.message || e?.toString?.() || 'Unknown error')) inside the
map so all user-controlled error strings are HTML-escaped before insertion.
In `@ghost/core/core/server/data/importer/import-manager.js`:
- Around line 532-533: Normalize err.errorDetails to an array before assigning
to importResult so result.data.errors is always an array; in import-manager.js
where importResult is set (importResult = {data: {errors: errorDetails}}),
replace the direct use of err.errorDetails with a normalized value such as: let
errorDetails = Array.isArray(err.errorDetails) ? err.errorDetails :
(err.errorDetails ? [err.errorDetails] : [err]); then set importResult = {data:
{errors: errorDetails}} to ensure templates using .map() won't crash.
In `@ghost/core/core/server/data/importer/importers/data/data-importer.js`:
- Around line 189-197: The local variable named errors is shadowing the imported
errors module, causing new errors.DataImportError(...) to fail; rename the local
let errors = [] (e.g., to collectedErrors or errorList) and update all
references in this file (including where errorMessages is built and where
importError.errorDetails is set) to use the new name, then instantiate the
actual DataImportError from the imported module (or destructured
DataImportError) instead of the local array so new DataImportError(...) succeeds
and importError contains the real error class.
🪄 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: 4122b631-2944-4a0c-955d-bdd8adaf418c
📒 Files selected for processing (3)
ghost/core/core/server/data/importer/email-template.tsghost/core/core/server/data/importer/import-manager.jsghost/core/core/server/data/importer/importers/data/data-importer.js
Comment on lines
+532
to
+533
| const errorDetails = err.errorDetails || [err]; | ||
| importResult = {data: {errors: errorDetails}}; |
Contributor
There was a problem hiding this comment.
Normalize errorDetails to an array before storing in importResult.
err.errorDetails is assumed to be array-like here, but other Ghost code paths attach non-array payloads to errorDetails. If that happens, result.data.errors is not an array and the email template’s .map(...) path can throw.
Suggested fix
- const errorDetails = err.errorDetails || [err];
+ const errorDetails = Array.isArray(err?.errorDetails)
+ ? err.errorDetails
+ : [err?.errorDetails ?? err];
importResult = {data: {errors: errorDetails}};🤖 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/data/importer/import-manager.js` around lines 532 -
533, Normalize err.errorDetails to an array before assigning to importResult so
result.data.errors is always an array; in import-manager.js where importResult
is set (importResult = {data: {errors: errorDetails}}), replace the direct use
of err.errorDetails with a normalized value such as: let errorDetails =
Array.isArray(err.errorDetails) ? err.errorDetails : (err.errorDetails ?
[err.errorDetails] : [err]); then set importResult = {data: {errors:
errorDetails}} to ensure templates using .map() won't crash.
When importing content with invalid data (e.g. invalid email addresses, too-long bios), errors were rejected as arrays instead of proper Error objects. This caused Node.js to emit only a generic warning: "a promise was rejected with a non-error: [object Array]" Changes: - Wrap error arrays in DataImportError with individual error messages so logging shows useful information instead of [object Array] - Preserve individual error details for the completion email - Update email template to list each error message individually instead of showing a generic "contact support" message - Fix typo: "error occured" → "errors occurred" Fixes TryGhost#26268
ref TryGhost#26268 - the new DataImportError was constructed via `errors.DataImportError`, but `errors` at that point is the local validation-error array, so the fix threw a TypeError on the exact path it was meant to handle — import DataImportError from @tryghost/errors and build it from the first validation error (message/context/help, err) with the full array preserved as errorDetails for the completion email - error messages were injected into the failure email HTML unescaped, so imported content could smuggle markup into the email — escape every message, and fall back to a generic label when an error has no message - cap the listed errors at 5 with an "and N more" pointer to the server logs so a large import can't generate an unbounded email - added unit tests for the doImport rejection shape (DataImportError with message/context and deep-equal errorDetails) and the email template (success path, multiple errors, the cap, XSS escaping, message-less fallback)
9larsons
force-pushed
the
fix-importer-silent-failures
branch
from
17de0eb to
5f960ad
Compare
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration:no-coverage |
✅ Succeeded | 1m 49s | View ↗ |
nx run ghost:test:ci:integration |
✅ Succeeded | 1m 8s | View ↗ |
nx run ghost:test:ci:e2e:no-coverage |
✅ Succeeded | 3m 53s | View ↗ |
nx run ghost:test:ci:e2e |
✅ Succeeded | 3m 48s | View ↗ |
nx run ghost:test:ci:legacy |
✅ Succeeded | 2m 24s | View ↗ |
nx build @tryghost/signup-form |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/sodo-search |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/activitypub |
✅ Succeeded | 1s | View ↗ |
Additional runs (10) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-06-13 17:57:03 UTC
Contributor
|
Thanks for the PR! You were first to the right fix for this, and I wanted to get it landed quickly, so I've pushed a commit on top of yours rather than going through review rounds. What the fixup does:
|
ref TryGhost#26268 The importer now rejects validation failures with DataImportError so the integration test needs to assert the errorDetails payload instead of the old raw array rejection.
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
2 participants
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.
Summary
When importing content with invalid data (e.g. invalid email addresses, too-long author bios), errors were rejected as arrays instead of proper Error objects. This caused Node.js to emit only a generic warning:
No useful information was shown in the console or the error email — making it nearly impossible to diagnose import failures.
Root Cause
In
data-importer.js, when validation errors occur, the error array is thrown directly (throw errors) instead of being wrapped in a proper Error object. Node.js considers this a "non-error" rejection and only emits a warning.Changes
data-importer.js: Wrap the error array in aDataImportErrorwith all individual error messages joined together. Attach the original error array aserrorDetailsfor downstream use.import-manager.js: Extract individual error details from the caught error (viaerrorDetails) instead of wrapping the whole thing as a single-element array. This preserves granularity for the email template.email-template.ts: List each error message individually in the notification email instead of showing a generic "contact support" message. Also fixed typo "error occured" → "errors occurred".Result
Before:
Email: "One or more error occured while importing your content. Please contact support."
After:
Email: Lists each individual error with actionable guidance.
Testing
Fixes #26268