Added file type validation and content-type control for file uploads#26869
Merged
Added file type validation and content-type control for file uploads#26869
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 pull request refactors file type handling by introducing a centralized content-type resolution function ( 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens the Admin “files/upload” endpoint by introducing an extension allowlist and by controlling the stored/served Content-Type to reduce the risk of executable content being rendered after moving uploads to a shared CDN domain.
Changes:
- Added extension allowlist validation middleware for
/files/uploadand updated config to include the allowed extensions. - Introduced
getStorageContentType()to derive a safe storage content-type from filename extension (override totext/plain, allow safe renderable types, otherwiseapplication/octet-stream). - Added/updated unit and e2e tests to cover extension rejection and content-type behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ghost/core/test/unit/server/services/files/file-type-utils.test.js | New unit tests for storage content-type derivation logic. |
| ghost/core/test/unit/server/data/importer/index.test.js | Updates importer extension/content-type expectations to align with new allowlist/config behavior. |
| ghost/core/test/e2e-api/admin/files.test.js | Adds e2e assertions for 415 on unsupported extensions and for safe stored content-types. |
| ghost/core/core/shared/config/overrides.json | Expands uploads files.extensions allowlist used by validation middleware. |
| ghost/core/core/server/web/api/middleware/upload.js | Adds fileValidation middleware and new error messages for file uploads. |
| ghost/core/core/server/web/api/endpoints/admin/routes.js | Wires fileValidation({type: 'files'}) into the files upload route. |
| ghost/core/core/server/services/files/file-type-utils.js | New helper to determine safe storage/served content type from extension. |
| ghost/core/core/server/api/endpoints/files.js | Switches file upload storage type derivation to getStorageContentType(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/core/server/services/files/file-type-utils.js`:
- Around line 12-34: The BROWSER_RENDERABLE_TYPES set in file-type-utils.js
omits 'video/quicktime', so mime.lookup('.mov') yields 'video/quicktime' and is
treated as non-renderable; update the BROWSER_RENDERABLE_TYPES constant to
include 'video/quicktime' (or broaden the renderable check to accept video/*) so
that mime.lookup results for .mov are preserved as renderable instead of being
forced to 'application/octet-stream' in the downstream logic that uses
BROWSER_RENDERABLE_TYPES.
In `@ghost/core/test/e2e-api/admin/files.test.js`:
- Around line 99-165: The tests use fixed temp filenames (tempFile with
'test.exe', 'spreadsheet.xlsx', 'page.html') in os.tmpdir(), causing races in
parallel runs; modify each test to create a unique temporary directory or
filename (e.g., via fs.mkdtempSync(path.join(os.tmpdir(), 'ghost-test-')) or
generate a random suffix) and write the file into that unique location, then use
that path for .attach(...) and fs.removeSync, ensuring the save spies and
assertions still reference the created file; update the three tests (the
"Rejects an unsupported file extension...", "Stores non-browser-renderable
files...", and "Stores HTML files..." blocks) to use the unique temp path for
tempFile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e70afb27-8a8e-44db-9e4d-e87c0a9f8a19
📒 Files selected for processing (8)
ghost/core/core/server/api/endpoints/files.jsghost/core/core/server/services/files/file-type-utils.jsghost/core/core/server/web/api/endpoints/admin/routes.jsghost/core/core/server/web/api/middleware/upload.jsghost/core/core/shared/config/overrides.jsonghost/core/test/e2e-api/admin/files.test.jsghost/core/test/unit/server/data/importer/index.test.jsghost/core/test/unit/server/services/files/file-type-utils.test.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/services/files/file-type-utils.js (1)
12-35: Add a drift guardrail test between allowed extensions and renderable MIME handling.This list is easy to drift from upload extension config (as seen with
.movearlier). A small unit test that snapshots expected resolution for all allowed extensions would catch regressions early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/files/file-type-utils.js` around lines 12 - 35, Add a unit test that prevents drift between allowed upload extensions and the MIME types considered renderable by the browser: load the canonical allowed-extensions list used by the uploader and, for each extension, resolve its expected MIME handling (using the same resolution logic your code uses) and assert it matches a stored snapshot or that the resolved MIME is present in the BROWSER_RENDERABLE_TYPES Set from file-type-utils.js; name the test clearly (e.g., "uploads: allowed extensions -> renderable MIME snapshot") so it fails when someone adds/removes extensions (like .mov) without updating BROWSER_RENDERABLE_TYPES.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/server/services/files/file-type-utils.js`:
- Around line 12-35: Add a unit test that prevents drift between allowed upload
extensions and the MIME types considered renderable by the browser: load the
canonical allowed-extensions list used by the uploader and, for each extension,
resolve its expected MIME handling (using the same resolution logic your code
uses) and assert it matches a stored snapshot or that the resolved MIME is
present in the BROWSER_RENDERABLE_TYPES Set from file-type-utils.js; name the
test clearly (e.g., "uploads: allowed extensions -> renderable MIME snapshot")
so it fails when someone adds/removes extensions (like .mov) without updating
BROWSER_RENDERABLE_TYPES.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33f2e89c-a0f3-4f6b-9f1f-9a0e52bbfee4
📒 Files selected for processing (2)
ghost/core/core/server/services/files/file-type-utils.jsghost/core/test/unit/server/services/files/file-type-utils.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/unit/server/services/files/file-type-utils.test.js
vershwal
force-pushed
the
fileValidation
branch
3 times, most recently
from
928a978 to
3c91860
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/web/api/middleware/upload.js (1)
363-397: Implementation looks solid; minor inconsistency with template parameter.The extension-only validation approach is well-documented and appropriate given MIME type unreliability. One small nit: line 391 passes
{extensions: extensions}totpl(), but thefiles.invalidFilemessage at line 58 doesn't use this placeholder.🧹 Suggested cleanup to remove unused parameter
if (!extensions.includes(req.file.ext)) { return next(new errors.UnsupportedMediaTypeError({ - message: tpl(messages[type].invalidFile, {extensions: extensions}) + message: tpl(messages[type].invalidFile) })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/web/api/middleware/upload.js` around lines 363 - 397, The tpl call in fileUploadValidation passes an unused parameter object ({extensions: extensions}) to tpl for messages[type].invalidFile; remove the extra parameter so tpl is invoked with only the formatted message (i.e., change the UnsupportedMediaTypeError message construction in fileValidation/fileUploadValidation to call tpl(messages[type].invalidFile) without the unused {extensions} arg) and ensure no other calls rely on that placeholder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/server/web/api/middleware/upload.js`:
- Around line 363-397: The tpl call in fileUploadValidation passes an unused
parameter object ({extensions: extensions}) to tpl for
messages[type].invalidFile; remove the extra parameter so tpl is invoked with
only the formatted message (i.e., change the UnsupportedMediaTypeError message
construction in fileValidation/fileUploadValidation to call
tpl(messages[type].invalidFile) without the unused {extensions} arg) and ensure
no other calls rely on that placeholder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66169af8-4b0e-410c-b85b-3dccb54b1bd3
📒 Files selected for processing (8)
ghost/core/core/server/api/endpoints/files.jsghost/core/core/server/services/files/file-type-utils.jsghost/core/core/server/web/api/endpoints/admin/routes.jsghost/core/core/server/web/api/middleware/upload.jsghost/core/core/shared/config/overrides.jsonghost/core/test/e2e-api/admin/files.test.jsghost/core/test/unit/server/data/importer/index.test.jsghost/core/test/unit/server/services/files/file-type-utils.test.js
🚧 Files skipped from review as they are similar to previous changes (5)
- ghost/core/test/e2e-api/admin/files.test.js
- ghost/core/core/server/api/endpoints/files.js
- ghost/core/core/server/web/api/endpoints/admin/routes.js
- ghost/core/test/unit/server/services/files/file-type-utils.test.js
- ghost/core/core/server/services/files/file-type-utils.js
vershwal
commented
Mar 18, 2026
| ".pdf", | ||
| ".json", | ||
| ".jsonld", | ||
| ".odp", |
Member
Author
There was a problem hiding this comment.
Removed these extensions because their count was very low in our data.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26869 +/- ##
==========================================
+ Coverage 73.08% 73.11% +0.02%
==========================================
Files 1535 1536 +1
Lines 121270 121368 +98
Branches 14645 14660 +15
==========================================
+ Hits 88633 88737 +104
- Misses 31609 31625 +16
+ Partials 1028 1006 -22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The files upload endpoint accepted any file type without validation, unlike images and media which check extension and MIME type. With the move to a shared CDN domain, we don't want to upload files with executable content types. This adds: - An allowlist of supported file extensions (based on Ghost Pro data) - Extension-only validation middleware on the upload route - Content-type resolution that serves browser-renderable types (images, PDF, JSON, audio, video, fonts) with their natural type, overrides formats (HTML, JS, CSS, XML) to text/plain, and defaults everything else to application/octet-stream (forced download) - Users can zip unsupported file types as an escape hatch Made-with: Cursor
vershwal
force-pushed
the
fileValidation
branch
from
7371ec1 to
b2b2b0d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/core/shared/config/overrides.json`:
- Line 97: The empty uploads.files.contentTypes array causes the upload
validator to always fail (it requires both MIME type and extension), so either
populate uploads.files.contentTypes with the expected MIME types for your
allowed extensions (e.g., image/png, image/jpeg, video/mp4, etc.) or update the
upload validation logic to treat an empty contentTypes as a wildcard/skip for
MIME checking (i.e., if uploads.files.contentTypes.length === 0, validate by
extension only or allow any MIME), ensuring the /files/upload validation path is
updated accordingly (adjust the validator used by the upload handler).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1098442-b543-4dfb-b9e4-c86884c22b7d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
ghost/core/core/server/api/endpoints/files.jsghost/core/core/server/services/files/file-type-utils.tsghost/core/core/server/web/api/endpoints/admin/routes.jsghost/core/core/server/web/api/middleware/upload.jsghost/core/core/shared/config/overrides.jsonghost/core/package.jsonghost/core/test/e2e-api/admin/files.test.jsghost/core/test/unit/server/data/importer/index.test.jsghost/core/test/unit/server/services/files/file-type-utils.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/core/server/web/api/endpoints/admin/routes.js
- ghost/core/core/server/services/files/file-type-utils.ts
- ghost/core/test/unit/server/services/files/file-type-utils.test.js
| "application/xml", | ||
| "application/atom+xml" | ||
| ] | ||
| "contentTypes": [] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find any middleware usage still validating files via MIME-based validation():"
rg -nP "validation\(\{\s*type:\s*['\"]files['\"]\s*\}\)|upload\.validation\(\{\s*type:\s*['\"]files['\"]\s*\}\)" --type=js --type=ts
echo
echo "2) Find direct consumers of uploads.files.contentTypes:"
rg -nP "config\.get\('uploads'\)\.files\.contentTypes|config\.get\(\"uploads\"\)\.files\.contentTypes|uploads'\)\.files\.contentTypes" --type=js --type=ts
echo
echo "3) Inspect importer wiring that still passes files.contentTypes through:"
rg -nP "contentTypes:\s*config\.get\('uploads'\)\.files\.contentTypes" --type=js --type=ts -C2Repository: TryGhost/Ghost
Length of output: 138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Step 1: Simple pattern search for contentTypes keyword ==="
rg -nw "contentTypes" --type=js --type=ts | head -30
echo
echo "=== Step 2: Look at the actual config file changes ==="
git show HEAD:ghost/core/core/shared/config/overrides.json | sed -n '85,110p' || echo "Using git diff to see the change..."
git diff HEAD~1 ghost/core/core/shared/config/overrides.json | head -40 || echo "Checking file directly..."
cat ghost/core/core/shared/config/overrides.json | sed -n '85,110p'
echo
echo "=== Step 3: Find upload/files handlers and validators ==="
fd -e "js" -e "ts" -path "*upload*" | grep -v node_modules | head -20
echo
echo "=== Step 4: Check for any files-related validation middleware ==="
rg -l "files.*upload|upload.*files" --type=js --type=ts | head -10Repository: TryGhost/Ghost
Length of output: 6905
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Reading upload.js around the contentTypes usage (lines 260-340) ==="
sed -n '260,340p' ghost/core/core/server/web/api/middleware/upload.js
echo
echo "=== Find the checkFileIsValid function definition ==="
rg -n "function checkFileIsValid|const checkFileIsValid" --type=js --type=ts -A 15
echo
echo "=== Check the exports/usage patterns in upload.js ==="
head -50 ghost/core/core/server/web/api/middleware/upload.js
echo
echo "=== Check which routes use this upload middleware for files ==="
rg -n "upload\." --type=js --type=ts -B 2 -A 2 | grep -E "(files|upload\..*function)" | head -20Repository: TryGhost/Ghost
Length of output: 7343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full checkFileIsValid function context (lines 150-175) ==="
sed -n '150,175p' ghost/core/core/server/web/api/middleware/upload.js
echo
echo "=== Verify files extension array is populated ==="
rg -n '"files"' ghost/core/core/shared/config/overrides.json -A 80 | grep -E "(extensions|contentTypes)" | head -5
echo
echo "=== Check for any recent changes or special handling ==="
rg -n "contentTypes.*empty|types\.length|types\.length === 0" --type=js --type=ts
echo
echo "=== Look at the fileValidation function structure ==="
sed -n '245,280p' ghost/core/core/server/web/api/middleware/upload.jsRepository: TryGhost/Ghost
Length of output: 1222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for extension-only validation or alternative logic ==="
rg -n "extension.*only|extensionsOnly" --type=js --type=ts | head -10
echo
echo "=== Check if checkFileIsValid logic was changed recently ==="
git log --oneline ghost/core/core/server/web/api/middleware/upload.js | head -5
echo
echo "=== Look for any TODO or FIXME comments in upload.js ==="
rg -n "TODO|FIXME|XXX" ghost/core/core/server/web/api/middleware/upload.js
echo
echo "=== Check if there's alternative validation using only extensions ==="
rg -n "extensions\.includes" --type=js --type=ts -B 2 -A 2 | head -30
echo
echo "=== View the full fileValidation function ==="
sed -n '239,290p' ghost/core/core/server/web/api/middleware/upload.jsRepository: TryGhost/Ghost
Length of output: 482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show the recent commit that changed the file type utils ==="
git show b2b2b0d1 --stat | head -30
echo
echo "=== Check if checkFileIsValid was modified in that commit ==="
git show b2b2b0d1 -- ghost/core/core/server/web/api/middleware/upload.js | grep -A 10 -B 10 "checkFileIsValid" | head -40
echo
echo "=== Full fileValidation middleware for 'files' type ==="
sed -n '239,295p' ghost/core/core/server/web/api/middleware/upload.js
echo
echo "=== Check if there's a separate or new validation path ==="
rg -n "fileValidation|uploadValidation" ghost/core/core/server/web/api/middleware/upload.js | head -10
echo
echo "=== Verify the actual behavior by seeing the complete exports ==="
tail -100 ghost/core/core/server/web/api/middleware/upload.js | grep -E "module.exports|exports\.|fileValidation"Repository: TryGhost/Ghost
Length of output: 1835
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check the complete fileValidation export ==="
rg -n "^module.exports|^exports\." ghost/core/core/server/web/api/middleware/upload.js -A 5
echo
echo "=== Check if contentTypes validation logic was modified ==="
sed -n '158,166p' ghost/core/core/server/web/api/middleware/upload.js
echo
echo "=== See all places where checkFileIsValid is called ==="
rg -n "checkFileIsValid" ghost/core/core/server/web/api/middleware/upload.js
echo
echo "=== Check the actual git diff for this specific file ==="
git diff HEAD~2 HEAD -- ghost/core/core/server/web/api/middleware/upload.js | head -100
echo
echo "=== Find what changed in overrides.json for files.contentTypes ==="
git diff HEAD~2 HEAD -- ghost/core/core/shared/config/overrides.json | grep -A 5 -B 5 "contentTypes"Repository: TryGhost/Ghost
Length of output: 1120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check the files.extensions array to confirm it's populated ==="
sed -n '46,98p' ghost/core/core/shared/config/overrides.json | head -40
echo
echo "=== Look for any conditional logic that handles empty contentTypes ==="
rg -n "types\.length|!types|types\s*\|\|" --type=js --type=ts | grep -v test | grep -v node_modules | head -20
echo
echo "=== Check if OR logic is used instead of AND somewhere ==="
rg -n "includes.*\|\|.*includes" --type=js --type=ts
echo
echo "=== Search for the PR or issue that introduced extension-only validation ==="
rg -n "extension" ghost/core/core/server/web/api/middleware/upload.js | head -20
echo
echo "=== Check the surrounding code in fileValidation to see context ==="
sed -n '235,300p' ghost/core/core/server/web/api/middleware/upload.jsRepository: TryGhost/Ghost
Length of output: 1184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Final confirmation: View checkFileIsValid and how it's used ==="
sed -n '158,166p' ghost/core/core/server/web/api/middleware/upload.js
echo
echo "=== Confirm files.contentTypes is indeed empty ==="
rg -A 2 '"files"' ghost/core/core/shared/config/overrides.json | grep -A 2 "contentTypes"
echo
echo "=== Double-check: is there any other validation path or fallback? ==="
rg -n "validate.*file|checkFile" --type=js --type=ts ghost/core/core/server/web/api/middleware/upload.js
echo
echo "=== Test: simulate the validation logic with empty contentTypes ==="
cat > /tmp/test_validation.js << 'TESTEOF'
const checkFileIsValid = (fileData, types, extensions) => {
const type = fileData.mimetype;
console.log(`Checking: mimetype=${type}, ext=${fileData.ext}`);
console.log(` types.includes(${type})? ${types.includes(type)}`);
console.log(` extensions.includes(${fileData.ext})? ${extensions.includes(fileData.ext)}`);
if (types.includes(type) && extensions.includes(fileData.ext)) {
return true;
}
return false;
};
// Test with empty contentTypes (current config)
const result1 = checkFileIsValid(
{mimetype: 'application/pdf', ext: '.pdf'},
[], // empty contentTypes from config
['.pdf', '.json'] // files.extensions
);
console.log(`\nResult with empty contentTypes: ${result1}`);
// Test with populated contentTypes
const result2 = checkFileIsValid(
{mimetype: 'application/pdf', ext: '.pdf'},
['application/pdf', 'application/json'],
['.pdf', '.json']
);
console.log(`Result with populated contentTypes: ${result2}`);
TESTEOF
node /tmp/test_validation.jsRepository: TryGhost/Ghost
Length of output: 392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check git log for recent changes to upload.js ==="
git log --oneline -- ghost/core/core/server/web/api/middleware/upload.js | head -10
echo
echo "=== Get the actual commit that modified overrides.json ==="
git log --oneline -- ghost/core/core/shared/config/overrides.json | head -10
echo
echo "=== Look at the commit message/details ==="
git log -1 --format="%H%n%s%n%b" -- ghost/core/core/shared/config/overrides.json
echo
echo "=== Check if there's a comment or PR that explains extension-only approach ==="
rg -n "extension.*only|MIME.*deprecated|contentTypes.*unused" --type=js --type=ts --type=md | head -10
echo
echo "=== Look at the actual upload endpoint handler to see if there's custom validation ==="
rg -n "api.files.upload" --type=js --type=ts -B 5 -A 10Repository: TryGhost/Ghost
Length of output: 2425
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show the actual commit diff for upload.js to see what changed ==="
git show b2b2b0d1 -- ghost/core/core/server/web/api/middleware/upload.js | grep -A 15 -B 5 "checkFileIsValid" | head -50
echo
echo "=== Show the actual commit diff for overrides.json ==="
git show b2b2b0d1 -- ghost/core/core/shared/config/overrides.json | grep -A 20 -B 5 '"files"' | head -80
echo
echo "=== Test the actual behavior: can ANY file pass with empty contentTypes? ==="
node << 'EOF'
const checkFileIsValid = (fileData, types, extensions) => {
const type = fileData.mimetype;
if (types.includes(type) && extensions.includes(fileData.ext)) {
return true;
}
return false;
};
// Simulate /files/upload validation
const testFiles = [
{mimetype: 'application/pdf', ext: '.pdf', name: 'document.pdf'},
{mimetype: 'text/plain', ext: '.txt', name: 'file.txt'},
{mimetype: 'application/json', ext: '.json', name: 'data.json'}
];
const emptyContentTypes = []; // current config
const fileExtensions = ['.pdf', '.txt', '.json', '.md']; // from files.extensions
console.log('Testing file validation with CURRENT config (empty contentTypes):');
testFiles.forEach(file => {
const result = checkFileIsValid(file, emptyContentTypes, fileExtensions);
console.log(` ${file.name}: ${result ? 'PASS' : 'FAIL'}`);
});
EOFRepository: TryGhost/Ghost
Length of output: 2562
Line 97: Empty uploads.files.contentTypes breaks all file uploads.
The validation logic requires BOTH a matching MIME type AND a matching extension (AND operator). With an empty contentTypes array, this check will reject all file uploads, making the /files/upload endpoint non-functional. Either populate contentTypes with valid MIME types or change the validation logic to extension-only checking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/core/shared/config/overrides.json` at line 97, The empty
uploads.files.contentTypes array causes the upload validator to always fail (it
requires both MIME type and extension), so either populate
uploads.files.contentTypes with the expected MIME types for your allowed
extensions (e.g., image/png, image/jpeg, video/mp4, etc.) or update the upload
validation logic to treat an empty contentTypes as a wildcard/skip for MIME
checking (i.e., if uploads.files.contentTypes.length === 0, validate by
extension only or allow any MIME), ensuring the /files/upload validation path is
updated accordingly (adjust the validator used by the upload handler).
Member
Author
There was a problem hiding this comment.
The key detail you missed is that the /files/upload route uses fileValidation, not the generic validation middleware. Looking at the route:
router.post('/files/upload',
mw.authAdminApi,
apiMw.upload.single('file'),
apiMw.upload.fileValidation({type: 'files'}),
http(api.files.upload)
);The fileValidation middleware (lines 373-397) only checks extensions — it never reads contentTypes at all. The empty contentTypes: [] in overrides.json exists solely to prevent undefined from leaking into the ImportManager (which aggregates content types from all upload handlers via lodash.flatMap). It has zero impact on the upload validation path.
The older validation() middleware is the one that checks both extensions and content types, but that's used by images/media — not files.
Contributor
There was a problem hiding this comment.
@vershwal, you're absolutely right — I missed that distinction. The /files/upload route wires fileValidation (extension-only), not the generic validation middleware that performs the AND-check on both MIME types and extensions. The empty contentTypes: [] is purely defensive, keeping ImportManager's lodash.flatMap aggregation safe from undefined rather than having any effect on the upload validation path itself. My concern was unfounded — apologies for the noise!
( ˘ᵕ˘) scribbles notes furiously
✏️ Learnings added
Learnt from: vershwal
Repo: TryGhost/Ghost PR: 26869
File: ghost/core/core/shared/config/overrides.json:97-97
Timestamp: 2026-03-18T13:42:05.540Z
Learning: In TryGhost/Ghost (`ghost/core`), the `/files/upload` route uses `apiMw.upload.fileValidation({type: 'files'})` (defined in `ghost/core/core/server/web/api/middleware/upload.js`, lines ~373-397), which performs **extension-only** validation and never reads the `contentTypes` field from the upload config. The empty `contentTypes: []` for `uploads.files` in `ghost/core/core/shared/config/overrides.json` is intentional: it prevents `undefined` from being spread into the `ImportManager`, which aggregates content types across all upload handlers via `lodash.flatMap`. The older `validation()` middleware (which ANDs MIME type + extension) is only used for images/media uploads, not files.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
allouis
approved these changes
Mar 18, 2026
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
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.
ref https://linear.app/ghost/issue/HKG-1643/
The files upload endpoint accepted any file type without validation, unlike images and media which check extension and MIME type. With the move to a shared CDN domain, we don't want to upload files with executable content types.
This PR adds:
These are the lists and rules we have for extensions and content-types for files:-
1. Allowed Extensions (48 total)
These are the top 44 extensions from our Ghost Pro data. These are all the extensions that have count > 40 (and 4 more that were already added in Ghost so I didn't remove those from the allow list).
Total: 48 extensions, ~177,444 files
If someone uploads a file with an extension that's NOT in this list, the upload is rejected with a 415 error. They can always zip the file and upload as
.zip.Once the file is uploaded, we decide on the content-type to store in the GCS. This is done in 3 phases:-
Neutralized Extensions (served as text/plain)
These extensions can be executed by browsers, which is a security risk on a shared CDN domain. We override their content type to text/plain so browsers display them as plain text instead of executing them.
Browser-Renderable (served with original content type)
These are safe file types where we keep the real content type so the browser can display/play them inline (e.g. show a PDF, play a video, render an image)
Everything else - Default (e.g. .docx, .xlsx, .psd, .zip, .epub, .svg, etc.) is served as application/octet-stream, which forces the browser to download the file instead of trying to open it.