Enforce acceptable media types on update and draft patch#864
Enforce acceptable media types on update and draft patch#864Schmarvinius wants to merge 2 commits into
Conversation
SummaryThe following content is AI-generated and provides a summary of the pull request: Enforce
|
![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 closes the media-type validation gap on the update and draft-patch paths, and the implementation follows the existing handler structure well. The main issues found are: (1) both new CdsRuntime constructor parameters are missing the project-mandated requireNonNull guard; (2) the validation logic has a blind spot for requests that update only the mimeType field without providing a fileName, since the entire check is filename-driven; and (3) a minor test maintainability concern around the hand-crafted OData key URL in the non-draft integration test.
PR Bot Information
Version: 1.28.2
- LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
01d00950-84ee-11f1-8656-3beb8ee12b6a - File Content Strategy: Full file content
- Agent Instructions:
- Event Trigger:
pull_request.opened
| this.persistence = persistence; | ||
| this.eventFactory = eventFactory; | ||
| this.defaultMaxSize = defaultMaxSize; | ||
| this.cdsRuntime = cdsRuntime; |
There was a problem hiding this comment.
Best Practices: Missing requireNonNull guard for the new cdsRuntime constructor parameter.
Same as UpdateAttachmentsHandler: the project mandates requireNonNull for every constructor parameter, but the new cdsRuntime field is stored without validation. A null value would cause an opaque NullPointerException at call time instead of a clear failure at wiring time.
| this.cdsRuntime = cdsRuntime; | |
| this.cdsRuntime = Objects.requireNonNull(cdsRuntime, "cdsRuntime must not be null"); |
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
Acceptable-media-types validation was only applied on create/draft-new. Non-draft updates (UpdateAttachmentsHandler) and draft patches (DraftPatchAttachmentsHandler) stored replacement content and metadata without validation, so a user able to update an attachment could replace it (including a metadata-only mimeType/fileName change) with a disallowed media type that bypassed @Core.AcceptableMediaTypes. Both handlers now validate via AttachmentValidationHelper before dispatching the modification, matching the create path. On update the check runs in a dedicated @before(BEFORE) method so it also covers metadata-only changes. CdsRuntime is injected into both handlers to resolve the CDS model.
Adds integration tests asserting that replacing an attachment with a disallowed media type is rejected (415) on the non-draft update path and on the draft patch path.
e03017f to
1e4cd58
Compare
1 participant
Issue
@Core.AcceptableMediaTypesvalidation was only applied on the create / draft-new path. Two write paths skipped it entirely:UpdateAttachmentsHandler) — a user able to update an attachment could replace its content, or even do a metadata-only change (e.g. setmimeTypetotext/html), bypassing the allowlist.DraftPatchAttachmentsHandler) — draft content/metadata was stored without validation before activation.Both paths only enforced the size limit, making the media type policy asymmetric with create.
Fix
Both handlers now call
AttachmentValidationHelper.validateMediaAttachmentsbefore dispatching the modification:UpdateAttachmentsHandlervalidates in a dedicated@Before(BEFORE)method so the check also runs for metadata-only changes (not just when content is present).DraftPatchAttachmentsHandlervalidates at the start of the draft-patch handling.CdsRuntimeis injected into both handlers to resolve the CDS model, mirroring the existing create handler.Tests
MediaValidatedAttachmentsNonDraftTest: updating an attachment to a disallowed media type →415.MediaValidatedAttachmentsDraftTest: patching a draft attachment to a disallowed media type →415.