Fix #6860 IPv6 Format #6861

gregw · 2021-09-17T06:22:35Z

Fix #6860 IPv6 format by adding a per context configuration.

Signed-off-by: Greg Wilkins gregw@webtide.com

Fix #6860 IPv6 format by adding a per context configuration. Signed-off-by: Greg Wilkins <gregw@webtide.com>

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts · 2021-09-17T06:52:27Z

@gregw I pushed a fix to this branch, it had about 60 checkstyle failures.
I think you laptop is not configured with the Jetty intellij code style.

jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java

jetty-server/src/main/java/org/eclipse/jetty/server/Request.java

updates from review Signed-off-by: Greg Wilkins <gregw@webtide.com>

sbordet · 2021-09-17T08:28:41Z

@gregw this PR is missing tests, I fear that the "unchanged" semantic may not work as the field could already be set "normalized".

unit test and fixes Signed-off-by: Greg Wilkins <gregw@webtide.com>

fix style Signed-off-by: Greg Wilkins <gregw@webtide.com>

lachlan-roberts · 2021-09-20T00:37:16Z

@gregw this still doesn't work too well with ForwardedRequestCustomizer, the problem is that it goes through HostPort first which will bracket the IPv6 address regardless of the IPv6Format. The other difference is that 9.3 treats the last segment of an unbracketed address as the port and 9.4 does not. So the behaviour is now:

=== Jetty 9.4 ===
UNBRACKETED: 1:2:3:4:5:6:7:8:8080 --> 1:2:3:4:5:6:7:8:8080
UNCHANGED:   1:2:3:4:5:6:7:8:8080 --> [1:2:3:4:5:6:7:8:8080]

=== Jetty 9.3 ===
1:2:3:4:5:6:7:8:8080 --> 1:2:3:4:5:6:7:8

gregw · 2021-09-20T01:08:32Z

@lachlan-roberts I'm open to suggestions? But I'm not sure that 1:2:3:4:5:6:7:8:8080 is really ever valid?

gregw · 2021-09-20T05:14:11Z

So 9.3 just assumed that the last : always indicated a port. I think this was a bad/wrong/dangerous thing. So I'm not sure we should be bug for bug compatible with that.

lachlan-roberts · 2021-09-20T05:18:04Z

So 9.3 just assumed that the last : always indicated a port. I think this was a bad/wrong/dangerous thing. So I'm not sure we should be bug for bug compatible with that.

That's fine. But I think the main issue is that HostPort already normalizes the address. This means even using ForwardedRequestCustomizer the IPv6Format.UNCHANGED acts the same as IPv6Format.BRACKETED.

gregw · 2021-09-20T05:21:26Z

Which is why we have the UNBRACKETED option. I don't think we can do anything else as this is a per webapp setting and the customizer is run before the webapp is known.
I think if we run with either UNBRACKETED or UNCHANGED then the sponsered use-case will work fine.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw · 2021-09-20T06:54:16Z

Both @lachlan-roberts and I were not totally happy that UNCHANGED didn't really work as we frequently normalize elsewhere. So instead of introducing new forever public configuration, this change now is the minimal that we can do to make the formatting in an easily extensible class for the sponsor. @sbordet please re-review.

lachlan-roberts · 2021-09-20T23:57:34Z

@gregw this PR has failures in DoSFilterTest:

[INFO] Running org.eclipse.jetty.servlets.DoSFilterTest
Running org.eclipse.jetty.servlets.DoSFilterTest.testRemotePortLoadIdCreationIpv6()
Expected: (is "[::192.9.5.5]:12345" or is "[0:0:0:0:0:0:c009:505]:12345")
     but: was "0:0:0:0:0:0:c009:505:12345"

Signed-off-by: Greg Wilkins <gregw@webtide.com>

sbordet

LGTM apart a glitch in Request.formatAddrOrHost().

sbordet · 2021-09-21T20:33:14Z

jetty-server/src/main/java/org/eclipse/jetty/server/Request.java

+
+    private String formatAddrOrHost(String name)
+    {
+        return _channel == null ? HostPort.normalizeHost(name) : _channel.formatAddrOrHost(name);


_channel is a final field. Can it ever be null?
If it is only null in tests, you may want to add a comment about that.

It should only be null in tests, but we have a specific test to check it works with null.

Fix #6860 IPv6 format by adding an extensible HttpChannel method Signed-off-by: Greg Wilkins <gregw@webtide.com> Co-authored-by: Lachlan Roberts <lachlan@webtide.com> Signed-off-by: Greg Wilkins <gregw@webtide.com> # Conflicts: # jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java # jetty-server/src/main/java/org/eclipse/jetty/server/Request.java # jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java # jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java

Fix #6860 IPv6 format by adding an extensible HttpChannel method Signed-off-by: Greg Wilkins <gregw@webtide.com> Co-authored-by: Lachlan Roberts <lachlan@webtide.com>

Fix #6860 IPv6 format by adding an extensible HttpChannel method Signed-off-by: Greg Wilkins <gregw@webtide.com> Co-authored-by: Lachlan Roberts <lachlan@webtide.com> Signed-off-by: Greg Wilkins <gregw@webtide.com> # Conflicts: # jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java # jetty-server/src/main/java/org/eclipse/jetty/server/Request.java # jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java # jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java

Fix #6860 IPv6 Format

79395c5

Fix #6860 IPv6 format by adding a per context configuration. Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw requested review from lachlan-roberts and sbordet September 17, 2021 06:22

gregw added this to In progress in Jetty 9.4.44 FROZEN via automation Sep 17, 2021

gregw added High Priority Sponsored This issue affects a user with a commercial support agreement labels Sep 17, 2021

gregw self-assigned this Sep 17, 2021

Fix checkstyle errors after IPv6 Format changes.

de46e0a

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

sbordet requested changes Sep 17, 2021

View reviewed changes

Jetty 9.4.44 FROZEN automation moved this from In progress to Review in progress Sep 17, 2021

Fix #6860 IPv6 Format

316b4bf

updates from review Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw added 2 commits September 19, 2021 13:42

Fix #6860 IPv6 Format

11720f7

unit test and fixes Signed-off-by: Greg Wilkins <gregw@webtide.com>

Fix #6860 IPv6 Format

38192fc

fix style Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw added 2 commits September 20, 2021 16:43

simplified solution to avoid adding new public APIs

1ddeeed

Signed-off-by: Greg Wilkins <gregw@webtide.com>

protect from NPE

8674bfd

Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw requested a review from sbordet September 20, 2021 06:54

joakime linked an issue Sep 20, 2021 that may be closed by this pull request

IPv6 format #6860

Closed

normalize if no channel

6ff3c04

Signed-off-by: Greg Wilkins <gregw@webtide.com>

sbordet approved these changes Sep 21, 2021

View reviewed changes

Jetty 9.4.44 FROZEN automation moved this from Review in progress to Reviewer approved Sep 21, 2021

gregw merged commit 9b3fb19 into jetty-9.4.x Sep 21, 2021

Jetty 9.4.44 FROZEN automation moved this from Reviewer approved to Done Sep 21, 2021

gregw deleted the jetty-9.4.x-6860-Ipv6-format branch September 21, 2021 22:21

gregw added a commit that referenced this pull request Sep 22, 2021

Fix #6860 IPv6 Format (#6861) (#6885)

9a14de0

Fix #6860 IPv6 format by adding an extensible HttpChannel method Signed-off-by: Greg Wilkins <gregw@webtide.com> Co-authored-by: Lachlan Roberts <lachlan@webtide.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #6860 IPv6 Format #6861

Fix #6860 IPv6 Format #6861

gregw commented Sep 17, 2021

lachlan-roberts commented Sep 17, 2021

sbordet commented Sep 17, 2021

lachlan-roberts commented Sep 20, 2021

gregw commented Sep 20, 2021

gregw commented Sep 20, 2021

lachlan-roberts commented Sep 20, 2021

gregw commented Sep 20, 2021

gregw commented Sep 20, 2021

lachlan-roberts commented Sep 20, 2021

sbordet left a comment

sbordet Sep 21, 2021

gregw Sep 21, 2021

Fix #6860 IPv6 Format #6861

Fix #6860 IPv6 Format #6861

Conversation

gregw commented Sep 17, 2021

lachlan-roberts commented Sep 17, 2021

sbordet commented Sep 17, 2021

lachlan-roberts commented Sep 20, 2021

gregw commented Sep 20, 2021

gregw commented Sep 20, 2021

lachlan-roberts commented Sep 20, 2021

gregw commented Sep 20, 2021

gregw commented Sep 20, 2021

lachlan-roberts commented Sep 20, 2021

sbordet left a comment

Choose a reason for hiding this comment

sbordet Sep 21, 2021

Choose a reason for hiding this comment

gregw Sep 21, 2021

Choose a reason for hiding this comment