Security patches from vanilla DSpace 7.6.7 (CVE-2026-49830, CVE-2026-49831)#1340
Merged
Conversation
(cherry picked from commit 7ac17f6)
* Safer Velocity configuration * New "message.templates.allowed-config" config * Remove "UnmodifiableConfiguration" in favour of a simple Map of whitelisted Config keys/values * Centralise Velocity config in core Utils * Small javadoc changes (cherry picked from commit b2d6141) (cherry picked from commit 5b31db5)
(cherry picked from commit 655fc62)
(cherry picked from commit 8a2eee9)
(cherry picked from commit 22bec44)
Removes some JDK >= 16 usage (cherry picked from commit 55905a2)
(cherry picked from commit 4502224)
(cherry picked from commit 277af82)
(cherry picked from commit a757221)
|
Warning Review limit reached
More reviews will be available in 5 minutes and 14 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds four independent security controls: a ChangesHTTP Request Security Filter
Filesystem Access Security
Velocity Template Config Restriction
OREIngestionCrosswalk URI Validation
Sequence Diagram(s)sequenceDiagram
participant Client
participant GlobalRequestSecurityFilter
participant FilterChain
Client->>GlobalRequestSecurityFilter: HTTP request with raw URI
GlobalRequestSecurityFilter->>GlobalRequestSecurityFilter: normalizeAndDecodeUri
alt traversal pattern detected
GlobalRequestSecurityFilter-->>Client: HTTP 403 Forbidden
else JSP execution pattern detected
GlobalRequestSecurityFilter-->>Client: HTTP 403 Forbidden
else URI is clean
GlobalRequestSecurityFilter->>FilterChain: doFilter(request, response)
end
sequenceDiagram
participant CurationCLI as Curation CLI
participant SecureFileAccess
participant ConfigurationService
participant Filesystem
CurationCLI->>SecureFileAccess: calculateAbsolutePathUsingCwd(taskFile)
CurationCLI->>ConfigurationService: getPropertyValue("curate.taskfile.base")
CurationCLI->>SecureFileAccess: getBufferedReader(absolutePath, allowedBasePaths, UTF-8)
SecureFileAccess->>SecureFileAccess: validatePathForRead(file, allowedBasePaths)
SecureFileAccess->>Filesystem: toRealPath() + normalize() boundary check
alt path within allowed base
SecureFileAccess-->>CurationCLI: BufferedReader
else path outside allowed base
SecureFileAccess-->>CurationCLI: IOException
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
🧹 Nitpick comments (1)
dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java (1)
121-126: ⚡ Quick winConsider adding mixed literal/encoded dot pattern detection.
The
%2e%2echeck catches double-encoded traversal (%252e%252e), but mixed patterns like.%252edecode to.%2eafter a single pass—this evades the current checks since it's neither literal..nor%2e%2e.While downstream controllers provide defense-in-depth (e.g.,
SitemapRestControllermatches exact filenames from directory listing), strengthening the filter would add another protective layer.🛡️ Proposed enhancement
private boolean isTraversalAttempt(String url) { return url.contains("../") || url.contains("/..") || url.contains("%2e%2e") + || url.contains(".%2e") + || url.contains("%2e.") || url.contains(".."); }🤖 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/security/GlobalRequestSecurityFilter.java` around lines 121 - 126, The isTraversalAttempt method is missing detection for mixed literal and encoded dot patterns that can evade the current checks. Specifically, patterns like .%2e or %2e. (mixing literal dots with percent-encoded dots) can decode to .. after a single decoding pass, bypassing the existing checks for literal .. and encoded %2e%2e. Add additional checks to the return statement in isTraversalAttempt to detect these mixed encoding patterns by including checks for .%2e, %2e., and case-insensitive variants (.%2E, %2E., etc.) alongside the existing pattern checks.
🤖 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/content/crosswalk/OREIngestionCrosswalk.java`:
- Around line 289-311: Add null checks before dereferencing the result of
getHost() calls to prevent NullPointerException when URIs lack authority. In the
allowed prefix validation loop, check if both allowedUri.getHost() and
resourceUri.getHost() are non-null before calling toLowerCase() on them.
Similarly, in the fallback validation checks, verify that entryUri.getHost() and
resourceUri.getHost() are non-null before calling toLowerCase() on them. If
either host is null, handle it appropriately by either skipping that comparison
or returning false as needed for the validation logic.
In `@dspace-api/src/main/java/org/dspace/core/Email.java`:
- Around line 367-371: The email templates are unable to access the lr.help.mail
configuration value because it is not included in the allowlist for template
configuration. In the Email.java file where vctx.put("config",
Utils.getAllowedTemplateConfig()) is called, the getAllowedTemplateConfig method
only returns allowlisted keys. Add lr.help.mail to the allowlist by updating
either the DEFAULT_ALLOWED_TEMPLATE_CONFIGS constant in Utils.java or the
message.templates.allowed-config setting in dspace.cfg, so that templates in
clarin_download_link_admin, clarin_token, matomo_report, and share_submission
can properly resolve the help email address instead of receiving null values.
In `@dspace-api/src/main/java/org/dspace/core/Utils.java`:
- Around line 648-654: The Javadoc comment for the method creating default,
secure Velocity configuration properties contains two syntax errors. First, the
`@see` tag on line 650 incorrectly uses {`@link` Email} syntax within it; replace
`@see {`@link` Email}` with the correct format `@see Email` for a direct
reference, or move the link inline into the description text. Second, line 653
uses the invalid Javadoc tag `@returns`; replace it with the correct tag
`@return` which is the standard Javadoc syntax for describing return values.
In `@dspace-api/src/main/java/org/dspace/storage/secure/SecureFileAccess.java`:
- Around line 41-53: The validatePathForWrite method in SecureFileAccess.java
has a security vulnerability where it does not canonicalize the target path
itself before validation, unlike validatePathForRead. Currently, the method
normalizes the path lexically and checks if resolvedPath starts with basePath,
but this check can be bypassed if a parent directory is a symlink pointing
outside the allowed base. To fix this, apply toRealPath() to the resolvedPath
variable (similar to how basePath is converted to its real path) before
performing the startsWith validation check. This will ensure that any symlinks
in parent directories are resolved to their actual filesystem locations and
verified to be within the allowed base path before the write operation is
permitted.
In `@dspace/config/modules/oai.cfg`:
- Around line 163-165: The example values for
oai.harvester.ore.file.allowedUrlPrefix configuration property are missing the
URI scheme component. Update the two commented-out example lines (lines 164 and
165) to include the full URI with scheme prefix (such as https:// or http://)
before the domain names, so they read as complete URIs (e.g.,
https://dspace.myinstitution.edu and https://files.myinstitution.edu) rather
than just host names, to match what the validator expects at runtime.
---
Nitpick comments:
In
`@dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java`:
- Around line 121-126: The isTraversalAttempt method is missing detection for
mixed literal and encoded dot patterns that can evade the current checks.
Specifically, patterns like .%2e or %2e. (mixing literal dots with
percent-encoded dots) can decode to .. after a single decoding pass, bypassing
the existing checks for literal .. and encoded %2e%2e. Add additional checks to
the return statement in isTraversalAttempt to detect these mixed encoding
patterns by including checks for .%2e, %2e., and case-insensitive variants
(.%2E, %2E., etc.) alongside the existing pattern checks.
🪄 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: 52b48de3-ccaf-46f1-bd56-7aa0cc422530
📒 Files selected for processing (13)
dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.javadspace-api/src/main/java/org/dspace/core/Email.javadspace-api/src/main/java/org/dspace/core/Utils.javadspace-api/src/main/java/org/dspace/curate/Curation.javadspace-api/src/main/java/org/dspace/curate/CurationCliScriptConfiguration.javadspace-api/src/main/java/org/dspace/curate/CurationClientOptions.javadspace-api/src/main/java/org/dspace/storage/secure/SecureFileAccess.javadspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.javadspace-server-webapp/src/test/java/org/dspace/app/rest/SitemapRestControllerIT.javadspace-server-webapp/src/test/java/org/dspace/curate/CurationScriptIT.javadspace/config/dspace.cfgdspace/config/modules/curate.cfgdspace/config/modules/oai.cfg
milanmajchrak
changed the title
There was a problem hiding this comment.
Pull request overview
Backports vanilla DSpace 7.6.7 security hardening into this fork (DSpace 7.6.5 base), focusing on ORE aggregated-resource URI validation, curation task/report file path traversal defenses, and tighter Velocity template configuration exposure / execution restrictions.
Changes:
- Added ORE aggregated-resource URI validation controls/config and implemented validation in
OREIngestionCrosswalk. - Introduced
SecureFileAccessand integrated it into curation taskfile/reporter handling; moved-r/-Tto CLI-only options. - Hardened Velocity usage for Email/templates via restricted config exposure and secure Velocity properties; added a global request hardening filter and updated related ITs.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| dspace/config/modules/oai.cfg | Adds new (commented) ORE URL validation/allowlist config keys. |
| dspace/config/modules/curate.cfg | Documents new base-path containment config for taskfiles and reporter outputs. |
| dspace/config/dspace.cfg | Adds allowlist config for template-exposed configuration and optional strict Velocity mode. |
| dspace-server-webapp/src/test/java/org/dspace/curate/CurationScriptIT.java | Disables two -T taskfile-related integration tests via @Ignore. |
| dspace-server-webapp/src/test/java/org/dspace/app/rest/SitemapRestControllerIT.java | Updates traversal tests to expect 403 Forbidden instead of servlet exception. |
| dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java | New global servlet filter to block traversal and JSP execution attempts. |
| dspace-api/src/main/java/org/dspace/storage/secure/SecureFileAccess.java | New utility for base-path validation and safe stream/reader creation. |
| dspace-api/src/main/java/org/dspace/curate/CurationCliScriptConfiguration.java | Adds CLI-only -r/--reporter and -T/--taskfile options. |
| dspace-api/src/main/java/org/dspace/curate/CurationClientOptions.java | Removes -r/-T from options available via REST/scripts endpoints; clarifies behavior in Javadoc. |
| dspace-api/src/main/java/org/dspace/curate/Curation.java | Validates taskfile and reporter paths against configured base dirs using SecureFileAccess. |
| dspace-api/src/main/java/org/dspace/core/Utils.java | Adds secure Velocity properties builder and a restricted template config map builder. |
| dspace-api/src/main/java/org/dspace/core/Email.java | Switches to secure Velocity properties and restricts template-accessible configuration. |
| dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.java | Adds ORE aggregated resource URI validation and related configuration usage. |
The 7.6.7 Velocity hardening restricts templates to an allowlisted
"config" map (Utils.getAllowedTemplateConfig). UFAL/CLARIN templates
(clarin_download_link_admin, clarin_token, matomo_report,
share_submission) reference config.get('lr.help.mail'), which vanilla
DSpace does not ship, so it was missing from the allowlist and those
emails would render a null help address. Add it to dspace.cfg only;
Utils.java stays identical to upstream.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Security patches for
dtq-dev(DSpace 7.6.5)Backports the vanilla DSpace 7.6.7 security set onto
dtq-devRef: dspace-community security announcement
Why this is needed
Without it, the base keeps re-seeding the vulnerable code into future rebases. Of the 4 advisories in the announcement, two apply to 7.x (≤7.6.6, fixed in 7.6.7); the Velocity RCE and LDN path-traversal advisories are 8.x/9.x-only but their hardening is included here as defense-in-depth (same scope as the customer 7.6.7 PRs).
Fixes included
SecureFileAccess, taskfile/reporter base-path containment,-r/-Tmoved to CLI-only)Utils.getSecureVelocityProperties, allowed-config map, removal ofUnmodifiableConfigurationService)GlobalRequestSecurityFilter(JSP/traversal request filtering, backported for javax)Conflict resolution (vs. the clean customer cherry-picks)
Two files needed manual resolution because
dtq-dev(UFAL) diverges from vanilla:Email.java— dropped the now-unusedjava.util.Propertiesimport (UFAL kept it), retainedCollectors(still used).Curation.java— kept UFAL's reporter abstraction (DoNothingReporter/SystemOutReporter/FilePrinterReporter) instead of vanilla's rawOutputStream. The reporter path is now validated viaSecureFileAccess.validatePathForWrite(...)againstcurate.reporter.basebefore opening — same security guarantee, preserves UFAL output formatting.Verification
mvn -pl dspace-api -am compilepasses locally.message.templates.allowed-config,curate.taskfile.base,curate.reporter.base,oai.harvester.ore.file.*.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
-r/--reporter(for output destination) and-T/--taskfile(for task specifications) options.Security Improvements