fix: GHSA-r899-h629-j84r#10523
Open
mtrezza wants to merge 1 commit into
Open
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
📝 WalkthroughWalkthrough
ChangesFilesRouter Content-Type Validation Strictness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (3 warnings)
✅ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.x.x #10523 +/- ##
=================================================
+ Coverage 92.07% 92.47% +0.39%
=================================================
Files 192 192
Lines 16175 16184 +9
Branches 234 234
=================================================
+ Hits 14893 14966 +73
+ Misses 1258 1198 -60
+ Partials 24 20 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Routers/FilesRouter.js`:
- Around line 227-256: The Content-Type parsing in FilesRouter’s save/validation
flow only rejects empty type/subtype pairs, so malformed tokens like double
slashes or comma-separated values can slip through. Tighten the validation
around the existing slashIndex/type/subtype parsing by rejecting any media-type
that is not a single well-formed token pair before reaching the blocklist check,
and keep using rejectExtension for blocked bare tokens while routing all other
malformed cases to the Invalid Content-Type Parse.Error path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1713dc7-ebd3-4b07-9137-60d0c526e374
📒 Files selected for processing (3)
spec/ParseFile.spec.jsspec/PurchaseValidation.spec.jssrc/Routers/FilesRouter.js
Comment on lines
+227
to
+256
| const slashIndex = contentType.indexOf('/'); | ||
| const type = slashIndex > 0 ? contentType.slice(0, slashIndex).trim() : ''; | ||
| const subtype = | ||
| slashIndex > 0 ? contentType.slice(slashIndex + 1).split(';')[0].trim() : ''; | ||
| if (!type || !subtype) { | ||
| // A Content-Type that does not parse as `type/subtype` with a non-empty | ||
| // type AND subtype is malformed: there is no valid MIME type without a | ||
| // subtype (RFC 9110 §8.3.1). Browsers cannot parse it and fall back to | ||
| // MIME-sniffing the file body, which can render HTML/script markers as | ||
| // active content on storage adapters that serve the stored Content-Type | ||
| // (e.g. `image`, `image/`). Surface the precise blocklist message when | ||
| // the bare token names a blocked extension (e.g. a no-slash `svg`), | ||
| // otherwise reject the unparseable Content-Type. | ||
| const bareToken = (slashIndex < 0 ? contentType.split(';')[0] : type).replace( | ||
| /\s+/g, | ||
| '' | ||
| ); | ||
| if (bareToken && !isValidExtension(bareToken)) { | ||
| rejectExtension(bareToken); | ||
| return; | ||
| } | ||
| next(new Parse.Error(Parse.Error.FILE_SAVE_ERROR, 'Invalid Content-Type.')); | ||
| return; | ||
| } | ||
| // Validate the well-formed Content-Type subtype against the blocklist, e.g. | ||
| // "image/svg+xml" -> "svg+xml", "image/svg+xml;charset=utf-8" -> "svg+xml". | ||
| // Valid custom/vendor types (e.g. "application/vnd.api+json") parse and are | ||
| // allowed; only blocked subtypes are rejected. | ||
| const contentTypeExtension = subtype.replace(/\s+/g, ''); | ||
| if (!isValidExtension(contentTypeExtension)) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject invalid media-type tokens, not only empty type/subtype.
This path accepts non-empty but malformed values (for example image//svg+xml or text/plain,application/json) because it only checks emptiness before subtype blocklist validation. That weakens the malformed Content-Type hardening for unknown extensions.
Suggested fix
if (!isExtensionRecognized && contentType && !allowsAllExtensions) {
const slashIndex = contentType.indexOf('/');
const type = slashIndex > 0 ? contentType.slice(0, slashIndex).trim() : '';
const subtype =
slashIndex > 0 ? contentType.slice(slashIndex + 1).split(';')[0].trim() : '';
+ const tokenRegex = /^[!#$%&'*+\-.^_`|~A-Za-z0-9]+$/;
+ const hasValidTypeSubtype = tokenRegex.test(type) && tokenRegex.test(subtype);
- if (!type || !subtype) {
+ if (!type || !subtype || !hasValidTypeSubtype) {
const bareToken = (slashIndex < 0 ? contentType.split(';')[0] : type).replace(
/\s+/g,
''
);
if (bareToken && !isValidExtension(bareToken)) {
rejectExtension(bareToken);
return;
}
next(new Parse.Error(Parse.Error.FILE_SAVE_ERROR, 'Invalid Content-Type.'));
return;
}📝 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
| const slashIndex = contentType.indexOf('/'); | |
| const type = slashIndex > 0 ? contentType.slice(0, slashIndex).trim() : ''; | |
| const subtype = | |
| slashIndex > 0 ? contentType.slice(slashIndex + 1).split(';')[0].trim() : ''; | |
| if (!type || !subtype) { | |
| // A Content-Type that does not parse as `type/subtype` with a non-empty | |
| // type AND subtype is malformed: there is no valid MIME type without a | |
| // subtype (RFC 9110 §8.3.1). Browsers cannot parse it and fall back to | |
| // MIME-sniffing the file body, which can render HTML/script markers as | |
| // active content on storage adapters that serve the stored Content-Type | |
| // (e.g. `image`, `image/`). Surface the precise blocklist message when | |
| // the bare token names a blocked extension (e.g. a no-slash `svg`), | |
| // otherwise reject the unparseable Content-Type. | |
| const bareToken = (slashIndex < 0 ? contentType.split(';')[0] : type).replace( | |
| /\s+/g, | |
| '' | |
| ); | |
| if (bareToken && !isValidExtension(bareToken)) { | |
| rejectExtension(bareToken); | |
| return; | |
| } | |
| next(new Parse.Error(Parse.Error.FILE_SAVE_ERROR, 'Invalid Content-Type.')); | |
| return; | |
| } | |
| // Validate the well-formed Content-Type subtype against the blocklist, e.g. | |
| // "image/svg+xml" -> "svg+xml", "image/svg+xml;charset=utf-8" -> "svg+xml". | |
| // Valid custom/vendor types (e.g. "application/vnd.api+json") parse and are | |
| // allowed; only blocked subtypes are rejected. | |
| const contentTypeExtension = subtype.replace(/\s+/g, ''); | |
| if (!isValidExtension(contentTypeExtension)) { | |
| const slashIndex = contentType.indexOf('/'); | |
| const type = slashIndex > 0 ? contentType.slice(0, slashIndex).trim() : ''; | |
| const subtype = | |
| slashIndex > 0 ? contentType.slice(slashIndex + 1).split(';')[0].trim() : ''; | |
| const tokenRegex = /^[!#$%&'*+\-.^_`|~A-Za-z0-9]+$/; | |
| const hasValidTypeSubtype = tokenRegex.test(type) && tokenRegex.test(subtype); | |
| if (!type || !subtype || !hasValidTypeSubtype) { | |
| // A Content-Type that does not parse as `type/subtype` with a non-empty | |
| // type AND subtype is malformed: there is no valid MIME type without a | |
| // subtype (RFC 9110 §8.3.1). Browsers cannot parse it and fall back to | |
| // MIME-sniffing the file body, which can render HTML/script markers as | |
| // active content on storage adapters that serve the stored Content-Type | |
| // (e.g. `image`, `image/`). Surface the precise blocklist message when | |
| // the bare token names a blocked extension (e.g. a no-slash `svg`), | |
| // otherwise reject the unparseable Content-Type. | |
| const bareToken = (slashIndex < 0 ? contentType.split(';')[0] : type).replace( | |
| /\s+/g, | |
| '' | |
| ); | |
| if (bareToken && !isValidExtension(bareToken)) { | |
| rejectExtension(bareToken); | |
| return; | |
| } | |
| next(new Parse.Error(Parse.Error.FILE_SAVE_ERROR, 'Invalid Content-Type.')); | |
| return; | |
| } | |
| // Validate the well-formed Content-Type subtype against the blocklist, e.g. | |
| // "image/svg+xml" -> "svg+xml", "image/svg+xml;charset=utf-8" -> "svg+xml". | |
| // Valid custom/vendor types (e.g. "application/vnd.api+json") parse and are | |
| // allowed; only blocked subtypes are rejected. | |
| const contentTypeExtension = subtype.replace(/\s+/g, ''); | |
| if (!isValidExtension(contentTypeExtension)) { |
🤖 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 `@src/Routers/FilesRouter.js` around lines 227 - 256, The Content-Type parsing
in FilesRouter’s save/validation flow only rejects empty type/subtype pairs, so
malformed tokens like double slashes or comma-separated values can slip through.
Tighten the validation around the existing slashIndex/type/subtype parsing by
rejecting any media-type that is not a single well-formed token pair before
reaching the blocklist check, and keep using rejectExtension for blocked bare
tokens while routing all other malformed cases to the Invalid Content-Type
Parse.Error path.
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 by CodeRabbit
Bug Fixes
Tests