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

WebSockets - MaxFramePayloadLength behaviour when using WebSocketServerSpec.compress(true) #3108

Closed
joebyneil opened this issue Mar 19, 2024 · 2 comments · Fixed by #3116
Closed
Assignees
Labels
type/documentation A documentation update
Milestone

Comments

@joebyneil
Copy link

Expected Behavior

When using compression, payloads handled by the server should be rejected if the uncompressed frame size is greater than the maxFramePayloadLength.

Unsure if this is an expected behaviour but thought I would raise for discussion.

Actual Behavior

Frames passed onto handler are larger than the configured maxFramePayloadLength.

This case also occurs regardless of value passed to WebSocketFrameAggregator(maxContentLength) via aggregateFrames(maxContentLength).

Steps to Reproduce

This test is modified version of existing tests in WebsocketTest.java

// Passes
@Test
void testServerMaxFramePayloadLengthSuccessWithoutCompress() {
    String repeat = new String(new char[300]).replace("\0", "m");
    doTestServerMaxFramePayloadLength(320,
            Flux.just("m", repeat), Flux.just("m", repeat), 2, false);
}

// Passes
@Test
void testServerMaxFramePayloadLengthFailedWithoutCompress() {
    String repeat = new String(new char[300]).replace("\0", "m");
    doTestServerMaxFramePayloadLength(8,
            Flux.just("m", repeat), Flux.just("m"), 2, false);
}

// This test fails, but is identical to above other than compression
@Test
void testServerMaxFramePayloadLengthFailedWithCompress() {
    String repeat = new String(new char[300]).replace("\0", "m");
    doTestServerMaxFramePayloadLength(8,
            Flux.just("m", repeat), Flux.just("m"), 2, true);
}

private void doTestServerMaxFramePayloadLength(int maxFramePayloadLength, Flux<String> input, Flux<String> expectation, int count, boolean compress) {
    disposableServer =
            createServer()
                        .handle((req, res) -> res.sendWebsocket((in, out) ->
                            out.sendObject(in.aggregateFrames()
                                            .receiveFrames()
                                            .map(WebSocketFrame::content)
                                            .map(byteBuf ->
                                                byteBuf.readCharSequence(byteBuf.readableBytes(), Charset.defaultCharset()).toString())
                                            .map(TextWebSocketFrame::new)),
                            WebsocketServerSpec.builder().maxFramePayloadLength(maxFramePayloadLength).compress(compress).build()))
                        .bindNow();

    AtomicReference<List<String>> output = new AtomicReference<>(new ArrayList<>());
    createClient(disposableServer.port())
                .websocket(WebsocketClientSpec.builder().compress(compress).build())
                .uri("/")
                .handle((in, out) -> out.sendString(input)
                                        .then(in.aggregateFrames()
                                                .receiveFrames()
                                                .map(WebSocketFrame::content)
                                                .map(byteBuf ->
                                                    byteBuf.readCharSequence(byteBuf.readableBytes(), Charset.defaultCharset()).toString())
                                                .take(count)
                                                .doOnNext(s -> output.get().add(s))
                                                .then()))
                .blockLast(Duration.ofSeconds(30));

    List<String> test = expectation.collectList().block(Duration.ofSeconds(30));
    assertThat(output.get()).isEqualTo(test);
}

Possible Solution

This could be expected behaviour, but wanted to confirm.

Maybe one idea could be to have a seperate limit for final message size and frame size limits, or otherwise make it clearer that the application may need to confirm the payload size when using compression.

Your Environment

Tested within the reactor-netty repo on commit: ab6c07aa2b5d6ef522f741bd09cadbbfc04cbb50

  • Other relevant libraries versions (eg. netty, ...): 4.1.107 final
  • JVM version (java -version): openjdk version "17.0.6" 2023-01-17 LTS
  • OS and version (eg. uname -a): Darwin Joebys-Laptop.local 23.4.0 Darwin Kernel Version 23.4.0: Wed Feb 21 21:44:43 PST 2024; root:xnu-10063.101.15~2/RELEASE_ARM64_T6000 arm64

Thanks for your help!

@joebyneil joebyneil added status/need-triage A new issue that still need to be evaluated as a whole type/bug A general bug labels Mar 19, 2024
@violetagg violetagg removed the status/need-triage A new issue that still need to be evaluated as a whole label Mar 19, 2024
@violetagg violetagg self-assigned this Mar 19, 2024
@violetagg
Copy link
Member

@joebyneil reactor.netty.http.websocket.WebsocketSpec#maxFramePayloadLength specifies the limit for the incoming packet, regardless whether it is compressed or not. May be we need to improve the javadoc description.

About reactor.netty.http.websocket.WebsocketInbound#aggregateFrames(int) it explicitly describes that the aggregation is related to fragmented frames: https://projectreactor.io/docs/netty/release/api/reactor/netty/http/websocket/WebsocketInbound.html#aggregateFrames-int-

@joebyneil
Copy link
Author

Thanks for your reply, I suspected this might be the case.

Updating javadocs sounds good.

I do think it would be useful to have the option to limit uncompressed size, but it's also easy enough to just add that check in our application code.

Thanks for your help!

@violetagg violetagg added type/documentation A documentation update and removed type/bug A general bug labels Mar 19, 2024
@violetagg violetagg added this to the 1.1.18 milestone Mar 19, 2024
@violetagg violetagg linked a pull request Mar 25, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/documentation A documentation update
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants