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

Validation of PartEventHttpMessageReader#maxParts is off by one #32122

Closed
kzander91 opened this issue Jan 25, 2024 · 3 comments
Closed

Validation of PartEventHttpMessageReader#maxParts is off by one #32122

kzander91 opened this issue Jan 25, 2024 · 3 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Milestone

Comments

@kzander91
Copy link
Contributor

kzander91 commented Jan 25, 2024

Boot: 3.2.2
Framework: 6.1.3

PartEventHttpMessageReader#maxParts, configured via spring.webflux.multipart.max-parts, is validated incorrectly. Sending a multipart request with as many parts as should be allowed by that property is rejected with Too many parts.
This means that if I want to allow at most 5 parts, I'll have to set spring.webflux.multipart.max-parts=6.

Controller:

@RestController
class TestController {

    @PostMapping
    public Mono<Void> upload(@RequestBody Flux<PartEvent> events) {
        return events
                .doOnError(Throwable::printStackTrace)
                .then();
    }

}

This test creates as many file parts as configured via spring.webflux.multipart.max-parts and sends them to the controller method above, expecting success. The request is however rejected and the following exception is thrown: org.springframework.core.codec.DecodingException: Too many parts (6/5 allowed)

@WebFluxTest(value = TestController.class, properties = "spring.webflux.multipart.max-parts=5")
@ImportAutoConfiguration(ReactiveMultipartAutoConfiguration.class)
class ReproducerTests {

    @Autowired
    private WebTestClient webTestClient;

    @Autowired
    private ReactiveMultipartProperties props;

    @Test
    void reproducer() {
        Flux<FilePartEvent> events = Flux.empty();
        for (int i = 0; i < props.getMaxParts(); i++) {
            byte[] bytes = new byte[1024];
            ThreadLocalRandom.current().nextBytes(bytes);
            events = events.concatWith(FilePartEvent.create(
                    "file-data",
                    "file-" + i + ".bin",
                    MediaType.APPLICATION_OCTET_STREAM,
                    Flux.just(DefaultDataBufferFactory.sharedInstance.wrap(bytes))
            ));
        }

        webTestClient.post()
                .body(events, PartEvent.class)
                .exchange()
                .expectStatus().isOk();
    }

}

Reproducer (extract and run ./mvnw test): demo.zip

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 25, 2024
@jhoeller jhoeller added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jan 25, 2024
@rstoyanchev
Copy link
Contributor

I can reproduce the behavior with the demo. From what I can see the calculation in PartEventHttpMessageReader is correct, but there is one extra emission from concatMap here in the beginning.

The Javadoc for windowUntil says "This variant can emit an empty window if the sequence starts with a separator" and since the sequence starts with a headers token, I think that is the reason for the extra emission.

Maybe the counting should be moved a few lines down to here inside the switchOnFirst where we know we are handling the headers for the next part?

What do you think @poutsma?

@rstoyanchev
Copy link
Contributor

Thanks for the report by the way @kzander91.

@snicoll snicoll added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 26, 2024
@snicoll snicoll added this to the 6.1.x milestone Jan 26, 2024
@poutsma poutsma self-assigned this Feb 1, 2024
@poutsma poutsma modified the milestones: 6.1.x, 6.1.4 Feb 5, 2024
@poutsma poutsma closed this as completed in c570f3b Feb 5, 2024
@poutsma
Copy link
Contributor

poutsma commented Feb 5, 2024

Maybe the counting should be moved a few lines down to here inside the switchOnFirst where we know we are handling the headers for the next part?

What do you think @poutsma?

Sounds good, so that's what I did in c570f3b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

6 participants