Validate supplied mimeType against acceptable media types#863
Validate supplied mimeType against acceptable media types#863Schmarvinius wants to merge 2 commits into
Conversation
The acceptable-media-types check only resolved the MIME type from the uploaded file name. The mimeType value supplied in the payload was persisted and served inline (Core.MediaType + inline ContentDisposition) without being validated against @Core.AcceptableMediaTypes. An allowed file name (e.g. report.pdf) could therefore be paired with a disallowed MIME type (e.g. text/html), enabling stored active content. Validation now additionally checks the explicitly supplied mimeType values against the acceptable media types.
Adds integration tests asserting that an allowed file name paired with a disallowed mimeType is rejected (415), and that a matching allowed mimeType is still accepted.
SummaryThe following content is AI-generated and provides a summary of the pull request: Validate Supplied
|
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR correctly addresses the XSS vector described in the issue — the new mimeType validation path closes a real security gap. However, there are several issues that need attention before merging: the unit tests in AttachmentValidationHelperTest do not stub the new extractMimeTypesByElement call, meaning the new code path is untested at the unit level; the silent skip when a MIME-type element key is absent from the allowlist map risks bypassing validation if key derivation diverges; and the shared buildUnsupportedFileTypeMessage method has a latent NPE risk on get(element) that should be hardened.
PR Bot Information
Version: 1.28.2
- Event Trigger:
pull_request.opened - Agent Instructions:
- Correlation ID:
d5304f10-84eb-11f1-8b83-b1d881083df8 - LLM:
anthropic--claude-4.6-sonnet - File Content Strategy: Full file content
| Map<String, Set<String>> mimeTypesByElementName = | ||
| AttachmentDataExtractor.extractMimeTypesByElement(entity, data); | ||
| validateAttachmentMimeTypes(mimeTypesByElementName, allowedTypesByElementName); |
There was a problem hiding this comment.
Logic Error: Unit tests for validateAttachmentMimeTypes do not stub extractMimeTypesByElement, causing it to return null in the mock context and silently skip MIME-type validation.
In doesNotThrow_whenNoFiles and both parametrized test methods (doesNotThrow_whenFilesAreValid, throwsException_whenFilesAreInvalid), AttachmentDataExtractor.extractMimeTypesByElement is never stubbed. When the static mock is active it returns null by default, and findInvalidMimeTypesByElementName short-circuits on the null guard at line 88, so the new validation path is never exercised by the unit tests. The happy-path and rejection tests may both pass for the wrong reason. The existing mocked-static setup in each test should also stub extractMimeTypesByElement and include scenarios where it returns a disallowed MIME type.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| List<String> acceptableTypes = acceptableMediaTypesByElementName.get(elementName); | ||
| if (acceptableTypes == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Logic Error: When acceptableTypes is null for a given element name the method silently skips it, but this means a client can supply any mimeType value for an element that is in mimeTypesByElementName but not in acceptableMediaTypesByElementName and it will never be rejected.
This is only safe if the two maps are guaranteed to be keyed identically. However, extractMimeTypesByElement uses element.getDeclaringType().getQualifiedName() as key, while getAcceptableMediaTypesFromEntity may produce keys derived differently. If those keys ever diverge (e.g., due to projections or mixins), the MIME type goes unvalidated. Consider logging a warning when acceptableTypes == null for a key present in mimeTypesByElementName, or verifying that the key-derivation strategies are provably identical and documenting that invariant.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| mimeTypesByElementName, acceptableMediaTypesByElementName); | ||
|
|
||
| if (!invalidMimeTypes.isEmpty()) { | ||
| throw buildUnsupportedFileTypeMessage(acceptableMediaTypesByElementName, invalidMimeTypes); |
There was a problem hiding this comment.
Bug: buildUnsupportedFileTypeMessage calls acceptableMediaTypesByElementName.get(element) on line 173 without a null check. When this method is reached via validateAttachmentMimeTypes, invalidMimeTypes can contain element names whose key exists in mimeTypesByElementName but not in acceptableMediaTypesByElementName — findInvalidMimeTypesByElementName returns early (return) for those elements rather than putting them into invalidMimeTypes, so in practice they won't appear there. However, if that guard is ever relaxed or the method is reused, get(element) will return null and String.join will throw a NullPointerException. The same risk exists for the file-name path. Should add a null-safe fallback, e.g. acceptableMediaTypesByElementName.getOrDefault(element, List.of()).
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| @Test | ||
| void shouldAcceptAttachment_whenAllowedFileNameAndAllowedMimeType() throws Exception { | ||
| String rootId = createRootAndReturnId(); | ||
| String attachmentMetadata = | ||
| objectMapper.writeValueAsString(Map.of("fileName", "image.jpg", "mimeType", "image/jpeg")); | ||
|
|
||
| requestHelper.assertPostStatus( | ||
| createUrl(rootId, MEDIA_VALIDATED_ATTACHMENTS), attachmentMetadata, status().isCreated()); |
There was a problem hiding this comment.
Best Practice: The new test shouldAcceptAttachment_whenAllowedFileNameAndAllowedMimeType only asserts the HTTP status code (201 Created) but does not verify that the mimeType value was actually persisted. Without a persistence assertion the test cannot detect a regression where the MIME type is silently dropped during storage. Consider reading back the created attachment and asserting the stored mimeType equals "image/jpeg", consistent with how other tests in this class (e.g. selectStoredRootWithMediaValidatedAttachments) verify stored data.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
lisajulia
left a comment
lisajulia
left a comment
There was a problem hiding this comment.
Possibly look at the things the bot has found, then this is fine :)
2 participants
Issue
The
@Core.AcceptableMediaTypescheck only resolved the MIME type from the uploaded file name. ThemimeTypesupplied in the request payload (or later via theContent-Typeheader) was persisted and served inline (Core.MediaType+Core.ContentDisposition: inline) without being validated against the allowlist.An authenticated user could therefore upload an allowed file name paired with a disallowed MIME type:
{ "fileName": "report.pdf", "mimeType": "text/html", "content": "<script>...</script>" }The filename check passed (
report.pdf→application/pdf→ allowed) whiletext/htmlwas stored and served inline — a stored XSS vector.Fix
AttachmentValidationHelpernow additionally extracts the explicitly suppliedmimeTypevalues (AttachmentDataExtractor.extractMimeTypesByElement) and validates them against@Core.AcceptableMediaTypes, rejecting the request with415 Unsupported Media Typeon a mismatch.Tests
Added integration tests in
MediaValidatedAttachmentsNonDraftTest:415201