Skip to content

Fix dspace-customers 1083 finding 9: check READ on the item resolved from the handle in allzip and metadatabitstreams byHandle - #1557

Open
jr-rk wants to merge 1 commit into
dtq-devfrom
fix/f09-allzip-byhandle-read
Open

jr-rk wants to merge 1 commit into
dtq-devfrom
fix/f09-allzip-byhandle-read

Conversation

@jr-rk

@jr-rk jr-rk commented Oct 27, 2026 •

Copy link
Copy Markdown

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 from handleId. Any caller with READ on one item could pass the handle of another. The title of the other item went out in Content-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 a ClassCastException (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 @PreAuthorize on the uuid (allzip) or on nothing (byHandle) never covered the object that was actually used. The wrapper is converted with MetadataBitstreamWrapperConverter.convert directly, not through ConverterService.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. The AuthorizeException is wrapped in RESTAuthorizationException, as in ClaimedTaskRestRepository; a plain AuthorizeException thrown 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 without Content-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:

mvn -B -pl dspace-server-webapp,dspace -am -P-assembly verify -DskipIntegrationTests=false -Dtest=none -DfailIfNoTests=false -Dit.test=MetadataBitstreamControllerIT,MetadataBitstreamRestRepositoryIT

On a running instance, with an item A readable by anonymous users and a private item B:

  • GET /server/api/core/items/<uuid A>/allzip?handleId=<handle B>: before, 200 with B's title in Content-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 byHandle for a withdrawn item now answers 401/403 for non-admins, the same as the item's files.

Not in scope:

  • allzip still checks bitstream READ and the CLARIN licence only inside 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 in finally, raw entry names).
  • byHandle lists 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.getRefboxInfo resolves a handle and returns the title and citation of the item without a READ check (same defect pattern, other feature).
  • The 401/403 vs. 422 difference on byHandle follows 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?

  • No (This PR does NOT include AI-generated or AI-assisted content that requires disclosure.)
  • Yes (This PR includes AI-generated or AI-assisted content that requires disclosure.)

Claude Code wrote the patch and the tests. Both IT classes pass (16 tests, including the two new ones) and Checkstyle passes.

Checklist

  • My PR is created against the main branch of code (unless it is a backport or is fixing an issue specific to an older branch).
  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & integration tests). Exceptions may be made if previously agreed upon.
  • My PR follows all coding best practices based on the Code Conventions Guide.
  • My PR passes Checkstyle validation based on the Code Style Guide.
  • My PR includes Javadoc for all new (or modified) public methods and classes. It also includes Javadoc for large or complex private methods.
  • My PR passes all tests and includes new/updated Unit or Integration Tests based on the Code Testing Guide.
  • My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
  • If my PR includes new libraries/dependencies (in any pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR modifies REST API endpoints, I've opened a separate REST Contract PR related to this change.
  • If my PR includes new configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

Written on dtq-dev (f9955da5f2). The production code cherry-picks cleanly onto customer/lindat; MetadataBitstreamControllerIT conflicts there because lindat does not have the #1368 tests yet (finding R61).

…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.
@jr-rk jr-rk self-assigned this Oct 27, 2026
@jr-rk
jr-rk requested a lite review from Copilot October 27, 2026 14:43
@jr-rk
jr-rk marked this pull request as ready for review October 27, 2026 14:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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 allzip with HTTP 422 before setting download headers.
  • Adds item-level READ authorization to metadatabitstreams/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

@jr-rk
jr-rk requested a review from milanmajchrak October 27, 2026 16:10

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants