UFAL/fix: RFC 5987 Content-Disposition for single-file + allzip download (backport vanilla #11260, port #1267) - #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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| // URLEncoder is form-encoding, which differs from RFC 5987 on exactly one character | ||
| return URLEncoder.encode(filename, StandardCharsets.UTF_8).replace("+", "%20"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is asterisk excluded from attr-char in RFC 5987?
💡 Result:
Yes, the asterisk character () is explicitly excluded from attr-char in RFC 5987 [1][2][3]. The grammar for attr-char is defined as: attr-char = ALPHA / DIGIT / "!" / "#" / "$" / "&" / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"; token except ( "" / "'" / "%") As shown in the comment accompanying the definition, attr-char consists of allowed characters derived from the token production, specifically excluding the asterisk (*), single quote ('), and percent (%) characters [1][2][3].
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc5987.txt
- 2: https://www.ietf.org/rfc/rfc5987
- 3: https://hjp.at/(de)/doc/rfc/rfc5987.html
🏁 Script executed:
sed -n '1,140p' dspace-api/src/main/java/org/dspace/util/ContentDispositionUtils.javaRepository: dataquest-dev/DSpace
Length of output: 3279
Percent-encode * in rfc5987Encode(). URLEncoder leaves * unescaped, but RFC 5987 filename* values require it to be percent-encoded. Add .replace("*", "%2A") alongside the existing space handling.
🤖 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/main/java/org/dspace/util/ContentDispositionUtils.java` around
lines 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.
| /** | ||
| * Creates a safe ASCII-only fallback filename by removing diacritics (accents) | ||
| * and replacing any remaining non-ASCII characters. | ||
| * E.g., "ä-ö-é.pdf" becomes "a-o-e.pdf". | ||
| * @param originalFilename The original filename. | ||
| * @return A string containing only ASCII characters. | ||
| */ | ||
| private String createFallbackAsciiName(String originalFilename) { | ||
| if (originalFilename == null) { | ||
| return ""; | ||
| } | ||
| String normalized = Normalizer.normalize(originalFilename, Normalizer.Form.NFD); | ||
| String withoutAccents = normalized.replaceAll("\\p{InCombiningDiacriticalMarks}+", ""); | ||
| return withoutAccents.replaceAll("[^\\x00-\\x7F]", ""); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Escape double quotes and strip control characters in the ASCII fallback.
While this file was intentionally left unchanged relative to vanilla DSpace to minimize merge conflicts, the duplicated fallback logic contains a critical bug: it fails to escape double quotes (") and backslashes (\), and does not strip ASCII control characters (like \r and \n).
If a filename contains a double quote, the filename="%s" format in initialiseHeaders will produce a prematurely closed quoted-string (e.g., filename="file"name.txt"). This results in an invalid Content-Disposition header, causing browsers to reject the download with ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_DISPOSITION. This is the exact scenario successfully fixed and tested by ContentDispositionUtils elsewhere in this PR. Furthermore, unstripped control characters can theoretically lead to HTTP response splitting.
To avoid duplicating security-sensitive encoding logic and retaining this bug, consider extracting a ContentDispositionUtils.format(disposition, filename) method in dspace-api and reusing it here. If you must keep the implementation entirely localized to this file, apply the necessary escaping and strip control characters:
🐛 Proposed fix for local escaping and control-character removal
private String createFallbackAsciiName(String originalFilename) {
if (originalFilename == null) {
return "";
}
String normalized = Normalizer.normalize(originalFilename, Normalizer.Form.NFD);
String withoutAccents = normalized.replaceAll("\\p{InCombiningDiacriticalMarks}+", "");
- return withoutAccents.replaceAll("[^\\x00-\\x7F]", "");
+ // Only keep printable ASCII characters (strips control chars like \r, \n)
+ String asciiOnly = withoutAccents.replaceAll("[^\\x20-\\x7E]", "");
+ // Escape backslashes and double quotes for the quoted-string
+ return asciiOnly.replace("\\", "\\\\").replace("\"", "\\\"");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Creates a safe ASCII-only fallback filename by removing diacritics (accents) | |
| * and replacing any remaining non-ASCII characters. | |
| * E.g., "ä-ö-é.pdf" becomes "a-o-e.pdf". | |
| * @param originalFilename The original filename. | |
| * @return A string containing only ASCII characters. | |
| */ | |
| private String createFallbackAsciiName(String originalFilename) { | |
| if (originalFilename == null) { | |
| return ""; | |
| } | |
| String normalized = Normalizer.normalize(originalFilename, Normalizer.Form.NFD); | |
| String withoutAccents = normalized.replaceAll("\\p{InCombiningDiacriticalMarks}+", ""); | |
| return withoutAccents.replaceAll("[^\\x00-\\x7F]", ""); | |
| } | |
| /** | |
| * Creates a safe ASCII-only fallback filename by removing diacritics (accents) | |
| * and replacing any remaining non-ASCII characters. | |
| * E.g., "ä-ö-é.pdf" becomes "a-o-e.pdf". | |
| * `@param` originalFilename The original filename. | |
| * `@return` A string containing only ASCII characters. | |
| */ | |
| private String createFallbackAsciiName(String originalFilename) { | |
| if (originalFilename == null) { | |
| return ""; | |
| } | |
| String normalized = Normalizer.normalize(originalFilename, Normalizer.Form.NFD); | |
| String withoutAccents = normalized.replaceAll("\\p{InCombiningDiacriticalMarks}+", ""); | |
| // Only keep printable ASCII characters (strips control chars like \r, \n) | |
| String asciiOnly = withoutAccents.replaceAll("[^\\x20-\\x7E]", ""); | |
| // Escape backslashes and double quotes for the quoted-string | |
| return asciiOnly.replace("\\", "\\\\").replace("\"", "\\\""); | |
| } |
🤖 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/utils/HttpHeadersInitializer.java`
around lines 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.
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*. |
| private String createFallbackAsciiName(String originalFilename) { | ||
| if (originalFilename == null) { | ||
| return ""; | ||
| } | ||
| String normalized = Normalizer.normalize(originalFilename, Normalizer.Form.NFD); | ||
| String withoutAccents = normalized.replaceAll("\\p{InCombiningDiacriticalMarks}+", ""); | ||
| return withoutAccents.replaceAll("[^\\x00-\\x7F]", ""); | ||
| } |
| String expectedUtf8Encoded = | ||
| "%E0%B8%A0%E0%B8%B2%E0%B8%A9%E0%B8%B2%E0%B9%84%E0%B8%97%E0%B8%A2-" | ||
| + "com-acentua%C3%A7%C3%A3o.pdf"; |
| name = item.getName() + ".zip"; | ||
| response.setHeader(HttpHeaders.CONTENT_DISPOSITION, String.format("attachment;filename=\"%s\"", name)); | ||
| response.setHeader(HttpHeaders.CONTENT_DISPOSITION, ContentDispositionUtils.attachment(name)); | ||
| response.setContentType("application/zip"); | ||
| List<Bundle> bundles = item.getBundles("ORIGINAL"); | ||
|
|
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
| private String createFallbackAsciiName(String originalFilename) { | ||
| if (originalFilename == null) { | ||
| return ""; | ||
| } | ||
| String normalized = Normalizer.normalize(originalFilename, Normalizer.Form.NFD); | ||
| String withoutAccents = normalized.replaceAll("\\p{InCombiningDiacriticalMarks}+", ""); | ||
| // Deviates from vanilla by escaping \ and ": the value is a quoted-string, and a name | ||
| // containing a quote closes it early. That is the bug #1267 fixed; vanilla still has it. | ||
| return withoutAccents.replaceAll("[^\\x00-\\x7F]", "") | ||
| .replace("\\", "\\\\") | ||
| .replace("\"", "\\\""); | ||
| return String.format("attachment; filename=\"%s\"; filename*=UTF-8''%s", | ||
| asciiFallback, encoded); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="dspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamByHandleRestController.java"
# Map the relevant structure first, then inspect the target range.
ast-grep outline "$file" --view expanded || true
echo
sed -n '260,340p' "$file" | cat -nRepository: dataquest-dev/DSpace
Length of output: 5175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect surrounding code that uses the fallback name and header setting.
rg -n -A4 -B4 "createFallbackAsciiName|setHeader|Content-Disposition|filename\*" \
dspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamByHandleRestController.javaRepository: dataquest-dev/DSpace
Length of output: 3119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant method and the call site context.
sed -n '290,340p' dspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamByHandleRestController.java | cat -nRepository: dataquest-dev/DSpace
Length of output: 2836
Strip ASCII control characters from the fallback filename
[^\x00-\x7F] still lets CR, LF, NUL, and the other C0 controls through, so a bitstream name can reach response.setHeader(...) inside a quoted filename. Strip [\x00-\x1F\x7F] here too.
🤖 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 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.
| private String createFallbackAsciiName(String originalFilename) { | ||
| if (originalFilename == null) { | ||
| return ""; | ||
| } | ||
| String normalized = Normalizer.normalize(originalFilename, Normalizer.Form.NFD); | ||
| String withoutAccents = normalized.replaceAll("\\p{InCombiningDiacriticalMarks}+", ""); | ||
| // Deviates from vanilla by escaping \ and ": the value is a quoted-string, and an item name | ||
| // containing a quote closes it early. That is the bug #1267 fixed; vanilla still has it. | ||
| return withoutAccents.replaceAll("[^\\x00-\\x7F]", "") | ||
| .replace("\\", "\\\\") | ||
| .replace("\"", "\\\""); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the controller implementation and the related controller mentioned in the comment.
git ls-files 'dspace-server-webapp/src/main/java/org/dspace/app/rest/*.java' | sort
printf '\n--- MetadataBitstreamController excerpt ---\n'
sed -n '140,210p' dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java
printf '\n--- BitstreamByHandleRestController excerpt ---\n'
sed -n '1,240p' dspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamByHandleRestController.javaRepository: dataquest-dev/DSpace
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the fallback name helper and any filename/header construction around it.
rg -n "createFallbackAsciiName|filename=|Content-Disposition|attachment|inline|diacritical|CombiningDiacriticalMarks" dspace-server-webapp/src/main/java/org/dspace/app/restRepository: dataquest-dev/DSpace
Length of output: 7197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the controller implementation and the related controller mentioned in the comment.
printf '\n--- MetadataBitstreamController excerpt ---\n'
sed -n '140,210p' dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java
printf '\n--- BitstreamByHandleRestController excerpt ---\n'
sed -n '1,240p' dspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamByHandleRestController.javaRepository: dataquest-dev/DSpace
Length of output: 13170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '150,220p' dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.javaRepository: dataquest-dev/DSpace
Length of output: 2316
🌐 Web query:
Spring Framework ContentDisposition filename escaping control characters CR LF quoted-string sanitize header value
💡 Result:
In the Spring Framework, the ContentDisposition class is designed to help construct secure Content-Disposition header values, specifically addressing risks like Reflected File Download (RFD) and header injection [1][2][3][4]. Regarding the handling of special characters, CR (carriage return), and LF (line feed) in filenames: 1. Automatic Escaping: The ContentDisposition.Builder handles the formatting of filenames as quoted-strings according to RFC 2616 [5][6]. It automatically escapes quote characters within the filename by prefixing them with a backslash (e.g., "foo"bar.txt" becomes "foo\"bar.txt") [6][7][8]. 2. Control Character Handling: Spring Framework versions have received multiple updates (including CVE-2020-5398 and CVE-2025-41234) to mitigate security vulnerabilities related to header injection and RFD [1][2][3][4]. Applications should not rely solely on the library to handle malicious user-supplied input; if a filename is derived from user input, it must be validated and sanitized by the application before being passed to the ContentDisposition.Builder [3]. 3. Security Best Practices: Recent vulnerabilities, such as CVE-2025-41234, highlight that if an application allows user-supplied input to influence the filename, it must sanitize that input [3]. The framework's ability to protect against injection is limited if the application passes unsanitized, malicious control characters or entire header structures directly into the builder [3]. 4. Recommended Usage: Use the provided ContentDisposition.builder() API rather than manually concatenating strings into headers [5][3]. When setting filenames, prefer the builder methods, and ensure that any input originating from an untrusted source is strictly validated for forbidden characters (such as CRLF) before it reaches the ContentDisposition object [3]. For developers, the critical defense against HTTP Response Splitting and RFD is to ensure that user input is never blindly trusted. If you are using an older version of the Spring Framework, you should upgrade to a version where these security issues have been patched [3].
Citations:
- 1: https://spring.io/security/cve-2025-41234/
- 2: https://spring.io/security/cve-2020-5398/
- 3: https://nvd.nist.gov/vuln/detail/CVE-2025-41234
- 4: https://securitylab.github.com/research/rfd-spring-mvc-CVE-2020-5398/
- 5: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/http/ContentDisposition.Builder.html
- 6: https://github.com/spring-projects/spring-framework/blob/b8a4961b/spring-web/src/main/java/org/springframework/http/ContentDisposition.java
- 7: https://github.com/spring-projects/spring-framework/blob/4cc91e46b210b4e4e7ed182f93994511391b54ed/spring-web/src/main/java/org/springframework/http/ContentDisposition.java
- 8: https://docs.spring.io/spring-framework/docs/5.3.22/javadoc-api/org/springframework/http/ContentDisposition.Builder.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the matching helper in BitstreamByHandleRestController and the shared utility version.
sed -n '280,320p' dspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamByHandleRestController.java
printf '\n--- HttpHeadersInitializer excerpt ---\n'
sed -n '270,320p' dspace-server-webapp/src/main/java/org/dspace/app/rest/utils/HttpHeadersInitializer.javaRepository: dataquest-dev/DSpace
Length of output: 3966
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any reusable header-sanitization helpers and other Content-Disposition builders.
rg -n "Content-Disposition|createFallbackAsciiName|sanitize.*header|header value|CRLF|\\r|\\n" \
dspace-server-webapp/src/main/java/org/dspace/app/rest dspace-server-webapp/src/main/java/org/dspace/app/rest/utilsRepository: dataquest-dev/DSpace
Length of output: 350
🌐 Web query:
ServletResponse setHeader CR LF illegal characters header value control characters Content-Disposition injection
💡 Result:
The use of HttpServletResponse.setHeader (and similar methods like addHeader) with unsanitized user input poses a critical security risk known as HTTP Response Splitting, or CRLF injection (CWE-113) [1][2][3]. CRLF Injection Risks Because HTTP headers are delimited by Carriage Return (CR, \r or %0d) and Line Feed (LF, \n or %0a) sequences, injecting these characters allows an attacker to terminate a legitimate header prematurely and inject arbitrary new headers or even a completely new response body [2][4][5]. When applied to the Content-Disposition header, attackers can exploit this to perform Reflected File Download (RFD) attacks, cross-site scripting (XSS), or cache poisoning [2][6][7][8]. Standard Prevention and Mitigation 1. Mandatory Sanitization: Before passing any user-controlled data to setHeader, you must strip CR and LF characters [1][2][4]. A common pattern is to use a regular expression to remove them [1][6][8]: value = value.replaceAll("[\r\n]", ""); 2. Comprehensive Control Character Filtering: Modern best practices suggest filtering all control characters (U+0000 to U+001F and U+007F), not just CR and LF, to prevent other injection or parsing anomalies [6][8]. value = value.replaceAll("[\r\n\u0000-\u001F\u007F]", ""); 3. Proper Content-Disposition Encoding: To specifically defend Content-Disposition injection, merely sanitizing CR/LF is often insufficient. You must also properly quote the filename and, where possible, use the RFC 6266 filename* parameter to support non-ASCII characters and avoid parsing ambiguity [9][8]. Ensure that double quotes within filenames are escaped or removed to prevent an attacker from closing the filename attribute prematurely [7][8]. 4. Leverage Framework Protections: While modern servlet containers (e.g., Tomcat 9+, Jetty 10+) have improved by rejecting raw CRLF characters in header values by default, you should never rely on the container alone [1][4]. URL-encoded variants (%0d%0a) may be decoded by intermediate proxies or browsers before reaching the container, making application-layer sanitization essential [1]. Always prioritize using built-in framework utilities for header creation (e.g., Spring's ContentDisposition builder) over manual string concatenation [7][4].
Citations:
- 1: https://docs.cli.vulnetix.com/docs/sast-rules/vnx-java-017/
- 2: https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/07-Input_Validation_Testing/15-Testing_for_HTTP_Response_Splitting
- 3: https://cwe.mitre.org/data/definitions/113.html
- 4: https://offensive360.com/knowledge-base/http-response-splitting/
- 5: https://cwe.mitre.org/data/definitions/113
- 6: https://turingsecure.com/vulnerability-database/CWE-644/
- 7: https://securitylab.github.com/research/rfd-spring-mvc-CVE-2020-5398/
- 8: HTTP response splitting in fieldnotereportdetail.jsp — residentName into Content-Disposition carlos-emr/carlos#1694
- 9: proposal/new requirement - served filename in content-disposition header must follow correct encoding OWASP/ASVS#1390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the shared header-building utility and any explicit sanitization logic nearby.
sed -n '150,220p' dspace-server-webapp/src/main/java/org/dspace/app/rest/utils/HttpHeadersInitializer.java
printf '\n--- nearby helpers ---\n'
sed -n '220,330p' dspace-server-webapp/src/main/java/org/dspace/app/rest/utils/HttpHeadersInitializer.javaRepository: dataquest-dev/DSpace
Length of output: 7409
Strip control characters from the fallback filename
Same gap as BitstreamByHandleRestController: createFallbackAsciiName() only escapes \ and ", so \r, \n, and other control chars can still reach filename="...". Strip them before building the header.
🤖 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 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.
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>
Downloading a file whose name has diacritics gives the wrong filename. There were two separate causes; this fixes both on
dtq-dev. Everything here is propagated from vanilla — no new fork API is introduced.Before / after, on a real DSpace
Two backend images built from source and run against the same Postgres, same item, same URL — only the image is swapped:
01abd129d3fcorigin/dtq-dev@00501a2db00e023581d69e1e1b1fea5fItem title and bitstream name are both
Příliš žluťoučký kůň. Headers are captured off the wire; filenames are whatever the client itself chose (Chrome via Playwright, andcurl -OJ). Raw JSON captures are on thedemo-evidence-cdbranch.Read the middle block before the top one. The single-file path shows no difference in Chrome, because Chrome and Firefox decode RFC 2047 even though RFC 6266 App. C.1 forbids it. That row is in the screenshot precisely because it does not show a win.
1. allzip — the path the report was actually about
MetadataBitstreamControllerbuiltattachment;filename="<raw name>"with no encoding at all. Raw UTF-8 in a header is not merely mangled — Tomcat drops the header entirely, so the response carries noContent-Dispositionand Chrome falls back to the URL segment and savesallzip.zip. The name is lost outright. #1267 fixed this but landed only oncustomer/zcu-data; this ports it.Item names containing a
"were the other half of #1267: an unescaped quote closes the header's quoted-string early (ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_DISPOSITION).2. Single file — cherry-pick from vanilla
HttpHeadersInitializerencoded the name withMimeUtility.encodeText, i.e. an RFC 2047 encoded-word (=?UTF-8?Q?...?=) — an email header format that RFC 6266 App. C.1 does not allow in HTTP. Chrome and Firefox decode it anyway; Safari andcurl -OJdo not, and since the encoded-word ends in?=the real extension ends up buried mid-name. That is why this bug looked intermittent.Clean
cherry-pick -xoffe4077acee(#11269, thedspace-7_xbackport of #11260, released in 7.6.6). The file is byte-identical to vanilla before and after, so the upstream sync stays conflict-free. #1340 put us nominally on 7.6.7 but cherry-picked only the CVE commits, so this never came along."breaks the header even on a clean 7.6.6. Not fixed here on purpose; it belongs upstream.3. Why the logic is copied, not shared
MetadataBitstreamController(allzip) andBitstreamByHandleRestController(curl endpoint) have no counterpart upstream — they are fork-only classes. Each now carries its own private copy of vanilla'screateFallbackAsciiName/createEncodedUtf8Name, exactly as vanilla's ownHttpHeadersInitializerdoes.That is deliberate duplication. A shared helper would be a fork-invented API to maintain forever, and would force divergence in a file we want to keep identical to upstream.
BitstreamByHandleRestControllerwas not broken — it already emittedfilename*ondtq-dev. Only its ASCII fallback changes here (P__li_ _lu_ou_k_ k__→Prilis zlutoucky kun), for consistency with the other two paths.One deviation from vanilla — please check in review
Both fork copies escape
\and"in the ASCII fallback; vanilla does not. Copying vanilla verbatim would have regressed #1267, which exists because of real ZCU item names likeSupported data for manuscript "Thermally-induced evolution". Marked in a comment at both sites.Behaviour change to note in review
The fallback now transliterates instead of blanking out, so
BitstreamByHandleRestControllerITexpects"Media (3).jfif"where it expected"M_di_ (3).jfif". Only clients that ignorefilename*ever see this value.Verified
mvn compile/test-compileondspace-api+dspace-server-webapp— passmvn checkstyle:check— 0 violationsHttpHeadersInitializerconfirmed byte-identical to vanillafe4077aceeScope of the demo — what it does not show
file.encoding=UTF-8, which is the best case; on a non-UTF-8 JVM the name is destroyed before it leaves the server, and not even Chrome can recover itcurl -OJ"after" saves the ASCII fallback, not the diacritics name, because curl reads onlyfilenameand ignoresfilename*. Unreadable → readable is the win there, not perfectFollow-ups (not in this PR)
customer/zcu-pub), #1370 (customer/zcu-data); S3 in #1371dtq-dev-9-basestill has the old allzip line, so the v9 migration would reintroduce cause 1demo-evidence-cdbranch once this is merged🤖 Generated with Claude Code