-
-
Notifications
You must be signed in to change notification settings - Fork 191
[bugfix] Parse multipart/form-data for all HTTP methods, not only POST #6581
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.
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
117 changes: 117 additions & 0 deletions
117
exist-core/src/test/java/org/exist/http/urlrewrite/MultipartMethodControllerTest.java
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| /* | ||
| * eXist-db Open Source Native XML Database | ||
| * Copyright (C) 2001 The eXist-db Authors | ||
| * | ||
| * info@exist-db.org | ||
| * http://www.exist-db.org | ||
| * | ||
| * This library is free software; you can redistribute it and/or | ||
| * modify it under the terms of the GNU Lesser General Public | ||
| * License as published by the Free Software Foundation; either | ||
| * version 2.1 of the License, or (at your option) any later version. | ||
| * | ||
| * This library is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
| * Lesser General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU Lesser General Public | ||
| * License along with this library; if not, write to the Free Software | ||
| * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA | ||
| */ | ||
| package org.exist.http.urlrewrite; | ||
|
|
||
| import org.exist.TestUtils; | ||
| import org.exist.http.AbstractHttpTest; | ||
| import org.exist.test.ExistWebServer; | ||
| import org.junit.ClassRule; | ||
| import org.junit.Test; | ||
|
|
||
| import java.io.IOException; | ||
| import java.net.HttpURLConnection; | ||
| import java.net.URI; | ||
| import java.net.http.HttpRequest; | ||
|
|
||
| import static java.nio.charset.StandardCharsets.UTF_8; | ||
| import static org.exist.http.urlrewrite.XQueryURLRewrite.LEGACY_XQUERY_CONTROLLER_FILENAME; | ||
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| /** | ||
| * Reproduction for multipart/form-data parsing on methods other than POST | ||
| * (eXist-db/exist#6580, #6578), via a controller.xql that reports what the | ||
| * {@code request:} module can see of an identical multipart body — the same | ||
| * path third-party routers (e.g. roaster) take. | ||
| */ | ||
| public class MultipartMethodControllerTest extends AbstractHttpTest { | ||
|
|
||
| @ClassRule | ||
| public static final ExistWebServer existWebServer = new ExistWebServer(true, false, true, true, false); | ||
|
|
||
| private static final String BOUNDARY = "wdbBoundary"; | ||
|
|
||
| private static final String MULTIPART_BODY = | ||
| "--" + BOUNDARY + "\r\n" | ||
| + "Content-Disposition: form-data; name=\"path\"\r\n" | ||
| + "\r\n" | ||
| + "edition/01/17410105.xml\r\n" | ||
| + "--" + BOUNDARY + "\r\n" | ||
| + "Content-Disposition: form-data; name=\"file\"; filename=\"17410105.xml\"\r\n" | ||
| + "Content-Type: application/xml\r\n" | ||
| + "\r\n" | ||
| + "<example>hello</example>\r\n" | ||
| + "--" + BOUNDARY + "--\r\n"; | ||
|
|
||
| private static final String CONTROLLER = | ||
| """ | ||
| xquery version "3.1"; | ||
| <result method="{request:get-method()}" | ||
| is-multipart="{request:is-multipart-content()}" | ||
| param-names="{string-join(request:get-parameter-names(), ',')}" | ||
| path="{request:get-parameter('path', ())}" | ||
| file-param="{request:get-parameter('file', ())}" | ||
| uploaded-files="{string-join(request:get-uploaded-file-name('file'), ',')}"/> | ||
| """; | ||
|
|
||
| @Test | ||
| public void multipartFormDataIsParsedForPostAndPut() throws IOException { | ||
| final String coll = "multipart-method-controller"; | ||
| store(coll, LEGACY_XQUERY_CONTROLLER_FILENAME, "application/xquery", CONTROLLER); | ||
|
|
||
| // A multipart/form-data body must be parsed identically regardless of HTTP method: | ||
| // both the form field ("path") and the uploaded file ("file") must be visible, and | ||
| // request:is-multipart-content() must report true. Prior to the fix, PUT reported | ||
| // is-multipart-content()=false and exposed neither the file nor its part (#6580), | ||
| // and the controller/RESTXQ path never exposed the uploaded file at all (#6578). | ||
| for (final String method : new String[]{"POST", "PUT"}) { | ||
| final String body = send(coll, method); | ||
| assertTrue(method + ": is-multipart-content() should be true: " + body, | ||
| body.contains("is-multipart=\"true\"")); | ||
| assertTrue(method + ": form field 'path' should be visible: " + body, | ||
| body.contains("path=\"edition/01/17410105.xml\"")); | ||
| assertTrue(method + ": uploaded file 'file' should be visible: " + body, | ||
| body.contains("uploaded-files=\"17410105.xml\"")); | ||
| } | ||
| } | ||
|
|
||
| private void store(final String coll, final String name, final String mediaType, final String content) throws IOException { | ||
| final HttpRequest request = authenticatedRequest( | ||
| URI.create(getRestUri(existWebServer) + "/db/apps/" + coll + "/" + name), | ||
| TestUtils.ADMIN_DB_USER, TestUtils.ADMIN_DB_PWD) | ||
| .header("Content-Type", mediaType) | ||
| .PUT(HttpRequest.BodyPublishers.ofString(content)) | ||
| .build(); | ||
| final int status = withHttpClient(client -> executeForStatus(client, request)); | ||
| assertEquals(HttpURLConnection.HTTP_CREATED, status); | ||
| } | ||
|
|
||
| private String send(final String coll, final String method) throws IOException { | ||
| final HttpRequest request = authenticatedRequest( | ||
| URI.create(getServerUri(existWebServer) + "/apps/" + coll + "/echo"), | ||
| TestUtils.ADMIN_DB_USER, TestUtils.ADMIN_DB_PWD) | ||
| .header("Content-Type", "multipart/form-data; boundary=" + BOUNDARY) | ||
| .method(method, HttpRequest.BodyPublishers.ofString(MULTIPART_BODY, UTF_8)) | ||
| .build(); | ||
| return withHttpClient(client -> executeForStatusAndBody(client, request).body()); | ||
| } | ||
| } |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This drops the
POST-only gate entirely instead of replacing it with an explicit allow-list. Confirmed live: aGETrequest with amultipart/form-databody now getsis-multipart-content()=trueand its uploaded files fully parsed and exposed viarequest:get-uploaded-file-name(), on both theRESTServerandXQueryURLRewritepaths — previouslyGETnever exposed uploaded files at all.Per RFC 9110 §9.3.1, a
GETrequest 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.:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@line-o Great, thanks. Work on these changes is in progress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]
Thanks @line-o — good catch, and agreed. Treating
GETas 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.So
GET/HEAD/DELETEare excluded while thePUT/PATCHfix stays intact. Added a test alongside the existing one: aGETcarrying amultipart/form-databody now reportsis-multipart-content()=falseand exposes no uploaded file, whilePOST/PUT/PATCHall parse it.You're right that the pre-existing leak of non-file form fields into the container's parameter map on
GETis 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_BODYand I would also propose to match against lower-case strings.Do we want to make this an enum, @reinhapa?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.PathFilterdoes"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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
METHODS_WITH_REQUEST_BODY👍🏼