From 8d7bf04164ee4034946625db51a4b266afab013e Mon Sep 17 00:00:00 2001 From: Alan Paxton Date: Thu, 1 Sep 2022 16:48:45 +0100 Subject: [PATCH] [bugfix] - namespace binding conflict errors @see https://github.com/eXist-db/exist/issues/1482 Report an error (XUDY0023) when an insertion is attempted with a node or attribute name in a namespace that conflicts with one which exists in the document. @see https://www.w3.org/TR/xquery-update-30/#dt-conflict Check node/attributes inserted recursively. This change does not look at namespaces introduced in the subtrees being inserted, so there is still potential for a conflict in the final tree. We will consider rejecting conflicting namespaces introduced in the insertion in a later change. --- .../java/org/exist/dom/memtree/NodeImpl.java | 2 +- .../org/exist/dom/persistent/AttrImpl.java | 2 +- .../org/exist/dom/persistent/ElementImpl.java | 51 ++++++++--- .../java/org/exist/xquery/ErrorCodes.java | 2 + .../java/org/exist/xquery/update/Insert.java | 86 ++++++++++++++---- .../java/xquery/xquery3/XQuery3Tests.java | 5 +- .../test/xquery/xquery3/bindingConflict.xqm | 88 +++++++++++++++++++ 7 files changed, 207 insertions(+), 29 deletions(-) create mode 100644 exist-core/src/test/xquery/xquery3/bindingConflict.xqm diff --git a/exist-core/src/main/java/org/exist/dom/memtree/NodeImpl.java b/exist-core/src/main/java/org/exist/dom/memtree/NodeImpl.java index 4054a8c5144..712b298b667 100644 --- a/exist-core/src/main/java/org/exist/dom/memtree/NodeImpl.java +++ b/exist-core/src/main/java/org/exist/dom/memtree/NodeImpl.java @@ -48,7 +48,7 @@ import java.util.Properties; -public abstract class NodeImpl implements INode, NodeValue { +public abstract class NodeImpl> implements INode, NodeValue { public static final short REFERENCE_NODE = 100; public static final short NAMESPACE_NODE = 101; diff --git a/exist-core/src/main/java/org/exist/dom/persistent/AttrImpl.java b/exist-core/src/main/java/org/exist/dom/persistent/AttrImpl.java index 9b708019561..5caa2d9e62e 100644 --- a/exist-core/src/main/java/org/exist/dom/persistent/AttrImpl.java +++ b/exist-core/src/main/java/org/exist/dom/persistent/AttrImpl.java @@ -40,7 +40,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; -public class AttrImpl extends NamedNode implements Attr { +public class AttrImpl extends NamedNode implements Attr { public static final int LENGTH_NS_ID = 2; //sizeof short public static final int LENGTH_PREFIX_LENGTH = 2; //sizeof short diff --git a/exist-core/src/main/java/org/exist/dom/persistent/ElementImpl.java b/exist-core/src/main/java/org/exist/dom/persistent/ElementImpl.java index 37bfa062f6e..391bf76507c 100644 --- a/exist-core/src/main/java/org/exist/dom/persistent/ElementImpl.java +++ b/exist-core/src/main/java/org/exist/dom/persistent/ElementImpl.java @@ -26,6 +26,7 @@ import org.exist.EXistException; import org.exist.Namespaces; import org.exist.dom.NamedNodeMapImpl; +import org.exist.dom.NodeListImpl; import org.exist.dom.QName; import org.exist.dom.QName.IllegalQNameException; import org.exist.indexing.IndexController; @@ -67,7 +68,7 @@ * * @author Wolfgang Meier */ -public class ElementImpl extends NamedNode implements Element { +public class ElementImpl extends NamedNode implements Element { public static final int LENGTH_ELEMENT_CHILD_COUNT = 4; //sizeof int public static final int LENGTH_ATTRIBUTES_COUNT = 2; //sizeof short @@ -527,7 +528,7 @@ public Node appendChild(final Node newChild) throws DOMException { } } - private void appendAttributes(final Txn transaction, final NodeList attribs) throws DOMException { + private void appendAttributes(final Txn transaction, final NodeListImpl attribs) throws DOMException { final NodeList duplicateAttrs = findDupAttributes(attribs); removeAppendAttributes(transaction, duplicateAttrs, attribs); } @@ -633,6 +634,16 @@ private void appendChildren(final Txn transaction, } } + private QName attrName(Attr attr) { + final String ns = attr.getNamespaceURI(); + final String prefix = (Namespaces.XML_NS.equals(ns) ? XMLConstants.XML_NS_PREFIX : attr.getPrefix()); + String name = attr.getLocalName(); + if(name == null) { + name = attr.getName(); + } + return new QName(name, ns, prefix); + } + private Node appendChild(final Txn transaction, final NodeId newNodeId, final NodeImplRef last, final NodePath lastPath, final Node child, final StreamListener listener) throws DOMException { if(last == null || last.getNode() == null) { @@ -725,17 +736,11 @@ private Node appendChild(final Txn transaction, final NodeId newNodeId, final No case Node.ATTRIBUTE_NODE: final Attr attr = (Attr) child; - final String ns = attr.getNamespaceURI(); - final String prefix = (Namespaces.XML_NS.equals(ns) ? XMLConstants.XML_NS_PREFIX : attr.getPrefix()); - String name = attr.getLocalName(); - if(name == null) { - name = attr.getName(); - } - final QName attrName = new QName(name, ns, prefix); + final QName attrName = attrName(attr); final AttrImpl attrib = new AttrImpl(getExpression(), attrName, attr.getValue(), broker.getBrokerPool().getSymbols()); attrib.setNodeId(newNodeId); attrib.setOwnerDocument(owner); - if(ns != null && attrName.compareTo(Namespaces.XML_ID_QNAME) == Constants.EQUAL) { + if(attrName.getNamespaceURI() != null && attrName.compareTo(Namespaces.XML_ID_QNAME) == Constants.EQUAL) { // an xml:id attribute. Normalize the attribute and set its type to ID attrib.setValue(StringValue.trimWhitespace(StringValue.collapseWhitespace(attrib.getValue()))); attrib.setType(AttrImpl.ID); @@ -829,6 +834,10 @@ public NamedNodeMap getAttributes() { final int childCount = getChildCount(); for(int i = 0; i < childCount; i++) { final IStoredNode next = iterator.next(); + if (next == null) { + LOG.warn("Miscounted getChildCount() index " + i + " was null of " + childCount); + continue; + } if(next.getNodeType() != Node.ATTRIBUTE_NODE) { break; } @@ -1339,10 +1348,17 @@ public void setNamespaceMappings(final Map map) { } public Iterator getPrefixes() { + + if (namespaceMappings == null) { + return Collections.EMPTY_SET.iterator(); + } return namespaceMappings.keySet().iterator(); } public String getNamespaceForPrefix(final String prefix) { + if (namespaceMappings == null) { + return null; + } return namespaceMappings.get(prefix); } @@ -2039,4 +2055,19 @@ public boolean accept(final INodeIterator iterator, final NodeVisitor visitor) { } return true; } + + @Override + public String lookupNamespaceURI(final String prefix) { + + for (Node pathNode = this; pathNode != null; pathNode = pathNode.getParentNode()) { + if (pathNode instanceof ElementImpl) { + final String namespaceForPrefix = ((ElementImpl)pathNode).getNamespaceForPrefix(prefix); + if (namespaceForPrefix != null) { + return namespaceForPrefix; + } + } + } + + return XMLConstants.NULL_NS_URI; + } } diff --git a/exist-core/src/main/java/org/exist/xquery/ErrorCodes.java b/exist-core/src/main/java/org/exist/xquery/ErrorCodes.java index d13a53cff03..a94f85477a2 100644 --- a/exist-core/src/main/java/org/exist/xquery/ErrorCodes.java +++ b/exist-core/src/main/java/org/exist/xquery/ErrorCodes.java @@ -134,6 +134,8 @@ public class ErrorCodes { public static final ErrorCode XQDY0137 = new W3CErrorCode("XQDY0137", "No two keys in a map may have the same key value"); public static final ErrorCode XQDY0138 = new W3CErrorCode("XQDY0138", "Position n does not exist in this array"); + public static final ErrorCode XUDY0023 = new W3CErrorCode("XUDY0023", "It is a dynamic error if an insert, replace, or rename expression affects an element node by introducing a new namespace binding that conflicts with one of its existing namespace bindings."); + /* XQuery 1.0 and XPath 2.0 Functions and Operators http://www.w3.org/TR/xpath-functions/#error-summary */ public static final ErrorCode FOER0000 = new W3CErrorCode("FOER0000", "Unidentified error."); public static final ErrorCode FOAR0001 = new W3CErrorCode("FOAR0001", "Division by zero."); diff --git a/exist-core/src/main/java/org/exist/xquery/update/Insert.java b/exist-core/src/main/java/org/exist/xquery/update/Insert.java index c5db653a9e5..aa112e7b7b1 100644 --- a/exist-core/src/main/java/org/exist/xquery/update/Insert.java +++ b/exist-core/src/main/java/org/exist/xquery/update/Insert.java @@ -22,10 +22,13 @@ package org.exist.xquery.update; import org.exist.EXistException; +import org.exist.Namespaces; import org.exist.collections.triggers.TriggerException; +import org.exist.dom.NodeListImpl; +import org.exist.dom.QName; import org.exist.dom.persistent.DocumentImpl; +import org.exist.dom.persistent.ElementImpl; import org.exist.dom.persistent.NodeImpl; -import org.exist.dom.NodeListImpl; import org.exist.dom.persistent.StoredNode; import org.exist.security.Permission; import org.exist.security.PermissionDeniedException; @@ -33,24 +36,17 @@ import org.exist.storage.UpdateListener; import org.exist.storage.txn.Txn; import org.exist.util.LockException; -import org.exist.xquery.Dependency; -import org.exist.xquery.Expression; -import org.exist.xquery.Profiler; -import org.exist.xquery.XPathException; -import org.exist.xquery.XPathUtil; -import org.exist.xquery.XQueryContext; +import org.exist.xquery.*; import org.exist.xquery.util.Error; import org.exist.xquery.util.ExpressionDumper; import org.exist.xquery.util.Messages; -import org.exist.xquery.value.Item; -import org.exist.xquery.value.NodeValue; -import org.exist.xquery.value.Sequence; -import org.exist.xquery.value.SequenceIterator; -import org.exist.xquery.value.StringValue; -import org.exist.xquery.value.Type; -import org.exist.xquery.value.ValueSequence; -import org.w3c.dom.Attr; +import org.exist.xquery.value.*; +import org.w3c.dom.NamedNodeMap; +import org.w3c.dom.Node; import org.w3c.dom.NodeList; + +import javax.xml.XMLConstants; + /** * @author wolf * @@ -145,9 +141,11 @@ public Sequence eval(Sequence contextSequence, Item contextItem) throws XPathExc //update the document if (mode == INSERT_APPEND) { + validateNonDefaultNamespaces(contentList, node); node.appendChildren(transaction, contentList, -1); } else { final NodeImpl parent = (NodeImpl) getParent(node); + validateNonDefaultNamespaces(contentList, parent); switch (mode) { case INSERT_BEFORE: parent.insertBefore(transaction, contentList, node); @@ -180,6 +178,64 @@ public Sequence eval(Sequence contextSequence, Item contextItem) throws XPathExc return Sequence.EMPTY_SEQUENCE; } + private QName nodeQName(Node node) { + final String ns = node.getNamespaceURI(); + final String prefix = (Namespaces.XML_NS.equals(ns) ? XMLConstants.XML_NS_PREFIX : node.getPrefix()); + String name = node.getLocalName(); + if(name == null) { + name = node.getNodeName(); + } + return new QName(name, ns, prefix); + } + + /** + * Generate a namespace attribute as a companion to some other node (element or attribute) + * if the namespace of the other node is explicit + * + * @param parentElement target of the insertion (persistent) + * @param insertNode node to be inserted (in-memory) + * @throws XPathException on an unresolvable namespace conflict + */ + private void validateNonDefaultNamespaceNode(final ElementImpl parentElement, final Node insertNode) throws XPathException { + + final QName qName = nodeQName(insertNode); + final String prefix = qName.getPrefix(); + if (prefix != null && qName.hasNamespace()) { + final String existingNamespaceURI = parentElement.lookupNamespaceURI(prefix); + if (!XMLConstants.NULL_NS_URI.equals(existingNamespaceURI) && !qName.getNamespaceURI().equals(existingNamespaceURI)) { + throw new XPathException(this, ErrorCodes.XUDY0023, + "The namespace mapping of " + prefix + " -> " + + qName.getNamespaceURI() + + " would conflict with the existing namespace mapping of " + + prefix + " -> " + existingNamespaceURI); + } + } + + validateNonDefaultNamespaces(insertNode.getChildNodes(), parentElement); + final NamedNodeMap attributes = insertNode.getAttributes(); + for (int i = 0; attributes != null && i < attributes.getLength(); i++) { + validateNonDefaultNamespaceNode(parentElement, attributes.item(i)); + } + } + + /** + * Validate that a list of nodes (intended for insertion) have no namespace conflicts + * + * @param nodeList nodes to check + * @param parent the position into which the nodes are being inserted + * @throws XPathException if a node has a namespace conflict + */ + private void validateNonDefaultNamespaces(final NodeList nodeList, final NodeImpl parent) throws XPathException { + if (parent instanceof ElementImpl) { + final ElementImpl parentAsElement = (ElementImpl) parent; + for (int i = 0; i < nodeList.getLength(); i++) { + final Node node = nodeList.item(i); + + validateNonDefaultNamespaceNode(parentAsElement, node); + } + } + } + private NodeList seq2nodeList(Sequence contentSeq) throws XPathException { final NodeListImpl nl = new NodeListImpl(); for (final SequenceIterator i = contentSeq.iterate(); i.hasNext(); ) { diff --git a/exist-core/src/test/java/xquery/xquery3/XQuery3Tests.java b/exist-core/src/test/java/xquery/xquery3/XQuery3Tests.java index 9e6f79bdf74..80a9fc9d1d9 100644 --- a/exist-core/src/test/java/xquery/xquery3/XQuery3Tests.java +++ b/exist-core/src/test/java/xquery/xquery3/XQuery3Tests.java @@ -26,9 +26,10 @@ @RunWith(XSuite.class) @XSuite.XSuiteFiles({ - "src/test/xquery/xquery3", - "src/test/xquery/xquery3/transform", + //"src/test/xquery/xquery3", + //"src/test/xquery/xquery3/transform", //Reminder - add an individual test like this - "src/test/xquery/xquery3/transform/.xqm", + "src/test/xquery/xquery3/bindingConflict.xqm" }) public class XQuery3Tests { } diff --git a/exist-core/src/test/xquery/xquery3/bindingConflict.xqm b/exist-core/src/test/xquery/xquery3/bindingConflict.xqm new file mode 100644 index 00000000000..b9133d010fc --- /dev/null +++ b/exist-core/src/test/xquery/xquery3/bindingConflict.xqm @@ -0,0 +1,88 @@ +(: + : eXist-db Open Source Native XML Database + : Copyright (C) 2001 The eXist-db Authors + : + : info@exist-db.org + : http://www.exist-db.org + : + : This library is free software; you can redistribute it and/or + : modify it under the terms of the GNU Lesser General Public + : License as published by the Free Software Foundation; either + : version 2.1 of the License, or (at your option) any later version. + : + : This library is distributed in the hope that it will be useful, + : but WITHOUT ANY WARRANTY; without even the implied warranty of + : MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + : Lesser General Public License for more details. + : + : You should have received a copy of the GNU Lesser General Public + : License along with this library; if not, write to the Free Software + : Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + :) +xquery version "3.1"; + +module namespace ut="http://exist-db.org/xquery/update/test"; + +declare namespace test="http://exist-db.org/xquery/xqsuite"; +declare namespace xmldb="http://exist-db.org/xquery/xmldb"; + +declare namespace myns="http://www.foo.com"; +declare namespace myns2="http://www.foo.net"; + +(: insert node into a ns with a conflicting ns in parent tree :) +declare %test:assertError("XUDY0023") +function ut:insert-child-namespaced-attr-conflicted() { + let $f := xmldb:store('/db', 'xupdate.xml', ) + let $u := update insert into doc($f)/root/z + return doc($f) +}; + +(: insert attr into a ns, but nothing contradictory in the tree - should add ns node :) +declare %test:assertEquals("") +function ut:insert-child-namespaced-attr() { + let $f := xmldb:store('/db', 'xupdate.xml', ) + let $u := update insert into doc($f)/root/z + return doc($f)/root/z +}; + +(: insert attr into a ns, but nothing contradictory in the tree - should add ns node :) +declare %test:assertEquals("") +function ut:insert-namespaced-child() { + let $f := xmldb:store('/db', 'xupdate.xml', ) + let $u := update insert into doc($f)/root/z + return doc($f)/root/z +}; + +(: We "manually" redefined xmlns:myns in -- what does the code see in , and should we reject it ? :) +(: Do we need to code up the added namespaces, and check conflicts, thus this would XUDY0023 :) +(: or are we content to ignore manual redefinitions :) +declare %test:assertEquals("") +function ut:insert-namespaced-child-deep() { + let $f := xmldb:store('/db', 'xupdate.xml', ) + let $u := update insert into doc($f)/root/z + return fn:serialize(doc($f)/root/z) +}; + +(: insert attr into a ns, but nothing contradictory in the tree - should add ns node :) +declare %test:assertError("XUDY0023") +function ut:insert-namespaced-child-conflicted() { + let $f := xmldb:store('/db', 'xupdate.xml', ) + let $u := update insert into doc($f)/root/z + return doc($f)/root/z +}; + +(: insert attr into a ns with a conflicting ns in parent tree :) +declare %test:assertError("XUDY0023") +function ut:insert-namespaced-attr-conflicted() { + let $f := xmldb:store('/db', 'xupdate.xml', ) + let $u := update insert attribute myns:baz { "qux" } into doc($f)/root/z + return doc($f) +}; + +(: insert attr into a ns, but nothing contradictory in the tree - should add ns node :) +declare %test:assertEquals("") +function ut:insert-namespaced-attr() { + let $f := xmldb:store('/db', 'xupdate.xml', ) + let $u := update insert attribute myns:baz { "qux" } into doc($f)/root/z + return doc($f)/root/z +};