From a57f4c57661d14bee6a68afdd4aa397954fe6315 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Sat, 16 May 2026 22:05:27 +0200 Subject: [PATCH 01/20] 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 4e19b94428b88eab0bf05dfa35ec6ac1439efe31 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Tue, 5 May 2026 16:24:29 +0200 Subject: [PATCH 02/20] 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 8330d7ee3cf1..0f06d42a2604 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 f3fbfa800965c2517662d49be48dc5bdc17db5d6 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Sat, 16 May 2026 15:55:24 +0200 Subject: [PATCH 03/20] 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 30d6c947c795922017f02affcd29095499549d3b Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 09:57:21 +0200 Subject: [PATCH 04/20] 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 05cc771fd5381d6952eb42a08ff47a5490780d12 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 15:39:41 +0200 Subject: [PATCH 05/20] 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 0f06d42a2604..8744f720d2a5 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 1484ceb0cd5a55849e79710dfb74129bed7d71eb Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Sat, 16 May 2026 20:54:09 +0200 Subject: [PATCH 06/20] 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 b5ac787a275f3a29995cbfd43090ea315f51379c Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Sat, 16 May 2026 22:18:27 +0200 Subject: [PATCH 07/20] 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 b51274bde69f983f5b4a48c60aafe11f666a6f76 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 11:00:17 +0200 Subject: [PATCH 08/20] 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 6f787505994ee2bc090ba96f47bce33f28ded98a Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 13:38:55 +0200 Subject: [PATCH 09/20] 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 c4cb116d1816f07da08015602d125796780cdf3f Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 16:30:28 +0200 Subject: [PATCH 10/20] 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 5d675cf3e4f17510d6a4536207244752a822b92f Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 13:45:20 +0200 Subject: [PATCH 11/20] 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 5d1e8bc92091d9a568460ee86ebfc9d8ab1e41fd Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 13:49:50 +0200 Subject: [PATCH 12/20] 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 3c014fec911d..bee636c9e6ac 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 e7f4604c7b6629d7e27589bcbdbf8adf4046a1d2 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 13:50:59 +0200 Subject: [PATCH 13/20] 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 d36578fa943d..58317d509c96 100644 --- a/dspace/config/modules/curate.cfg +++ b/dspace/config/modules/curate.cfg @@ -30,3 +30,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 6612d9f326ec758a3279d96692ce5870b03b6f6e Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Tue, 26 May 2026 14:06:14 +0200 Subject: [PATCH 14/20] 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 139c7b112e3b6cf45ab494f1d8108ebc5a4bf55a Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 16:57:21 +0200 Subject: [PATCH 15/20] 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 7a3facf560f34b46905ccf7dcc3c683cb70faadf Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 14:38:00 +0200 Subject: [PATCH 16/20] 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 8745613d7af6..347ed0935f05 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 3e250e2b9f34759a5993233e1a0d57e5a4149449 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Wed, 27 May 2026 06:43:22 +0200 Subject: [PATCH 17/20] 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 640e52c67dc5a5c8d1e8c790ddd1ffc67363df12 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 11 Jun 2026 12:28:55 +0200 Subject: [PATCH 18/20] Fix BitstreamFormatRestRepositoryIT format count The bitstream format registry on this branch contains 107 formats but the test still expected 95, so the create* tests have been failing on the base branch as well (pre-existing). Align the constant with the actual registry content. --- .../org/dspace/app/rest/BitstreamFormatRestRepositoryIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamFormatRestRepositoryIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamFormatRestRepositoryIT.java index af963696f0b7..2fed1f27783a 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamFormatRestRepositoryIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamFormatRestRepositoryIT.java @@ -56,7 +56,7 @@ public class BitstreamFormatRestRepositoryIT extends AbstractControllerIntegrati @Autowired private BitstreamFormatConverter bitstreamFormatConverter; - private final int DEFAULT_AMOUNT_FORMATS = 95; + private final int DEFAULT_AMOUNT_FORMATS = 107; @Test public void findAllPaginationTest() throws Exception { From 0e1aa494f52ab83c36e87c4ca580ffcfadf56709 Mon Sep 17 00:00:00 2001 From: Tim Donohue Date: Fri, 13 Feb 2026 09:49:22 -0600 Subject: [PATCH 19/20] 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 ++ 1 file changed, 2 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); } } From 14d802a24d433c5020dfa7ecb3278daf4bcb28a0 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 11 Jun 2026 15:48:43 +0200 Subject: [PATCH 20/20] Make BitstreamFormatRestRepositoryIT count assertions order-robust The format count in the test DB depends on test class execution order (other test classes on this branch leak formats into the registry), so the create* tests randomly failed with a fixed expected constant - both on this branch and on the base branch. Capture the current count before each create attempt and assert relative to it, instead of relying on DEFAULT_AMOUNT_FORMATS. --- .../rest/BitstreamFormatRestRepositoryIT.java | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamFormatRestRepositoryIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamFormatRestRepositoryIT.java index 2fed1f27783a..06fe0b19d247 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamFormatRestRepositoryIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamFormatRestRepositoryIT.java @@ -56,7 +56,6 @@ public class BitstreamFormatRestRepositoryIT extends AbstractControllerIntegrati @Autowired private BitstreamFormatConverter bitstreamFormatConverter; - private final int DEFAULT_AMOUNT_FORMATS = 107; @Test public void findAllPaginationTest() throws Exception { @@ -180,9 +179,24 @@ public void createAdminAccess() throws Exception { } + /** + * Get the current number of bitstream formats via REST. Other test classes + * may leak formats into the registry depending on execution order, so + * count-based assertions must be relative to the current state. + */ + private int getCurrentAmountOfFormats() throws Exception { + AtomicReference totalRef = new AtomicReference<>(); + getClient().perform(get("/api/core/bitstreamformats/")) + .andExpect(status().isOk()) + .andDo(result -> totalRef.set( + read(result.getResponse().getContentAsString(), "$.page.totalElements"))); + return totalRef.get(); + } + @Test public void createNonValidSupportLevel() throws Exception { ObjectMapper mapper = new ObjectMapper(); + int amountOfFormatsBefore = getCurrentAmountOfFormats(); BitstreamFormatRest bitstreamFormatRest = this.createRandomMockBitstreamRest(false); bitstreamFormatRest.setSupportLevel("NONVALID SUPPORT LVL"); //Attempt to create bitstream with a non-valid support lvl @@ -194,12 +208,13 @@ public void createNonValidSupportLevel() throws Exception { // Check that no new bitstreamformat was created getClient().perform(get("/api/core/bitstreamformats/")) .andExpect(status().isOk()) - .andExpect(jsonPath("$.page.totalElements", is(DEFAULT_AMOUNT_FORMATS))); + .andExpect(jsonPath("$.page.totalElements", is(amountOfFormatsBefore))); } @Test public void createNoAccess() throws Exception { ObjectMapper mapper = new ObjectMapper(); + int amountOfFormatsBefore = getCurrentAmountOfFormats(); BitstreamFormatRest bitstreamFormatRest = this.createRandomMockBitstreamRest(false); //Try to create bitstreamFormat without auth token @@ -210,12 +225,13 @@ public void createNoAccess() throws Exception { // Check that no new bitstreamformat was created getClient().perform(get("/api/core/bitstreamformats/")) .andExpect(status().isOk()) - .andExpect(jsonPath("$.page.totalElements", is(DEFAULT_AMOUNT_FORMATS))); + .andExpect(jsonPath("$.page.totalElements", is(amountOfFormatsBefore))); } @Test public void createNonAdminAccess() throws Exception { ObjectMapper mapper = new ObjectMapper(); + int amountOfFormatsBefore = getCurrentAmountOfFormats(); BitstreamFormatRest bitstreamFormatRest = this.createRandomMockBitstreamRest(false); context.turnOffAuthorisationSystem(); EPerson user = EPersonBuilder.createEPerson(context) @@ -234,12 +250,13 @@ public void createNonAdminAccess() throws Exception { // Check that no new bitstreamformat was created getClient().perform(get("/api/core/bitstreamformats/")) .andExpect(status().isOk()) - .andExpect(jsonPath("$.page.totalElements", is(DEFAULT_AMOUNT_FORMATS))); + .andExpect(jsonPath("$.page.totalElements", is(amountOfFormatsBefore))); } @Test public void createAlreadyExisting() throws Exception { ObjectMapper mapper = new ObjectMapper(); + int amountOfFormatsBefore = getCurrentAmountOfFormats(); BitstreamFormatRest bitstreamFormatRest = this.createRandomMockBitstreamRest(true); // Capture the Id of the created BitstreamFormat (see andDo() below) @@ -264,7 +281,7 @@ public void createAlreadyExisting() throws Exception { // Check that the new bitstreamformat was created only once getClient().perform(get("/api/core/bitstreamformats/")) .andExpect(status().isOk()) - .andExpect(jsonPath("$.page.totalElements", is(DEFAULT_AMOUNT_FORMATS + 1))); + .andExpect(jsonPath("$.page.totalElements", is(amountOfFormatsBefore + 1))); } finally {