diff --git a/exist-core/src/main/java/org/exist/http/servlets/HttpRequestWrapper.java b/exist-core/src/main/java/org/exist/http/servlets/HttpRequestWrapper.java index 9ab5a111aaf..587fa300eb1 100644 --- a/exist-core/src/main/java/org/exist/http/servlets/HttpRequestWrapper.java +++ b/exist-core/src/main/java/org/exist/http/servlets/HttpRequestWrapper.java @@ -62,6 +62,12 @@ public class HttpRequestWrapper implements RequestWrapper { private static final Logger LOG = LogManager.getLogger(HttpRequestWrapper.class); + // HTTP methods whose request may legitimately carry a request body. GET, HEAD and DELETE are + // deliberately excluded: a body on those methods has no defined semantics (RFC 9110 §9.3.1), + // and parsing one would expose attacker-suppliable multipart content (including file uploads) + // to handlers not written to expect a body. + private static final Set METHODS_WITH_REQUEST_BODY = Set.of("POST", "PUT", "PATCH"); + private static final Path TMP_DIR; static { try { @@ -135,10 +141,14 @@ public HttpRequestWrapper(final HttpServletRequest servletRequest, this.pathInfo = servletRequest.getPathInfo(); this.servletPath = servletRequest.getServletPath(); - // Determine if request is a multipart - + // Determine if request is a multipart. A multipart/form-data body may accompany any + // body-carrying method (e.g. PUT and PATCH, not just POST), so recognize the whole + // allow-list rather than POST alone — while still excluding GET/HEAD/DELETE (see + // METHODS_WITH_REQUEST_BODY). See https://github.com/eXist-db/exist/issues/6580 and + // https://github.com/eXist-db/exist/issues/6578 @Nullable final String contentType = servletRequest.getContentType(); - isMultipartContent = "POST".equalsIgnoreCase(servletRequest.getMethod()) && contentType != null && contentType.toLowerCase(Locale.ENGLISH).startsWith("multipart/"); + isMultipartContent = METHODS_WITH_REQUEST_BODY.contains(servletRequest.getMethod().toUpperCase(Locale.ENGLISH)) + && contentType != null && contentType.toLowerCase(Locale.ENGLISH).startsWith("multipart/"); // Get multi-part formdata parameters when it is a mpfd request // and when instructed to do so diff --git a/exist-core/src/main/java/org/exist/http/urlrewrite/XQueryURLRewrite.java b/exist-core/src/main/java/org/exist/http/urlrewrite/XQueryURLRewrite.java index 7319032cb5d..7b9a6bb3d0a 100644 --- a/exist-core/src/main/java/org/exist/http/urlrewrite/XQueryURLRewrite.java +++ b/exist-core/src/main/java/org/exist/http/urlrewrite/XQueryURLRewrite.java @@ -934,7 +934,11 @@ private SourceInfo getSource(final DBBroker broker, final String moduleLoadPath) } private void declareVariables(final XQueryContext context, final SourceInfo sourceInfo, final URLRewrite staticRewrite, final String basePath, final RequestWrapper request, final HttpServletResponse response) throws XPathException { - final HttpRequestWrapper reqw = new HttpRequestWrapper(request, UTF_8.name(), UTF_8.name(), false); + // parseMultipart=true so that multipart/form-data uploads (including file parts) + // are exposed to controllers and RESTXQ resource functions for every HTTP method, + // not only POST. See https://github.com/eXist-db/exist/issues/6580 and + // https://github.com/eXist-db/exist/issues/6578 + final HttpRequestWrapper reqw = new HttpRequestWrapper(request, UTF_8.name(), UTF_8.name(), true); final HttpResponseWrapper respw = new HttpResponseWrapper(response); // context.declareNamespace(RequestModule.PREFIX, // RequestModule.NAMESPACE_URI); diff --git a/exist-core/src/test/java/org/exist/http/urlrewrite/MultipartMethodControllerTest.java b/exist-core/src/test/java/org/exist/http/urlrewrite/MultipartMethodControllerTest.java new file mode 100644 index 00000000000..b1b80564e1f --- /dev/null +++ b/exist-core/src/test/java/org/exist/http/urlrewrite/MultipartMethodControllerTest.java @@ -0,0 +1,133 @@ +/* + * 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" + + "hello\r\n" + + "--" + BOUNDARY + "--\r\n"; + + private static final String CONTROLLER = + """ + xquery version "3.1"; + + """; + + @Test + public void multipartFormDataIsParsedForBodyMethods() 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 for every body-carrying 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/PATCH 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", "PATCH"}) { + 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\"")); + } + } + + @Test + public void multipartFormDataIsNotParsedForGet() throws IOException { + final String coll = "multipart-method-controller-get"; + store(coll, LEGACY_XQUERY_CONTROLLER_FILENAME, "application/xquery", CONTROLLER); + + // GET is a safe method with no defined semantics for a request body (RFC 9110 §9.3.1), + // so a multipart/form-data body on GET must not be parsed: is-multipart-content() is false + // and no uploaded file is exposed to the handler. (Non-file form fields may still leak via + // the servlet container's parameter map — a pre-existing quirk this fix does not change.) + final String body = send(coll, "GET"); + assertTrue("GET: is-multipart-content() must be false: " + body, + body.contains("is-multipart=\"false\"")); + assertTrue("GET: uploaded file must not be exposed: " + body, + body.contains("uploaded-files=\"\"")); + } + + 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()); + } +}