[NicoPiel]

What

Rewrites the File connector's WebDavConnection on Sardine 5.10, an Apache 2.0 WebDAV client built on httpclient 4.5. It also removes the vendored webdavclient4j-core-0.92.jar, a 2008 Jakarta Slide build on commons-httpclient 3.

Why

webdavclient4j has been dead since 2008 and was the last real user of commons-httpclient 3.0.1. WebDAV has also been broken since 4.5.0: Slide needs jdom 1, which was replaced by jdom2.

Sardine 5.10 is the last release on javax.xml.bind, which the engine already ships. 5.11 and later need Jakarta JAXB 4, and their org.glassfish.jaxb:jaxb-runtime would clash with ours (2.4). Sardine is added with transitive = false, so the only new jar is sardine-5.10.jar, in extensions/file/lib.

Reviewer notes

• WebDavConnection uses plain Sardine. Sardine handles the requests, multistatus parsing, PROPFIND redirects and authentication. WebDavConnection only maps its results onto FileSystemConnection.
• Authentication is challenge-response (NTLM, then Digest, then Basic). PUT streams with Expect: 100-continue. Over HTTPS, Basic is also sent preemptively, as Slide did, so an upload never has to be resent.
• canWrite only checks that the directory exists. It no longer checks for write locks: checking first is racy, and a failed write is logged when it happens.
• WebDavConnectionTest mocks the Sardine interface, like the SMB and FTP tests. List, read, write with MKCOL, move and delete were also checked by hand against Apache mod_dav.
• Fixed along the way, all already broken on main:
  • The configured port was ignored, so WebDAV always used port 80 or 443.
  • No connect or read timeouts were set. They now use the connector's timeout setting.
  • Listing returned wrong child paths, zero sizes and dates, and no directories.
• Credentials are scoped to the target host and port.
• Behavior change: TLS hostname verification is now on. commons-httpclient 3 never checked the hostname, so a server whose certificate doesn't match the configured host now fails the handshake. Worth a line in the release notes.
• Not tested: NTLM, IIS and Nextcloud.
• Follow-ups:
  • Failed PUT, DELETE and MOVE are still only logged, as before. See the comment on this PR.
  • commons-httpclient 3 can be removed once this, the HttpStatus change and the HTTPUtil change are merged.

🤖 Generated with Claude Code

[gibson9583]

Every upload is now copied into additional full-file heap buffers before the PUT starts. FileDispatcher already holds the original payload, and BufferedHttpEntity copies it again through a growing byte buffer and then another complete byte array. The previous connector streamed the upload. Large binary messages or reattached attachments that previously fit in memory can now exhaust the shared engine heap, especially with concurrent sends. This happens even for anonymous or already-authenticated uploads. WebDavConnection.java:342.

[NicoPiel]

Good catch, I'm also still testing this in the GUI

[NicoPiel]

Tested this in the Administrator against Apache mod_dav (bytemark/webdav, Basic auth). Two fixes came out of it and are now on the branch:

• Apache answers PROPFIND /in with a 301 to /in/, and httpclient doesn't follow redirects for PROPFIND, so every directory check failed (Test Read included). PROPFIND now retries with a trailing slash after a 3xx.
• PUT no longer buffers the body (that was added for the challenged-PUT case and cost extra full-size heap copies per upload). It streams again and sends Expect: 100-continue, so a 401 arrives before the body and the retry still works.

After that, polling (including subdirectories), filters, read, after-processing move, writer auto-MKCOL and PUT all worked end to end.

Suggestion for a follow-up, deliberately not in this PR: failed PUT, DELETE and MOVE are still only logged, to keep parity with the old Slide-based connector. In practice that means:

• a failed after-processing move leaves the file in the source directory while the receiver treats it as handled, so the next poll picks it up again and the message is duplicated;
• a failed upload marks the message as sent although nothing was written.

I saw the first case during testing: with the move-to directory set to ../done, the connector sent MKCOL /../done and a MOVE whose destination contained /../done, Apache rejected both with 400, and only a log line showed it. (Relative move-to paths aren't resolved for WebDAV at all; FTP/SFTP handle them. Same as before, but surprising.)

I'd make non-2xx responses to PUT, DELETE and MOVE throw, so the message errors or gets queued and the source file isn't reprocessed. The mayNotExist delete would stay lenient for 404. That's a behaviour change versus the old connector, so I'd rather do it as its own PR.

[NicoPiel]

@gibson9583 Good catch, thanks. Fixed in 0dd0a84: the PUT streams InputStreamEntity(is, contentLength) again, with no BufferedHttpEntity. To keep the challenged-PUT case working without buffering, the PUT sends Expect: 100-continue, so a 401 arrives before any body bytes are written and httpclient can still retry the unconsumed request. putIsRetriedWhenOnlyWritesAreChallenged covers that: the server allows anonymous reads and challenges only the PUT, and the test also asserts that every PUT carried the Expect header. Without buffering and without 100-continue it fails with ClientProtocolException. One tradeoff: a proxy that ignores Expect adds httpclient's default 3 s wait before the body is sent.

[github-actions]

Test Results

130 files  ± 0  130 suites  ±0   3m 3s ⏱️ -8s
800 tests +11  800 ✅ +11  0 💤 ±0  0 ❌ ±0 
926 runs  +11  920 ✅ +11  6 💤 ±0  0 ❌ ±0 

Results for commit 097e2e6. ± Comparison against base commit 4589238.

This pull request removes 2 and adds 13 tests. Note that renamed tests count towards both.

com.mirth.connect.donkey.server.data.jdbc.OracleJdbcDaoTest ‑ testBatchInsertMessageContentKeepsStatementOpenUntilExecuted
com.mirth.connect.donkey.server.data.jdbc.OracleJdbcDaoTest ‑ testFailedBatchInsertClearsAndClosesTheStatement

com.mirth.connect.connectors.file.filesystems.WebDavConnectionTest ‑ existsAndCanRead
com.mirth.connect.connectors.file.filesystems.WebDavConnectionTest ‑ listFiles
com.mirth.connect.connectors.file.filesystems.WebDavConnectionTest ‑ listRoot
com.mirth.connect.connectors.file.filesystems.WebDavConnectionTest ‑ listingErrorIncludesStatus
com.mirth.connect.connectors.file.filesystems.WebDavConnectionTest ‑ moveDoesNotOverwrite
com.mirth.connect.connectors.file.filesystems.WebDavConnectionTest ‑ pathsAreEncoded
com.mirth.connect.connectors.file.filesystems.WebDavConnectionTest ‑ readFile
com.mirth.connect.connectors.file.filesystems.WebDavConnectionTest ‑ selfEntryBehindRewritingProxy
com.mirth.connect.connectors.file.filesystems.WebDavConnectionTest ‑ unreachableServerIsInvalid
com.mirth.connect.connectors.file.filesystems.WebDavConnectionTest ‑ writeFileCreatesMissingDirectory
…

♻️ This comment has been updated with latest results.

[gibson9583]

The extra upload buffering is fixed, but the replacement breaks authentication when reads are anonymous and writes require credentials behind nginx. With default request buffering, nginx sends 100 Continue and consumes the body before the upstream server returns 401. The InputStreamEntity has already been consumed, so HttpClient throws NonRepeatableRequestException instead of retrying with credentials. The first challenged upload fails; with Keep Connection Open disabled, this repeats on each attempt. Expect: 100-continue does not guarantee that the authentication challenge arrives before the body. WebDavConnection.java:341–344.

[NicoPiel]

@gibson9583 Right, 100-continue isn't a guarantee. Fixed in afaef4e: FileDispatcher hands the connector a ByteArrayInputStream, so the PUT entity is now repeatable by resetting that stream before each send. The payload still isn't copied, and the retry no longer depends on when the 401 arrives. Only other stream types fall back to streaming with Expect: 100-continue. The new test putIsRetriedWhenTheBodyIsConsumedBeforeTheChallenge makes the server read the whole unauthenticated body before answering 401, as nginx does with request buffering. It failed with ClientProtocolException before the change and passes now.

🤖 Addressed by Claude Code

[gibson9583]

Both previously reported issues are resolved. Looks safe to merge.

[mgaffigan]

We should not be maintaining a WebDAV client. This needs to use some pre existing library.

If none exists, the functionality may need to be removed to a plugin.

[NicoPiel]

@mgaffigan Agreed. 154b1f3 moves the connector onto Sardine 5.10 (Apache 2.0, built on httpclient 4.5). Sardine now handles the requests, the multistatus parsing, PROPFIND redirects and authentication. WebDavConnection only maps its results onto the file connector's FileSystemConnection interface.

• Why 5.10: it's the last Sardine release on javax.xml.bind, which the engine already ships. It is added with transitive = false, so the only new jar is sardine-5.10.jar. 5.11 and later need Jakarta JAXB 4, whose org.glassfish.jaxb:jaxb-runtime would clash with the engine's 2.4.
• What's still ours:
  • a small SardineImpl subclass. DavResource drops lockdiscovery, and canWrite needs it.
  • the ByteArrayInputStream entity that lets a PUT be resent after an auth challenge.
• Testing: the same 41 tests against the embedded WebDAV server pass unchanged. The connector also works end to end against Apache mod_dav: list, read, write with MKCOL, move, delete.

🤖 Addressed by Claude Code

[mgaffigan]

Ok - that's not so bad, then. Can we drop the custom webdav server and extensions to sardine then? Presumably we do not need to check if we can write since it is inherently a TOCTOU. Ditto for the auth issue - is there a normal auth method implementation?

[NicoPiel]

@mgaffigan Done in 097e2e6:

• No Sardine extensions: the SardineImpl subclass is gone. canWrite now only checks that the directory exists, and a failed write is logged when it happens.
• Standard auth: the custom upload entity is gone. PUT uses Sardine's own streaming put with Expect: 100-continue and the usual challenge-response (NTLM, then Digest, then Basic). Over HTTPS, Basic is also sent preemptively, like the old Slide client did, so an upload never has to be resent. Over plain HTTP it still waits for the challenge, so a Digest server never gets the password in cleartext.
• No test server: the Jetty WebDAV server and its tests are removed. A short Mockito test against the Sardine interface replaces them, in the same style as the SMB and FTP tests. I also checked list, read, write, move and delete by hand against Apache mod_dav.

🤖 Addressed by Claude Code