From 8432a64ed646c6972c93f39c6ef51f54f822dce4 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Sat, 16 May 2026 22:05:27 +0200 Subject: [PATCH 01/18] ORE aggregated resource URI validation (cherry picked from commit 7ac17f68432cff1e9463f05421fc8a29516000b8) --- .../crosswalk/OREIngestionCrosswalk.java | 84 +++++++++++++++++-- dspace/config/modules/oai.cfg | 7 ++ 2 files changed, 83 insertions(+), 8 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.java b/dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.java index f756aae22577..9e890a6046fa 100644 --- a/dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.java +++ b/dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.java @@ -11,6 +11,8 @@ import java.io.IOException; import java.io.InputStream; import java.net.ConnectException; +import java.net.URI; +import java.net.URISyntaxException; import java.net.URL; import java.sql.SQLException; import java.text.NumberFormat; @@ -18,6 +20,8 @@ import java.util.Date; import java.util.HashSet; import java.util.List; +import java.util.Locale; +import java.util.Objects; import java.util.Set; import org.apache.logging.log4j.Logger; @@ -34,6 +38,8 @@ import org.dspace.content.service.ItemService; import org.dspace.core.Constants; import org.dspace.core.Context; +import org.dspace.services.ConfigurationService; +import org.dspace.services.factory.DSpaceServicesFactory; import org.jdom2.Attribute; import org.jdom2.Document; import org.jdom2.Element; @@ -76,6 +82,7 @@ public class OREIngestionCrosswalk .getBitstreamFormatService(); protected BundleService bundleService = ContentServiceFactory.getInstance().getBundleService(); protected ItemService itemService = ContentServiceFactory.getInstance().getItemService(); + protected ConfigurationService configurationService = DSpaceServicesFactory.getInstance().getConfigurationService(); @Override @@ -173,9 +180,13 @@ public void ingest(Context context, DSpaceObject dso, Element root, boolean crea try { // Make sure the url string escapes all the oddball characters String processedURL = encodeForURL(href); - // Generate a requeset for the aggregated resource - ARurl = new URL(processedURL); - in = ARurl.openStream(); + if (validResourceUri(entryId, processedURL)) { + // Generate a request for the aggregated resource + ARurl = new URL(processedURL); + in = ARurl.openStream(); + } else { + throw new FileNotFoundException("Failed to validate " + processedURL); + } } catch (FileNotFoundException fe) { log.error("The provided URI failed to return a resource: " + href); } catch (ConnectException fe) { @@ -219,17 +230,17 @@ public void ingest(Context context, DSpaceObject dso, Element root, boolean crea * @param sourceString source unescaped string */ private String encodeForURL(String sourceString) { - Character lowalpha[] = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', + Character[] lowalpha = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z'}; - Character upalpha[] = {'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', + Character[] upalpha = {'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'}; - Character digit[] = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9'}; - Character mark[] = {'-', '_', '.', '!', '~', '*', '\'', '(', ')'}; + Character[] digit = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9'}; + Character[] mark = {'-', '_', '.', '!', '~', '*', '\'', '(', ')'}; // reserved - Character reserved[] = {';', '/', '?', ':', '@', '&', '=', '+', '$', ',', '%', '#'}; + Character[] reserved = {';', '/', '?', ':', '@', '&', '=', '+', '$', ',', '%', '#'}; Set URLcharsSet = new HashSet(); URLcharsSet.addAll(Arrays.asList(lowalpha)); @@ -251,4 +262,61 @@ private String encodeForURL(String sourceString) { return processedString.toString(); } + /** + * Validate a resource URI against the host and scheme of the remote OAI endpoint, or a configured + * list of allowed prefixes. + * This still implicitly "trusts" the remote OAI server, but will reject resource URIs with a totally + * different hostname to avoid downloading malicious resources from a compromised endpoint. + * Even if the URL prefix validation is disabled, schemes will still be enforced to http(s) so file:/// and + * other unwanted schemes cannot be used + * @param entryUrl the entryId of the parent ORE resource + * @param resourceUrl the resource URL of the aggregated ORE resource + * @return result of the validation + */ + private boolean validResourceUri(String entryUrl, String resourceUrl) { + try { + Set allowedSchemes = Set.of("http", "https"); + URI entryUri = new URI(entryUrl).normalize(); + URI resourceUri = new URI(resourceUrl).normalize(); + String scheme = resourceUri.getScheme(); + + if (scheme == null || + !allowedSchemes.contains(scheme.toLowerCase(Locale.ROOT))) { + log.warn("Illegal scheme requested for ORE resource: {}", resourceUri); + return false; + } + + if (configurationService.getBooleanProperty("oai.harvester.ore.file.validateUrlPrefix", false)) { + for (String allowedPrefix : configurationService + .getArrayProperty("oai.harvester.ore.file.allowedUrlPrefix")) { + URI allowedUri = new URI(allowedPrefix).normalize(); + // Return true on the first allowed prefix match + if (Objects.equals(resourceUri.getScheme(), allowedUri.getScheme()) + && Objects.equals(resourceUri.getHost().toLowerCase(Locale.ROOT), + allowedUri.getHost().toLowerCase(Locale.ROOT))) { + return true; + } + } + + // If no allowed prefixes were matched, we require scheme + host to match the remote OAI server + if (!Objects.equals(entryUri.getScheme(), resourceUri.getScheme())) { + log.warn("Illegal scheme requested for ORE resource: {}", resourceUri); + return false; + } + if (!Objects.equals( + entryUri.getHost().toLowerCase(Locale.ROOT), + resourceUri.getHost().toLowerCase(Locale.ROOT))) { + log.warn("Illegal host requested for ORE resource: {}", resourceUri); + return false; + } + } + + return true; + + } catch (URISyntaxException e) { + log.warn("Could not validate ORE resource URI: {}", resourceUrl); + return false; + } + } + } diff --git a/dspace/config/modules/oai.cfg b/dspace/config/modules/oai.cfg index b08addfda999..ca13335213f8 100644 --- a/dspace/config/modules/oai.cfg +++ b/dspace/config/modules/oai.cfg @@ -145,3 +145,10 @@ oai.harvester.unknownSchema = fail # when attempting to find the handle of harvested items. If there is a match with # this config parameter, a new handle will be minted instead. Default value: 123456789. #oai.harvester.rejectedHandlePrefix = 123456789, myTestHandle + +# If ingesting files with ORE, only files with URLs that match the base URL of the remote +# OAI endpoint's domain name are accepted, or a list of other URL prefixes defined below +#oai.harvester.ore.file.validateUrlPrefix = true +# Prefixes that are allowed globally (for any endpoint) are below +#oai.harvester.ore.file.allowedUrlPrefix = dspace.myinstitution.edu +#oai.harvester.ore.file.allowedUrlPrefix = files.myinstitution.edu From 61ff6df715c1da4b0401024e17a3e01d24be6e7f Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Tue, 5 May 2026 16:24:29 +0200 Subject: [PATCH 02/18] Velocity and template safety for Email and LDN messages * 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 b2d6141389f5652970b366325ed9deff21a86836) (cherry picked from commit 5b31db512f62bb530b71cb8fe85b1300f35e5601) --- .../src/main/java/org/dspace/core/Email.java | 83 ++++++------------- .../src/main/java/org/dspace/core/Utils.java | 65 +++++++++++++++ dspace/config/dspace.cfg | 12 +++ 3 files changed, 101 insertions(+), 59 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/core/Email.java b/dspace-api/src/main/java/org/dspace/core/Email.java index 98fcccae4c3c..0da9cb156694 100644 --- a/dspace-api/src/main/java/org/dspace/core/Email.java +++ b/dspace-api/src/main/java/org/dspace/core/Email.java @@ -22,7 +22,6 @@ import java.util.Date; import java.util.Enumeration; import java.util.List; -import java.util.Properties; import java.util.stream.Collectors; import javax.activation.DataHandler; import javax.activation.DataSource; @@ -45,12 +44,10 @@ import org.apache.logging.log4j.Logger; import org.apache.velocity.Template; import org.apache.velocity.VelocityContext; -import org.apache.velocity.app.Velocity; import org.apache.velocity.app.VelocityEngine; import org.apache.velocity.exception.MethodInvocationException; import org.apache.velocity.exception.ParseErrorException; import org.apache.velocity.exception.ResourceNotFoundException; -import org.apache.velocity.runtime.resource.loader.StringResourceLoader; import org.apache.velocity.runtime.resource.util.StringResourceRepository; import org.dspace.services.ConfigurationService; import org.dspace.services.factory.DSpaceServicesFactory; @@ -73,7 +70,7 @@ * Apache Velocity. They may contain VTL directives and property * placeholders. *

- * {@link addArgument(string)} adds a property to the {@code params} array + * {@link #addArgument(Object)} adds a property to the {@code params} array * in the Velocity context, which can be used to replace placeholder tokens * in the message. These arguments are indexed by number in the order they were * added to the message. @@ -81,9 +78,9 @@ * The DSpace configuration properties are also available to templates as the * array {@code config}, indexed by name. Example: {@code ${config.get('dspace.name')}} *

- * Recipients and attachments may be added as needed. See {@link addRecipient}, - * {@link addAttachment(File, String)}, and - * {@link addAttachment(InputStream, String, String)}. + * Recipients and attachments may be added as needed. See {@link #addRecipient}, + * {@link #addAttachment(File, String)}, and + * {@link #addAttachment(InputStream, String, String)}. *

* Headers such as Subject may be supplied by the template, by defining them * using the VTL directive {@code #set()}. Only headers named in the DSpace @@ -126,8 +123,8 @@ * *

* There are two ways to load a message body. One can create an instance of - * {@link Email} and call {@link setContent} on it, passing the body as a String. Or - * one can use the static factory method {@link getEmail} to load a file by its + * {@link Email} and call {@link #setContent} on it, passing the body as a String. Or + * one can use the static factory method {@link #getEmail} to load a file by its * complete filesystem path. In either case the text will be loaded into a * Velocity template. * @@ -173,18 +170,6 @@ public class Email { /** Velocity template settings. */ private static final String RESOURCE_REPOSITORY_NAME = "Email"; - private static final Properties VELOCITY_PROPERTIES = new Properties(); - static { - VELOCITY_PROPERTIES.put(Velocity.RESOURCE_LOADERS, "string"); - VELOCITY_PROPERTIES.put("resource.loader.string.description", - "Velocity StringResource loader"); - VELOCITY_PROPERTIES.put("resource.loader.string.class", - StringResourceLoader.class.getName()); - VELOCITY_PROPERTIES.put("resource.loader.string.repository.name", - RESOURCE_REPOSITORY_NAME); - VELOCITY_PROPERTIES.put("resource.loader.string.repository.static", - "false"); - } /** Velocity template for a message body */ private Template template; @@ -203,6 +188,13 @@ public Email() { charset = null; } + /** + * Get configuration service + */ + private static ConfigurationService getConfigurationService() { + return DSpaceServicesFactory.getInstance().getConfigurationService(); + } + /** * Add a recipient. * @@ -225,7 +217,7 @@ public void setContent(String name, String content) { arguments.clear(); VelocityEngine templateEngine = new VelocityEngine(); - templateEngine.init(VELOCITY_PROPERTIES); + templateEngine.init(Utils.getSecureVelocityProperties(RESOURCE_REPOSITORY_NAME)); StringResourceRepository repo = (StringResourceRepository) templateEngine.getApplicationAttribute(RESOURCE_REPOSITORY_NAME); @@ -255,7 +247,8 @@ public void setReplyTo(String email) { /** * Fill out the next argument in the template. * - * @param arg the value for the next argument + * @param arg the value for the next argument. If {@code null}, + * a zero-length string is substituted. */ public void addArgument(Object arg) { arguments.add(arg); @@ -333,7 +326,7 @@ public void reset() { * {@code mail.message.headers} then that name and its value will be added * to the message's headers. * - *

"subject" is treated specially: if {@link setSubject()} has not been + *

"subject" is treated specially: if {@link #setSubject} has not been * called, the value of any "subject" property will be used as if setSubject * had been called with that value. Thus a template may define its subject, * but the caller may override it. @@ -347,16 +340,13 @@ public void send() throws MessagingException, IOException { throw new MessagingException("Email has no body"); } - ConfigurationService config - = DSpaceServicesFactory.getInstance().getConfigurationService(); - // Get the mail configuration properties - String from = config.getProperty("mail.from.address"); - boolean disabled = config.getBooleanProperty("mail.server.disabled", false); + String from = getConfigurationService().getProperty("mail.from.address"); + boolean disabled = getConfigurationService().getBooleanProperty("mail.server.disabled", false); // If no character set specified, attempt to retrieve a default if (charset == null) { - charset = config.getProperty("mail.charset"); + charset = getConfigurationService().getProperty("mail.charset"); } // Get session @@ -371,11 +361,13 @@ public void send() throws MessagingException, IOException { new InternetAddress(recipient)); } // Get headers defined by the template. - String[] templateHeaders = config.getArrayProperty("mail.message.headers"); + String[] templateHeaders = getConfigurationService().getArrayProperty("mail.message.headers"); // Format the mail message body VelocityContext vctx = new VelocityContext(); - vctx.put("config", new UnmodifiableConfigurationService(config)); + // Pass a restricted (via configuration) list of resolved Configuration keys and values, for + // template lookup + vctx.put("config", Utils.getAllowedTemplateConfig()); vctx.put("params", Collections.unmodifiableList(arguments)); StringWriter writer = new StringWriter(); @@ -672,31 +664,4 @@ public OutputStream getOutputStream() throws IOException { throw new IOException("Cannot write to this read-only resource"); } } - - /** - * Wrap ConfigurationService to prevent templates from modifying - * the configuration. - */ - public static class UnmodifiableConfigurationService { - private final ConfigurationService configurationService; - - /** - * Swallow an instance of ConfigurationService. - * - * @param cs the real instance, to be wrapped. - */ - public UnmodifiableConfigurationService(ConfigurationService cs) { - configurationService = cs; - } - - /** - * Look up a key in the actual ConfigurationService. - * - * @param key to be looked up in the DSpace configuration. - * @return whatever value ConfigurationService associates with {@code key}. - */ - public String get(String key) { - return configurationService.getProperty(key); - } - } } diff --git a/dspace-api/src/main/java/org/dspace/core/Utils.java b/dspace-api/src/main/java/org/dspace/core/Utils.java index 047e0793713b..7e3661fef607 100644 --- a/dspace-api/src/main/java/org/dspace/core/Utils.java +++ b/dspace-api/src/main/java/org/dspace/core/Utils.java @@ -30,16 +30,22 @@ import java.util.Collections; import java.util.Date; import java.util.GregorianCalendar; +import java.util.List; +import java.util.Map; +import java.util.Properties; import java.util.Random; import java.util.StringTokenizer; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; import com.coverity.security.Escape; import org.apache.commons.lang3.StringUtils; import org.apache.commons.text.StringSubstitutor; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.velocity.app.Velocity; +import org.apache.velocity.runtime.resource.loader.StringResourceLoader; import org.dspace.services.ConfigurationService; import org.dspace.services.factory.DSpaceServicesFactory; import org.hibernate.Session; @@ -108,6 +114,14 @@ public final class Utils { private static final Calendar outCal = GregorianCalendar.getInstance(); + // Allowed configuration properties to pass to Velocity templates (Email, LDN) + private static final String[] DEFAULT_ALLOWED_TEMPLATE_CONFIGS = { + "dspace.name", "dspace.shortname", "dspace.ui.url", + "mail.helpdesk", "mail.message.helpdesk.telephone", "mail.admin", "mail.admin.name"}; + + private static final ConfigurationService configurationService = + DSpaceServicesFactory.getInstance().getConfigurationService(); + /** * Private constructor */ @@ -608,4 +622,55 @@ public static String fetchUUIDFromUrl(String urlString) { throw new IllegalArgumentException("Invalid URL or UUID format: " + e.getMessage(), e); } } + + /** + * Get a list of allowed DSpace configuration property keys that will be exposed to Velocity templates + * (used in Email and LDN messages) as a simple Map of strings. + * @return Map of strings representing resolved configuration properties + */ + public static Map getAllowedTemplateConfig() { + // Pass a restricted (via configuration) list of resolved Configuration keys and values, for + // template lookup + List allowedConfigurationKeys = List.of(configurationService.getArrayProperty( + "message.templates.allowed-config", DEFAULT_ALLOWED_TEMPLATE_CONFIGS)); + return allowedConfigurationKeys.stream() + .map(key -> Map.entry(key, configurationService.getProperty(key))) + .filter(entry -> entry.getValue() != null) + .collect(Collectors.toMap( + Map.Entry::getKey, + Map.Entry::getValue + )); + } + + /** + * Create and return a set of default, secure Velocity configuration properties. + * @see {@link Email} + * + * @param resourceRepositoryName the templating context e.g. "LDN", "Email" + * @returns secure Velocity configuration for use with templating + */ + public static Properties getSecureVelocityProperties(String resourceRepositoryName) { + Properties secureVelocityProperties = new Properties(); + // Basic Velocity configuration + secureVelocityProperties.setProperty(Velocity.RESOURCE_LOADERS, "string"); + secureVelocityProperties.setProperty("resource.loader.string.description", + "Velocity StringResource loader"); + secureVelocityProperties.setProperty("resource.loader.string.class", + StringResourceLoader.class.getName()); + secureVelocityProperties.setProperty("resource.loader.string.repository.name", + resourceRepositoryName); + secureVelocityProperties.setProperty("resource.loader.string.repository.static", + "false"); + // Set secure default introspection and class restriction handling in Velocity + secureVelocityProperties.setProperty("introspector.uberspect.class", + "org.apache.velocity.util.introspection.SecureUberspector"); + secureVelocityProperties.setProperty("introspector.restrict.classes", + "java.lang.Class,java.lang.Runtime,java.lang.System"); + secureVelocityProperties.setProperty( "introspector.restrict.packages", + "java.lang.reflect,java.io,java.nio"); + secureVelocityProperties.setProperty("runtime.strict_mode.enable", "true"); + + return secureVelocityProperties; + } + } diff --git a/dspace/config/dspace.cfg b/dspace/config/dspace.cfg index 1a45050e1c84..0860edb24153 100644 --- a/dspace/config/dspace.cfg +++ b/dspace/config/dspace.cfg @@ -162,6 +162,7 @@ mail.from.address = dspace-noreply@myu.edu # will use the above settings to create a Session. #mail.session.name = Session + # When feedback is submitted via the Feedback form, it is sent to this address # Currently limited to one recipient! # if this property is empty or commented out, feedback form is disabled @@ -228,6 +229,17 @@ mail.message.headers = charset # Helpdesk telephone. Not email, but should be with other contact info. Optional. #mail.message.helpdesk.telephone = +1 555 555 5555 +# Allowed configuration properties, to pass in a "config" map to email and LDN templates. +# This allows templates to easily access dynamic configuration properties, without +# exposing sensitive information to the templating engine +message.templates.allowed-config = dspace.name +message.templates.allowed-config = dspace.shortname +message.templates.allowed-config = dspace.ui.url +message.templates.allowed-config = mail.helpdesk +message.templates.allowed-config = mail.message.helpdesk.telephone +message.templates.allowed-config = mail.admin +message.templates.allowed-config = mail.admin.name + ##### Asset Storage (bitstreams / files) ###### # Moved to config/spring/api/bitstore.xml From 3b3de1ebde847102520948c4c0e53f9f67db526a Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Sat, 16 May 2026 15:55:24 +0200 Subject: [PATCH 03/18] Better null checking in allowed config props (cherry picked from commit 6b665313cb48131ada04ae0840ff531b08b31dad) (cherry picked from commit 46a0dfb38197dfd9fa9970aa293e3a11a67c12b0) --- dspace-api/src/main/java/org/dspace/core/Utils.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/core/Utils.java b/dspace-api/src/main/java/org/dspace/core/Utils.java index 7e3661fef607..30dd2f8f2754 100644 --- a/dspace-api/src/main/java/org/dspace/core/Utils.java +++ b/dspace-api/src/main/java/org/dspace/core/Utils.java @@ -32,6 +32,7 @@ import java.util.GregorianCalendar; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Properties; import java.util.Random; import java.util.StringTokenizer; @@ -634,8 +635,11 @@ public static Map getAllowedTemplateConfig() { List allowedConfigurationKeys = List.of(configurationService.getArrayProperty( "message.templates.allowed-config", DEFAULT_ALLOWED_TEMPLATE_CONFIGS)); return allowedConfigurationKeys.stream() - .map(key -> Map.entry(key, configurationService.getProperty(key))) - .filter(entry -> entry.getValue() != null) + .map(key -> { + String value = configurationService.getProperty(key); + return value != null ? Map.entry(key, value) : null; + }) + .filter(Objects::nonNull) .collect(Collectors.toMap( Map.Entry::getKey, Map.Entry::getValue From 4d0d12ed6b7fa7ffcf0b34a628a5f34d90e46416 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 09:57:21 +0200 Subject: [PATCH 04/18] Access configurationService at runtime, not rely on class setup (cherry picked from commit 5803819ba65e7211a4f49318d0d3bbf2246e21c1) (cherry picked from commit 4be430f4f0d404b88ad87454af8aeafefae9c042) --- dspace-api/src/main/java/org/dspace/core/Utils.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/core/Utils.java b/dspace-api/src/main/java/org/dspace/core/Utils.java index 30dd2f8f2754..285d2ae6eb1b 100644 --- a/dspace-api/src/main/java/org/dspace/core/Utils.java +++ b/dspace-api/src/main/java/org/dspace/core/Utils.java @@ -120,9 +120,6 @@ public final class Utils { "dspace.name", "dspace.shortname", "dspace.ui.url", "mail.helpdesk", "mail.message.helpdesk.telephone", "mail.admin", "mail.admin.name"}; - private static final ConfigurationService configurationService = - DSpaceServicesFactory.getInstance().getConfigurationService(); - /** * Private constructor */ @@ -632,6 +629,8 @@ public static String fetchUUIDFromUrl(String urlString) { public static Map getAllowedTemplateConfig() { // Pass a restricted (via configuration) list of resolved Configuration keys and values, for // template lookup + ConfigurationService configurationService = + DSpaceServicesFactory.getInstance().getConfigurationService(); List allowedConfigurationKeys = List.of(configurationService.getArrayProperty( "message.templates.allowed-config", DEFAULT_ALLOWED_TEMPLATE_CONFIGS)); return allowedConfigurationKeys.stream() From 00eea950c0750032125727ced7d8fb502e8292d6 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 15:39:41 +0200 Subject: [PATCH 05/18] Remove strict mode Velocity engine configuration (allow nulls) (cherry picked from commit 655fc62874e9e4e5cf95ef2ff1e05b908484fc9f) --- dspace-api/src/main/java/org/dspace/core/Utils.java | 6 +++++- dspace/config/dspace.cfg | 5 +++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/dspace-api/src/main/java/org/dspace/core/Utils.java b/dspace-api/src/main/java/org/dspace/core/Utils.java index 285d2ae6eb1b..aca608c51745 100644 --- a/dspace-api/src/main/java/org/dspace/core/Utils.java +++ b/dspace-api/src/main/java/org/dspace/core/Utils.java @@ -671,7 +671,11 @@ public static Properties getSecureVelocityProperties(String resourceRepositoryNa "java.lang.Class,java.lang.Runtime,java.lang.System"); secureVelocityProperties.setProperty( "introspector.restrict.packages", "java.lang.reflect,java.io,java.nio"); - secureVelocityProperties.setProperty("runtime.strict_mode.enable", "true"); + // Set strict mode if configured (default: false, as we've always treated null values as blanks) + if (DSpaceServicesFactory.getInstance().getConfigurationService() + .getBooleanProperty("message.templates.strict_mode", false)) { + secureVelocityProperties.setProperty("runtime.strict_mode.enable", "true"); + } return secureVelocityProperties; } diff --git a/dspace/config/dspace.cfg b/dspace/config/dspace.cfg index 0860edb24153..a134362dd6e7 100644 --- a/dspace/config/dspace.cfg +++ b/dspace/config/dspace.cfg @@ -240,6 +240,11 @@ message.templates.allowed-config = mail.message.helpdesk.telephone message.templates.allowed-config = mail.admin message.templates.allowed-config = mail.admin.name +# Whether to run Velocity in strict mode (null parameter values in templates for LDN or Email will result +# in an Exception instead of a blank string) +# Default: false (this can introduce unwanted side-effects if e.g. a submitter eperson is deleted for a workflow task) +#message.templates.strict_mode = false + ##### Asset Storage (bitstreams / files) ###### # Moved to config/spring/api/bitstore.xml From 5596fb2164c4dc39f1f95a8faf30547fb83732cb Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Sat, 16 May 2026 20:54:09 +0200 Subject: [PATCH 06/18] Filter requests for JSPs or traversal (cherry picked from commit cf9be8554d3597e2c80958cd62336b40b79ba19d) (cherry picked from commit dc3e4553641bdd91bac344d0402ad796b80a75f9) --- .../security/GlobalRequestSecurityFilter.java | 134 ++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java diff --git a/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java b/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java new file mode 100644 index 000000000000..53d6f7065333 --- /dev/null +++ b/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java @@ -0,0 +1,134 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.app.rest.security; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.springframework.core.Ordered; +import org.springframework.core.annotation.Order; +import org.springframework.stereotype.Component; +import org.springframework.web.filter.OncePerRequestFilter; + +import java.io.IOException; +import java.net.URI; +import java.net.URLDecoder; +import java.nio.charset.StandardCharsets; +import java.util.Locale; + +/** + * Global filter acting on all requests (not just /api/) to provide some additional hardening + * against common attacks or RCE, if a malicious payload was somehow written to a directory + * executable by the servlet container. + * The decoding and normalisation is designed to be tolerant of malformed URLs or broken clients, etc. + * so that this additional security filter does not introduce false positives or unintended side effects. + * + * @author Kim Shepherd + */ +@Component +@Order(Ordered.HIGHEST_PRECEDENCE) +public class GlobalRequestSecurityFilter extends OncePerRequestFilter { + + @Override + protected void doFilterInternal( + HttpServletRequest request, + HttpServletResponse response, + FilterChain filterChain + ) throws ServletException, IOException { + String normalizedPath = normaliseUrl(request.getRequestURI()); + // Return 403 forbidden if JSP execution or URL traversal is attempted + if (isTraversalAttempt(normalizedPath) || isJspExecutionAttempt(normalizedPath)) { + response.sendError(HttpServletResponse.SC_FORBIDDEN); + return; + } + filterChain.doFilter(request, response); + } + + /** + * Normalise the URI similarly to Tomcat, for testing how it will be interpreted + * @param rawUrl the unvalidated URL string + * @return a decoded, normalise URL + */ + private String normaliseUrl(String rawUrl) throws IOException { + if (rawUrl == null || rawUrl.isBlank()) { + throw new IOException("Empty URL"); + } + String url = rawUrl.split("\\?")[0]; + // Strip ;jspsession=... and so on + int semicolon = url.indexOf(';'); + if (semicolon >= 0) { + url = url.substring(0, semicolon); + } + url = decodeUrl(url); + if (url == null || url.isBlank()) { + throw new IOException("Decoded URL path is empty"); + } + url = normaliseUrlPath(url); + if (url == null || url.isBlank()) { + throw new IOException("Normalised URL path is empty"); + } + return url.toLowerCase(Locale.ROOT); + } + + /** + * Decode URL, falling back to original URL if it's malformed or undecodable + * @param url the encoded / unvalidated URL + * @return decoded URL or the original URL on error + */ + private String decodeUrl(String url) { + try { + return URLDecoder.decode(url, StandardCharsets.UTF_8); + } catch (IllegalArgumentException ex) { + // if we can't decode it, just return raw string + return url; + } + } + + /** + * Normalise the URL path and ensure it ends in a / + * @param url the URL path to normalise + * @return normalised path or the original parameter on error + */ + private String normaliseUrlPath(String url) { + try { + if (!url.startsWith("/")) { + url = "/" + url; + } + return new URI(url).normalize().getPath(); + } catch (Exception e) { + // if we can't use or normalise the path, just return the raw string + return url; + } + } + + /** + * Detect traversal after normalisation + * @param url the URL path to validate + * @return true if this looks like a traversal attempt + */ + private boolean isTraversalAttempt(String url) { + return url.contains("../") + || url.contains("/..") + || url.contains("%2e%2e") + || url.contains(".."); + } + + /** + * Block JSP execution attempts + * @param url the URL path to validate + */ + private boolean isJspExecutionAttempt(String url) { + return url.endsWith(".jsp") + || url.endsWith(".jspx") + || url.contains(".jsp/") + || url.contains(".jspx/") + || url.contains(".jsp\0") + || url.contains(".jspx\0"); + } +} \ No newline at end of file From 7128dfa9ef601a4c1a1f74dce02d506f8dce57c2 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Sat, 16 May 2026 22:18:27 +0200 Subject: [PATCH 07/18] Add additional logging to GlobalRequestSecurityFilter (cherry picked from commit 295a046fba14502619b3e3d96a7f6abdc9a4a5fc) (cherry picked from commit 0b1deae3fe94f55fb3dcda8dd6d6ba436b417f38) --- .../app/rest/security/GlobalRequestSecurityFilter.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java b/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java index 53d6f7065333..13740a53c7a4 100644 --- a/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java +++ b/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java @@ -43,7 +43,13 @@ protected void doFilterInternal( ) throws ServletException, IOException { String normalizedPath = normaliseUrl(request.getRequestURI()); // Return 403 forbidden if JSP execution or URL traversal is attempted - if (isTraversalAttempt(normalizedPath) || isJspExecutionAttempt(normalizedPath)) { + if (isTraversalAttempt(normalizedPath)) { + logger.warn("Path traversal attempt detected. Skipping request: " + request.getRequestURI()); + response.sendError(HttpServletResponse.SC_FORBIDDEN); + return; + } + if (isJspExecutionAttempt(normalizedPath)) { + logger.warn("JSP execution attempt detected. Skipping request: " + request.getRequestURI()); response.sendError(HttpServletResponse.SC_FORBIDDEN); return; } From 89f2a3d34fe57974296bf75d86beaf3d9d9846c8 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 11:00:17 +0200 Subject: [PATCH 08/18] Fix import order (cherry picked from commit e2e6a796fd8d19de18a80b735e20a62d29c3c5cd) (cherry picked from commit 2e400771353f2a6e50bfb0067e2c58ca602e04c9) --- .../rest/security/GlobalRequestSecurityFilter.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java b/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java index 13740a53c7a4..78e433a8fad9 100644 --- a/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java +++ b/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java @@ -7,6 +7,12 @@ */ package org.dspace.app.rest.security; +import java.io.IOException; +import java.net.URI; +import java.net.URLDecoder; +import java.nio.charset.StandardCharsets; +import java.util.Locale; + import jakarta.servlet.FilterChain; import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletRequest; @@ -16,12 +22,6 @@ import org.springframework.stereotype.Component; import org.springframework.web.filter.OncePerRequestFilter; -import java.io.IOException; -import java.net.URI; -import java.net.URLDecoder; -import java.nio.charset.StandardCharsets; -import java.util.Locale; - /** * Global filter acting on all requests (not just /api/) to provide some additional hardening * against common attacks or RCE, if a malicious payload was somehow written to a directory @@ -137,4 +137,4 @@ private boolean isJspExecutionAttempt(String url) { || url.contains(".jsp\0") || url.contains(".jspx\0"); } -} \ No newline at end of file +} From b504e4a8029bdec2964b8aae6817371b030acad6 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 13:38:55 +0200 Subject: [PATCH 09/18] Update sitemap traversal test expectations (cherry picked from commit 56ae2871eaba764900e4d9e23685a9472f485069) (cherry picked from commit 1a3dfd7c1a341ce9c04ee1aef0bea9f45c24dcbc) --- .../org/dspace/app/rest/SitemapRestControllerIT.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/SitemapRestControllerIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/SitemapRestControllerIT.java index 04d22718e846..084b8272bd09 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/SitemapRestControllerIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/SitemapRestControllerIT.java @@ -14,8 +14,6 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; -import javax.servlet.ServletException; - import org.dspace.app.rest.test.AbstractControllerIntegrationTest; import org.dspace.authorize.service.ResourcePolicyService; import org.dspace.builder.CollectionBuilder; @@ -131,18 +129,20 @@ public void testSitemap_notValidSiteMapFile() throws Exception { .andExpect(status().isNotFound()); } - @Test(expected = ServletException.class) + @Test public void testSitemap_fileSystemTraversal_dspaceCfg() throws Exception { //** WHEN ** //We attempt to use endpoint for malicious file system traversal - getClient().perform(get("/" + SITEMAPS_ENDPOINT + "/%2e%2e/config/dspace.cfg")); + getClient().perform(get("/" + SITEMAPS_ENDPOINT + "/%2e%2e/config/dspace.cfg")) + .andExpect(status().isForbidden()); } - @Test(expected = ServletException.class) + @Test public void testSitemap_fileSystemTraversal_dspaceCfg2() throws Exception { //** WHEN ** //We attempt to use endpoint for malicious file system traversal - getClient().perform(get("/" + SITEMAPS_ENDPOINT + "/%2e%2e%2fconfig%2fdspace.cfg")); + getClient().perform(get("/" + SITEMAPS_ENDPOINT + "/%2e%2e%2fconfig%2fdspace.cfg")) + .andExpect(status().isForbidden()); } @Test From 90b2a58d32c05d1d53cbe3be68bf06a9784cf7b3 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 16:30:28 +0200 Subject: [PATCH 10/18] Backport GlobalRequestSecurityFilter for javax (cherry picked from commit 8a2eee9d4adcbb4d858252877df8b020426e0802) --- .../app/rest/security/GlobalRequestSecurityFilter.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java b/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java index 78e433a8fad9..983da703878b 100644 --- a/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java +++ b/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java @@ -13,10 +13,10 @@ import java.nio.charset.StandardCharsets; import java.util.Locale; -import jakarta.servlet.FilterChain; -import jakarta.servlet.ServletException; -import jakarta.servlet.http.HttpServletRequest; -import jakarta.servlet.http.HttpServletResponse; +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import org.springframework.core.Ordered; import org.springframework.core.annotation.Order; import org.springframework.stereotype.Component; From 215f4278376df7e275a2b7811cb20945f5169709 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 13:45:20 +0200 Subject: [PATCH 11/18] Add secure file access methods (cherry picked from commit 22bec4459def712f529ff41283a8c7c5bcd1889c) --- .../storage/secure/SecureFileAccess.java | 166 ++++++++++++++++++ 1 file changed, 166 insertions(+) create mode 100644 dspace-api/src/main/java/org/dspace/storage/secure/SecureFileAccess.java diff --git a/dspace-api/src/main/java/org/dspace/storage/secure/SecureFileAccess.java b/dspace-api/src/main/java/org/dspace/storage/secure/SecureFileAccess.java new file mode 100644 index 000000000000..45727d9ee5ae --- /dev/null +++ b/dspace-api/src/main/java/org/dspace/storage/secure/SecureFileAccess.java @@ -0,0 +1,166 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.storage.secure; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +/** + * Decent I/O path validation - not perfect when symlinks are used and we are writing + * as 'toRealPath' check on the resolved path fails for new files + * + * @author Kim Shepherd + */ +public final class SecureFileAccess { + + private SecureFileAccess() {} + + /** + * Validate a given path against an allowed base path. Does not attempt to calculate "real path" + * before validation, as this breaks for new files which don't yet exist. This can make the resulting + * validation still vulnerable to symlink traversal in some cases + * @param file the unvalidated file, usually derived from user input or configuration + * This MUST be an absolute path, and the caller is expected to calculate it based on best + * context (e.g. configured base path, CWD, dspace.dir, and so on) + * @param allowedBasePaths list of allowed base paths for this use case as per system configuration + * @param purpose the name of the calling component / use case for logging and inspection + * @throws IOException on validation failure + */ + public static Path validatePathForWrite(String file, List allowedBasePaths, String purpose) + throws IOException { + Path filePath = Path.of(file); + if (!filePath.isAbsolute()) { + throw new IOException("Absolute path required for I/O (" + purpose + "): " + file); + } + for (String allowedBasePath : allowedBasePaths) { + Path basePath = Path.of(allowedBasePath) + .toRealPath() + .normalize(); + Path resolvedPath = basePath.resolve(file).normalize(); + if (resolvedPath.startsWith(basePath)) { + return resolvedPath; + } + } + + // If no valid path was resolved and returned by now + // we raise an exception and treat this as illegal access + throw new IOException("Illegal file path attempted for I/O (" + purpose + "): " + file); + } + + /** + * Validate a given path against an allowed base path. + * More secure than the 'write' variant because we can explicitly resolve links as well. + * + * @param file the unvalidated file, usually derived from user input or configuration + * This MUST be an absolute path, and the caller is expected to calculate it based on best + * context (e.g. configured base path, CWD, dspace.dir, and so on) + * @param allowedBasePaths the allowed base paths for this use case as per system configuration + * @param purpose the name of the calling component / use case for logging and inspection + * @throws IOException on validation failure + */ + public static Path validatePathForRead(String file, List allowedBasePaths, String purpose) + throws IOException { + Path filePath = Path.of(file); + if (!filePath.isAbsolute()) { + throw new IOException("Absolute path required for I/O (" + purpose + "): " + file); + } + for (String allowedBasePath : allowedBasePaths) { + Path basePath = Path.of(allowedBasePath) + .toRealPath() + .normalize(); + Path resolvedPath = basePath.resolve(file).toRealPath().normalize(); + if (resolvedPath.startsWith(basePath)) { + return resolvedPath; + } + } + // If no valid path was resolved and returned by now + // we raise an exception and treat this as illegal access + throw new IOException("Illegal file path attempted for I/O (" + purpose + "): " + file); + } + + /** + * Get a buffered reader after validating file path. + * @param unvalidatedFile the unvalidated file, usually derived from user input or configuration + * @param allowedBasePaths the allowed base paths for this use case as per system configuration + * @param purpose the name of the calling component / use case for logging and inspection + * @throws IOException on validation failure + */ + public static BufferedReader getBufferedReader(String unvalidatedFile, List allowedBasePaths, + String purpose, Charset charset) throws IOException { + if (charset == null) { + charset = StandardCharsets.UTF_8; + } + Path validatedFile = validatePathForRead(unvalidatedFile, allowedBasePaths, purpose); + return Files.newBufferedReader(validatedFile, charset); + } + + /** + * Get an input stream after validating file path. + * @param unvalidatedFile the unvalidated file, usually derived from user input or configuration + * @param allowedBasePaths the allowed base paths for this use case as per system configuration + * @param purpose the name of the calling component / use case for logging and inspection + * @throws IOException on validation failure + */ + public static InputStream getInputStream(String unvalidatedFile, List allowedBasePaths, String purpose) + throws IOException { + Path validatedFile = validatePathForRead(unvalidatedFile, allowedBasePaths, purpose); + return Files.newInputStream(validatedFile); + + } + + /** + * Get an output stream after validating file path. New files can't use toRealPath() for link calculation so + * there is a bit of a trade-off in allowing some symlink traversal to occur + * @param unvalidatedFile the unvalidated file, usually derived from user input or configuration + * @param allowedBasePaths the allowed base paths for this use case as per system configuration + * @param purpose the name of the calling component / use case for logging and inspection + * @throws IOException on validation failure + */ + public static OutputStream getOutputStream(String unvalidatedFile, List allowedBasePaths, String purpose) + throws IOException { + Path validatedFile = validatePathForWrite(unvalidatedFile, allowedBasePaths, purpose); + return Files.newOutputStream(validatedFile); + } + + /** + * Calculate an absolute path (if not already absolute) using current working dir as a root + * for relative file paths + * @param file the relative or absolute file given as input + * @return absolute path calculated from file and cwd + */ + public static String calculateAbsolutePathUsingCwd(String file) { + String filePath = file; + Path path = Path.of(filePath); + if (!path.isAbsolute()) { + filePath = Path.of("").toAbsolutePath().resolve(path).normalize().toString(); + } + return filePath; + } + + /** + * Calculate an absolute path (if not already absolute) using a given base dir as a root + * for relative file paths + * @param file the relative or absolute file given as input + * @return absolute path calculated from file and base dir + */ + public static String calculateAbsolutePathUsingBaseDir(String file, String baseDir) { + String filePath = file; + Path path = Path.of(filePath); + if (!path.isAbsolute()) { + filePath = Path.of(baseDir).toAbsolutePath().resolve(path).normalize().toString(); + } + return filePath; + } +} From b07ab74d9a7cd4a191d577386f00bfbefcb5a24f Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 13:49:50 +0200 Subject: [PATCH 12/18] Backport Curation I/O using secure file access Removes some JDK >= 16 usage (cherry picked from commit 55905a2fc46b98194f92ec38e0fb8bafa7fee21a) --- .../main/java/org/dspace/curate/Curation.java | 31 +++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/curate/Curation.java b/dspace-api/src/main/java/org/dspace/curate/Curation.java index b894dcd85f03..43decd4add72 100644 --- a/dspace-api/src/main/java/org/dspace/curate/Curation.java +++ b/dspace-api/src/main/java/org/dspace/curate/Curation.java @@ -10,15 +10,18 @@ import java.io.BufferedReader; import java.io.File; import java.io.FileNotFoundException; -import java.io.FileReader; import java.io.IOException; import java.io.OutputStream; import java.io.OutputStreamWriter; import java.io.PrintStream; import java.io.Writer; +import java.nio.charset.StandardCharsets; import java.sql.SQLException; +import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.Iterator; +import java.util.List; import java.util.Map; import java.util.UUID; @@ -38,6 +41,7 @@ import org.dspace.handle.service.HandleService; import org.dspace.scripts.DSpaceRunnable; import org.dspace.services.factory.DSpaceServicesFactory; +import org.dspace.storage.secure.SecureFileAccess; import org.dspace.utils.DSpace; /** @@ -112,8 +116,16 @@ private void handleCurationTask(Curator curator) throws IOException, SQLExceptio } else if (commandLine.hasOption('T')) { // load taskFile BufferedReader reader = null; + // in this case, Curation CLI expects to calculate the -T parameter from the user's current working dir + String taskFilePath = SecureFileAccess.calculateAbsolutePathUsingCwd(this.taskFile); try { - reader = new BufferedReader(new FileReader(this.taskFile)); + String dspaceDir = DSpaceServicesFactory.getInstance() + .getConfigurationService().getProperty("dspace.dir"); + List allowedTaskFileBasePath = new ArrayList<>( + Arrays.asList(DSpaceServicesFactory.getInstance().getConfigurationService() + .getArrayProperty("curate.taskfile.base", new String[]{dspaceDir}))); + reader = SecureFileAccess.getBufferedReader(taskFilePath, allowedTaskFileBasePath, + "curation-taskfile", StandardCharsets.UTF_8); while ((taskName = reader.readLine()) != null) { if (verbose) { super.handler.logInfo("Adding task: " + taskName); @@ -189,12 +201,25 @@ private void endScript(long timeRun) throws SQLException { private Curator initCurator() throws FileNotFoundException { Curator curator = new Curator(handler); OutputStream reporterStream; + String dspaceDir = DSpaceServicesFactory.getInstance() + .getConfigurationService().getProperty("dspace.dir"); + List allowedReporterBasePaths = new ArrayList<>(Arrays.asList(DSpaceServicesFactory.getInstance() + .getConfigurationService().getArrayProperty("curate.reporter.base", + new String[]{dspaceDir + File.separatorChar + "log"}))); if (null == this.reporter) { reporterStream = NullOutputStream.NULL_OUTPUT_STREAM; } else if ("-".equals(this.reporter)) { reporterStream = System.out; } else { - reporterStream = new PrintStream(this.reporter); + // Reporter param comes from CLI execution. Calculate abs path from user's current working dir + String reporterFilePath = SecureFileAccess.calculateAbsolutePathUsingCwd(this.reporter); + try { + reporterStream = new PrintStream( + SecureFileAccess.getOutputStream( + reporterFilePath, allowedReporterBasePaths, "curation-reporter")); + } catch (IOException e) { + throw new FileNotFoundException(e.getLocalizedMessage()); + } } Writer reportWriter = new OutputStreamWriter(reporterStream); curator.setReporter(reportWriter); From 3e6af593f346a8465e6fee120f0701a0b3f2d1e9 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 13:50:59 +0200 Subject: [PATCH 13/18] Curation config support for allowed base paths (cherry picked from commit 45022245be2fabb5ba26d50b335f1aa1f905a660) --- dspace/config/modules/curate.cfg | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/dspace/config/modules/curate.cfg b/dspace/config/modules/curate.cfg index 6e75738de543..dad40d615454 100644 --- a/dspace/config/modules/curate.cfg +++ b/dspace/config/modules/curate.cfg @@ -29,3 +29,15 @@ curate.taskqueue.dir = ${dspace.dir}/ctqueues # Maximum amount of redirects set to 0 for none and -1 for unlimited curate.checklinks.max-redirect = 0 + +# allowed base path(s) of curation task files +# it is recommended to restrict this path as much as possible +# so that the DSpace Processes framework may only load files as "tasks" +# from a trusted location. For multiple paths, repeat this configuration +# property for each trusted path +# Default: ${dspace.dir} +#curate.taskfile.base = ${dspace.dir} + +# allowed base path of reporter output. +# Default: ${dspace.dir}/log +#curate.reporter.base = ${dspace.dir}/log From 9358a7e0570284c385280f829b143c349acf4cdc Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Tue, 26 May 2026 14:06:14 +0200 Subject: [PATCH 14/18] Move curation -r reporter param to CLI only (cherry picked from commit 277af8233261ec0f61d71a4ce0908341c30e5e89) --- .../java/org/dspace/curate/CurationCliScriptConfiguration.java | 3 +++ .../src/main/java/org/dspace/curate/CurationClientOptions.java | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/curate/CurationCliScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/curate/CurationCliScriptConfiguration.java index eaa04f477829..07684f3643d0 100644 --- a/dspace-api/src/main/java/org/dspace/curate/CurationCliScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/curate/CurationCliScriptConfiguration.java @@ -20,6 +20,9 @@ public Options getOptions() { options = super.getOptions(); options.addOption("e", "eperson", true, "email address of curating eperson"); options.getOption("e").setRequired(true); + options.addOption("r", "reporter", true, + "relative or absolute path to the desired report file. Use '-' to report to console. If absent, no " + + "reporting"); return options; } } diff --git a/dspace-api/src/main/java/org/dspace/curate/CurationClientOptions.java b/dspace-api/src/main/java/org/dspace/curate/CurationClientOptions.java index 8ec0f14697c0..a8a3d358ce67 100644 --- a/dspace-api/src/main/java/org/dspace/curate/CurationClientOptions.java +++ b/dspace-api/src/main/java/org/dspace/curate/CurationClientOptions.java @@ -59,9 +59,6 @@ protected static Options constructOptions() { "Id (handle) of object to perform task on, or 'all' to perform on whole repository"); options.addOption("p", "parameter", true, "a task parameter 'NAME=VALUE'"); options.addOption("q", "queue", true, "name of task queue to process"); - options.addOption("r", "reporter", true, - "relative or absolute path to the desired report file. Use '-' to report to console. If absent, no " + - "reporting"); options.addOption("s", "scope", true, "transaction scope to impose: use 'object', 'curation', or 'open'. If absent, 'open' applies"); options.addOption("v", "verbose", false, "report activity to stdout"); From 0d088eca1ad09316abc5504e68229d3c653941ca Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 16:57:21 +0200 Subject: [PATCH 15/18] Fix import order (cherry picked from commit a7572212c135155fb8420a2bfc95869f1ba6959d) --- .../dspace/app/rest/security/GlobalRequestSecurityFilter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java b/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java index 983da703878b..a9f0eea8d728 100644 --- a/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java +++ b/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/GlobalRequestSecurityFilter.java @@ -12,11 +12,11 @@ import java.net.URLDecoder; import java.nio.charset.StandardCharsets; import java.util.Locale; - import javax.servlet.FilterChain; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; + import org.springframework.core.Ordered; import org.springframework.core.annotation.Order; import org.springframework.stereotype.Component; From e2719452a4f86df61498ad4d94b9cde52c7cfd77 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 14:38:00 +0200 Subject: [PATCH 16/18] Ignore CurationScriptIT -T taskFile tests, to rewrite w/ CLI (cherry picked from commit 6437472b8277b9aa815dd71e14b499ba7515f87d) (cherry picked from commit 37cd6eb791d4f61bb54fedf899aa96f99504e38d) --- .../src/test/java/org/dspace/curate/CurationScriptIT.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dspace-server-webapp/src/test/java/org/dspace/curate/CurationScriptIT.java b/dspace-server-webapp/src/test/java/org/dspace/curate/CurationScriptIT.java index 8c0744a09cce..dfcfe212b32f 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/curate/CurationScriptIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/curate/CurationScriptIT.java @@ -49,6 +49,7 @@ import org.dspace.scripts.configuration.ScriptConfiguration; import org.dspace.scripts.factory.ScriptServiceFactory; import org.dspace.scripts.service.ScriptService; +import org.junit.Ignore; import org.junit.Test; import org.springframework.beans.factory.annotation.Autowired; @@ -214,6 +215,7 @@ public void curateScript_InvalidScope() throws Exception { .andExpect(status().isBadRequest()); } + @Ignore @Test public void curateScript_InvalidTaskFile() throws Exception { String token = getAuthToken(admin.getEmail(), password); @@ -286,6 +288,7 @@ public void curateScript_validRequest_Task() throws Exception { } } + @Ignore @Test public void curateScript_validRequest_TaskFile() throws Exception { context.turnOffAuthorisationSystem(); From fa2b964b85b963e63d813d1043da5548a09c6624 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 06:43:22 +0200 Subject: [PATCH 17/18] Move taskfile -T option to CLI script config only (cherry picked from commit 00e4979a60fd69adbf4a7476926701ef59207ce7) (cherry picked from commit 27708ea6d70abe433f131cf3b875dfdc067d3c12) --- .../org/dspace/curate/CurationCliScriptConfiguration.java | 1 + .../main/java/org/dspace/curate/CurationClientOptions.java | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/curate/CurationCliScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/curate/CurationCliScriptConfiguration.java index 07684f3643d0..925bd4f2d232 100644 --- a/dspace-api/src/main/java/org/dspace/curate/CurationCliScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/curate/CurationCliScriptConfiguration.java @@ -23,6 +23,7 @@ public Options getOptions() { options.addOption("r", "reporter", true, "relative or absolute path to the desired report file. Use '-' to report to console. If absent, no " + "reporting"); + options.addOption("T", "taskfile", true, "file containing curation task names"); return options; } } diff --git a/dspace-api/src/main/java/org/dspace/curate/CurationClientOptions.java b/dspace-api/src/main/java/org/dspace/curate/CurationClientOptions.java index a8a3d358ce67..03ad2f34b230 100644 --- a/dspace-api/src/main/java/org/dspace/curate/CurationClientOptions.java +++ b/dspace-api/src/main/java/org/dspace/curate/CurationClientOptions.java @@ -31,7 +31,8 @@ public enum CurationClientOptions { /** * This method resolves the CommandLine parameters to figure out which action the curation script should perform * - * @param commandLine The relevant CommandLine for the curation script + * @param commandLine The relevant CommandLine for the curation script. Note that -T is passed only + * from CurationCliScriptConfig and is not accessible from UI processes * @return The curation option to be ran, parsed from the CommandLine */ protected static CurationClientOptions getClientOption(CommandLine commandLine) { @@ -54,7 +55,6 @@ protected static Options constructOptions() { Options options = new Options(); options.addOption("t", "task", true, "curation task name; options: " + getTaskOptions()); - options.addOption("T", "taskfile", true, "file containing curation task names"); options.addOption("i", "id", true, "Id (handle) of object to perform task on, or 'all' to perform on whole repository"); options.addOption("p", "parameter", true, "a task parameter 'NAME=VALUE'"); From e45020b118b810c664534cf6ac6c4ac3764264dd Mon Sep 17 00:00:00 2001 From: Tim Donohue Date: Fri, 13 Feb 2026 09:49:22 -0600 Subject: [PATCH 18/18] Fix broken tests due to random order execution. When CreateMissingIdentifiersIT is run before WorkflowCurationIT, it was causing the latter to fail. (cherry picked from commit 816dfbe473effcdc4b6df210e79d3be6d6c7d3bb) (cherry picked from commit 6a781f636f1344f246d18886ed76997489ba8cc4) --- .../org/dspace/ctask/general/CreateMissingIdentifiersIT.java | 2 ++ .../test/java/org/dspace/workflow/WorkflowCurationIT.java | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/dspace-api/src/test/java/org/dspace/ctask/general/CreateMissingIdentifiersIT.java b/dspace-api/src/test/java/org/dspace/ctask/general/CreateMissingIdentifiersIT.java index 3b50258a5a23..8ecddc6cd50c 100644 --- a/dspace-api/src/test/java/org/dspace/ctask/general/CreateMissingIdentifiersIT.java +++ b/dspace-api/src/test/java/org/dspace/ctask/general/CreateMissingIdentifiersIT.java @@ -45,6 +45,7 @@ public void testPerform() // Must remove any cached named plugins before creating a new one CoreServiceFactory.getInstance().getPluginService().clearNamedPluginClasses(); // Define a new task dynamically + String[] prevTaskDef = configurationService.getArrayProperty(P_TASK_DEF); configurationService.setProperty(P_TASK_DEF, CreateMissingIdentifiers.class.getCanonicalName() + " = " + TASK_NAME); @@ -82,5 +83,6 @@ public void testPerform() curator.curate(context, item); int status = curator.getStatus(TASK_NAME); assertEquals("Curation should succeed", Curator.CURATE_SUCCESS, status); + configurationService.setProperty(P_TASK_DEF, prevTaskDef); } } diff --git a/dspace-api/src/test/java/org/dspace/workflow/WorkflowCurationIT.java b/dspace-api/src/test/java/org/dspace/workflow/WorkflowCurationIT.java index 66dd2cee807f..dfe61a30b2b2 100644 --- a/dspace-api/src/test/java/org/dspace/workflow/WorkflowCurationIT.java +++ b/dspace-api/src/test/java/org/dspace/workflow/WorkflowCurationIT.java @@ -22,6 +22,7 @@ import org.dspace.content.Community; import org.dspace.content.MetadataValue; import org.dspace.content.service.ItemService; +import org.dspace.core.LegacyPluginServiceImpl; import org.dspace.ctask.testing.MarkerTask; import org.dspace.eperson.EPerson; import org.dspace.util.DSpaceConfigurationInitializer; @@ -29,6 +30,7 @@ import org.dspace.xmlworkflow.storedcomponents.XmlWorkflowItem; import org.junit.Test; import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit4.SpringRunner; @@ -46,6 +48,8 @@ public class WorkflowCurationIT extends AbstractIntegrationTestWithDatabase { @Inject private ItemService itemService; + @Autowired + private LegacyPluginServiceImpl legacyPluginService; /** * Basic smoke test of a curation task attached to a workflow step. @@ -56,6 +60,7 @@ public class WorkflowCurationIT public void curationTest() throws Exception { context.turnOffAuthorisationSystem(); + legacyPluginService.clearNamedPluginClasses(); //** GIVEN **