Repository navigation
Conversation
…from the handle
allzip checked READ on {uuid} but zipped the object resolved from handleId, so
the title of another item leaked in Content-Disposition and its files were
streamed whenever their own bitstream policies allowed it. The handle must now
identify the item {uuid}; any other handle, including an unknown one, gets the
same 422 before a header is set.
metadatabitstreams/search/byHandle had no READ check at all. It now requires
READ on the resolved item (401/403) before any preview content is generated.
There was a problem hiding this comment.
🟢 Approval recommended
The reviewed changes address the identified authorization gaps and include coverage for the relevant access-control scenarios.
No open issues 🎉
What changed in this PR
This pull request closes authorization gaps in handle-based bitstream endpoints by validating handle/item identity and enforcing item-level READ access before exposing metadata or generating previews.
Changes:
- Rejects mismatched, unknown, and non-item handles in
allzipwith HTTP 422 before setting download headers. - Adds item-level
READauthorization tometadatabitstreams/search/byHandle. - Adds integration coverage for authorization failures, preview protection, invalid handles, and valid requests.
| File | Description |
|---|---|
dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamRestRepositoryIT.java |
Tests item-level authorization and prevention of unauthorized preview generation. |
dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java |
Tests mismatched, unknown, private, and matching handles for ZIP downloads. |
dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/MetadataBitstreamRestRepository.java |
Authorizes READ access on the handle-resolved item before listing bitstreams or generating previews. |
dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java |
Validates that the resolved handle identifies the requested item before creating the ZIP response. |
Review effort: Lite
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
References
dspace-customers issue 1083, finding #9 (internal R05; overlaps point 5 of R41).
Description
Two CLARIN endpoints resolved an item from a handle without checking the caller's rights on that item:
GET/HEAD /api/core/items/{uuid}/allzip?handleId=…checked READ on{uuid}(@PreAuthorize), but built the zip from the object resolved fromhandleId. Any caller with READ on one item could pass the handle of another. The title of the other item went out inContent-Disposition, and its files were streamed whenever their own bitstream policies allowed it (by default bitstreams inherit the collection's anonymous READ, also when the item itself is private). A handle of a non-item object ended in aClassCastException(500).GET /api/core/metadatabitstreams/search/byHandle?handle=…had no READ check at all. Anyone could list the files of a private or workspace item (names, sizes, checksums, formats, descriptions, preview links) and could trigger preview generation for them.Root cause: the handle is resolved inside the method, so the
@PreAuthorizeon the uuid (allzip) or on nothing (byHandle) never covered the object that was actually used. The wrapper is converted withMetadataBitstreamWrapperConverter.convertdirectly, not throughConverterService.toRest, so the usual READ filter of the REST layer does not apply either.Instructions for Reviewers
List of changes in this PR:
MetadataBitstreamController.downloadFileZip: the handle must resolve to the item{uuid}; otherwise 422. An unknown handle, a handle of a non-item and a handle of another item all get the same 422 with the same wording, before any header is set, so the response does not reveal whether a handle exists. The separate "DSO is null" branch is folded into this check. READ on{uuid}is still checked by the existing@PreAuthorize, which now also covers the item that is zipped.MetadataBitstreamRestRepository.findByHandle:authorizeService.authorizeAction(context, item, READ)right after the handle is resolved, before any preview content is generated or stored. TheAuthorizeExceptionis wrapped inRESTAuthorizationException, as inClaimedTaskRestRepository; a plainAuthorizeExceptionthrown from a search method ends as 500. Anonymous callers get 401, authenticated callers without READ get 403 (DSpaceApiExceptionControllerAdvice), the same convention as/api/core/items/{uuid}.MetadataBitstreamControllerIT.downloadAllZipRefusesHandleOfAnotherItem: uuid of a public item + handle of a private item → 422 withoutContent-Disposition(anonymous and admin); unknown handle → the same 422; the reverse pair → 401; matching pairs still give 200 (anonymous on the public item, admin on the private one).MetadataBitstreamRestRepositoryIT.findByHandleRequiresReadOnItem: private item → 401 anonymous, 403 for a user without READ, no preview content created; admin still gets the file list.How to test:
On a running instance, with an item
Areadable by anonymous users and a private itemB:GET /server/api/core/items/<uuid A>/allzip?handleId=<handle B>: before, 200 with B's title inContent-Disposition; after, 422.GET /server/api/core/metadatabitstreams/search/byHandle?handle=<handle B>anonymously: before, 200 with B's files; after, 401.Behaviour change for legitimate users: none for the UI, which always sends the uuid and handle of the same item. A withdrawn item has its READ policies switched to WITHDRAWN_READ, so
byHandlefor a withdrawn item now answers 401/403 for non-admins, the same as the item's files.Not in scope:
bitstreamService.retrieve, after the zip headers are out (CLARIN-DSpace v9/Refuse a licensed allzip before the zip headers go out #1489 on v9; the dtq-dev port is a follow-up), and the rest of R41/R26 (no server-side size limit, HEAD streams everything and records a download, DB connection held during the stream, streams not closed infinally, raw entry names).byHandlelists every bitstream of a readable item, including ones the caller cannot read (e.g. embargoed or licence-gated files). The CLARIN item page relies on that to show licence-gated files; filtering per bitstream is a separate decision.ClarinRefBoxController.getRefboxInforesolves a handle and returns the title and citation of the item without a READ check (same defect pattern, other feature).byHandlefollows the DSpace convention; the existence oracle as such is findings feature/se-5-handleDate #12–feature/se-6-openaire #17.AI Disclosure (REQUIRED)
Were AI tools used in the creation of this PR?
Claude Code wrote the patch and the tests. Both IT classes pass (16 tests, including the two new ones) and Checkstyle passes.
Checklist
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.Written on
dtq-dev(f9955da5f2). The production code cherry-picks cleanly ontocustomer/lindat;MetadataBitstreamControllerITconflicts there because lindat does not have the #1368 tests yet (finding R61).