Validate header-derived file name and MIME type on create#865
Validate header-derived file name and MIME type on create#865Schmarvinius wants to merge 2 commits into
Conversation
CreateAttachmentEvent falls back to the Content-Disposition/slug and Content-Type headers for the file name and MIME type. This fallback ran AFTER acceptable-media-type validation, so a direct media upload that supplied the file name / MIME type only via headers bypassed @Core.AcceptableMediaTypes while storage still derived and persisted those header values. The header fallback logic is extracted into HeaderMediaMetadataResolver and now also applied to the request data before validation, so the allowlist is enforced against the exact values that will be persisted and served. CreateAttachmentEvent reuses the same resolver.
Adds HeaderMediaMetadataResolverTest verifying that file name and MIME type are resolved from the Content-Disposition/slug and Content-Type headers and filled into the attachment data (without overriding payload values), and a CreateAttachmentsHandler test asserting the header fallback is applied before acceptable-media-type validation.
SummaryThe following content is AI-generated and provides a summary of the pull request: Fix: Validate Header-Derived File Name and MIME Type Before
|
![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 is a clean refactoring with a meaningful security fix: header-derived metadata is now normalized into the data before acceptable-media-type validation runs, closing the bypass window. Three issues were flagged: (1) repeated regex matching per attachment row inside the validator lambda — the header values should be resolved once before the CdsDataProcessor loop; (2) the RFC 5987 decoded filename is not trimmed before URLDecoder.decode, potentially including trailing whitespace/CR characters unlike the plain-filename branch; and (3) the new handler test asserts call-order only weakly via MockedStatic.verify without an InOrder constraint, so ordering is not actually enforced.
PR Bot Information
Version: 1.28.2
- Correlation ID:
a08b9b30-84ef-11f1-8b3f-4cb56ca6c8fd - Agent Instructions:
- Event Trigger:
pull_request.opened - LLM:
anthropic--claude-4.6-sonnet - File Content Strategy: Full file content
| CdsDataProcessor.create() | ||
| .addValidator( | ||
| ApplicationHandlerHelper.MEDIA_CONTENT_FILTER, | ||
| (path, element, value) -> { | ||
| Map<String, Object> values = path.target().values(); | ||
| if (values.get(MediaData.FILE_NAME) == null) { | ||
| extractFileNameFromHeader(eventContext) | ||
| .ifPresent(fn -> values.put(MediaData.FILE_NAME, fn)); | ||
| } | ||
| if (values.get(MediaData.MIME_TYPE) == null) { | ||
| extractMimeTypeFromHeader(eventContext) | ||
| .ifPresent(mt -> values.put(MediaData.MIME_TYPE, mt)); | ||
| } | ||
| }) | ||
| .process(data, entity); |
There was a problem hiding this comment.
Logic Error: applyHeaderFallback calls extractFileNameFromHeader and extractMimeTypeFromHeader once per matching content element, but these methods always read from the same headers — the same header values will be applied identically to every attachment. For a bulk create with multiple attachments having different values, this is fine by design, but the real problem is that the two extract* calls happen inside the validator lambda, meaning the headers are re-read (and regex matched) on every single attachment row. For requests with many attachments this is wasteful, but more importantly it is logically cleaner and efficient to resolve the header values once outside the lambda and close over them.
Consider extracting the header lookups before the CdsDataProcessor call:
Optional<String> headerFileName = extractFileNameFromHeader(eventContext);
Optional<String> headerMimeType = extractMimeTypeFromHeader(eventContext);
if (headerFileName.isEmpty() && headerMimeType.isEmpty()) {
return;
}
CdsDataProcessor.create()
.addValidator(
ApplicationHandlerHelper.MEDIA_CONTENT_FILTER,
(path, element, value) -> {
Map<String, Object> values = path.target().values();
if (values.get(MediaData.FILE_NAME) == null) {
headerFileName.ifPresent(fn -> values.put(MediaData.FILE_NAME, fn));
}
if (values.get(MediaData.MIME_TYPE) == null) {
headerMimeType.ifPresent(mt -> values.put(MediaData.MIME_TYPE, mt));
}
})
.process(data, entity);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
| // Try RFC 5987 encoded filename first (filename*=UTF-8''...) | ||
| Matcher utf8Matcher = RFC5987_FILENAME_PATTERN.matcher(header); | ||
| if (utf8Matcher.find()) { | ||
| return Optional.of(URLDecoder.decode(utf8Matcher.group(1), StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
Bug: The RFC 5987 pattern filename\\*=UTF-8''([^;]+) does not strip trailing whitespace from the captured group before passing it to URLDecoder.decode. If the header contains trailing spaces or a trailing \r (as is common in raw HTTP headers), the decoded file name will include them.
Should call .trim() on the captured group before decoding, consistent with how the plain-filename branch handles trimming:
return Optional.of(URLDecoder.decode(utf8Matcher.group(1).trim(), StandardCharsets.UTF_8));| return Optional.of(URLDecoder.decode(utf8Matcher.group(1), StandardCharsets.UTF_8)); | |
| return Optional.of(URLDecoder.decode(utf8Matcher.group(1).trim(), StandardCharsets.UTF_8)); |
Double-check suggestion before committing. Edit this comment for amendments.
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
| try (MockedStatic<HeaderMediaMetadataResolver> resolver = | ||
| mockStatic(HeaderMediaMetadataResolver.class); | ||
| MockedStatic<AttachmentValidationHelper> helper = | ||
| mockStatic(AttachmentValidationHelper.class)) { | ||
| // when | ||
| new CreateAttachmentsHandler(eventFactory, storageReader, "400MB", runtime) | ||
| .processBeforeForMetadata(context, data); | ||
|
|
||
| // then header-derived metadata is normalized into the data and validation runs over it | ||
| resolver.verify(() -> HeaderMediaMetadataResolver.applyHeaderFallback(entity, data, context)); | ||
| helper.verify( | ||
| () -> AttachmentValidationHelper.validateMediaAttachments(entity, data, runtime)); | ||
| } |
There was a problem hiding this comment.
Best Practice: The existing processBeforeForMetadata_executesValidation test still mocks HeaderMediaMetadataResolver only implicitly (it does not mock it at all), so when both tests run the MockedStatic<HeaderMediaMetadataResolver> from the new test is unrelated to the old one. However, the old test now implicitly calls the real applyHeaderFallback with a mocked CdsEntity and mocked CdsData — this could silently succeed or fail in unexpected ways as the real implementation evolves. The two tests are also partially redundant: the ordering assertion in the new test (fallback before validation) is only weakly enforced by call-order in MockedStatic.verify, which does not actually assert invocation order.
Consider using InOrder from Mockito to assert that applyHeaderFallback is called before validateMediaAttachments, making the ordering guarantee explicit and reliable:
try (MockedStatic<HeaderMediaMetadataResolver> resolver =
mockStatic(HeaderMediaMetadataResolver.class);
MockedStatic<AttachmentValidationHelper> helper =
mockStatic(AttachmentValidationHelper.class)) {
InOrder inOrder = inOrder(resolver, helper);
// when
new CreateAttachmentsHandler(eventFactory, storageReader, "400MB", runtime)
.processBeforeForMetadata(context, data);
// then header-derived metadata is normalized into the data and validation runs over it
inOrder.verify(resolver, () -> HeaderMediaMetadataResolver.applyHeaderFallback(entity, data, context));
inOrder.verify(helper, () -> AttachmentValidationHelper.validateMediaAttachments(entity, data, runtime));
}Note: MockedStatic.InOrder is available since Mockito 4.x via MockedStatic#inOrder; if that API is unavailable here, an AtomicInteger counter can enforce the order.
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
1 participant
Issue
For direct media uploads the file name comes from the
Content-Disposition/slugheader and the MIME type from theContent-Typeheader.CreateAttachmentEventapplied this header fallback after@Core.AcceptableMediaTypesvalidation had already run over the (header-less) payload.As a result a create that supplied the file name / MIME type only via headers could bypass the media type allowlist, while storage still derived and persisted those header values — the same values later served inline.
Fix
HeaderMediaMetadataResolver(extractFileNameFromHeader,extractMimeTypeFromHeader,applyHeaderFallback).CreateAttachmentsHandler.processBeforeForMetadatanow normalizes the header-derived file name / MIME type into the request data before validation, so the allowlist is enforced against the exact values that will be persisted and served.CreateAttachmentEventreuses the same resolver (no behavioral change to its own fallback).Tests
HeaderMediaMetadataResolverTest: header extraction andapplyHeaderFallback(fills when absent, never overrides payload values, no-op without content).CreateAttachmentsHandlerTest: header fallback is applied before validation.