Skip to content

Fix dspace-customers 1083 V4: bind vocabulary node id as an XPath variable #1556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
milanmajchrak merged 3 commits into from
Oct 28, 2026
Merged

Fix dspace-customers 1083 V4: bind vocabulary node id as an XPath variable #1556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e37bc04
Port upstream DSpace PR 11075 to 7_x
mwoodiupui Jul 28, 2025
3e0555a
[DURACOM-447] Fix Xpath injection
AdamF42 Mar 15, 2026
b8b3e8d
Test that vocabulary node ids are not evaluated as XPath
jr-rk Oct 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 103 additions & 42 deletions dspace-api/src/main/java/org/dspace/content/authority/DSpaceControlledVocabulary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,36 +34,38 @@
* 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 = \
* <p>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
* <p>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 = <true|false>
* # Store entire hierarchy along with selected value. Default: TRUE
* vocabulary.plugin._plugin_.hierarchy.suggest =
* <true|false> # Display entire hierarchy in the suggestion list. Default: TRUE
* vocabulary.plugin._plugin_.delimiter = "<string>"
* # Delimiter to use when building hierarchy strings. Default: "::"
* }
* <p>Each configured plugin comes with three configuration options:
* <ul>
* <li>{@code vocabulary.plugin._plugin_.hierarchy.store = <true|false>}
* # Store entire hierarchy along with selected value. Default: TRUE</li>
* <li>{@code vocabulary.plugin._plugin_.hierarchy.suggest =
* <true|false> # Display entire hierarchy in the suggestion list. Default: TRUE}</li>
* <li>{@code vocabulary.plugin._plugin_.delimiter = "<string>"
* # Delimiter to use when building hierarchy strings. Default: "::"}</li>
* </ul>
*
* @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']";
protected static String idParentTemplate = "//node[@id = '%s']/parent::isComposedBy/parent::node";
"'abcdefghijklmnopqrstuvwxyz'),%s)]";
protected static String idTemplate = "//node[@id = %s]";
protected static String idTemplateQuoted = "//node[@id = $nodeId]";
protected static String labelTemplate = "//node[@label = %s]";
protected static String idParentTemplate = "//node[@id = $nodeId]/parent::isComposedBy/parent::node";
protected static String rootTemplate = "/node";
protected static String pluginNames[] = null;

Expand Down Expand Up @@ -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<String> names = new ArrayList<String>();
List<String> names = new ArrayList<>();
for (String filename : xmlFiles) {
names.add((new File(filename)).getName().replace(".xml", ""));
}
Expand Down Expand Up @@ -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("'", "&apos;").toLowerCase());
xpathExpressionBuilder.append(String.format(xpathTemplate, "$var" + i));
}
String xpathExpression = xpathExpressionBuilder.toString();
XPath xpath = XPathFactory.newInstance().newXPath();
int total = 0;
List<Choice> choices = new ArrayList<Choice>();
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<Choice> choices;
try {
NodeList results = (NodeList) xpath.evaluate(xpathExpression, vocabulary, XPathConstants.NODESET);
total = results.getLength();
Expand All @@ -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("'", "&apos;"));
xpathExpressionBuilder.append(String.format(labelTemplate, "$var" + i));
}
String xpathExpression = xpathExpressionBuilder.toString();
XPath xpath = XPathFactory.newInstance().newXPath();
List<Choice> choices = new ArrayList<Choice>();
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<Choice> choices;
try {
NodeList results = (NodeList) xpath.evaluate(xpathExpression, vocabulary, XPathConstants.NODESET);
choices = getChoicesFromNodeList(results, 0, 1);
Expand Down Expand Up @@ -240,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(idTemplate, 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);
Expand All @@ -264,17 +282,23 @@ 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);
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 {
Expand All @@ -284,7 +308,7 @@ private Node getNodeFromXPath(String xpathExpression) throws XPathExpressionExce
}

private List<Choice> getChoicesFromNodeList(NodeList results, int start, int limit) {
List<Choice> choices = new ArrayList<Choice>();
List<Choice> choices = new ArrayList<>();
for (int i = 0; i < results.getLength(); i++) {
if (i < start) {
continue;
Expand All @@ -303,14 +327,14 @@ private List<Choice> getChoicesFromNodeList(NodeList results, int start, int lim

private Map<String, String> addOtherInformation(String parentCurr, String noteCurr,
List<String> childrenCurr, String authorityCurr) {
Map<String, String> extras = new HashMap<String, String>();
Map<String, String> 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");
Expand Down Expand Up @@ -368,7 +392,7 @@ private String getNote(Node node) {
}

private List<String> getChildren(Node node) {
List<String> children = new ArrayList<String>();
List<String> children = new ArrayList<>();
NodeList childNodes = node.getChildNodes();
for (int ci = 0; ci < childNodes.getLength(); ci++) {
Node firstChild = childNodes.item(ci);
Expand All @@ -391,7 +415,7 @@ private List<String> 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;
}
Expand All @@ -418,7 +442,7 @@ private String getAuthority(Node node) {
}

private Choices getChoicesByXpath(String xpathExpression, int start, int limit) {
List<Choice> choices = new ArrayList<Choice>();
List<Choice> choices = new ArrayList<>();
XPath xpath = XPathFactory.newInstance().newXPath();
try {
Node parentNode = (Node) xpath.evaluate(xpathExpression, vocabulary, XPathConstants.NODE);
Expand Down Expand Up @@ -448,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<Choice> 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),
Expand Down
6 changes: 5 additions & 1 deletion dspace-api/src/test/data/dspaceFolder/config/controlled-vocabularies/farm.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
<node label='the farm'>
<isComposedBy>
<node label='north 40'/>
<node id='s40' label='south 40'/>
<node id='s40' label='south 40'>
<isComposedBy>
<node id='pond' label='pond'/>
</isComposedBy>
</node>
</isComposedBy>
</node>
35 changes: 35 additions & 0 deletions dspace-api/src/test/java/org/dspace/content/authority/DSpaceControlledVocabularyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
package org.dspace.content.authority;

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;

Expand Down Expand Up @@ -86,9 +88,42 @@ 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);
}

/**
* 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.
*/
Expand Down
Loading