From e37bc044a63a1225ce55062e203118689dd93dfd Mon Sep 17 00:00:00 2001 From: "Mark H. Wood" Date: Mon, 28 Jul 2025 13:42:28 -0400 Subject: [PATCH 1/3] Port upstream DSpace PR 11075 to 7_x (cherry picked from commit ceab9526c414d1ead05f4c18d69b70b07d991ae6) Co-Authored-By: Claude Opus 5.5 --- .../authority/DSpaceControlledVocabulary.java | 89 +++++++++++-------- .../DSpaceControlledVocabularyTest.java | 2 + 2 files changed, 55 insertions(+), 36 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/content/authority/DSpaceControlledVocabulary.java b/dspace-api/src/main/java/org/dspace/content/authority/DSpaceControlledVocabulary.java index 444332df97d2..3f7f926c07a4 100644 --- a/dspace-api/src/main/java/org/dspace/content/authority/DSpaceControlledVocabulary.java +++ b/dspace-api/src/main/java/org/dspace/content/authority/DSpaceControlledVocabulary.java @@ -34,35 +34,37 @@ * from {@code ${dspace.dir}/config/controlled-vocabularies/*.xml} and turns * them into autocompleting authorities. * - * Configuration: This MUST be configured as a self-named plugin, e.g.: {@code - * plugin.selfnamed.org.dspace.content.authority.ChoiceAuthority = \ + *

Configuration: This MUST be configured as a self-named plugin, e.g.: {@code + * plugin.selfnamed.org.dspace.content.authority.ChoiceAuthority = * org.dspace.content.authority.DSpaceControlledVocabulary * } * - * It AUTOMATICALLY configures a plugin instance for each XML file in the + *

It AUTOMATICALLY configures a plugin instance for each XML file in the * controlled vocabularies directory. The name of the plugin is the basename of * the file; e.g., {@code ${dspace.dir}/config/controlled-vocabularies/nsi.xml} * would generate a plugin called "nsi". * - * Each configured plugin comes with three configuration options: {@code - * vocabulary.plugin._plugin_.hierarchy.store = - * # Store entire hierarchy along with selected value. Default: TRUE - * vocabulary.plugin._plugin_.hierarchy.suggest = - * # Display entire hierarchy in the suggestion list. Default: TRUE - * vocabulary.plugin._plugin_.delimiter = "" - * # Delimiter to use when building hierarchy strings. Default: "::" - * } + *

Each configured plugin comes with three configuration options: + *

* * @author Michael B. Klein */ public class DSpaceControlledVocabulary extends SelfNamedPlugin implements HierarchicalAuthority { - private static Logger log = org.apache.logging.log4j.LogManager.getLogger(DSpaceControlledVocabulary.class); + private static final Logger log = org.apache.logging.log4j.LogManager.getLogger(); protected static String xpathTemplate = "//node[contains(translate(@label,'ABCDEFGHIJKLMNOPQRSTUVWXYZ'," + - "'abcdefghijklmnopqrstuvwxyz'),'%s')]"; - protected static String idTemplate = "//node[@id = '%s']"; - protected static String labelTemplate = "//node[@label = '%s']"; + "'abcdefghijklmnopqrstuvwxyz'),%s)]"; + protected static String idTemplate = "//node[@id = %s]"; + protected static String idTemplateQuoted = "//node[@id = '%s']"; + protected static String labelTemplate = "//node[@label = %s]"; protected static String idParentTemplate = "//node[@id = '%s']/parent::isComposedBy/parent::node"; protected static String rootTemplate = "/node"; protected static String pluginNames[] = null; @@ -106,7 +108,7 @@ public boolean accept(File dir, String name) { File.separator + "config" + File.separator + "controlled-vocabularies"; String[] xmlFiles = (new File(vocabulariesPath)).list(new xmlFilter()); - List names = new ArrayList(); + List names = new ArrayList<>(); for (String filename : xmlFiles) { names.add((new File(filename)).getName().replace(".xml", "")); } @@ -162,14 +164,23 @@ protected String buildString(Node node) { public Choices getMatches(String text, int start, int limit, String locale) { init(); log.debug("Getting matches for '" + text + "'"); - String xpathExpression = ""; String[] textHierarchy = text.split(hierarchyDelimiter, -1); + StringBuilder xpathExpressionBuilder = new StringBuilder(); for (int i = 0; i < textHierarchy.length; i++) { - xpathExpression += String.format(xpathTemplate, textHierarchy[i].replaceAll("'", "'").toLowerCase()); + xpathExpressionBuilder.append(String.format(xpathTemplate, "$var" + i)); } + String xpathExpression = xpathExpressionBuilder.toString(); XPath xpath = XPathFactory.newInstance().newXPath(); - int total = 0; - List choices = new ArrayList(); + xpath.setXPathVariableResolver(variableName -> { + String varName = variableName.getLocalPart(); + if (varName.startsWith("var")) { + int index = Integer.parseInt(varName.substring(3)); + return textHierarchy[index].toLowerCase(); + } + throw new IllegalArgumentException("Unexpected variable: " + varName); + }); + int total; + List choices; try { NodeList results = (NodeList) xpath.evaluate(xpathExpression, vocabulary, XPathConstants.NODESET); total = results.getLength(); @@ -185,14 +196,23 @@ public Choices getMatches(String text, int start, int limit, String locale) { @Override public Choices getBestMatch(String text, String locale) { init(); - log.debug("Getting best matches for '" + text + "'"); - String xpathExpression = ""; + log.debug("Getting best matches for {}'", text); String[] textHierarchy = text.split(hierarchyDelimiter, -1); + StringBuilder xpathExpressionBuilder = new StringBuilder(); for (int i = 0; i < textHierarchy.length; i++) { - xpathExpression += String.format(labelTemplate, textHierarchy[i].replaceAll("'", "'")); + xpathExpressionBuilder.append(String.format(labelTemplate, "$var" + i)); } + String xpathExpression = xpathExpressionBuilder.toString(); XPath xpath = XPathFactory.newInstance().newXPath(); - List choices = new ArrayList(); + xpath.setXPathVariableResolver(variableName -> { + String varName = variableName.getLocalPart(); + if (varName.startsWith("var")) { + int index = Integer.parseInt(varName.substring(3)); + return textHierarchy[index]; + } + throw new IllegalArgumentException("Unexpected variable: " + varName); + }); + List choices; try { NodeList results = (NodeList) xpath.evaluate(xpathExpression, vocabulary, XPathConstants.NODESET); choices = getChoicesFromNodeList(results, 0, 1); @@ -240,7 +260,7 @@ public Choices getTopChoices(String authorityName, int start, int limit, String @Override public Choices getChoicesByParent(String authorityName, String parentId, int start, int limit, String locale) { init(); - String xpathExpression = String.format(idTemplate, parentId); + String xpathExpression = String.format(idTemplateQuoted, parentId); return getChoicesByXpath(xpathExpression, start, limit); } @@ -264,15 +284,12 @@ public Integer getPreloadLevel() { } private boolean isRootElement(Node node) { - if (node != null && node.getOwnerDocument().getDocumentElement().equals(node)) { - return true; - } - return false; + return node != null && node.getOwnerDocument().getDocumentElement().equals(node); } private Node getNode(String key) throws XPathExpressionException { init(); - String xpathExpression = String.format(idTemplate, key); + String xpathExpression = String.format(idTemplateQuoted, key); Node node = getNodeFromXPath(xpathExpression); return node; } @@ -284,7 +301,7 @@ private Node getNodeFromXPath(String xpathExpression) throws XPathExpressionExce } private List getChoicesFromNodeList(NodeList results, int start, int limit) { - List choices = new ArrayList(); + List choices = new ArrayList<>(); for (int i = 0; i < results.getLength(); i++) { if (i < start) { continue; @@ -303,14 +320,14 @@ private List getChoicesFromNodeList(NodeList results, int start, int lim private Map addOtherInformation(String parentCurr, String noteCurr, List childrenCurr, String authorityCurr) { - Map extras = new HashMap(); + Map extras = new HashMap<>(); if (StringUtils.isNotBlank(parentCurr)) { extras.put("parent", parentCurr); } if (StringUtils.isNotBlank(noteCurr)) { extras.put("note", noteCurr); } - if (childrenCurr.size() > 0) { + if (!childrenCurr.isEmpty()) { extras.put("hasChildren", "true"); } else { extras.put("hasChildren", "false"); @@ -368,7 +385,7 @@ private String getNote(Node node) { } private List getChildren(Node node) { - List children = new ArrayList(); + List children = new ArrayList<>(); NodeList childNodes = node.getChildNodes(); for (int ci = 0; ci < childNodes.getLength(); ci++) { Node firstChild = childNodes.item(ci); @@ -391,7 +408,7 @@ private List getChildren(Node node) { private boolean isSelectable(Node node) { Node selectableAttr = node.getAttributes().getNamedItem("selectable"); if (null != selectableAttr) { - return Boolean.valueOf(selectableAttr.getNodeValue()); + return Boolean.parseBoolean(selectableAttr.getNodeValue()); } else { // Default is true return true; } @@ -418,7 +435,7 @@ private String getAuthority(Node node) { } private Choices getChoicesByXpath(String xpathExpression, int start, int limit) { - List choices = new ArrayList(); + List choices = new ArrayList<>(); XPath xpath = XPathFactory.newInstance().newXPath(); try { Node parentNode = (Node) xpath.evaluate(xpathExpression, vocabulary, XPathConstants.NODE); diff --git a/dspace-api/src/test/java/org/dspace/content/authority/DSpaceControlledVocabularyTest.java b/dspace-api/src/test/java/org/dspace/content/authority/DSpaceControlledVocabularyTest.java index 255b070e5eac..524c6407b7bd 100644 --- a/dspace-api/src/test/java/org/dspace/content/authority/DSpaceControlledVocabularyTest.java +++ b/dspace-api/src/test/java/org/dspace/content/authority/DSpaceControlledVocabularyTest.java @@ -8,6 +8,7 @@ package org.dspace.content.authority; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import java.io.IOException; @@ -86,6 +87,7 @@ public void testGetMatches() throws IOException, ClassNotFoundException { CoreServiceFactory.getInstance().getPluginService().getNamedPlugin(Class.forName(PLUGIN_INTERFACE), "farm"); assertNotNull(instance); Choices result = instance.getMatches(text, start, limit, locale); + assertNotEquals("At least one match expected", 0, result.values.length); assertEquals("north 40", result.values[0].value); } From 3e0555a4f5fef9731d3cb676035acbd82e15d99d Mon Sep 17 00:00:00 2001 From: AdamF42 Date: Sun, 15 Mar 2026 13:40:33 +0100 Subject: [PATCH 2/3] [DURACOM-447] Fix Xpath injection (cherry picked from commit aa419b904f1883930e80950bb2ce16a2c6c84761) Co-Authored-By: Claude Opus 5.5 --- .../authority/DSpaceControlledVocabulary.java | 62 ++++++++++++++++--- 1 file changed, 53 insertions(+), 9 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/content/authority/DSpaceControlledVocabulary.java b/dspace-api/src/main/java/org/dspace/content/authority/DSpaceControlledVocabulary.java index 3f7f926c07a4..13b6a88d06b7 100644 --- a/dspace-api/src/main/java/org/dspace/content/authority/DSpaceControlledVocabulary.java +++ b/dspace-api/src/main/java/org/dspace/content/authority/DSpaceControlledVocabulary.java @@ -63,9 +63,9 @@ public class DSpaceControlledVocabulary extends SelfNamedPlugin implements Hiera protected static String xpathTemplate = "//node[contains(translate(@label,'ABCDEFGHIJKLMNOPQRSTUVWXYZ'," + "'abcdefghijklmnopqrstuvwxyz'),%s)]"; protected static String idTemplate = "//node[@id = %s]"; - protected static String idTemplateQuoted = "//node[@id = '%s']"; + protected static String idTemplateQuoted = "//node[@id = $nodeId]"; protected static String labelTemplate = "//node[@label = %s]"; - protected static String idParentTemplate = "//node[@id = '%s']/parent::isComposedBy/parent::node"; + protected static String idParentTemplate = "//node[@id = $nodeId]/parent::isComposedBy/parent::node"; protected static String rootTemplate = "/node"; protected static String pluginNames[] = null; @@ -260,16 +260,14 @@ public Choices getTopChoices(String authorityName, int start, int limit, String @Override public Choices getChoicesByParent(String authorityName, String parentId, int start, int limit, String locale) { init(); - String xpathExpression = String.format(idTemplateQuoted, parentId); - return getChoicesByXpath(xpathExpression, start, limit); + return getChoicesByXpathWithId(idTemplateQuoted, parentId, start, limit); } @Override public Choice getParentChoice(String authorityName, String childId, String locale) { init(); try { - String xpathExpression = String.format(idParentTemplate, childId); - Choice choice = createChoiceFromNode(getNodeFromXPath(xpathExpression)); + Choice choice = createChoiceFromNode(getNodeByIdFromXPath(idParentTemplate, childId)); return choice; } catch (XPathExpressionException e) { log.error(e.getMessage(), e); @@ -289,9 +287,18 @@ private boolean isRootElement(Node node) { private Node getNode(String key) throws XPathExpressionException { init(); - String xpathExpression = String.format(idTemplateQuoted, key); - Node node = getNodeFromXPath(xpathExpression); - return node; + return getNodeByIdFromXPath(idTemplateQuoted, key); + } + + private Node getNodeByIdFromXPath(String xpathTemplate, String nodeId) throws XPathExpressionException { + XPath xpath = XPathFactory.newInstance().newXPath(); + xpath.setXPathVariableResolver(variableName -> { + if ("nodeId".equals(variableName.getLocalPart())) { + return nodeId; + } + throw new IllegalArgumentException("Unexpected variable: " + variableName); + }); + return (Node) xpath.evaluate(xpathTemplate, vocabulary, XPathConstants.NODE); } private Node getNodeFromXPath(String xpathExpression) throws XPathExpressionException { @@ -465,6 +472,43 @@ private Choices getChoicesByXpath(String xpathExpression, int start, int limit) return new Choices(false); } + private Choices getChoicesByXpathWithId(String xpathTemplate, String nodeId, int start, int limit) { + List choices = new ArrayList<>(); + XPath xpath = XPathFactory.newInstance().newXPath(); + xpath.setXPathVariableResolver(variableName -> { + if ("nodeId".equals(variableName.getLocalPart())) { + return nodeId; + } + throw new IllegalArgumentException("Unexpected variable: " + variableName); + }); + try { + Node parentNode = (Node) xpath.evaluate(xpathTemplate, vocabulary, XPathConstants.NODE); + int count = 0; + if (parentNode != null) { + NodeList childNodes = (NodeList) xpath.evaluate(".//isComposedBy", parentNode, XPathConstants.NODE); + if (null != childNodes) { + for (int i = 0; i < childNodes.getLength(); i++) { + Node childNode = childNodes.item(i); + if (childNode != null && "node".equals(childNode.getNodeName())) { + if (count < start || choices.size() >= limit) { + count++; + continue; + } + count++; + choices.add(createChoiceFromNode(childNode)); + } + } + } + return new Choices(choices.toArray(new Choice[choices.size()]), start, count, + Choices.CF_AMBIGUOUS, false); + } + } catch (XPathExpressionException e) { + log.warn(e.getMessage(), e); + return new Choices(true); + } + return new Choices(false); + } + private Choice createChoiceFromNode(Node node) { if (node != null && !isRootElement(node)) { Choice choice = new Choice(getAuthority(node), getLabel(node), getValue(node), From b8b3e8df11109ac74e427dfb4be9be91817c77ca Mon Sep 17 00:00:00 2001 From: Juraj Roka <95219754+jr-rk@users.noreply.github.com> Date: Wed, 28 Oct 2026 13:14:28 +0100 Subject: [PATCH 3/3] Test that vocabulary node ids are not evaluated as XPath Covers the upstream DURACOM-447 change in getChoice, getChoicesByParent and getParentChoice. A 'pond' node under 's40' gives the parent and children lookups a real target. dspace-customers issue 1083, finding V4. Co-Authored-By: Claude Opus 5.5 --- .../config/controlled-vocabularies/farm.xml | 6 +++- .../DSpaceControlledVocabularyTest.java | 33 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/dspace-api/src/test/data/dspaceFolder/config/controlled-vocabularies/farm.xml b/dspace-api/src/test/data/dspaceFolder/config/controlled-vocabularies/farm.xml index c77d1c5ff291..e4c1469b6fd7 100644 --- a/dspace-api/src/test/data/dspaceFolder/config/controlled-vocabularies/farm.xml +++ b/dspace-api/src/test/data/dspaceFolder/config/controlled-vocabularies/farm.xml @@ -2,6 +2,10 @@ - + + + + + diff --git a/dspace-api/src/test/java/org/dspace/content/authority/DSpaceControlledVocabularyTest.java b/dspace-api/src/test/java/org/dspace/content/authority/DSpaceControlledVocabularyTest.java index 524c6407b7bd..b774aacbd8ee 100644 --- a/dspace-api/src/test/java/org/dspace/content/authority/DSpaceControlledVocabularyTest.java +++ b/dspace-api/src/test/java/org/dspace/content/authority/DSpaceControlledVocabularyTest.java @@ -10,6 +10,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import java.io.IOException; @@ -91,6 +92,38 @@ public void testGetMatches() throws IOException, ClassNotFoundException { assertEquals("north 40", result.values[0].value); } + /** + * Node ids (entryID, parent, child) must be compared as plain strings, not spliced into the XPath expression. + * @throws java.lang.ClassNotFoundException passed through. + */ + @Test + public void testNodeIdIsNotEvaluatedAsXPath() throws ClassNotFoundException { + DSpaceControlledVocabulary instance = (DSpaceControlledVocabulary) + CoreServiceFactory.getInstance().getPluginService() + .getNamedPlugin(Class.forName("org.dspace.content.authority.ChoiceAuthority"), "farm"); + assertNotNull(instance); + + Choice choice = instance.getChoice("s40", null); + assertNotNull(choice); + assertEquals("south 40", choice.label); + + // Spliced into the expression this would select //node[@id = 'x' or @label='south 40'] + assertNull(instance.getChoice("x' or @label='south 40", null)); + // ... and here the root node, returning its children + Choices children = instance.getChoicesByParent("farm", "x' or @label='the farm", 0, 10, null); + assertEquals(0, children.values.length); + + children = instance.getChoicesByParent("farm", "s40", 0, 10, null); + assertEquals(1, children.values.length); + assertEquals("pond", children.values[0].authority); + + Choice parent = instance.getParentChoice("farm", "pond", null); + assertNotNull(parent); + assertEquals("s40", parent.authority); + // ... and here the parent of 'pond' + assertNull(instance.getParentChoice("farm", "x' or @id='pond", null)); + } + /** * Test of getBestMatch method, of class DSpaceControlledVocabulary. */