Enforce rescan-on-download for all content read shapes#868
Enforce rescan-on-download for all content read shapes#868Schmarvinius wants to merge 3 commits into
Conversation
The stale-clean rescan-on-download check only ran for content-only reads (empty target keys). Keyed reads that stream content went through LazyProxyInputStream, which validated only the stored status and skipped the scannedAt freshness check, so a stale clean attachment could be downloaded through a keyed path without triggering a rescan. The status and freshness policy is now encapsulated in a ContentReadGuard that LazyProxyInputStream invokes before serving bytes, so every read shape enforces the same rescan-on-download and status validation. The eager content-only check is retained so those responses are still blocked before serialization.
Adds ReadAttachmentsHandler tests asserting that a keyed read of a stale-clean attachment triggers the rescan and blocks the download when the content is actually read, and that a keyed infected attachment is blocked on content read. Updates LazyProxyInputStreamTest for the new ContentReadGuard.
SummaryThe following content is AI-generated and provides a summary of the pull request: Enforce Rescan-on-Download for All Content Read ShapesBug Fix🐛 Fixed a security gap where the malware scan freshness check (rescan-on-download) was only enforced for content-only ( ChangesThe fix introduces a
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.
Summary
The PR correctly fixes the core security gap — keyed content reads now enforce the same rescan-on-download freshness policy as content-only reads. The design is clean and the ContentReadGuard abstraction is well-motivated.
Two issues were flagged:
-
Critical bug in
LazyProxyInputStream.getDelegate():contentReadGuard.verify()runs unconditionally on everyread()call rather than only before the delegate is first initialized. This causesenforceReadPolicy(and potentiallytransitionToScanning+scanAsync) to execute once per read chunk — which is both a correctness problem and a significant performance regression for large file downloads. The fix is to movecontentReadGuard.verify()inside thedelegate == nullguard. -
Missing test coverage: The
supplierOnlyCalledOncetest verifies the supplier is called once but doesn't assert the guard is also called only once, leaving the repeated-invocation bug undetected by the test suite.
PR Bot Information
Version: 1.28.2
- LLM:
anthropic--claude-4.6-sonnet - Agent Instructions:
- Correlation ID:
d9c531c0-84f1-11f1-9ab7-cac9cf9afaf1 - File Content Strategy: Full file content
- Event Trigger:
pull_request.opened
|
|
||
| private InputStream getDelegate() { | ||
| attachmentStatusValidator.verifyStatus(status); | ||
| contentReadGuard.verify(); |
There was a problem hiding this comment.
Bug: contentReadGuard.verify() is invoked on every read call, not just the first.
getDelegate() calls contentReadGuard.verify() unconditionally each time, while the delegate-initialization guard (delegate == null) is right below it. For a single content download this means enforceReadPolicy runs once per read()/read(byte[]) call, re-executing the staleness check, potentially calling transitionToScanning and scanAsync again on each chunk. The guard should only fire once — before the delegate is initialized.
| contentReadGuard.verify(); | |
| private InputStream getDelegate() { | |
| if (delegate == null) { | |
| contentReadGuard.verify(); | |
| logger.debug("Creating delegate input stream"); | |
| delegate = inputStreamSupplier.get(); | |
| } | |
| return delegate; | |
| } |
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
| * Enforces the status and freshness policy before content is served. | ||
| * | ||
| * @throws AttachmentStatusException if the content must not be served (e.g. not clean or a rescan | ||
| * has been triggered) |
There was a problem hiding this comment.
Best Practice: The Javadoc @throws tag references AttachmentStatusException without importing or qualifying it, so the cross-reference is broken in generated Javadoc. Use a fully-qualified name or add the import to make the link resolvable.
| * has been triggered) | |
| * @throws com.sap.cds.feature.attachments.handler.applicationservice.readhelper.AttachmentStatusException if the content must not be served (e.g. not clean or a rescan | |
| * has been triggered) |
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
| cut.read(); | ||
|
|
||
| verify(contentReadGuard).verify(); | ||
| verify(attachmentService).readAttachment(any()); |
There was a problem hiding this comment.
Bug: supplierOnlyCalledOnce does not verify that the guard is also invoked only once across two reads. With the current implementation contentReadGuard.verify() fires on every read() call (once per chunk), masking the repeated-guard-invocation bug. A verify(contentReadGuard, times(1)).verify() assertion should be added here.
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
The stale-clean rescan-on-download check in
ReadAttachmentsHandler.verifyStatusonly ran for content-only reads (empty target keys). Keyed reads that stream content went throughLazyProxyInputStream, which validated only the stored status (blockingINFECTED) but skipped thescannedAtfreshness check. A stale-clean attachment could therefore be downloaded via a keyed path without triggering a rescan.Fix
The status + freshness policy is encapsulated in a new
ContentReadGuardthatLazyProxyInputStreaminvokes right before serving bytes, so every read shape (content-only$valueand keyed content reads) enforces the same rescan-on-download and status validation.ReadAttachmentsHandlerbuilds a guard per attachment (enforceReadPolicy) and keeps the eager content-only check (verifyStatus) so those responses are still blocked before serialization.LazyProxyInputStreamnow takes aContentReadGuardinstead of a status string + validator.Metadata-only reads remain cheap: the guard only runs when content bytes are actually read.
Tests
ReadAttachmentsHandlerTest: keyed stale-clean attachment triggers rescan and blocks on content read; keyed infected attachment blocked on content read.LazyProxyInputStreamTest: updated forContentReadGuard(guard verified before serving, blocks on throw).