Use a finite default upload size when no scanner is present#867
Use a finite default upload size when no scanner is present#867Schmarvinius wants to merge 2 commits into
Conversation
Without a malware scanner binding the default maximum upload size was Long.MAX_VALUE (effectively unlimited), so a missing scanner silently allowed unbounded attachment uploads and the associated resource exhaustion risk. The default is now a finite 400 MB regardless of the scanner binding (DEFAULT_MAX_UPLOAD_SIZE). Deployments that intentionally allow larger uploads can opt in via the new cds.attachments.maxUploadSize property, or override per entity with @Validation.Maximum.
Adds RegistrationTest cases for resolveDefaultMaxSize (finite default when unconfigured or blank, configured override honored) and updates handler tests to the renamed DEFAULT_MAX_UPLOAD_SIZE constant.
SummaryThe following content is AI-generated and provides a summary of the pull request: Use a Finite Default Upload Size When No Scanner Is PresentBug Fix🐛 Previously, when no malware scanner binding was present, the default maximum attachment upload size fell back to The default maximum upload size is now a finite 400 MB ( Changes
PR Bot InformationVersion:
|
![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 well-structured and achieves its stated goal cleanly. Removing the scanner-dependent UNLIMITED_SIZE path and centralizing resolution in resolveDefaultMaxSize is a clear security improvement, and the test coverage for the new method is thorough. The one issue flagged above is that an invalid cds.attachments.maxUploadSize configuration value is only caught at upload time rather than at startup, which could be confusing for operators. Addressing that would make the configuration contract more robust.
PR Bot Information
Version: 1.28.2
- Correlation ID:
dcf30e90-84f0-11f1-830e-eed41851c4d1 - Event Trigger:
pull_request.opened - File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet - Agent Instructions:
| static String resolveDefaultMaxSize(CdsEnvironment environment) { | ||
| String configured = | ||
| environment.getProperty("cds.attachments.maxUploadSize", String.class, null); | ||
| if (configured != null && !configured.isBlank()) { | ||
| return configured; | ||
| } | ||
| return ModifyApplicationHandlerHelper.DEFAULT_MAX_UPLOAD_SIZE; | ||
| } |
There was a problem hiding this comment.
Best Practice: resolveDefaultMaxSize accepts any non-blank configured value without validating its format, but an invalid value (e.g. "400 megabytes" or "abc") is only caught later at upload time when FileSizeUtils.parseFileSizeToBytes throws IllegalArgumentException, turning a configuration error into a runtime crash on the first upload attempt.
Consider validating the configured value at resolution time by calling FileSizeUtils.parseFileSizeToBytes and falling back to the default (or re-throwing with a clear message) if it is malformed:
| static String resolveDefaultMaxSize(CdsEnvironment environment) { | |
| String configured = | |
| environment.getProperty("cds.attachments.maxUploadSize", String.class, null); | |
| if (configured != null && !configured.isBlank()) { | |
| return configured; | |
| } | |
| return ModifyApplicationHandlerHelper.DEFAULT_MAX_UPLOAD_SIZE; | |
| } | |
| static String resolveDefaultMaxSize(CdsEnvironment environment) { | |
| String configured = | |
| environment.getProperty("cds.attachments.maxUploadSize", String.class, null); | |
| if (configured != null && !configured.isBlank()) { | |
| try { | |
| FileSizeUtils.parseFileSizeToBytes(configured); // validate format eagerly | |
| } catch (IllegalArgumentException e) { | |
| throw new IllegalArgumentException( | |
| "Invalid value for cds.attachments.maxUploadSize: '" + configured + "'", e); | |
| } | |
| return configured; | |
| } | |
| return ModifyApplicationHandlerHelper.DEFAULT_MAX_UPLOAD_SIZE; | |
| } |
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
1 participant
Issue
When no malware scanner binding was present, the default maximum attachment upload size was
Long.MAX_VALUE(UNLIMITED_SIZE, documented as "effectively unlimited"). A deployment without a scanner therefore silently allowed unbounded uploads, with the associated resource-exhaustion risk, unless the app happened to set@Validation.Maximum.Fix
The default maximum upload size is now a finite
400MB(DEFAULT_MAX_UPLOAD_SIZE) regardless of the scanner binding. Deployments that intentionally allow larger uploads opt in explicitly via the newcds.attachments.maxUploadSizeproperty, or override per entity with@Validation.Maximum.Resolution is centralized in
Registration.resolveDefaultMaxSize(explicit config wins, otherwise the finite default). The redundantUNLIMITED_SIZE/ scanner-dependent constants were consolidated into a singleDEFAULT_MAX_UPLOAD_SIZE.Tests
RegistrationTest: finite default when unconfigured or blank, and configured override honored.