Copilot commented on code in PR #898: URL: https://github.com/apache/maven-wagon/pull/898#discussion_r3738788661
########## wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/MultiStatus.java: ########## @@ -0,0 +1,255 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.wagon.providers.webdav; + +import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; + +import java.io.IOException; +import java.io.InputStream; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; +import org.xml.sax.SAXException; +import org.xml.sax.helpers.DefaultHandler; + +import static org.apache.maven.wagon.providers.webdav.DavMethods.DAV_NAMESPACE; +import static org.apache.maven.wagon.providers.webdav.DavMethods.PROPERTY_RESOURCETYPE; +import static org.apache.maven.wagon.providers.webdav.DavMethods.XML_COLLECTION; + +/** + * The {@code 207 Multi-Status} body of a PROPFIND response, reduced to what this Wagon needs: the + * href of each response, in document order, and whether that response describes a collection. + * <p> + * Responses keep their document order because {@code getFileList} skips the first one, taking it to + * be the requested collection itself. RFC 4918 does not order responses; that a server lists the + * request URI first is an observed behaviour, and the assumption predates this class. + * + * @since 4.0.0 + */ +final class MultiStatus { + + private static final String DISALLOW_DOCTYPE_DECL = "http://apache.org/xml/features/disallow-doctype-decl";; + + private static final String EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities";; + + private static final String EXTERNAL_PARAMETER_ENTITIES = "http://xml.org/sax/features/external-parameter-entities";; + + private final List<Response> responses; + + private MultiStatus(List<Response> responses) { + this.responses = responses; + } + + /** + * A single {@code DAV:response} element. + */ + static final class Response { + private final String href; + private final boolean collection; + + Response(String href, boolean collection) { + this.href = href; + this.collection = collection; + } + + String getHref() { + return href; + } + + /** + * Whether the {@code resourcetype} property carried a {@code collection} child. Only + * {@code propstat} elements with a {@code 200} status are considered; a resource whose + * {@code resourcetype} is absent or empty is not a collection. + */ + boolean isCollection() { + return collection; + } + } + + List<Response> getResponses() { + return responses; + } + + /** + * Parses a multistatus document. + * + * @param in the response body, never {@code null} + * @return the parsed responses, possibly empty but never {@code null} + * @throws IOException if the parser cannot be configured, or if the body is not a well-formed + * multistatus document + */ + static MultiStatus parse(InputStream in) throws IOException { + DocumentBuilder builder; + try { + builder = newDocumentBuilder(); + } catch (ParserConfigurationException e) { + // a configuration problem is not the server's fault, so say so rather than blaming the + // response body + throw new IOException("XML parser configuration error: " + e.getMessage(), e); + } + + Document document; + try { + document = builder.parse(in); + } catch (SAXException e) { + throw new IOException("Cannot parse multistatus response: " + e.getMessage(), e); + } + + Element root = document.getDocumentElement(); + if (root == null || !isDavElement(root, "multistatus")) { + throw new IOException("Expected a DAV:multistatus response body"); + } + + // an href must occur only once per RFC 4918; should a server repeat one, the last wins and + // keeps the position of the first, which is what the Jackrabbit-backed code did. A + // LinkedHashMap also preserves the document order the callers depend on. + Map<String, Response> responses = new LinkedHashMap<>(); + for (Element response : childElements(root, "response")) { + String href = null; + for (Element hrefElement : childElements(response, "href")) { + href = hrefElement.getTextContent(); + break; + } + if (href != null) { + href = href.trim(); + responses.put(href, new Response(href, isCollection(response))); + } Review Comment: A `DAV:response` requires a non-empty `DAV:href`; silently dropping a malformed response changes the prior Jackrabbit behavior, which rejected it. This can also shift a later child into position 0, making `isDirectory` inspect the wrong resource and making `getFileList` skip the wrong entry. Reject missing or blank hrefs as parse errors. ########## wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java: ########## @@ -209,21 +191,17 @@ public List<String> getFileList(String destinationDirectory) String repositoryUrl = repository.getUrl(); String url = repositoryUrl + (repositoryUrl.endsWith("/") ? "" : "/") + destinationDirectory; - HttpPropfind method = null; + DavMethods.HttpPropfind method = null; CloseableHttpResponse closeableHttpResponse = null; try { if (isDirectory(url)) { - DavPropertyNameSet nameSet = new DavPropertyNameSet(); - nameSet.add(DavPropertyName.create(DavConstants.PROPERTY_DISPLAYNAME)); - - method = new HttpPropfind(url, nameSet, DavConstants.DEPTH_1); + method = new DavMethods.HttpPropfind(url, PROPERTY_DISPLAYNAME, DEPTH_1); closeableHttpResponse = execute(method); - if (method.succeeded(closeableHttpResponse)) { + List<MultiStatus.Response> responses = readMultiStatus(closeableHttpResponse); + if (responses != null) { ArrayList<String> dirs = new ArrayList<>(); - MultiStatus multiStatus = method.getResponseBodyAsMultiStatus(closeableHttpResponse); - for (int i = 0; i < multiStatus.getResponses().length; i++) { - MultiStatusResponse response = multiStatus.getResponses()[i]; - String entryUrl = response.getHref(); + for (int i = 0; i < responses.size(); i++) { + String entryUrl = responses.get(i).getHref(); Review Comment: The loop index is still used below to skip response 0 as the collection itself, but RFC 4918 does not assign significance to `response` order. A compliant Depth-1 response can put a child first, causing that child directory to be dropped and the requested collection to be returned as an entry. Identify and skip the response whose normalized `href` matches the requested URI instead of relying on `i == 0`. ########## wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/MultiStatus.java: ########## @@ -0,0 +1,255 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.wagon.providers.webdav; + +import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; + +import java.io.IOException; +import java.io.InputStream; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; +import org.xml.sax.SAXException; +import org.xml.sax.helpers.DefaultHandler; + +import static org.apache.maven.wagon.providers.webdav.DavMethods.DAV_NAMESPACE; +import static org.apache.maven.wagon.providers.webdav.DavMethods.PROPERTY_RESOURCETYPE; +import static org.apache.maven.wagon.providers.webdav.DavMethods.XML_COLLECTION; + +/** + * The {@code 207 Multi-Status} body of a PROPFIND response, reduced to what this Wagon needs: the + * href of each response, in document order, and whether that response describes a collection. + * <p> + * Responses keep their document order because {@code getFileList} skips the first one, taking it to + * be the requested collection itself. RFC 4918 does not order responses; that a server lists the + * request URI first is an observed behaviour, and the assumption predates this class. + * + * @since 4.0.0 + */ +final class MultiStatus { + + private static final String DISALLOW_DOCTYPE_DECL = "http://apache.org/xml/features/disallow-doctype-decl";; + + private static final String EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities";; + + private static final String EXTERNAL_PARAMETER_ENTITIES = "http://xml.org/sax/features/external-parameter-entities";; + + private final List<Response> responses; + + private MultiStatus(List<Response> responses) { + this.responses = responses; + } + + /** + * A single {@code DAV:response} element. + */ + static final class Response { + private final String href; + private final boolean collection; + + Response(String href, boolean collection) { + this.href = href; + this.collection = collection; + } + + String getHref() { + return href; + } + + /** + * Whether the {@code resourcetype} property carried a {@code collection} child. Only + * {@code propstat} elements with a {@code 200} status are considered; a resource whose + * {@code resourcetype} is absent or empty is not a collection. + */ + boolean isCollection() { + return collection; + } + } + + List<Response> getResponses() { + return responses; + } + + /** + * Parses a multistatus document. + * + * @param in the response body, never {@code null} + * @return the parsed responses, possibly empty but never {@code null} + * @throws IOException if the parser cannot be configured, or if the body is not a well-formed + * multistatus document + */ + static MultiStatus parse(InputStream in) throws IOException { + DocumentBuilder builder; + try { + builder = newDocumentBuilder(); + } catch (ParserConfigurationException e) { + // a configuration problem is not the server's fault, so say so rather than blaming the + // response body + throw new IOException("XML parser configuration error: " + e.getMessage(), e); + } + + Document document; + try { + document = builder.parse(in); + } catch (SAXException e) { + throw new IOException("Cannot parse multistatus response: " + e.getMessage(), e); + } + + Element root = document.getDocumentElement(); + if (root == null || !isDavElement(root, "multistatus")) { + throw new IOException("Expected a DAV:multistatus response body"); + } + + // an href must occur only once per RFC 4918; should a server repeat one, the last wins and + // keeps the position of the first, which is what the Jackrabbit-backed code did. A + // LinkedHashMap also preserves the document order the callers depend on. + Map<String, Response> responses = new LinkedHashMap<>(); + for (Element response : childElements(root, "response")) { + String href = null; + for (Element hrefElement : childElements(response, "href")) { + href = hrefElement.getTextContent(); + break; + } + if (href != null) { + href = href.trim(); + responses.put(href, new Response(href, isCollection(response))); + } + } + return new MultiStatus(Collections.unmodifiableList(new ArrayList<>(responses.values()))); + } + + /** + * Looks for {@code resourcetype/collection} inside any {@code propstat} that reported a + * {@code 200} status. + */ + private static boolean isCollection(Element response) { + for (Element propstat : childElements(response, "propstat")) { + if (!isOkStatus(propstat)) { + continue; + } + for (Element prop : childElements(propstat, "prop")) { + for (Element resourceType : childElements(prop, PROPERTY_RESOURCETYPE)) { + if (!childElements(resourceType, XML_COLLECTION).isEmpty()) { + return true; + } + } + } + } + return false; + } + + /** + * Reads the {@code status} child, whose text is a status line such as {@code HTTP/1.1 200 OK}. + * <p> + * RFC 4918 requires the element, and a {@code propstat} lacking one used to be skipped + * outright, which made every property of such a response invisible. It is read as successful + * here instead, so that a server omitting the status still gets its properties honoured. + */ + private static boolean isOkStatus(Element propstat) { + List<Element> statusElements = childElements(propstat, "status"); + if (statusElements.isEmpty()) { + return true; Review Comment: Treating a missing mandatory `status` as success contradicts the stated fidelity rule that only propstats reporting 200 are consulted. Jackrabbit ignored a propstat unless both `status` and `prop` were present; this fallback can therefore classify a collection from a property whose success was never reported. Return false when status is absent and update the corresponding test. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
