Replace the jackrabbit-webdav dependency with the WebDAV requests actually used

[slachiewicz]

wagon-webdav-jackrabbit speaks a very small subset of WebDAV. Its entire use of jackrabbit-webdav is three requests:

1. MKCOL — create a collection when deploying.
2. PROPFIND Depth: 0 on resourcetype — is this a collection? (isDirectory)
3. PROPFIND Depth: 1 — list a collection. (getFileList)

The third is thinner than it looks: it nominally asks for displayname but never reads it, deriving every entry from the response href.

jackrabbit-webdav models all of WebDAV — locking, observation, ordering, versioning, search, transactions. Three requests were pulling in a dependency whose scope far exceeds what is needed. This moves the requests and the multistatus parsing into the module, in Wagon's own package rather than squatting Jackrabbit's.

Commits

1. Remove the vendored copies of two Jackrabbit classes. This module shipped org/apache/jackrabbit/webdav/MultiStatus.class, which also exists in jackrabbit-webdav — so which of the two won depended on classpath order. It was forked only to keep responses in document order, and Jackrabbit's own MultiStatus has since become a LinkedHashMap, so the fork no longer differs from the class it shadows. XmlRequestEntity is left over from Jackrabbit 2.14, which no longer ships or references it, and nothing in this module used it. This commit stands alone — the full suite passes with the dependency still in place.
2. Replace the dependency with DavMethods (99 lines) and MultiStatus (207 lines).

Fidelity

The parser preserves what the callers rely on:

• Responses stay in document order, because getFileList expects the requested collection first, per RFC 4918 §9.1.
• Hrefs are deduplicated, matching Jackrabbit's href-keyed map.
• Only propstat elements reporting 200 are consulted for resourcetype.
• Elements are matched on local name in the DAV: namespace, tolerating servers that use a default namespace or a different prefix.

Two behaviour changes, both in the safe direction: a PROPFIND answering something other than 207 Multi-Status is no longer parsed for a body, and an empty multistatus no longer raises ArrayIndexOutOfBoundsException from isDirectory. Since a multistatus body is remote input, the parser also refuses DOCTYPE declarations.

Verification

• 292 tests pass — the 283 already here, including the integration tests that run against a real WebDAV server, plus 9 new MultiStatusTest cases.
• The existing suite genuinely covers this code: changing the collection element name to a bogus string fails 10 tests across both the integration and unit tests, so it is not passing vacuously.
• The built jar now contains 7 classes, all under org.apache.maven.wagon.providers.webdav, and no longer places anything in the org.apache.jackrabbit namespace.

Notes

The artifactId keeps the jackrabbit name so consumers do not break, even though the dependency is gone.

This also unblocks a future move to HttpClient 5. jackrabbit-webdav is on httpclient 4.5.14 in every release including the latest 2.22.4 and 2.23.x-beta, so it pinned this module to HttpClient 4 regardless of version. What remains here is two base classes (HttpRequestBase / HttpEntityEnclosingRequestBase), which become HttpUriRequestBase in HttpClient 5.

---

• Your pull request should address just one issue, without pulling in other changes.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
• Run mvn verify to make sure basic checks pass.
• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

[slachiewicz]

Pushed a third commit addressing review feedback. Four fixes, all in the new code:

The parser could hard-break the transport. disallow-doctype-decl is a Xerces feature, and setFeature throws for features a parser does not know. DocumentBuilderFactory.newInstance() is overridable via system property and ServiceLoader, and Maven runs with plugin and extension classpaths that can carry alternate JAXP implementations — so a non-platform parser would have made every PROPFIND fail. Worse, the exception was caught alongside SAXException and reported as Cannot parse multistatus response, blaming a server that had answered perfectly. The feature is now applied only where supported, falling back to refusing external general and parameter entities, and a configuration failure is reported as one.

No ErrorHandler on the DocumentBuilder. Without one the parser writes [Fatal Error] :1:1: ... to stderr before throwing. A server answering 207 with an HTML error page would have littered the build output on top of the wrapped exception. Verified the noise and confirmed it is gone.

A 207 with no body was downgraded to a missing resource. It was read as "not a collection", which surfaced as ResourceDoesNotExistException — telling the resolver the artifact is simply absent, when in fact the transport misbehaved. It is now an IOException, so it reaches callers as TransferFailedException as it did before.

Comment corrected on repeated hrefs. It claimed later duplicates were dropped; LinkedHashMap.put in fact lets the last win at the first one's position. That matches what the Jackrabbit-backed code did, so the behaviour stands and the comment was wrong. The test now pins which entry wins rather than only asserting the count.

Two deliberate semantic differences from the Jackrabbit version, now recorded in javadoc and covered by tests:

• A propstat without a DAV:status is read as successful. RFC 4918 requires the element; Jackrabbit skipped any propstat lacking it, which made every property of such a response invisible.
• A DOCTYPE is refused outright rather than neutered by an EntityResolver.

293 tests pass.

[slachiewicz]

A second review pass turned up two things the new code asserted about itself that were simply untrue. Both corrected in the latest commit.

The javadoc invented an RFC guarantee. It said getFileList may take the first response to be the requested collection "as mandated by RFC 4918 section 9.1". RFC 4918 mandates no response ordering whatsoever. The comment this replaced had it right — it attributed first-entry-is-parent to observed server behaviour. The i == 0 skip is unchanged from before this PR, so there is no regression, but the justification is now stated honestly rather than dressed up as a spec requirement.

Worth recording for whoever touches this next: against a server that does not list the request URI first, the parent collection stays in the listing and the first child collection is dropped. That is pre-existing behaviour, not something introduced here. The robust fix is to match hrefs against the request path instead of relying on position — out of scope for this PR.

HttpStatus.SC_MULTI_STATUS exists. A local constant was added with a comment claiming HttpCore does not define it. It does, as 207, in httpcore 4.4.16. The constant is gone and the library one is used.

Also in this commit: dropped the local-name fallback in isDavElement, which a namespace-aware parser can never reach, and switched the PROPFIND body to application/xml; charset=UTF-8, which is what Jackrabbit put on the wire (text/xml is equally legal per RFC 4918 §8.2, but matching the old bytes is the safer port).

Deliberate relaxations versus the Jackrabbit behaviour

Both are documented in javadoc and covered by tests, and both make this Wagon work against servers it previously failed on:

• A propstat without a DAV:status is read as successful. MultiStatusResponse.createFromXml required the element and dropped every property of such a response, so a directory served that way was reported "not a collection" and getFileList then threw ResourceDoesNotExistException.
• Elements in no namespace are accepted; Jackrabbit required DAV: exactly.

Three incidental robustness gains over the code being replaced, all from the old MultiStatusResponse handling: an empty multistatus no longer throws ArrayIndexOutOfBoundsException out of isDirectory, a response missing its href no longer throws IllegalArgumentException, and an empty resourcetype no longer risks a cast/NPE.

293 tests pass, 0 checkstyle violations.

[slachiewicz]

One more commit, closing a hole in the XXE fallback added earlier.

Where disallow-doctype-decl is unavailable, the parser falls back to refusing external general and parameter entities — but those two calls were allowed to fail as silently as the first one. A parser supporting none of the three would have gone on to read remote input with only FEATURE_SECURE_PROCESSING in force, and secure processing bounds resource use; it says nothing about what a document may reach. The fallback now requires both entity features and reports a parser it cannot secure as a configuration error, rather than quietly parsing without protection.

It also denies the external DTD subset via ACCESS_EXTERNAL_DTD, which the entity features do not govern (note setAttribute throws IllegalArgumentException, not ParserConfigurationException, so it needs its own catch).

That branch only executes on a JAXP implementation other than the platform one, so it cannot be reached from a unit test. It was verified by configuring a factory the same way by hand: a DOCTYPE parses — as it did under Jackrabbit, which neutered entities with an EntityResolver rather than banning the doctype — and an external entity resolves to empty text with no file contents disclosed. On the platform parser the primary path applies and testDoctypeIsRejected covers it.

293 tests pass, 0 checkstyle violations.

[Copilot]

Pull request overview

Replaces the broad Jackrabbit WebDAV dependency with focused request and response handling.

Changes:

• Adds MKCOL/PROPFIND request implementations and multistatus parsing.
• Updates WebDavWagon to use the new implementation.
• Removes Jackrabbit classes, dependency, and documentation references.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
DavMethods.java	Implements WebDAV requests.
MultiStatus.java	Parses PROPFIND multistatus responses.
WebDavWagon.java	Uses the replacement implementation.
MultiStatusTest.java	Tests multistatus parsing.
org/apache/jackrabbit/webdav/MultiStatus.java	Removes vendored Jackrabbit class.
XmlRequestEntity.java	Removes unused legacy class.
wagon-webdav-jackrabbit/pom.xml	Removes Jackrabbit dependency.
pom.xml	Updates Javadoc package grouping.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.