Validate header-derived file name and MIME type on create #865
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,117 @@ | ||||||
| /* | ||||||
| * © 2026 SAP SE or an SAP affiliate company and cds-feature-attachments contributors. | ||||||
| */ | ||||||
| package com.sap.cds.feature.attachments.handler.applicationservice.helper; | ||||||
|
|
||||||
| import com.sap.cds.CdsData; | ||||||
| import com.sap.cds.CdsDataProcessor; | ||||||
| import com.sap.cds.feature.attachments.generated.cds4j.sap.attachments.MediaData; | ||||||
| import com.sap.cds.feature.attachments.handler.common.ApplicationHandlerHelper; | ||||||
| import com.sap.cds.reflect.CdsEntity; | ||||||
| import com.sap.cds.services.EventContext; | ||||||
| import java.net.URLDecoder; | ||||||
| import java.nio.charset.StandardCharsets; | ||||||
| import java.util.List; | ||||||
| import java.util.Map; | ||||||
| import java.util.Optional; | ||||||
| import java.util.regex.Matcher; | ||||||
| import java.util.regex.Pattern; | ||||||
|
|
||||||
| /** | ||||||
| * Resolves attachment metadata (file name, MIME type) from HTTP headers and applies it as a | ||||||
| * fallback to the request data. | ||||||
| * | ||||||
| * <p>For direct media uploads the file name is typically provided via the {@code | ||||||
| * Content-Disposition} or {@code slug} header and the MIME type via the {@code Content-Type} header | ||||||
| * rather than in the request payload. This resolver normalizes those header values into the | ||||||
| * attachment data <em>before</em> acceptable-media-type validation runs, so that validation and | ||||||
| * storage operate on the same file name / MIME type that is ultimately persisted and served. | ||||||
| */ | ||||||
| public final class HeaderMediaMetadataResolver { | ||||||
|
|
||||||
| private static final Pattern RFC5987_FILENAME_PATTERN = | ||||||
| Pattern.compile("filename\\*=UTF-8''([^;]+)", Pattern.CASE_INSENSITIVE); | ||||||
| private static final Pattern PLAIN_FILENAME_PATTERN = | ||||||
| Pattern.compile("(?<!\\*)filename=\"?([^\";]+)\"?", Pattern.CASE_INSENSITIVE); | ||||||
|
|
||||||
| /** | ||||||
| * Fills the {@code fileName} and {@code mimeType} of every media content attachment in the given | ||||||
| * data from the request headers, unless those values are already present in the payload. | ||||||
| * | ||||||
| * @param entity the {@link CdsEntity} type of the given data | ||||||
| * @param data the request data to normalize | ||||||
| * @param eventContext the current {@link EventContext} carrying the request headers | ||||||
| */ | ||||||
| public static void applyHeaderFallback( | ||||||
| CdsEntity entity, List<? extends CdsData> data, EventContext eventContext) { | ||||||
| if (entity == null | ||||||
| || data == null | ||||||
| || data.isEmpty() | ||||||
| || eventContext.getParameterInfo() == null) { | ||||||
| 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) { | ||||||
| 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); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Extracts the file name from the {@code Content-Disposition} header or falls back to the {@code | ||||||
| * slug} header. Supports RFC 5987 encoded file names ({@code filename*=UTF-8''...}) and plain | ||||||
| * file names. | ||||||
| */ | ||||||
| public static Optional<String> extractFileNameFromHeader(EventContext eventContext) { | ||||||
| String header = eventContext.getParameterInfo().getHeader("Content-Disposition"); | ||||||
| if (header != null) { | ||||||
| // 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)); | ||||||
|
Contributor
There was a problem hiding this comment. Bug: The RFC 5987 pattern Should call return Optional.of(URLDecoder.decode(utf8Matcher.group(1).trim(), StandardCharsets.UTF_8));
Suggested change
Double-check suggestion before committing. Edit this comment for amendments. Please provide feedback on the review comment by checking the appropriate box:
|
||||||
| } | ||||||
| // Fall back to plain filename= | ||||||
| Matcher plainMatcher = PLAIN_FILENAME_PATTERN.matcher(header); | ||||||
| if (plainMatcher.find()) { | ||||||
| return Optional.of(plainMatcher.group(1).trim()); | ||||||
| } | ||||||
| } | ||||||
| // Fiori Elements may use the slug header instead | ||||||
| String slug = eventContext.getParameterInfo().getHeader("slug"); | ||||||
| if (slug != null) { | ||||||
| return Optional.of(URLDecoder.decode(slug, StandardCharsets.UTF_8)); | ||||||
| } | ||||||
| return Optional.empty(); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Extracts the MIME type from the {@code Content-Type} header, stripping charset and other | ||||||
| * parameters. Returns empty if the {@code Content-Type} is null or empty. | ||||||
| */ | ||||||
| public static Optional<String> extractMimeTypeFromHeader(EventContext eventContext) { | ||||||
| String contentType = eventContext.getParameterInfo().getHeader("Content-Type"); | ||||||
| if (contentType == null) { | ||||||
| return Optional.empty(); | ||||||
| } | ||||||
| String mimeType = contentType.split(";")[0].trim(); | ||||||
| if (mimeType.isEmpty()) { | ||||||
| return Optional.empty(); | ||||||
| } | ||||||
| return Optional.of(mimeType); | ||||||
| } | ||||||
|
|
||||||
| private HeaderMediaMetadataResolver() { | ||||||
| // to prevent instantiation | ||||||
| } | ||||||
| } | ||||||
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| /* | ||
| * © 2026 SAP SE or an SAP affiliate company and cds-feature-attachments contributors. | ||
| */ | ||
| package com.sap.cds.feature.attachments.handler.applicationservice.helper; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| import com.sap.cds.feature.attachments.generated.cds4j.sap.attachments.Attachments; | ||
| import com.sap.cds.feature.attachments.handler.helper.RuntimeHelper; | ||
| import com.sap.cds.reflect.CdsEntity; | ||
| import com.sap.cds.services.EventContext; | ||
| import com.sap.cds.services.request.ParameterInfo; | ||
| import com.sap.cds.services.runtime.CdsRuntime; | ||
| import java.io.InputStream; | ||
| import java.util.List; | ||
| import org.junit.jupiter.api.BeforeAll; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class HeaderMediaMetadataResolverTest { | ||
|
|
||
| private static final String MEDIA_ENTITY = "unit.test.Attachment"; | ||
|
|
||
| private static CdsRuntime runtime; | ||
|
|
||
| private EventContext eventContext; | ||
| private ParameterInfo parameterInfo; | ||
|
|
||
| @BeforeAll | ||
| static void classSetup() { | ||
| runtime = RuntimeHelper.runtime; | ||
| } | ||
|
|
||
| @BeforeEach | ||
| void setup() { | ||
| eventContext = mock(EventContext.class); | ||
| parameterInfo = mock(ParameterInfo.class); | ||
| when(eventContext.getParameterInfo()).thenReturn(parameterInfo); | ||
| } | ||
|
|
||
| @Test | ||
| void extractsFileNameFromContentDispositionHeader() { | ||
| when(parameterInfo.getHeader("Content-Disposition")) | ||
| .thenReturn("attachment; filename=\"report.pdf\""); | ||
|
|
||
| assertThat(HeaderMediaMetadataResolver.extractFileNameFromHeader(eventContext)) | ||
| .contains("report.pdf"); | ||
| } | ||
|
|
||
| @Test | ||
| void extractsFileNameFromSlugHeader() { | ||
| when(parameterInfo.getHeader("Content-Disposition")).thenReturn(null); | ||
| when(parameterInfo.getHeader("slug")).thenReturn("document.docx"); | ||
|
|
||
| assertThat(HeaderMediaMetadataResolver.extractFileNameFromHeader(eventContext)) | ||
| .contains("document.docx"); | ||
| } | ||
|
|
||
| @Test | ||
| void extractsMimeTypeFromContentTypeHeaderStrippingParameters() { | ||
| when(parameterInfo.getHeader("Content-Type")).thenReturn("text/html; charset=utf-8"); | ||
|
|
||
| assertThat(HeaderMediaMetadataResolver.extractMimeTypeFromHeader(eventContext)) | ||
| .contains("text/html"); | ||
| } | ||
|
|
||
| @Test | ||
| void returnsEmptyWhenNoHeadersPresent() { | ||
| assertThat(HeaderMediaMetadataResolver.extractFileNameFromHeader(eventContext)).isEmpty(); | ||
| assertThat(HeaderMediaMetadataResolver.extractMimeTypeFromHeader(eventContext)).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| void fillsFileNameAndMimeTypeFromHeadersWhenAbsentInData() { | ||
| CdsEntity entity = runtime.getCdsModel().getEntity(MEDIA_ENTITY); | ||
| Attachments data = Attachments.create(); | ||
| data.setContent(mock(InputStream.class)); | ||
| when(parameterInfo.getHeader("Content-Disposition")) | ||
| .thenReturn("attachment; filename=\"notes.txt\""); | ||
| when(parameterInfo.getHeader("Content-Type")).thenReturn("text/html"); | ||
|
|
||
| HeaderMediaMetadataResolver.applyHeaderFallback(entity, List.of(data), eventContext); | ||
|
|
||
| assertThat(data.getFileName()).isEqualTo("notes.txt"); | ||
| assertThat(data.getMimeType()).isEqualTo("text/html"); | ||
| } | ||
|
|
||
| @Test | ||
| void doesNotOverridePayloadValuesWithHeaderValues() { | ||
| CdsEntity entity = runtime.getCdsModel().getEntity(MEDIA_ENTITY); | ||
| Attachments data = Attachments.create(); | ||
| data.setContent(mock(InputStream.class)); | ||
| data.setFileName("payload.png"); | ||
| data.setMimeType("image/png"); | ||
| when(parameterInfo.getHeader("Content-Disposition")) | ||
| .thenReturn("attachment; filename=\"notes.txt\""); | ||
| when(parameterInfo.getHeader("Content-Type")).thenReturn("text/html"); | ||
|
|
||
| HeaderMediaMetadataResolver.applyHeaderFallback(entity, List.of(data), eventContext); | ||
|
|
||
| assertThat(data.getFileName()).isEqualTo("payload.png"); | ||
| assertThat(data.getMimeType()).isEqualTo("image/png"); | ||
| } | ||
|
|
||
| @Test | ||
| void doesNothingWhenDataHasNoContentElement() { | ||
| CdsEntity entity = runtime.getCdsModel().getEntity(MEDIA_ENTITY); | ||
| Attachments data = Attachments.create(); | ||
| when(parameterInfo.getHeader("Content-Disposition")) | ||
| .thenReturn("attachment; filename=\"notes.txt\""); | ||
|
|
||
| HeaderMediaMetadataResolver.applyHeaderFallback(entity, List.of(data), eventContext); | ||
|
|
||
| assertThat(data.getFileName()).isNull(); | ||
| assertThat(data.getMimeType()).isNull(); | ||
| } | ||
| } |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic Error:
applyHeaderFallbackcallsextractFileNameFromHeaderandextractMimeTypeFromHeaderonce 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 twoextract*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
CdsDataProcessorcall:Please provide feedback on the review comment by checking the appropriate box: