UFAL/fix: RFC 5987 Content-Disposition for single-file + allzip download - #1368
Conversation
(cherry picked from commit fe4077a)
📝 WalkthroughWalkthroughContent-Disposition generation now includes an ASCII-safe ChangesContent-Disposition filename handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
dspace-api/src/test/java/org/dspace/util/ContentDispositionUtilsTest.java (1)
25-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate test to verify asterisk encoding.
To ensure the replacement logic for asterisks is validated alongside spaces, consider updating this test (or adding a new one) to include an asterisk.
♻️ Proposed refactor
`@Test` - public void spacesArePercentEncodedNotPluses() { - // URLEncoder would emit '+' here, which RFC 5987 reads as a literal plus sign - assertEquals("attachment; filename=\"my file.txt\"; filename*=UTF-8''my%20file.txt", - ContentDispositionUtils.attachment("my file.txt")); + public void spacesAndAsterisksAreCorrectlyEncoded() { + // URLEncoder emits '+' for space and leaves '*' unencoded (which violates RFC 5987 attr-char) + assertEquals("attachment; filename=\"my * file.txt\"; filename*=UTF-8''my%20%2A%20file.txt", + ContentDispositionUtils.attachment("my * file.txt")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dspace-api/src/test/java/org/dspace/util/ContentDispositionUtilsTest.java` around lines 25 - 30, Update spacesArePercentEncodedNotPluses in ContentDispositionUtilsTest to use a filename containing an asterisk alongside a space, and adjust the expected ContentDispositionUtils.attachment output to assert the asterisk is percent-encoded in filename* while the existing space encoding remains validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dspace-api/src/main/java/org/dspace/util/ContentDispositionUtils.java`:
- Around line 72-74: Update rfc5987Encode() to percent-encode asterisks by
adding "*” to "%2A" replacement alongside the existing space replacement, while
preserving the current UTF-8 URLEncoder behavior.
In
`@dspace-server-webapp/src/main/java/org/dspace/app/rest/utils/HttpHeadersInitializer.java`:
- Around line 273-287: Update createFallbackAsciiName and its use in
initialiseHeaders to reuse ContentDispositionUtils.format for
Content-Disposition filename encoding, or implement equivalent local handling
that escapes double quotes and backslashes and removes ASCII control characters
while preserving diacritic removal and non-ASCII filtering.
---
Nitpick comments:
In `@dspace-api/src/test/java/org/dspace/util/ContentDispositionUtilsTest.java`:
- Around line 25-30: Update spacesArePercentEncodedNotPluses in
ContentDispositionUtilsTest to use a filename containing an asterisk alongside a
space, and adjust the expected ContentDispositionUtils.attachment output to
assert the asterisk is percent-encoded in filename* while the existing space
encoding remains validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f78df7ea-e6b7-4140-8ef0-41595d3acb0c
📒 Files selected for processing (8)
dspace-api/src/main/java/org/dspace/util/ContentDispositionUtils.javadspace-api/src/test/java/org/dspace/util/ContentDispositionUtilsTest.javadspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamByHandleRestController.javadspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.javadspace-server-webapp/src/main/java/org/dspace/app/rest/utils/HttpHeadersInitializer.javadspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamByHandleRestControllerIT.javadspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamRestControllerIT.javadspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java
There was a problem hiding this comment.
Pull request overview
This PR fixes Content-Disposition filename handling for both single-bitstream downloads and the “allzip” download so that UTF-8 names (diacritics, non‑Latin scripts), spaces, and quotes arrive correctly in browsers by using RFC 5987 (filename*) with an ASCII filename fallback.
Changes:
- Update single-bitstream download headers to emit
filename+filename*(RFC 5987) instead of RFC 2047 encoded-words. - Update allzip download to use a shared
ContentDispositionUtils.attachment(...)builder and add/adjust integration tests for quotes + non-ASCII names. - Introduce
ContentDispositionUtils(+ unit tests) to centralize safe Content-Disposition construction (escaping and control-char stripping in fallback).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java | Adds IT assertions for allzip Content-Disposition with quotes and diacritics. |
| dspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamRestControllerIT.java | Updates single-bitstream filename expectations to RFC 5987 filename* + ASCII fallback. |
| dspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamByHandleRestControllerIT.java | Updates expected ASCII fallback behavior (transliteration) for by-handle download. |
| dspace-server-webapp/src/main/java/org/dspace/app/rest/utils/HttpHeadersInitializer.java | Switches single-bitstream download header generation away from RFC 2047 to RFC 5987-style parameters. |
| dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java | Uses ContentDispositionUtils for allzip attachment header generation. |
| dspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamByHandleRestController.java | Replaces local Content-Disposition builder with ContentDispositionUtils. |
| dspace-api/src/test/java/org/dspace/util/ContentDispositionUtilsTest.java | Adds unit tests for RFC 5987 encoding, ASCII fallback behavior, escaping, and control-char stripping. |
| dspace-api/src/main/java/org/dspace/util/ContentDispositionUtils.java | New utility to build safe Content-Disposition values with escaped ASCII fallback + RFC 5987 filename*. |
Ports the allzip fix from customer/zcu-data (#1267) to dtq-dev and aligns the fork's own endpoints with the encoding vanilla now uses. The allzip endpoint still built its header with a bare `attachment;filename="<name>"`, so item names with diacritics reached the browser mangled and names containing a double quote closed the quoted-string early (ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_DISPOSITION). MetadataBitstreamController and BitstreamByHandleRestController have no counterpart upstream, so each carries its own private copy of vanilla's createFallbackAsciiName / createEncodedUtf8Name rather than a shared fork utility. Copying keeps every endpoint tracking upstream behaviour and adds no fork-invented API to maintain. HttpHeadersInitializer stays byte-identical to vanilla and keeps its own copy for the same reason. One deliberate deviation from vanilla, marked in both copies: the ASCII fallback escapes \ and ". Vanilla omits this, so a name containing a quote closes the quoted-string early — exactly the bug #1267 was raised for. Because the fallback now transliterates instead of blanking out, BitstreamByHandleRestControllerIT expects "Media (3).jfif" where it used to expect "M_di_ (3).jfif". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fcdb270 to
1e1b1fe
Compare
Backport of the allzip half of #1368 (dtq-dev) to customer/zcu-pub. The allzip endpoint built its header as a bare `attachment;filename="<name>"`, so item names with diacritics arrived mangled and a double quote in a name closed the quoted-string early (ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_DISPOSITION). MetadataBitstreamController has no counterpart upstream, so it carries its own private copy of vanilla's createFallbackAsciiName / createEncodedUtf8Name rather than a shared fork utility — same as on dtq-dev. One deliberate deviation from vanilla, marked in the code: the ASCII fallback escapes \ and ". Vanilla omits this, so a name containing a quote closes the quoted-string early — exactly the bug #1267 was raised for. Differs from #1368 in one way: BitstreamByHandleRestController does not exist on this branch (the curl endpoint from #1252 was never backported here), so it is not touched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java (1)
156-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate helper trio vs.
BitstreamByHandleRestController.Identical to the trio in
BitstreamByHandleRestController.java(Lines 282-327). See the consolidated comment for the shared root cause and fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java` around lines 156 - 193, Remove the duplicate helper trio buildContentDisposition, createFallbackAsciiName, and createEncodedUtf8Name from MetadataBitstreamController, and reuse the shared implementation already provided by BitstreamByHandleRestController or the common utility introduced for both controllers. Preserve the existing Content-Disposition formatting and filename escaping/encoding behavior.dspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamByHandleRestController.java (1)
282-327: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
buildContentDisposition/createFallbackAsciiName/createEncodedUtf8Nametrio vs.MetadataBitstreamController.This exact helper set is copy-pasted in
MetadataBitstreamController.java(Lines 150-193). The PR objectives state a sharedContentDispositionUtilswas added todspace-apito consolidate this logic; both of these DSpace-custom endpoints (no upstream counterpart) could use it instead of independently duplicating it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamByHandleRestController.java` around lines 282 - 327, Replace the duplicated buildContentDisposition, createFallbackAsciiName, and createEncodedUtf8Name methods in BitstreamByHandleRestController with calls to the shared ContentDispositionUtils from dspace-api. Update MetadataBitstreamController to use the same utility, remove both local helper trios, and preserve the existing Content-Disposition output and escaping behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@dspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamByHandleRestController.java`:
- Around line 302-313: Update createFallbackAsciiName so the fallback filename
removes ASCII control characters, including C0 controls and DEL, before escaping
backslashes and quotes. Replace the current non-ASCII-only filtering in the
withoutAccents transformation while preserving accent removal and quoted-string
escaping.
In
`@dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java`:
- Around line 168-179: Update createFallbackAsciiName() to remove all control
characters from the normalized, accent-free filename before escaping backslashes
and quotes. Preserve the existing null handling, ASCII filtering, and
quoted-string escaping while ensuring characters such as carriage returns and
newlines cannot reach the filename header.
---
Nitpick comments:
In
`@dspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamByHandleRestController.java`:
- Around line 282-327: Replace the duplicated buildContentDisposition,
createFallbackAsciiName, and createEncodedUtf8Name methods in
BitstreamByHandleRestController with calls to the shared ContentDispositionUtils
from dspace-api. Update MetadataBitstreamController to use the same utility,
remove both local helper trios, and preserve the existing Content-Disposition
output and escaping behavior.
In
`@dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java`:
- Around line 156-193: Remove the duplicate helper trio buildContentDisposition,
createFallbackAsciiName, and createEncodedUtf8Name from
MetadataBitstreamController, and reuse the shared implementation already
provided by BitstreamByHandleRestController or the common utility introduced for
both controllers. Preserve the existing Content-Disposition formatting and
filename escaping/encoding behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8619919a-9b59-45df-b107-39d82b2cdc13
📒 Files selected for processing (4)
dspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamByHandleRestController.javadspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.javadspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamByHandleRestControllerIT.javadspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java
🚧 Files skipped from review as they are similar to previous changes (2)
- dspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamByHandleRestControllerIT.java
- dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java
Backport of the single-file half of #1368 (dtq-dev) to customer/zcu-data. allzip was already fixed here by #1267, but single-file download was not: HttpHeadersInitializer encoded the name with MimeUtility.encodeText, i.e. RFC 2047 encoded-words, which RFC 6266 Appendix C.1 forbids in HTTP. Safari shows the raw =?UTF-8?Q?...?= string; Chrome and Firefox decode it anyway, which is why this looked intermittent. Also aligns the private buildContentDisposition added by #1267 with vanilla's createFallbackAsciiName / createEncodedUtf8Name, so allzip and single-file render the same way and both track upstream. The fallback now transliterates rather than blanking out, so it reads "Prilis zlutoucky kun.zip" instead of "P__li_ _lu_ou_k_ k__.zip"; the IT from #1267 is updated accordingly. Only clients that ignore filename* ever see that value. The escaping of \ and " that #1267 added is kept, and marked in the code as a deliberate deviation — vanilla omits it and still has that bug. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…wnload (backport #1368) (#1369) * fix(DSpace#11191): Align Content-Disposition with RFC 5987/6266 (cherry picked from commit fe4077a) (cherry picked from commit f317911) * ZCU-PUB/fix: use RFC 5987 Content-Disposition for allzip Backport of the allzip half of #1368 (dtq-dev) to customer/zcu-pub. The allzip endpoint built its header as a bare `attachment;filename="<name>"`, so item names with diacritics arrived mangled and a double quote in a name closed the quoted-string early (ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_DISPOSITION). MetadataBitstreamController has no counterpart upstream, so it carries its own private copy of vanilla's createFallbackAsciiName / createEncodedUtf8Name rather than a shared fork utility — same as on dtq-dev. One deliberate deviation from vanilla, marked in the code: the ASCII fallback escapes \ and ". Vanilla omits this, so a name containing a quote closes the quoted-string early — exactly the bug #1267 was raised for. Differs from #1368 in one way: BitstreamByHandleRestController does not exist on this branch (the curl endpoint from #1252 was never backported here), so it is not touched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ZCU-PUB/fix: escape quoted-string + drop dead UTF-8 catch in HttpHeadersInitializer Address Copilot review on #1369: - createFallbackAsciiName now escapes \ and " so a filename containing a quote can't close the filename="..." quoted-string early (the ERR_RESPONSE_ HEADERS_MULTIPLE_CONTENT_DISPOSITION class of bug, #1267). Brings it to parity with the sibling method in MetadataBitstreamController. - createEncodedUtf8Name now uses URLEncoder.encode(String, Charset); UTF-8 is always supported, so the UnsupportedEncodingException catch was dead code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: JohnnyMendesC <177888064+JohnnyMendesC@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…backport #1368) (#1370) * fix(DSpace#11191): Align Content-Disposition with RFC 5987/6266 (cherry picked from commit fe4077a) (cherry picked from commit f317911) (cherry picked from commit 9b63181) * ZCU-DATA/fix: use RFC 5987 Content-Disposition for single-file download Backport of the single-file half of #1368 (dtq-dev) to customer/zcu-data. allzip was already fixed here by #1267, but single-file download was not: HttpHeadersInitializer encoded the name with MimeUtility.encodeText, i.e. RFC 2047 encoded-words, which RFC 6266 Appendix C.1 forbids in HTTP. Safari shows the raw =?UTF-8?Q?...?= string; Chrome and Firefox decode it anyway, which is why this looked intermittent. Also aligns the private buildContentDisposition added by #1267 with vanilla's createFallbackAsciiName / createEncodedUtf8Name, so allzip and single-file render the same way and both track upstream. The fallback now transliterates rather than blanking out, so it reads "Prilis zlutoucky kun.zip" instead of "P__li_ _lu_ou_k_ k__.zip"; the IT from #1267 is updated accordingly. Only clients that ignore filename* ever see that value. The escaping of \ and " that #1267 added is kept, and marked in the code as a deliberate deviation — vanilla omits it and still has that bug. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ZCU-DATA/fix: harden ASCII Content-Disposition fallback (Copilot review) Restrict createFallbackAsciiName to printable ASCII ([\x20-\x7E]) in both HttpHeadersInitializer (single-file) and MetadataBitstreamController (allzip), so control chars — notably CR/LF — can no longer reach the quoted-string filename= value and inject a header. HttpHeadersInitializer additionally now escapes \ and ", matching MetadataBitstreamController; previously the two paths were inconsistent. MetadataBitstreamController had regressed from [\x20-\x7E] to [\x00-\x7F] while aligning with vanilla; this restores the printable-only filter while keeping the NFD transliteration. Existing IT assertions are unaffected (all use printable names). Addresses Copilot review comments on #1370. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: JohnnyMendesC <177888064+JohnnyMendesC@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…duled-IT de-flake)
…la ASCII fallback The fallback transliterates (NFD + strip combining marks + drop non-ASCII), it does not substitute underscores. Two expectations in this file still assumed the old underscore output.
…on fallback A bitstream name containing a double quote closed the quoted-string early and produced an invalid Content-Disposition, which browsers reject. The allzip and by-handle paths in this branch already escape; this brings the single-file download path in line and adds an IT for it. This is a deliberate deviation from vanilla HttpHeadersInitializer, which still has the bug. Also fixes the continuation indent of an expected value in the same IT.
References
fe4077acee, thedspace-7_xbackport released in 7.6.6.customer/zcu-data(7b9966f1c9).Description
Downloading a file whose name has diacritics gives the wrong filename. Two independent causes, both fixed here:
MetadataBitstreamControllerbuiltattachment;filename="<raw name>"with no encoding. Raw UTF-8 in a header makes Tomcat drop the header entirely, so the browser falls back to the URL segment and savesallzip.zip. The name is lost outright.HttpHeadersInitializerusedMimeUtility.encodeText, i.e. an RFC 2047 encoded-word, which RFC 6266 App. C.1 forbids in HTTP. Chrome and Firefox decode it anyway, so this half is only visible in Safari andcurl -OJ.All three download paths — single file, allzip, by-handle — now emit an ASCII fallback in
filenamefor pre-RFC-5987 clients plus the real name percent-encoded infilename*.Two deliberate deviations from vanilla, both worth a look in review:
\and". Vanilla does not, so a name containing a quote closes the quoted-string early and the browser rejects the response withERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_DISPOSITION. Real ZCU items have such names.MetadataBitstreamControllerandBitstreamByHandleRestControllereach carry a private copy of the two helpers rather than sharing one. Both are fork-only classes with no upstream counterpart, so copying keeps them tracking vanilla and adds no fork API to maintain.Behaviour change: the fallback transliterates instead of blanking out, so
Médiá (3).jfifyieldsMedia (3).jfif, notM_di_ (3).jfif. Only clients that ignorefilename*ever see that value.Instructions for Reviewers
Two backend images built from source and run against the same Postgres, the same item and the same URL — only the image is swapped (
origin/dtq-dev@00501a2db0versus this branch @1e1b1fea5f). Item title and bitstream name are bothPříliš žluťoučký kůň; headers are captured off the wire and filenames are whatever the client chose. Raw captures live on thedemo-evidence-cdbranch, which can be deleted once this merges.Read the middle block before the top one: the single-file row shows no difference in Chrome, and it is in the screenshot precisely because it shows no win.
To reproduce by hand:
allzip. Before:allzip.zip. After: the item name."in the item name and downloadallzip. Before:ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_DISPOSITION.curl -OJthe single-file endpoint. Before: a name ending in?=with the extension buried mid-name. After: the ASCII fallback, since curl ignoresfilename*.What the demo does not cover: the S3 presigned-URL path, as there is no S3 in this compose. Pre-fix output also depends on the server's default charset — this container runs
file.encoding=UTF-8, which is the best case; on a non-UTF-8 JVM the name is destroyed before it leaves the server.Checklist
BitstreamRestControllerIT,MetadataBitstreamControllerIT,BitstreamByHandleRestControllerIT.Content-Dispositionheader on the bitstream content endpoints. The JSON representations are untouched, so there is no REST Contract change.