Skip to content
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

MAX_ENTITY_SIZE or max-http-post-size configuration parameters are overridden by spring.servlet.multipart.max-request-size when using Undertow #18555

Closed
jansu76 opened this issue Oct 10, 2019 · 10 comments
Labels
status: superseded An issue that has been superseded by another type: bug A general bug

Comments

@jansu76
Copy link

jansu76 commented Oct 10, 2019

Tried with Spring Boot 2.1.3.RELEASE, 2.1.6.RELEASE, and 2.1.9.RELEASE.

Setting application property server.undertow.max-http-post or server option UndertowOptions.MAX_ENTITY_SIZE does not seem to work, when using Undertow.

Detailed explanation at https://stackoverflow.com/questions/58222154/unable-to-use-undertowservletwebserverfactory-to-limit-max-entity-size

Minimal test repo to replicate the problem: https://github.com/jansu76/gs-spring-boot/tree/undertow-testing/complete

Diff to master to see what I have changed from https://github.com/spring-guides/gs-spring-boot:
spring-guides/gs-spring-boot@master...jansu76:undertow-testing

To test:

cd complete
./gradlew build
java -jar build/libs/gs-spring-boot-0.1.0.jar
(different terminal) cd src/test/resources/
./test-post.sh

Script should do a large POST request to the application, which should be limited by Undertow max post size config. But it is not.

It seems UndertowServletWebServerFactory sets port to 9090 but UndertowOptions.MAX_ENTITY_SIZE = 100 has no effect. server.undertow.max-http-post-size=10 sets some max entity size values for some time, but those are overwritten in io.undertow.servlet.handlers.ServletInitialHandler.handleRequest:181 which overwrites maxEntitySize value from 10 to 10485760.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 10, 2019
@wilkinsona
Copy link
Member

Thank you for the sample, @jansu76.

In looking at it I have just learned that Undertow has two ways of configuring the maximum entity size for a request. The MAX_ENTITY_SIZE option (which is also what our server.undertow.max-http-post-size property maps to) applies across the server as a whole. The max request size configured on any MultipartConfigElement of the servlet that is the target of the request is also considered and takes precedence over the server-wide MAX_ENTITY_SIZE option. I find this confusing as it's applying a multipart-specific setting to all requests but that's the way that Undertow works.

For many Boot apps where all requests are handled by a servlet, this means that setting spring.servlet.multipart.max-request-size will be sufficient to configure the maximum size for all requests. I think it may still be necessary to set server.undertow.max-http-post-size when servlets aren't in the picture (for example when using WebFlux on top of Undertow).

We'll need to straighten this out but I'm not yet sure how best to do that. In the meantime setting spring.servlet.multipart.max-request-size should result in the behaviour that you want.

@wilkinsona wilkinsona changed the title MAX_ENTITY_SIZE or max-http-post-size configuration parameters don't work when using Undertow MAX_ENTITY_SIZE or max-http-post-size configuration parameters are overridden by spring.servlet.multipart.max-request-size when using Undertow Oct 10, 2019
@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Oct 10, 2019
@wilkinsona wilkinsona self-assigned this Oct 11, 2019
@wilkinsona
Copy link
Member

Before we proceed on this, we should open an Undertow issue as the current behaviour may not be intentional. It feels wrong that a multi-part specific piece of configuration affects all requests to a servlet.

@wilkinsona
Copy link
Member

I've opened UNDERTOW-1601.

@wilkinsona wilkinsona added status: blocked An issue that's blocked on an external project change type: bug A general bug and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Oct 14, 2019
@wilkinsona wilkinsona added this to the 2.1.x milestone Oct 14, 2019
@wilkinsona wilkinsona removed their assignment Oct 14, 2019
@jansu76
Copy link
Author

jansu76 commented Oct 23, 2019

Just to comment, using spring.servlet.multipart.max-request-size is not really a usable option for us since we would like to allow large multipart uploads while limiting other types of http requests more strictly.

Ideally we would have perhaps

spring.servlet.multipart.max-request-size=50MB
server.undertow.max-http-post=50KB

in our configuration.

But I understand https://issues.jboss.org/browse/UNDERTOW-1601 is needed for this to be possible, fingers crossed it gets timely resolution!

@jansu76
Copy link
Author

jansu76 commented Oct 23, 2019

UndertowOptions has MULTIPART_MAX_ENTITY_SIZE and javadocs hint that you should be able to use it to what I am after

The default maximum size of the HTTP entity body when using the mutiltipart parser. Generall this will be larger than {@link #MAX_ENTITY_SIZE}

I tried to test with a modified version of the code posted in UNDERTOW-1601

package example;

import io.undertow.Undertow;
import io.undertow.UndertowOptions;
import io.undertow.server.HttpHandler;
import io.undertow.servlet.api.DeploymentInfo;
import io.undertow.servlet.api.DeploymentManager;

import javax.servlet.ServletException;
import javax.servlet.annotation.MultipartConfig;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import java.io.IOException;

import static io.undertow.servlet.Servlets.defaultContainer;
import static io.undertow.servlet.Servlets.deployment;
import static io.undertow.servlet.Servlets.servlet;

public class MaxRequestSizeConfigurationProblem {

    public static void main(final String[] args) {
        try {
            DeploymentInfo servletBuilder = deployment()
                                                    .setClassLoader(MaxRequestSizeConfigurationProblem.class.getClassLoader()).setContextPath("/")
                                                    .setDeploymentName("test.war")
                                                    .addServlets(servlet("TestServlet", TestServlet.class).addMapping("/*"));
            DeploymentManager manager = defaultContainer().addDeployment(servletBuilder);
            manager.deploy();
            HttpHandler servletHandler = manager.start();
            Undertow server = Undertow.builder().setServerOption(UndertowOptions.MAX_ENTITY_SIZE, 20L)
                                      .setServerOption(UndertowOptions.MULTIPART_MAX_ENTITY_SIZE, 300L)
                                      .addHttpListener(8080, "localhost").setHandler(servletHandler).build();
            server.start();
        }
        catch (ServletException e) {
            throw new RuntimeException(e);
        }
    }

    @MultipartConfig
    public static class TestServlet extends HttpServlet {

        @Override
        protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
            byte[] bytes = new byte[8192];
            int read;
            System.out.println("Content-Type: " + req.getHeader("Content-Type"));
            while ((read = req.getInputStream().read(bytes)) > 0) {
                System.out.println("Read " + read + " bytes");
            }
        }
    }
}

but to my disappointment both normal posts and multipart posts were limited to the same MAX_ENTITY_SIZE value of 20 bytes.

Not sure if this gives any new information, but posting it in any case.

@wilkinsona
Copy link
Member

That's an interesting finding, @jansu76. Thank you. The presence of a multipart config element may still cause problems, but I wonder if there's something Undertow-specific that we can do here.

@wilkinsona
Copy link
Member

wilkinsona commented Oct 24, 2019

Doing something Undertow-specific in Boot itself isn't straightforward, but there is a workaround that you can use in your own application by adding a couple of beans.

First we define a custom MultipartConfigElement that ignores any size limits:

@Bean
public MultipartConfigElement multipartConfigElement(MultipartProperties properties) {
    MultipartConfigFactory factory = new MultipartConfigFactory();
    PropertyMapper map = PropertyMapper.get().alwaysApplyingWhenNonNull();
    map.from(properties::getFileSizeThreshold).to(factory::setFileSizeThreshold);
    map.from(properties::getLocation).whenHasText().to(factory::setLocation);
    return factory.createMultipartConfig();
}

Then we customise Undertow to set its max entity size and multipart entity size:

@Bean
public WebServerFactoryCustomizer<UndertowServletWebServerFactory> undertowCustomizer() {
    return (undertowFactory) -> undertowFactory.addBuilderCustomizers((builder) -> {
        builder.setServerOption(UndertowOptions.MULTIPART_MAX_ENTITY_SIZE, 52428800L);
        builder.setServerOption(UndertowOptions.MAX_ENTITY_SIZE, 51200L);
    });
}

With changes similar to these in place, Undertow rejects the large post request as required, but does not reject a large multipart post.

@philwebb philwebb modified the milestones: 2.1.x, 2.2.x Jun 10, 2020
@wilkinsona
Copy link
Member

There's been no traction at all on the Undertow issue I opened 12 months ago. If this issue is important to you, please comment on or vote for the Undertow issue so that the team can prioritise it accordingly.

Given the lack of traction on Undertow issues that we have raised such as UNDERTOW-1601 and UNDERTOW-1743 and the Undertow team choosing not to fix CVEs in 2.1 which GAed only 5 months ago, I think it's worth anyone using Undertow re-evaluating the benefits that attracted them to it.

Bugs not being fixed and requiring minor upgrades to avoid security vulnerabilities would weigh very heavily for me against any other benefits. I'd want to be using a container with a maintenance approach that better met my needs.

I'm moving this issue to the general backlog as it doesn't feel realistic to plan for a fix during 2.2.x's lifespan. As and when the necessary changes are made in Undertow we can schedule any work that is necessary in Boot to make use of them.

@wilkinsona wilkinsona modified the milestones: 2.2.x, General Backlog Nov 3, 2020
@wilkinsona
Copy link
Member

A fix for UNDERTOW-1601 is coming in Undertow 2.2.9. Assigning this to 2.6.x so that we can see what, if anything, we can do/need to do on top of that fix.

@wilkinsona wilkinsona removed the status: blocked An issue that's blocked on an external project change label Jun 22, 2021
@wilkinsona wilkinsona modified the milestones: General Backlog, 2.6.x Jun 22, 2021
@wilkinsona wilkinsona added status: blocked An issue that's blocked on an external project change and removed status: blocked An issue that's blocked on an external project change labels Jul 15, 2021
@wilkinsona
Copy link
Member

This works as expected with Undertow 2.2.9 without any other changes. The server side logs a warning:

2021-07-26 11:33:51.761  WARN 88378 --- [  XNIO-1 task-1] .w.s.m.s.DefaultHandlerExceptionResolver : Resolved [org.springframework.http.converter.HttpMessageNotReadableException: I/O error while reading input message; nested exception is io.undertow.server.RequestTooBigException: UT000020: Connection terminated as request was larger than 10]

And the client-side receives a 400 response (test script modified to invoke curl with -v:

./test-post.sh
Note: Unnecessary use of -X or --request, POST is already inferred.
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 9090 (#0)
> POST / HTTP/1.1
> Host: localhost:9090
> User-Agent: curl/7.64.1
> Accept: */*
> Content-Type: application/json
> Content-Length: 267919
> Expect: 100-continue
> 
< HTTP/1.1 100 Continue
< Content-Length: 0
* We are completely uploaded and fine
< HTTP/1.1 400 Bad Request
< Connection: close
< Content-Type: application/json
< Date: Mon, 26 Jul 2021 10:34:04 GMT
< 
* Excess found in a non pipelined read: excess = 104 url = / (zero-length body)
* Closing connection 0

@wilkinsona wilkinsona removed this from the 2.6.x milestone Jul 26, 2021
@wilkinsona wilkinsona added the status: superseded An issue that has been superseded by another label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants