[bugfix] Parse multipart/form-data for all HTTP methods, not only POST

[joewiz]

[This PR was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

Summary

Multipart/form-data uploads only worked for POST. A multipart PUT (or PATCH) was effectively unusable: request:is-multipart-content() returned false, the uploaded file part was silently dropped, and the request stream was consumed so nothing downstream could recover it. This fixes multipart parsing so it works for every HTTP method that carries a multipart body, and so uploaded files are exposed to controllers and RESTXQ resource functions (via the request: module) regardless of method.

Closes #6580. Substantially addresses #6578 (see Scope below).

Root cause — two coupled defects

1. HttpRequestWrapper gated multipart detection on POST. isMultipartContent was only ever true when getMethod() was POST, so for PUT/PATCH the request was never treated as multipart: request:is-multipart-content() returned false and getParts() was never called.

2. XQueryURLRewrite built its request wrapper with parseMultipart=false. On the controller and RESTXQ path — the path a PUT can actually reach, since REST PUT stores rather than executes — the wrapper fell back to the servlet container's getParameterMap(). That exposes non-file form fields but omits file parts entirely, so an uploaded file was never visible there for any method, and reading the parameters consumed the body so it could not be re-parsed by hand either.

The two are coupled: fixing only (1) would have flipped request:is-multipart-content() to true without making the file available on that path — which would move consumers onto a code path that returns an empty file. Both are fixed together.

What changed

• HttpRequestWrapper — recognize a multipart body by its Content-Type (multipart/…) regardless of the HTTP method, instead of requiring POST.
• XQueryURLRewrite — construct the request wrapper with parseMultipart=true, so multipart content (including file parts) is parsed via getParts() and exposed to controllers and RESTXQ resource functions through the request: module.

Reproduction / test

MultipartMethodControllerTest stores a controller.xql that reports, via the request: module, what it sees of an identical multipart body sent as POST and as PUT — the same path a third-party router (e.g. the roaster library, eeditiones/roaster#107) takes. Before the fix, PUT reported is-multipart-content()=false and exposed neither the file field nor the uploaded file; the controller path never exposed the uploaded file for either method. After the fix, both methods see the form field, the uploaded file, and is-multipart-content()=true.

Behavior before vs. after, from that test (controller path):

	is-multipart-content()	form field path	uploaded file file
POST — before	true	✓	✗
PUT — before	false	✓	✗
POST — after	true	✓	✓
PUT — after	true	✓	✓

Scope

• [BUG] parsing of multipart does not work correctly for PUT #6580 — fully fixed.
• [BUG] auto-parsing of multipart-formdata is incomplete #6578 — the core defect (uploaded files not parsed for non-POST methods / on the controller and RESTXQ path) is fixed: files are now parsed and available through the request: module for every method, so the "parse the raw body yourself" workaround is no longer needed. One aspect it deliberately does not add is a way to read an individual part's own headers (e.g. a file part's Content-Type) through the request: module — that would be a small additive API and is better discussed separately. Marked "Relates to" rather than "Closes" [BUG] auto-parsing of multipart-formdata is incomplete #6578 for that reason; happy to close it if reviewers consider the remaining item out of scope.
• Makes the client-side multipart-parsing workaround in feat(put multipart): add parsing for multipart content with methods other than POST eeditiones/roaster#107 unnecessary once released.

Test plan

• MultipartMethodControllerTest (new) — multipart POST and PUT through a controller.xql, asserting the field, the file, and is-multipart-content().
• org.exist.http.urlrewrite.* and org.exist.xquery.functions.request.* — full packages green (incl. ControllerTest, GetParameterTest, PatchTest, GetData*Test).
• RESTServiceTest green.
• Codacy (PMD) on the changed files — no new findings.

[line-o]

I built this branch and tested it live against both the direct REST API path (RESTServer) and the controller/RESTXQ path (XQueryURLRewrite). The PUT/PATCH fix itself works as described, but it has a side effect: GET requests are now treated as multipart too.

Before this PR, a GET request carrying a multipart/form-data body already leaked non-file form fields (a pre-existing, unrelated quirk of the servlet container's parameter map), but request:is-multipart-content() reported false and uploaded files were silently dropped. After this PR, GET is handled identically to POST/PUT: is-multipart-content() returns true and uploaded files become fully visible via request:get-uploaded-file-name(), on both paths I tested.

GET is specified as a safe method with no defined semantics for a request body. RFC 9110 §9.3.1 ("GET") is explicit about this: a client SHOULD NOT generate content in a GET request unless it is made directly to an origin server that has previously indicated, in or out of band, that such a request has a purpose and will be adequately supported — since intermediaries along the request chain are often unaware of such private agreements. (The predecessor spec, RFC 7231 §4.3.1, put it more bluntly: "A payload within a GET request message has no defined semantics; sending a payload body on a GET request might cause some existing implementations to reject the request.")

Requesting changes: rather than dropping the method check entirely, could isMultipartContent use an explicit allow-list of methods that legitimately carry a body (POST, PUT, PATCH), or otherwise explicitly exclude GET (and HEAD/DELETE)? That keeps the PUT/PATCH fix intact without also making GET a vector for parsed, attacker-suppliable multipart content (including file uploads) on every RESTXQ/controller function bound to it — which is a meaningful change of trust boundary for any handler that wasn't written expecting a body on GET.

See inline comment on the specific line.

[line-o]

This drops the POST-only gate entirely instead of replacing it with an explicit allow-list. Confirmed live: a GET request with a multipart/form-data body now gets is-multipart-content()=true and its uploaded files fully parsed and exposed via request:get-uploaded-file-name(), on both the RESTServer and XQueryURLRewrite paths — previously GET never exposed uploaded files at all.

Per RFC 9110 §9.3.1, a GET request body has no defined semantics and a client SHOULD NOT send one absent prior agreement with the origin server. Suggest scoping this to an explicit set of methods that carry a body (POST, PUT, PATCH) rather than every method, e.g.:

private static final Set<String> MULTIPART_METHODS = Set.of("POST", "PUT", "PATCH");
...
isMultipartContent = MULTIPART_METHODS.contains(servletRequest.getMethod().toUpperCase(Locale.ENGLISH))
        && contentType != null && contentType.toLowerCase(Locale.ENGLISH).startsWith("multipart/");

[joewiz]

@line-o Great, thanks. Work on these changes is in progress.

[joewiz]

[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

Thanks @line-o — good catch, and agreed. Treating GET as multipart is a real change of trust boundary, and dropping the method gate entirely was too broad. Fixed in d836b11 with your suggested approach: an explicit allow-list of body-carrying methods.

private static final Set<String> MULTIPART_METHODS = Set.of("POST", "PUT", "PATCH");
...
isMultipartContent = MULTIPART_METHODS.contains(servletRequest.getMethod().toUpperCase(Locale.ENGLISH))
        && contentType != null && contentType.toLowerCase(Locale.ENGLISH).startsWith("multipart/");

So GET/HEAD/DELETE are excluded while the PUT/PATCH fix stays intact. Added a test alongside the existing one: a GET carrying a multipart/form-data body now reports is-multipart-content()=false and exposes no uploaded file, while POST/PUT/PATCH all parse it.

You're right that the pre-existing leak of non-file form fields into the container's parameter map on GET is separate — this change doesn't alter it, and it seemed out of scope here; happy to file it as its own issue if you think it's worth chasing.

[line-o]

hmm, why do we call this MULTIPART_METHODS rather then making it clear that these methods can carry a body.
How about METHOD_CAN_CARY_A_BODY and I would also propose to match against lower-case strings.
Do we want to make this an enum, @reinhapa?

[joewiz]

[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

Good points, @line-o.

Name — agreed, it should say why these methods qualify rather than what we use them for. I'd suggest METHODS_WITH_REQUEST_BODY (clear, and avoids the singular/plural mismatch). Happy to use whatever wording you prefer.

Case — the current check is already case-insensitive (getMethod().toUpperCase(...) against an uppercase set). I'd lean towards keeping uppercase: HTTP method names are canonically uppercase, and the existing method checks in this codebase compare against uppercase literals (e.g. PathFilter does "PUT".equalsIgnoreCase(...)), so an uppercase set reads most obviously as "these HTTP methods". But it's a one-line flip either way — glad to match lower-case if you'd rather.

Enum — over to @reinhapa, since it's really your call. For context: there's no existing HTTP-method enum in the codebase today; method dispatch is done with string comparisons throughout, so this would be net-new infrastructure for a three-element set. I don't feel strongly and am happy to add one if you'd prefer it.

I'll hold the rename until we've settled on the name and the enum question, so it lands in one clean change rather than renaming twice.

[line-o]

• Name: METHODS_WITH_REQUEST_BODY 👍🏼
• Case: Agreed, uppercase it is
• Enum: let's do this in a followup (if at all)

[joewiz]

[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

Done — renamed to METHODS_WITH_REQUEST_BODY, kept the values uppercase, and left the enum for a possible follow-up. Landed as a separate commit on top so the delta is easy to read.

[line-o]

Nice!