Add theme upload size limits#27903
Merged
Merged
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR implements configurable theme upload size limit enforcement. It introduces a new error reporter module that detects and reports upload size violations to logging and Sentry, extends the upload middleware with theme-specific compressed-size validation, passes uncompressed size limits to the gscan validator, integrates error reporting into storage error handling, and wires the new theme upload middleware to the /themes/upload endpoint. The changes include configuration defaults for three upload limits and comprehensive test coverage for the reporter, middleware, and validation layers. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ghost/core/test/unit/server/services/themes/validate.test.js (1)
52-57: ⚡ Quick winConsider sourcing config values dynamically in tests.
The test hardcodes
536870912and4294967296, which match the current defaults. If the config defaults change, this test could fail or become misleading.♻️ Optional: Import config to keep test values in sync
+const config = require('../../../../../core/shared/config'); + it('[success] validates a valid zipped theme', function () { checkZipStub.resolves({}); formatStub.returns({results: {error: []}}); return validate.check(testTheme.name, testTheme, {isZip: true}) .then((checkedTheme) => { sinon.assert.calledOnce(checkZipStub); sinon.assert.calledWith(checkZipStub, testTheme); sinon.assert.calledWith(checkZipStub, testTheme, sinon.match({ limits: { - perEntryUncompressedBytes: 536870912, - totalUncompressedBytes: 4294967296 + perEntryUncompressedBytes: config.get('theme:uploadLimits:entryUncompressedBytes'), + totalUncompressedBytes: config.get('theme:uploadLimits:totalUncompressedBytes') } }));🤖 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/test/unit/server/services/themes/validate.test.js` around lines 52 - 57, The test currently asserts hardcoded byte limits (536870912 and 4294967296) when calling checkZipStub with testTheme; change the test to read the expected limits from the runtime/config defaults instead of literals so it stays correct if defaults change — import or require the app config in validate.test.js and use the config keys that correspond to perEntryUncompressedBytes and totalUncompressedBytes when building the sinon.match limits object passed to checkZipStub (keep the same property names: perEntryUncompressedBytes and totalUncompressedBytes and the existing identifiers checkZipStub and testTheme).ghost/core/test/unit/server/web/api/middleware/upload.test.js (1)
47-63: ⚡ Quick winConsider sourcing config value dynamically in test.
The test hardcodes
limitBytes: 1073741824, which matches the current default. If the config default changes, this assertion could fail or become misleading.♻️ Optional: Import config to keep test value in sync
+const config = require('../../../../../../core/shared/config'); + describe('getCompressedSizeLimitError', function () { it('returns a structured theme upload size limit error', function () { const err = validation.getCompressedSizeLimitError({field: 'file'}, { get: () => '1073741825' }); assert.equal(err.errorType, 'UnsupportedMediaTypeError'); assert.equal(err.message, 'Theme upload exceeds maximum compressed size.'); assert.equal(err.context, 'Theme upload exceeds the maximum compressed size.'); assert.equal(err.code, 'COMPRESSED_TOO_LARGE'); assert.deepEqual(err.errorDetails, { observedBytes: 1073741825, - limitBytes: 1073741824, + limitBytes: config.get('theme:uploadLimits:compressedBytes'), fieldName: 'file' }); }); });🤖 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/test/unit/server/web/api/middleware/upload.test.js` around lines 47 - 63, The test hardcodes the expected compressed limit (1073741824) which can drift; instead import the same config source used by validation and compute expected values dynamically: require the config used by the module under test, read the compressed limit value (the key used by your app's config that getCompressedSizeLimitError/validation uses), set expectedLimit = Number(config.get(...)) and expectedObserved = expectedLimit + 1, then assert err.errorDetails.limitBytes === expectedLimit and err.errorDetails.observedBytes === expectedObserved while keeping references to validation.getCompressedSizeLimitError and the errorDetails.fieldName assertion unchanged.ghost/core/core/server/services/themes/storage.js (1)
101-103: ⚡ Quick winHarden the reporting call so it cannot mask the original error.
If
reportThemeUploadSizeLimitErrorthrows on Line 102, the original upload failure can be replaced and later catch-flow handling won’t run. Wrap the reporter call in a nested try/catch and only log reporter failures.Suggested patch
} catch (error) { if (isThemeUploadSizeLimitError(error)) { - reportThemeUploadSizeLimitError(error, {themeName, zip}); + try { + reportThemeUploadSizeLimitError(error, {themeName, zip}); + } catch (reportingError) { + logging.error(reportingError); + } }🤖 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/themes/storage.js` around lines 101 - 103, In the isThemeUploadSizeLimitError branch, wrap the call to reportThemeUploadSizeLimitError(themeName, zip) in its own try/catch so any errors from the reporter are caught and logged (e.g. via the module's logger or console.error) instead of propagating; do not rethrow from the reporter catch so the original upload failure error is preserved and continues through the outer catch flow.
🤖 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 `@ghost/core/core/server/services/themes/storage.js`:
- Around line 101-103: In the isThemeUploadSizeLimitError branch, wrap the call
to reportThemeUploadSizeLimitError(themeName, zip) in its own try/catch so any
errors from the reporter are caught and logged (e.g. via the module's logger or
console.error) instead of propagating; do not rethrow from the reporter catch so
the original upload failure error is preserved and continues through the outer
catch flow.
In `@ghost/core/test/unit/server/services/themes/validate.test.js`:
- Around line 52-57: The test currently asserts hardcoded byte limits (536870912
and 4294967296) when calling checkZipStub with testTheme; change the test to
read the expected limits from the runtime/config defaults instead of literals so
it stays correct if defaults change — import or require the app config in
validate.test.js and use the config keys that correspond to
perEntryUncompressedBytes and totalUncompressedBytes when building the
sinon.match limits object passed to checkZipStub (keep the same property names:
perEntryUncompressedBytes and totalUncompressedBytes and the existing
identifiers checkZipStub and testTheme).
In `@ghost/core/test/unit/server/web/api/middleware/upload.test.js`:
- Around line 47-63: The test hardcodes the expected compressed limit
(1073741824) which can drift; instead import the same config source used by
validation and compute expected values dynamically: require the config used by
the module under test, read the compressed limit value (the key used by your
app's config that getCompressedSizeLimitError/validation uses), set
expectedLimit = Number(config.get(...)) and expectedObserved = expectedLimit +
1, then assert err.errorDetails.limitBytes === expectedLimit and
err.errorDetails.observedBytes === expectedObserved while keeping references to
validation.getCompressedSizeLimitError and the errorDetails.fieldName assertion
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68c121a5-89bd-4cfc-8513-01db9f7a9b35
📒 Files selected for processing (9)
ghost/core/core/server/services/themes/storage.jsghost/core/core/server/services/themes/upload-size-limit-reporter.jsghost/core/core/server/services/themes/validate.jsghost/core/core/server/web/api/endpoints/admin/routes.jsghost/core/core/server/web/api/middleware/upload.jsghost/core/core/shared/config/defaults.jsonghost/core/test/unit/server/services/themes/upload-size-limit-reporter.test.jsghost/core/test/unit/server/services/themes/validate.test.jsghost/core/test/unit/server/web/api/middleware/upload.test.js
Keep the original upload failure flowing if reporting fails, so cleanup and error handling still see the real validation error. Read upload limit defaults from config in tests to avoid duplicated constants drifting from the runtime settings.
ErisDS
added a commit
to JohnONolan/Ghost
that referenced
this pull request
May 14, 2026
When the new server-side upload caps (PR TryGhost#27903) reject a save, the response carries a structured Ghost error with a code (COMPRESSED_TOO_LARGE / ENTRY_TOO_LARGE / TOTAL_TOO_LARGE) and errorDetails (entryName, observedBytes, limitBytes). The editor was throwing the parsed message into the generic useHandleError() pipeline, which collapsed it to "Something went wrong, please try again." The user had no way to know which file was too large or what the cap was. Intercept the three size-limit codes before the throw, format the errorDetails into a user-friendly toast, and skip the generic handler: - COMPRESSED_TOO_LARGE → "The theme archive is too large to upload (max X MB)." - ENTRY_TOO_LARGE → "The file 'foo.hbs' is too large (max X MB per file)." - TOTAL_TOO_LARGE → "The theme contents exceed the maximum allowed size of X MB." All other non-422 failures still fall through to the existing throw + handleError path. The 422 InvalidThemeModal branch (gscan validation errors) is unchanged. Test-first: a new acceptance test mocks /themes/upload/ to return a 415 with an ENTRY_TOO_LARGE shape and asserts the toast mentions the offending file and the limit (not the generic message). The test was failing before this change. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.
Summary
Testing
pnpm test:single test/unit/server/web/api/middleware/upload.test.jspnpm test:single test/unit/server/services/themes/validate.test.jspnpm test:single test/unit/server/services/themes/upload-size-limit-reporter.test.jspnpm lint:server(passes with existing warnings)pnpm lint:test