Skip to content

Validate supplied mimeType against acceptable media types #863

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Schmarvinius wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Validate supplied mimeType against acceptable media types #863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2cf9fd0
fix: validate supplied mimeType against acceptable media types
Schmarvinius Jul 21, 2026
43035d2
test: cover filename/mimeType mismatch media validation bypass
Schmarvinius Jul 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions ...chments/handler/applicationservice/helper/mimeTypeValidation/AttachmentDataExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,11 @@

public final class AttachmentDataExtractor {
private static final String FILE_NAME_FIELD = "fileName";
private static final String MIME_TYPE_FIELD = "mimeType";
public static final Filter FILE_NAME_FILTER =
(path, element, type) -> element.getName().contentEquals(FILE_NAME_FIELD);
public static final Filter MIME_TYPE_FILTER =
(path, element, type) -> element.getName().contentEquals(MIME_TYPE_FIELD);

/**
* Extracts and validates file names of attachments from the given entity data.
Expand All @@ -42,6 +45,34 @@ public static Map<String, Set<String>> extractAndValidateFileNamesByElement(
return fileNamesByElementName;
}

/**
* Extracts the explicitly supplied {@code mimeType} values of attachments from the given entity
* data, keyed by the qualified name of the declaring media entity.
*
* <p>Unlike file names, these values are not derived from the file extension but taken verbatim
* from the request payload (or, later, HTTP headers). They must therefore be validated against
* the acceptable media types as well, otherwise an allowed file name could be paired with a
* disallowed MIME type that is subsequently persisted and served inline.
*
* @param entity the CDS entity definition
* @param data the incoming data containing attachment values
* @return a map of declaring media entity names to sets of supplied MIME types
*/
public static Map<String, Set<String>> extractMimeTypesByElement(
CdsEntity entity, List<? extends CdsData> data) {
Map<String, Set<String>> mimeTypesByElementName = new HashMap<>();
CdsDataProcessor processor = CdsDataProcessor.create();
Validator mimeTypeValidator =
(path, element, value) -> {
if (value instanceof String mimeType && !mimeType.isBlank()) {
String key = element.getDeclaringType().getQualifiedName();
mimeTypesByElementName.computeIfAbsent(key, k -> new HashSet<>()).add(mimeType.trim());
}
};
processor.addValidator(MIME_TYPE_FILTER, mimeTypeValidator).process(data, entity);
return mimeTypesByElementName;
}

private static Map<String, Set<String>> collectFileNamesByElementName(
CdsEntity entity, List<? extends CdsData> data) {
// Use CdsProcessor to traverse the data and collect file names for elements
Expand Down
48 changes: 48 additions & 0 deletions ...ents/handler/applicationservice/helper/mimeTypeValidation/AttachmentValidationHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,54 @@ public static void validateMediaAttachments(
Map<String, Set<String>> fileNamesByElementName =
AttachmentDataExtractor.extractAndValidateFileNamesByElement(entity, data);
validateAttachmentMediaTypes(fileNamesByElementName, allowedTypesByElementName);

// validate the explicitly supplied mime types as well. The file name check alone can be
// bypassed by pairing an allowed file name (e.g. "report.pdf") with a disallowed MIME type
// (e.g. "text/html") that is persisted and served inline.
Map<String, Set<String>> mimeTypesByElementName =
AttachmentDataExtractor.extractMimeTypesByElement(entity, data);
validateAttachmentMimeTypes(mimeTypesByElementName, allowedTypesByElementName);
Comment on lines +67 to +69

@hyperspace-pr-bot hyperspace-pr-bot Bot Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

private static void validateAttachmentMimeTypes(
Map<String, Set<String>> mimeTypesByElementName,
Map<String, List<String>> acceptableMediaTypesByElementName) {

Map<String, List<String>> invalidMimeTypes =
findInvalidMimeTypesByElementName(
mimeTypesByElementName, acceptableMediaTypesByElementName);

if (!invalidMimeTypes.isEmpty()) {
throw buildUnsupportedFileTypeMessage(acceptableMediaTypesByElementName, invalidMimeTypes);

@hyperspace-pr-bot hyperspace-pr-bot Bot Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

private static Map<String, List<String>> findInvalidMimeTypesByElementName(
Map<String, Set<String>> mimeTypesByElementName,
Map<String, List<String>> acceptableMediaTypesByElementName) {
if (mimeTypesByElementName == null || mimeTypesByElementName.isEmpty()) {
return Map.of();
}
Map<String, List<String>> invalidMimeTypes = new HashMap<>();
mimeTypesByElementName.forEach(
(elementName, mimeTypes) -> {
List<String> acceptableTypes = acceptableMediaTypesByElementName.get(elementName);
if (acceptableTypes == null) {
return;
}
Comment on lines +94 to +97

@hyperspace-pr-bot hyperspace-pr-bot Bot Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


List<String> invalid =
mimeTypes.stream()
.filter(
mimeType -> !MediaTypeService.isMimeTypeAllowed(acceptableTypes, mimeType))
.toList();

if (!invalid.isEmpty()) {
invalidMimeTypes.put(elementName, invalid);
}
});

return invalidMimeTypes;
}

private static void validateAttachmentMediaTypes(
Expand Down
22 changes: 22 additions & 0 deletions ...e/attachments/integrationtests/nondraftservice/MediaValidatedAttachmentsNonDraftTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,28 @@ void shouldRejectAttachment_whenFileNameIsEmpty() throws Exception {
status().isBadRequest());
}

@Test
void shouldRejectAttachment_whenAllowedFileNameCarriesDisallowedMimeType() throws Exception {
String rootId = createRootAndReturnId();
String attachmentMetadata =
objectMapper.writeValueAsString(Map.of("fileName", "image.jpg", "mimeType", "text/html"));

requestHelper.assertPostStatus(
createUrl(rootId, MEDIA_VALIDATED_ATTACHMENTS),
attachmentMetadata,
status().isUnsupportedMediaType());
}

@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());
Comment on lines +97 to +104

@hyperspace-pr-bot hyperspace-pr-bot Bot Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@Test
void shouldAcceptUppercaseExtension_whenMimeTypeIsAllowed() throws Exception {
String rootId = createRootAndReturnId();
Expand Down
Loading