fix(webdav): reject document type declarations in WebDAV XML request bodies - #37167
Queued
swicken wants to merge 7 commits into
Queued
fix(webdav): reject document type declarations in WebDAV XML request bodies#37167swicken wants to merge 7 commits into
swicken wants to merge 7 commits into
Conversation
Add a filter on /webdav/* that parses PROPFIND, PROPPATCH and LOCK bodies with disallow-doctype-decl and rejects any document carrying a document type declaration with a 400. RFC 4918 bodies are plain namespaced XML, so no real client sends a DTD. The check has to sit in a filter rather than in the parser: the WebDAV servlet comes from a third-party library whose parsers cannot be configured from here, and one of the three parses happens in a static method. Bodies are capped at 512KB, since validating one means buffering it. Empty bodies are left alone, as PROPFIND with no body means allprop and LOCK with no body is a refresh. The consumed body is replayed to the servlet intact.
Contributor
|
Claude finished @swicken's task in 1m 45s —— View job Code Review — WebDAV XML validation filterI read New IssuesNo blocking issues found. Notes (non-blocking)
Resolved
Verification of the correctness-critical claims
Nice work — this is a clean, well-documented change with good test coverage. The one note above is optional and doesn't block. · |
…factory Detect the declaration through the lexical handler instead of relying on a parse failure. The servlet tolerates a body it cannot parse -- a PROPFIND with broken XML falls back to allprop and still answers 207 -- so failing those was a behaviour change unrelated to DTDs. A malformed body is now forwarded untouched, with a test covering it. Build the reader from ThreadLocalSaxParserFactory rather than configuring a new SAXParserFactory per request. Its factory already disables external entities and external DTD loading while still reporting the declaration, which is what detection needs, so this also drops a fourth copy of the feature block. Make the size cap operator-tunable as WEBDAV_MAX_XML_BODY_BYTES, matching BoundedBufferedReader and MultiPartSecurityRequestWrapper. Fix getReader(): default to ISO-8859-1 as the servlet spec requires when the request states no encoding, and translate a bad charset token into the checked UnsupportedEncodingException the contract declares. Drop the javadoc claim that this parser and the servlet's cannot disagree. They resolve through independent mechanisms, JAXP here and XMLReaderFactory there. Build the test request from DotCMSMockRequest instead of hand-rolling a mock and a ServletInputStream.
The wrapper built a fresh replay stream per call, so a second caller would have silently re-read the body from the start. A request body is read once: build it lazily and cache it. This also matches the real container request, where a second call returns the same, already-consumed stream. Covered by asserting the same instance comes back twice and that the body is not re-readable once consumed.
…ed read The detection parse signalled a declaration by throwing a shared exception instance and the caller compared identity. Any layer that wrapped what the handler threw would have made that comparison miss, and the failure mode was a DTD forwarded silently. Use a private exception type, catch broadly, and walk the cause chain, bounded so a cyclic chain cannot spin. Replace the hand-rolled capped read loop with InputStream.readNBytes: reading one byte past the cap is enough to know it was exceeded.
![semgrep-dotcms[bot]](https://avatars.githubusercontent.com/u/1005263?s=60&v=4){kind=small}
…r the cause chain toUpperCase() without a locale is a footgun: under a Turkish default locale "propfind" uppercases to "PROPFIND" with a dotted capital I, which would not match and would skip validation entirely. Pinned by a test that sets tr-TR and fails without the fix. Replace the hand-rolled cause-chain walk and its depth guard with ExceptionUtils.indexOfType from commons-lang3, already a direct dependency. It tolerates a cyclic chain, so the guard goes too.
mbiuki
self-requested a review
August 24, 2026 14:37
wezell
requested changes
Aug 24, 2026
wezell
left a comment
wezell
left a comment
Member
There was a problem hiding this comment.
I would use the security logger here - this should never happen in normal operations.
erickgonzalez
approved these changes
Aug 24, 2026
wezell
added this pull request to the merge queue
Aug 24, 2026
Any commits made after this event will not be merged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The WebDAV endpoints take XML request bodies on
PROPFIND,PROPPATCHandLOCK.RFC 4918 defines those as plain namespaced documents, and no WebDAV client sends a
document type declaration, so there is no reason for us to accept one. This adds a
filter on
/webdav/*that validates those bodies up front, returning400foranything carrying a DTD, plus a size cap.
The check sits in a filter rather than in the parser because the WebDAV servlet comes
from a third-party library whose parsers are not configurable from our code, and one
of the three parses happens in a static method. A filter is the only place that
covers all three consistently.
Behaviour
PROPFIND,PROPPATCHandLOCKare inspected. Other methods are forwarded untouched.400rather than the500it used to produce.413. Validating a body means buffering it, so it needs a bound.PROPFINDwith no body means allprop, andLOCKwith no body is a refresh.Testing
Six unit tests in
WebDavXmlValidationFilterTestcover rejection, pass-through with anintact body, empty bodies, non-XML methods, the size cap, and detection across encodings.
Also exercised against a running instance: the existing
WebDavPostman collection passesall 14 of its requests, and
PROPFIND(with and without a body),OPTIONS,MKCOL,PUT,GET,PROPPATCHandLOCKall behave as they did before the change.Closes dotCMS/private-issues#671