Skip to content

[WAGON-583] WebDavWagon and tests now adhere to RFC 4918 #845

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

Draft
slachiewicz wants to merge 1 commit into
base: wagon-3.x
Choose a base branch
from
Draft

[WAGON-583] WebDavWagon and tests now adhere to RFC 4918 #845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 48 additions & 3 deletions ...-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,16 +122,24 @@ protected void mkdirs(String dir) throws IOException {
do {
String url = baseUrl + "/" + navigator.getPath();
status = doMkCol(url);
if (status == HttpStatus.SC_CREATED || status == HttpStatus.SC_METHOD_NOT_ALLOWED) {
// RFC 4918: Accept 201 Created, 200 OK (already exists), 405 Method Not Allowed (already exists)
if (status == HttpStatus.SC_CREATED
|| status == HttpStatus.SC_OK
|| status == HttpStatus.SC_METHOD_NOT_ALLOWED) {
break;
}
// RFC 4918: 409 Conflict means intermediate collection is missing, continue traversing backwards
if (status == HttpStatus.SC_CONFLICT) {
continue;
}
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backward traversal loop doesn't handle unexpected status codes properly. If doMkCol returns a status code that is not CREATED, OK, METHOD_NOT_ALLOWED, or CONFLICT (e.g., 403 Forbidden, 401 Unauthorized, 500 Internal Server Error), the loop will continue traversing backwards indefinitely until reaching the root. This could lead to incorrect behavior or silent failures. Consider throwing an exception for unexpected error status codes.

Suggested change
}
}
// Any other status is unexpected here: stop traversal and report an error
throw new IOException("Unexpected status code while creating collection: " + url
+ "; status code = " + status);

Copilot uses AI. Check for mistakes.
} while (navigator.backward());

// traverse forward creating missing directories
while (navigator.forward()) {
String url = baseUrl + "/" + navigator.getPath();
status = doMkCol(url);
if (status != HttpStatus.SC_CREATED) {
// RFC 4918: Accept 201 Created or 200 OK (if collection already exists from another request)
if (status != HttpStatus.SC_CREATED && status != HttpStatus.SC_OK) {
throw new IOException("Unable to create collection: " + url + "; status code = " + status);
}
}
Expand All @@ -140,7 +148,21 @@ protected void mkdirs(String dir) throws IOException {
private int doMkCol(String url) throws IOException {
HttpMkcol method = new HttpMkcol(url);
try (CloseableHttpResponse closeableHttpResponse = execute(method)) {
return closeableHttpResponse.getStatusLine().getStatusCode();
int statusCode = closeableHttpResponse.getStatusLine().getStatusCode();

// RFC 4918: Handle redirects for MKCOL
// 3xx redirects should be followed to the new location
if (statusCode >= HttpStatus.SC_MULTIPLE_CHOICES && statusCode < HttpStatus.SC_BAD_REQUEST) {
org.apache.http.Header locationHeader = closeableHttpResponse.getFirstHeader("Location");
if (locationHeader != null) {
String redirectUrl = locationHeader.getValue();
// Recursive call to handle redirect - execute() will handle the redirect automatically
// but we need to return the final status
Comment on lines +159 to +160
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on line 159 is inconsistent with the actual implementation. The comment states "execute() will handle the redirect automatically" but then the code makes a recursive call to handle the redirect manually. Either the redirect is handled automatically by the HTTP client (in which case the manual handling code is unnecessary), or it needs to be handled manually (in which case the comment is misleading). Clarify the comment to accurately reflect the implementation.

Suggested change
// Recursive call to handle redirect - execute() will handle the redirect automatically
// but we need to return the final status
// Follow the redirect by recursively invoking doMkCol on the new URL
// and return the status code from the final resolved location

Copilot uses AI. Check for mistakes.
return doMkCol(redirectUrl);
}
Comment on lines +155 to +162
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The redirect handling logic has a potential infinite recursion issue. If a server responds with a redirect that points back to the original URL or creates a redirect loop, this will cause a stack overflow. The recursive call should include protection against redirect loops, such as tracking visited URLs or limiting the maximum number of redirects.

Copilot uses AI. Check for mistakes.
}

return statusCode;
} catch (HttpException e) {
throw new IOException(e.getMessage(), e);
} finally {
Expand Down Expand Up @@ -172,6 +194,29 @@ public void putDirectory(File sourceDirectory, String destinationDirectory)
}
}

protected boolean collectionExists(String url) throws IOException {
DavPropertyNameSet nameSet = new DavPropertyNameSet();
nameSet.add(DavPropertyName.create(DavConstants.PROPERTY_RESOURCETYPE));

CloseableHttpResponse closeableHttpResponse = null;
HttpPropfind method = null;
try {
method = new HttpPropfind(url, nameSet, DavConstants.DEPTH_0);
closeableHttpResponse = execute(method);
int statusCode = closeableHttpResponse.getStatusLine().getStatusCode();
return statusCode == HttpStatus.SC_OK || statusCode == HttpStatus.SC_MULTI_STATUS;
} catch (HttpException e) {
throw new IOException(e.getMessage(), e);
} finally {
if (method != null) {
method.releaseConnection();
}
if (closeableHttpResponse != null) {
closeableHttpResponse.close();
}
Comment on lines +198 to +216
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The collectionExists method doesn't validate whether the resource is actually a collection. It returns true for any resource (file or collection) that returns OK or MULTI_STATUS. According to RFC 4918, PROPFIND should verify that the resourcetype property contains a collection element. Consider parsing the response to check if the resource is specifically a collection, similar to the isDirectory method implementation.

Suggested change
DavPropertyNameSet nameSet = new DavPropertyNameSet();
nameSet.add(DavPropertyName.create(DavConstants.PROPERTY_RESOURCETYPE));
CloseableHttpResponse closeableHttpResponse = null;
HttpPropfind method = null;
try {
method = new HttpPropfind(url, nameSet, DavConstants.DEPTH_0);
closeableHttpResponse = execute(method);
int statusCode = closeableHttpResponse.getStatusLine().getStatusCode();
return statusCode == HttpStatus.SC_OK || statusCode == HttpStatus.SC_MULTI_STATUS;
} catch (HttpException e) {
throw new IOException(e.getMessage(), e);
} finally {
if (method != null) {
method.releaseConnection();
}
if (closeableHttpResponse != null) {
closeableHttpResponse.close();
}
try {
return isDirectory(url);
} catch (DavException e) {
throw new IOException(e.getMessage(), e);

Copilot uses AI. Check for mistakes.
}
}

private boolean isDirectory(String url) throws IOException, DavException {
DavPropertyNameSet nameSet = new DavPropertyNameSet();
nameSet.add(DavPropertyName.create(DavConstants.PROPERTY_RESOURCETYPE));
Expand Down
40 changes: 19 additions & 21 deletions ...dav-jackrabbit/src/test/java/org/apache/maven/wagon/providers/webdav/WebDavWagonTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,34 +137,35 @@ public void testMkdirs() throws Exception {
wagon.connect(testRepository, getAuthInfo());

try {
File dir = getRepositoryDirectory();

// check basedir also doesn't exist and will need to be created
dir = new File(dir, testRepository.getBasedir());
assertFalse(dir.exists());
String repositoryUrl = testRepository.getUrl();

// test leading /
assertFalse(new File(dir, "foo").exists());
String fooUrl = repositoryUrl + (repositoryUrl.endsWith("/") ? "" : "/") + "foo";
assertFalse("Collection should not exist before creation", wagon.collectionExists(fooUrl));
wagon.mkdirs("/foo");
assertTrue(new File(dir, "foo").exists());
assertTrue("Collection should exist after creation", wagon.collectionExists(fooUrl));

// test trailing /
assertFalse(new File(dir, "bar").exists());
String barUrl = repositoryUrl + (repositoryUrl.endsWith("/") ? "" : "/") + "bar";
assertFalse("Collection should not exist before creation", wagon.collectionExists(barUrl));
wagon.mkdirs("bar/");
assertTrue(new File(dir, "bar").exists());
assertTrue("Collection should exist after creation", wagon.collectionExists(barUrl));

// test when already exists
// test when already exists (should not fail)
wagon.mkdirs("bar");
assertTrue("Collection should still exist", wagon.collectionExists(barUrl));

// test several parts
assertFalse(new File(dir, "1/2/3/4").exists());
String deepUrl = repositoryUrl + (repositoryUrl.endsWith("/") ? "" : "/") + "1/2/3/4";
assertFalse("Deep collection should not exist before creation", wagon.collectionExists(deepUrl));
wagon.mkdirs("1/2/3/4");
assertTrue(new File(dir, "1/2/3/4").exists());
assertTrue("Deep collection should exist after creation", wagon.collectionExists(deepUrl));

// test additional part and trailing /
assertFalse(new File(dir, "1/2/3/4/5").exists());
String deeperUrl = repositoryUrl + (repositoryUrl.endsWith("/") ? "" : "/") + "1/2/3/4/5";
assertFalse("Deeper collection should not exist before creation", wagon.collectionExists(deeperUrl));
wagon.mkdirs("1/2/3/4/5/");
assertTrue(new File(dir, "1/2/3/4").exists());
assertTrue("Deeper collection should exist after creation", wagon.collectionExists(deeperUrl));
} finally {
wagon.disconnect();

Expand All @@ -186,16 +187,13 @@ public void testMkdirsWithNoBasedir() throws Exception {
wagon.connect(testRepository, getAuthInfo());

try {
File dir = getRepositoryDirectory();

// check basedir also doesn't exist and will need to be created
dir = new File(dir, testRepository.getBasedir());
assertTrue(dir.exists());
String repositoryUrl = testRepository.getUrl();

// test leading /
assertFalse(new File(dir, "foo").exists());
String fooUrl = repositoryUrl + (repositoryUrl.endsWith("/") ? "" : "/") + "foo";
assertFalse("Collection should not exist before creation", wagon.collectionExists(fooUrl));
wagon.mkdirs("/foo");
assertTrue(new File(dir, "foo").exists());
assertTrue("Collection should exist after creation", wagon.collectionExists(fooUrl));
} finally {
wagon.disconnect();

Expand Down