Discard the following HttpContent for preflight request by skyguard1 · Pull Request #15941 · netty/netty

skyguard1 · 2025-11-29T02:53:51Z

Motivation:
HttpServerCodec always constructs an EmptyLastHttpContent and passes it to the Handler chain when processing OPTIONS requests, The EmptyLastHttpContent propagate through the handler chain.

However, the CORS handler might still propagate the EmptyLastHttpContent to downstream handlers via fireChannelRead(), causing subsequent handlers to receive only this empty content and lose access to the original request URL.
Because CorsHandler does not consume this message, it calls ctx.fireChannelRead(msg) for the EmptyLastHttpContent.

Downstream handlers then observe:

No HttpRequest

A LastHttpContent with empty content

This often breaks other handlers that rely on receiving the HttpRequest first or expect consistent HTTP message.

This PR fixes an issue in CorsHandler where, after handling a CORS preflight (OPTIONS) request, the handler still propagates the subsequent EmptyLastHttpContent sent by the HttpServerCodec to downstream handler.
Modification:

This PR adds option to CorsHandler to track when a preflight request has been fully handled.
Track handled preflight and consume the following inbound HttpContent,LastHttpContent instead of firing it downstream.
Scope: Only affects CORS preflight flow; keeps normal request forwarding unchanged.
Tests: Added cases to verify empty and non-empty LastHttpContent are discarded after preflight and that normal requests continue to forward HttpRequest and content.

It also improves compatibility with application frameworks and routing handlers that expect well-formed HTTP request/response flows.

Result:

Fixes #15148

CorsHandler track handled preflight and consume the following HttpContent, LastHttpContent until the next HttpRequest is forwarded instead of firing it downstream.

…lass, int) ignores sampling interval

This reverts commit 87531c7

Signed-off-by: skyguard1 <qhdxssm@qq.com>

normanmaurer · 2025-12-01T09:04:40Z

hmm... I don't think this is correct. We either need to always pass down a HttpRequest + LastHttpContent or non of these at all. I don't think there should be any configuration be involved at all.

skyguard1 · 2025-12-01T09:19:08Z

hmm... I don't think this is correct. We either need to always pass down a HttpRequest + LastHttpContent or non of these at all. I don't think there should be any configuration be involved at all.

However, for preflight requests, the HttpRequest is not being passed down, but the LastHttpContent is. Did I missing something?

normanmaurer · 2025-12-01T09:22:33Z

@skyguard1 I am just saying we either always need to pass down the request + last content or never.. it always needs to be the combination or none at all.

skyguard1 · 2025-12-01T09:25:35Z

Preflight (OPTIONS + ACRM) is not a business request; it's merely a control request used internally by the browser to retrieve CORS metadata.

Therefore, the business logic should never:

receive the preflight HttpRequest

receive its HttpContent/LastHttpContent. So, for preflight requests, if you don't pass the HttpRequest, you should also not pass the LastHttpContent. This PR is essentially discarding the LastHttpContent for preflight requests, so I don't know what I'm missing.

normanmaurer · 2025-12-01T09:27:12Z

@skyguard1 What I am saying is that I don't think this should be configurable at all...

skyguard1 · 2025-12-01T09:28:41Z

I got it. So you think the flag should be removed, but this fix is valid. Right?

skyguard1 · 2025-12-01T09:31:10Z

My idea is to first minimize changes through the configuration, and then remove the flag after the release when users have configured and validated the behavior of discarding LastHttpContent. This satisfies the principle of minimal changes. Of course, we could also directly delete the flag to perform this fix.

normanmaurer · 2025-12-01T09:32:20Z

imho we should just fix it and not have the configuration API. As this is just another API that we need to keep. We could have a system property to fallback to the old behavior but I am not sure this is needed at all.

skyguard1 · 2025-12-01T09:34:16Z

Agreed. Then I will delete this configuration.

…t_http_content

Signed-off-by: skyguard1 <qhdxssm@qq.com>

skyguard1 · 2025-12-01T10:50:28Z

Done

…uest is forwarded Signed-off-by: skyguard1 <qhdxssm@qq.com>

Signed-off-by: skyguard1 <qhdxssm@qq.com>

…HandlerTest.java Co-authored-by: Norman Maurer <norman_maurer@apple.com>

skyguard1 · 2025-12-02T02:19:31Z

AutoScalingEventExecutorChooserFactoryTest.testScaleUp. It seems that the failure of this test case is not related to this PR

normanmaurer · 2025-12-02T07:41:17Z

@yawkat @vietj PTAL

normanmaurer

Almost there... Please also update the PR description etc to reflect what is actual done now

normanmaurer · 2025-12-02T07:42:58Z

    }
+
+    @Test
+    public void allowPrivateNetwork() {


nit: This seems to be not related to the PR

Yes. But I found this allowPrivateNetwork is missing for unit test, maybe remove this?

just do it in a separate PR

normanmaurer · 2025-12-02T07:43:49Z

+        assertFalse(ch.writeInbound(preflight));
+        ReferenceCountUtil.release(ch.readOutbound());
+
+        LastHttpContent first = LastHttpContent.EMPTY_LAST_CONTENT;


nit: You might also want to test with one non-empty one as it should be the same behavior

Signed-off-by: skyguard1 <qhdxssm@qq.com>

…_content

…t_http_content

…nt' into fix_cors_handler_last_http_content

Signed-off-by: skyguard1 <qhdxssm@qq.com>

normanmaurer · 2025-12-03T17:59:47Z

@skyguard1 thanks!

Motivation: HttpServerCodec always constructs an `EmptyLastHttpContent` and passes it to the Handler chain when processing OPTIONS requests, The `EmptyLastHttpContent` propagate through the handler chain. However, the CORS handler might still propagate the EmptyLastHttpContent to downstream handlers via fireChannelRead(), causing subsequent handlers to receive only this empty content and lose access to the original request URL. Because `CorsHandler` does not consume this message, it calls `ctx.fireChannelRead(msg)` for the EmptyLastHttpContent. Downstream handlers then observe: No HttpRequest A LastHttpContent with empty content This often breaks other handlers that rely on receiving the `HttpRequest` first or expect consistent HTTP message. This PR fixes an issue in `CorsHandler` where, after handling a CORS preflight (OPTIONS) request, the handler still propagates the subsequent `EmptyLastHttpContent` sent by the `HttpServerCodec` to downstream handler. Modification: - `CorsHandler` track handled preflight and consume the following HttpContent, LastHttpContent until the next HttpRequest is forwarded instead of firing it downstream. - Add tests Result: Fixes #15148 It also improves compatibility with application frameworks and routing handlers that expect well-formed HTTP request/response flows. --------- Signed-off-by: skyguard1 <qhdxssm@qq.com> Co-authored-by: xingrufei <xingrufei@yl.com> Co-authored-by: code-xing_wcar <code.xing@wirelesscar.com> Co-authored-by: Norman Maurer <norman_maurer@apple.com>

) Motivation: HttpServerCodec always constructs an `EmptyLastHttpContent` and passes it to the Handler chain when processing OPTIONS requests, The `EmptyLastHttpContent` propagate through the handler chain. However, the CORS handler might still propagate the EmptyLastHttpContent to downstream handlers via fireChannelRead(), causing subsequent handlers to receive only this empty content and lose access to the original request URL. Because `CorsHandler` does not consume this message, it calls `ctx.fireChannelRead(msg)` for the EmptyLastHttpContent. Downstream handlers then observe: No HttpRequest A LastHttpContent with empty content This often breaks other handlers that rely on receiving the `HttpRequest` first or expect consistent HTTP message. This PR fixes an issue in `CorsHandler` where, after handling a CORS preflight (OPTIONS) request, the handler still propagates the subsequent `EmptyLastHttpContent` sent by the `HttpServerCodec` to downstream handler. Modification: - `CorsHandler` track handled preflight and consume the following HttpContent, LastHttpContent until the next HttpRequest is forwarded instead of firing it downstream. - Add tests Result: Fixes #15148 It also improves compatibility with application frameworks and routing handlers that expect well-formed HTTP request/response flows. Signed-off-by: skyguard1 <qhdxssm@qq.com> Co-authored-by: skyguard1 <qhdxssm@qq.com> Co-authored-by: xingrufei <xingrufei@yl.com> Co-authored-by: code-xing_wcar <code.xing@wirelesscar.com>

skyguard1 · 2025-12-04T06:02:41Z

It is my pleasure

) Motivation: HttpServerCodec always constructs an `EmptyLastHttpContent` and passes it to the Handler chain when processing OPTIONS requests, The `EmptyLastHttpContent` propagate through the handler chain. However, the CORS handler might still propagate the EmptyLastHttpContent to downstream handlers via fireChannelRead(), causing subsequent handlers to receive only this empty content and lose access to the original request URL. Because `CorsHandler` does not consume this message, it calls `ctx.fireChannelRead(msg)` for the EmptyLastHttpContent. Downstream handlers then observe: No HttpRequest A LastHttpContent with empty content This often breaks other handlers that rely on receiving the `HttpRequest` first or expect consistent HTTP message. This PR fixes an issue in `CorsHandler` where, after handling a CORS preflight (OPTIONS) request, the handler still propagates the subsequent `EmptyLastHttpContent` sent by the `HttpServerCodec` to downstream handler. Modification: - `CorsHandler` track handled preflight and consume the following HttpContent, LastHttpContent until the next HttpRequest is forwarded instead of firing it downstream. - Add tests Result: Fixes #15148 It also improves compatibility with application frameworks and routing handlers that expect well-formed HTTP request/response flows. Signed-off-by: skyguard1 <qhdxssm@qq.com> Co-authored-by: skyguard1 <qhdxssm@qq.com> Co-authored-by: xingrufei <xingrufei@yl.com> Co-authored-by: code-xing_wcar <code.xing@wirelesscar.com>

xingrufei and others added 11 commits June 30, 2020 19:21

Fix netty#10378,ResourceLeakDetectorFactory.newResourceLeakDetector(C…

aac7a96

…lass, int) ignores sampling interval

format code

87531c7

Revert "format code"

0f04075

This reverts commit 87531c7

Verify that the samplingInterval parameter is positive

d7b6d67

Merge branch 'netty:4.1' into 4.1

4ca4309

Merge branch 'netty:4.1' into 4.1

8ee614a

Merge branch '4.1' of https://github.com/skyguard1/netty into 4.1

994ed46

Merge remote-tracking branch 'upstream/4.1' into 4.1

173ffde

Allow user to discard the lastHttpContent for preflight request

f80be2d

Signed-off-by: skyguard1 <qhdxssm@qq.com>

Fix checkstyle issue

a0adc6b

Signed-off-by: skyguard1 <qhdxssm@qq.com>

Remove channelInactive in CorsHandler

ee678ea

Signed-off-by: skyguard1 <qhdxssm@qq.com>

skyguard1 added 2 commits December 1, 2025 18:40

Merge remote-tracking branch 'upstream/4.2' into fix_cors_handler_las…

f365d1d

…t_http_content

Remove discardNextEmptyLast in CorsHandler

56e06cf

Signed-off-by: skyguard1 <qhdxssm@qq.com>

skyguard1 changed the title ~~Allow user to discard the lastHttpContent for preflight request~~ Discard the lastHttpContent for preflight request Dec 1, 2025

normanmaurer added this to the 4.2.8.Final milestone Dec 1, 2025

normanmaurer added needs-cherry-pick-4.1 This PR should be cherry-picked to 4.1 once merged. needs-cherry-pick-5.0 This PR should be cherry-picked to 5.0 once merged. labels Dec 1, 2025

normanmaurer requested changes Dec 1, 2025

View reviewed changes

Comment thread codec-http/src/main/java/io/netty/handler/codec/http/cors/CorsHandler.java

Discard following HttpContent, LastHttpContent until the next HttpReq…

38d0832

…uest is forwarded Signed-off-by: skyguard1 <qhdxssm@qq.com>

normanmaurer requested changes Dec 1, 2025

View reviewed changes

Comment thread codec-http/src/test/java/io/netty/handler/codec/http/cors/CorsHandlerTest.java Outdated

skyguard1 added 2 commits December 1, 2025 21:56

Update by review

9bd89fb

Signed-off-by: skyguard1 <qhdxssm@qq.com>

Add assertNotNull for outbound request

2348c19

Signed-off-by: skyguard1 <qhdxssm@qq.com>

normanmaurer requested changes Dec 1, 2025

View reviewed changes

skyguard1 and others added 4 commits December 2, 2025 10:02

Update codec-http/src/test/java/io/netty/handler/codec/http/cors/Cors…

cdd2b41

…HandlerTest.java Co-authored-by: Norman Maurer <norman_maurer@apple.com>

Update codec-http/src/test/java/io/netty/handler/codec/http/cors/Cors…

5fd3579

…HandlerTest.java Co-authored-by: Norman Maurer <norman_maurer@apple.com>

Update codec-http/src/test/java/io/netty/handler/codec/http/cors/Cors…

b8c2d20

…HandlerTest.java Co-authored-by: Norman Maurer <norman_maurer@apple.com>

Update codec-http/src/test/java/io/netty/handler/codec/http/cors/Cors…

98c16ea

…HandlerTest.java Co-authored-by: Norman Maurer <norman_maurer@apple.com>

normanmaurer requested a review from yawkat December 2, 2025 07:40

normanmaurer reviewed Dec 2, 2025

View reviewed changes

Comment thread codec-http/src/main/java/io/netty/handler/codec/http/cors/CorsHandler.java Outdated

normanmaurer requested changes Dec 2, 2025

View reviewed changes

skyguard1 changed the title ~~Discard the lastHttpContent for preflight request~~ Discard the following HttpContent for preflight request Dec 2, 2025

skyguard1 added 4 commits December 2, 2025 18:53

Update by review

7af403e

Signed-off-by: skyguard1 <qhdxssm@qq.com>

Merge remote-tracking branch 'origin' into fix_cors_handler_last_http…

1ea49ce

…_content

Merge remote-tracking branch 'upstream/4.2' into fix_cors_handler_las…

11016c5

…t_http_content

Merge remote-tracking branch 'origin/fix_cors_handler_last_http_conte…

c197123

…nt' into fix_cors_handler_last_http_content

chrisvest reviewed Dec 2, 2025

View reviewed changes

Comment thread codec-http/src/test/java/io/netty/handler/codec/http/cors/CorsHandlerTest.java

Add preflightEmptyLastDiscardedThenNewRequestForwarded test case

8e3034f

Signed-off-by: skyguard1 <qhdxssm@qq.com>

chrisvest approved these changes Dec 3, 2025

View reviewed changes

normanmaurer merged commit 28e8017 into netty:4.2 Dec 3, 2025
19 checks passed

Uh oh!

Conversation

skyguard1 commented Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

normanmaurer commented Dec 1, 2025

Uh oh!

skyguard1 commented Dec 1, 2025

Uh oh!

normanmaurer commented Dec 1, 2025

Uh oh!

skyguard1 commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

normanmaurer commented Dec 1, 2025

Uh oh!

skyguard1 commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skyguard1 commented Dec 1, 2025

Uh oh!

normanmaurer commented Dec 1, 2025

Uh oh!

skyguard1 commented Dec 1, 2025

Uh oh!

skyguard1 commented Dec 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skyguard1 commented Dec 2, 2025

Uh oh!

normanmaurer commented Dec 2, 2025

Uh oh!

Uh oh!

normanmaurer left a comment

Choose a reason for hiding this comment

Uh oh!

normanmaurer Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

skyguard1 Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

normanmaurer Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

normanmaurer commented Dec 3, 2025

Uh oh!

skyguard1 commented Dec 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

skyguard1 commented Nov 29, 2025 •

edited

Loading

skyguard1 commented Dec 1, 2025 •

edited

Loading

skyguard1 commented Dec 1, 2025 •

edited

Loading

skyguard1 Dec 2, 2025 •

edited

Loading