Encode IPV6 Zone IDs (%) in ReactorServerHttpRequest

[ls-urs-keller]

Affects: 6.0.7 (spring-web)

@Test
void test() {
	UriComponentsBuilder
		.fromUri(URI.create("http://[0:0:0:0:0:0:0:1%0]:8082/graphql"))
		.build(true);
}

Fails with

Invalid encoded sequence "%0]"

The code doesn't handle the ipv6 address zone id / scope id correctly.

[sbrannen]

Hi @ls-urs-keller,

Thanks for reporting the issue.

I converted your example to a test and have confirmed that it fails as you pointed out.

We will investigate our options.

Related Resources

• https://datatracker.ietf.org/doc/html/rfc3986
• https://datatracker.ietf.org/doc/html/rfc4007
• https://datatracker.ietf.org/doc/html/rfc6874

[ls-urs-keller]

@sbrannen Thank you for the quick reply.
This happens to us when doing calls on side cars in a multi-container k8s pod.
E.g. 1 pod with 2 containers, the container on port 8080 calls the container on port 8081 through localhost.
Using Spring graphql. We noticed the regression somewhere between spring boot 3.0.0 and 3.0.4, which probably relates to a netty update.

Our current workaround is to bind to the ipv4 loopback address through server.address: 127.0.0.1
But that only works for the specific case where there is only container-to-container traffic.

[poutsma]

RFC 6874, Section 2 shows us that

a zone identifier is attached to the textual representation of an IPv6 address by concatenating "%" followed by <zone_id>, where <zone_id> is a string identifying the zone of the address

and

any occurrences of literal "%" symbols in a URI MUST be percent-encoded and represented in the form "%25".

I believe URI.create should throw an IllegalArgumentException for http://[0:0:0:0:0:0:0:1%0]:8082/graphql, because the % has not been encoded to %25.

Passing true to UriComponentsBuilder::build indicates that the builder is in a fully encoded state. To ensure it is in a legal state, and that all parts of the URI have indeed been properly encoded, the UriComponentsBuilder performs an verification step, which throws the exception when it comes across the unencoded %.

Passing an encoded URI seems to work fine:

UriComponentsBuilder
    .fromUri(URI.create("http://[0:0:0:0:0:0:0:1%250]:8082/graphql"))
    .build(true);

as does passing false to build:

UriComponentsBuilder
    .fromUri(URI.create("http://[0:0:0:0:0:0:0:1%0]:8082/graphql"))
    .build(false); // or build()

@ls-urs-keller Where does the URI that triggers this behavior come from? Can you provide a stack trace? I am guessing it is from Reactor Netty, but I'd like to be sure. In any case, the code that creates that URI should be fixed to properly encode the %.

I also think that a bug should be reported to OpenJDK, as URI::create should throw an exception for that unencoded URI string.

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[sbrannen]

I also think that a bug should be reported to OpenJDK, as URI::create should throw an exception for that unencoded URI string.

This is a bit of a tricky topic.

Section 3, Web Browsers in RFC-6874 states the following (emphasis mine).

It is desirable for all browsers to recognise a ZoneID preceded by a percent-encoded "%". In the spirit of "be liberal with what you accept", we also suggest that URI parsers accept bare "%" signs when possible (i.e., a "%" not followed by two valid and meaningful hexadecimal characters).

Thus, it would appear that the JDK team took the same approach in java.net.URI, which is elaborated on in various parts of the code.

// Special case of server authority that represents an IPv6 address
// In this case, a % does not signify an escape sequence
private static final long L_SERVER_PERCENT

// 2) The only place where a percent can be followed by anything
// other than hexadecimal digits is in the authority component
// (for a IPv6 scope) and the whole authority component is case
// insensitive.
percentNormalizedComparison(String, String, boolean)

// Exception: any "%" found between "[]" is left alone. It is an IPv6 literal
//            with a scope_id
//
decode(String)

However, Section 3 of RFC-6874 goes on to say:

Such bare "%" signs are for user interface convenience, and need to be turned into properly encoded characters (where "%25" encodes "%") before the URI is used in any protocol or HTML document. However, URIs including a ZoneID have no meaning outside the originating node. It would therefore be highly desirable for a browser to remove the ZoneID from a URI before including that URI in an HTTP request.

So, I suppose it's a question of context.

In any case, I'm wondering if HierarchicalUriComponents.verifyUriComponent() should also be lenient in this regard when validating IPv6 addresses.

@rstoyanchev, @poutsma, @jhoeller: thoughts?

[poutsma]

I think you're right, I'll see what I can do about relaxing that check.

However, there's two parts to the Robustness principe, not just "be liberal with what you accept", but also "be conservative in what you produce". So you could argue that java.net.URL should escape the '%' when producing a string, even though it was not escaped when creating the URL.

@ls-urs-keller Even though we're going to fix this, I'd still like to know the source of that URI. If it is coming from Spring or Reactor, we should fix the code that produces the URI that as well.

[sbrannen]

Should this be backported to 5.3.x?

[poutsma]

Should this be backported to 5.3.x?

I am not sure why, considering it's not a bug. My initial though was to assign it to 6.1 even.

[sbrannen]

Good point: it's not a bug. So 6.0.x or 6.1 sounds good.

[poutsma]

After discussion with @bclozel, we have come to the conclusion that this is a regression, most likely caused by 9624ea3. This commit removed the encoding performed by the multi argument URI constructor, in favor of the single-argument constructor and string concatenation, which does not encode. This commit was part of 6.0.6, in line with the reporter's timeframe, and was related to PR #30047. PR #30062 seems also related.

Reading back at the arguments for making UriComponentsBuilder more lenient to % characters in host names, they do not look as convincing anymore. Section 3 of RFC-6874 refers to URI parsing in the context of Web Browsers. Browsers accept a whole range of characters that are not valid in URIs, for instance spaces. It's the browser's job to turn the input into a valid URI.

The fact that java.net.URI chooses to be lenient in this regard is also unconvincing. In fact, one of the reasons for introducing UriComponents in Spring Framework was because URI had known encoding issues, which meant that we had to create our own abstraction that did follow the related RFCs. And as mentioned above, accepting the % is one thing, but it really should be encoded on output, in accordance with RFC 6874.

Finally, there are plenty of places where UriComponents is lenient, and accepts unencoded data. However, when passing true to the build method, you are actively disabling that lenience, and indicate that the builder is in an encoded state, while in fact it contains an unencoded % symbol.

Considering all that, I think the way to proceed is to fix the regression in ReactorServerHttpRequest, and leave UriComponentsBuilder as is.

[poutsma]

Fixed by adding custom encoding logic for the hostname. I could not similar logic use UriComponentBuilder, because that lives in the web package, while ReactorServerHttpRequest lives in http.

Since ReactorServerHttpRequest::uri returns an encoded request uri, the only encoding necessary is for the hostname.

I also moved all URI-creation logic into a separate helper class, as it was getting quite significant.

[rstoyanchev]

A fix has been applied in ReactorServerHttpRequest for 5.3.x as well in #30314, simply undoing the optimization.

[ls-urs-keller]

@ls-urs-keller Where does the URI that triggers this behavior come from? Can you provide a stack trace? I am guessing it is from Reactor Netty, but I'd like to be sure. In any case, the code that creates that URI should be fixed to properly encode the %.

@poutsma the problem was present in spring boot 3.0.3 and disappeared in 3.0.4. Here is a reproducer project, instructions in the Readme.md:
https://github.com/ls-urs-keller/spring_ipv6_prob_reproducer

Stacktrace:

2023-04-17T16:44:20.514Z ERROR 1 --- [or-http-epoll-2] a.w.r.e.AbstractErrorWebExceptionHandler : [7aa7c144-1]  500 Server Error for HTTP POST "/graphql"
                                                                        
java.lang.IllegalArgumentException: Invalid encoded sequence "%0]"                                                                              
        at org.springframework.web.util.HierarchicalUriComponents.verifyUriComponent(HierarchicalUriComponents.java:410) ~[spring-web-6.0.5.jar:6.0.5]
        Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: 
Error has been observed at the following site(s):
        *__checkpoint ? HTTP POST "/graphql" [ExceptionHandlingWebHandler]
Original Stack Trace:          
                at org.springframework.web.util.HierarchicalUriComponents.verifyUriComponent(HierarchicalUriComponents.java:410) ~[spring-web-6.0.5.jar:6.0.5]
                at org.springframework.web.util.HierarchicalUriComponents.verify(HierarchicalUriComponents.java:384) ~[spring-web-6.0.5.jar:6.0.5]
                at org.springframework.web.util.HierarchicalUriComponents.<init>(HierarchicalUriComponents.java:145) ~[spring-web-6.0.5.jar:6.0.5]
                at org.springframework.web.util.UriComponentsBuilder.buildInternal(UriComponentsBuilder.java:484) ~[spring-web-6.0.5.jar:6.0.5]
                at org.springframework.web.util.UriComponentsBuilder.build(UriComponentsBuilder.java:472) ~[spring-web-6.0.5.jar:6.0.5]         
                at org.springframework.graphql.server.WebGraphQlRequest.<init>(WebGraphQlRequest.java:67) ~[spring-graphql-1.1.2.jar:1.1.2]
                at org.springframework.graphql.server.webflux.GraphQlHttpHandler.lambda$handleRequest$0(GraphQlHttpHandler.java:79) ~[spring-graphql-1.1.2.jar:1.1.2]
                at reactor.core.publisher.MonoFlatMap$FlatMapMain.onNext(MonoFlatMap.java:132) ~[reactor-core-3.5.3.jar:3.5.3]
                at reactor.core.publisher.FluxOnErrorResume$ResumeSubscriber.onNext(FluxOnErrorResume.java:79) ~[reactor-core-3.5.3.jar:3.5.3]  
                at reactor.core.publisher.FluxOnErrorResume$ResumeSubscriber.onNext(FluxOnErrorResume.java:79) ~[reactor-core-3.5.3.jar:3.5.3]
                at reactor.core.publisher.MonoFlatMap$FlatMapMain.onNext(MonoFlatMap.java:158) ~[reactor-core-3.5.3.jar:3.5.3]                  
                at reactor.core.publisher.FluxContextWrite$ContextWriteSubscriber.onNext(FluxContextWrite.java:107) ~[reactor-core-3.5.3.jar:3.5.3]
                at reactor.core.publisher.FluxMapFuseable$MapFuseableConditionalSubscriber.onNext(FluxMapFuseable.java:299) ~[reactor-core-3.5.3.jar:3.5.3]
                at reactor.core.publisher.FluxFilterFuseable$FilterFuseableConditionalSubscriber.onNext(FluxFilterFuseable.java:337) ~[reactor-core-3.5.3.jar:3.5.3]
                at reactor.core.publisher.Operators$BaseFluxToMonoOperator.completePossiblyEmpty(Operators.java:2071) ~[reactor-core-3.5.3.jar:3.5.3]
                at reactor.core.publisher.MonoCollect$CollectSubscriber.onComplete(MonoCollect.java:145) ~[reactor-core-3.5.3.jar:3.5.3]
                at reactor.core.publisher.FluxMap$MapSubscriber.onComplete(FluxMap.java:144) ~[reactor-core-3.5.3.jar:3.5.3]
                at reactor.core.publisher.FluxPeek$PeekSubscriber.onComplete(FluxPeek.java:260) ~[reactor-core-3.5.3.jar:3.5.3]
                at reactor.core.publisher.FluxMap$MapSubscriber.onComplete(FluxMap.java:144) ~[reactor-core-3.5.3.jar:3.5.3]
                at reactor.netty.channel.FluxReceive.onInboundComplete(FluxReceive.java:415) ~[reactor-netty-core-1.1.3.jar:1.1.3]
                at reactor.netty.channel.ChannelOperations.onInboundComplete(ChannelOperations.java:431) ~[reactor-netty-core-1.1.3.jar:1.1.3]
                at reactor.netty.http.server.HttpServerOperations.onInboundNext(HttpServerOperations.java:648) ~[reactor-netty-http-1.1.3.jar:1.1.3]
                at reactor.netty.channel.ChannelOperationsHandler.channelRead(ChannelOperationsHandler.java:113) ~[reactor-netty-core-1.1.3.jar:1.1.3]
                at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at reactor.netty.http.server.HttpTrafficHandler.channelRead(HttpTrafficHandler.java:274) ~[reactor-netty-http-1.1.3.jar:1.1.3]
                at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:442) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.fireChannelRead(CombinedChannelDuplexHandler.java:436) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:346) ~[netty-codec-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:318) ~[netty-codec-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.CombinedChannelDuplexHandler.channelRead(CombinedChannelDuplexHandler.java:251) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:442) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:440) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919) ~[netty-transport-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:800) ~[netty-transport-classes-epoll-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:499) ~[netty-transport-classes-epoll-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:397) ~[netty-transport-classes-epoll-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997) ~[netty-common-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) ~[netty-common-4.1.89.Final.jar:4.1.89.Final]
                at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) ~[netty-common-4.1.89.Final.jar:4.1.89.Final]
                at java.base/java.lang.Thread.run(Unknown Source) ~[na:na]

[poutsma]

Thank you for providing that sample, @ls-urs-keller, it helped us verify that the current Framework branches are not affected by this bug.

The culprit seems to have been the fix for #28601, which was introduced in 6.0.5, but revised in 6.0.6.

[ls-urs-keller]

Thank you for providing that sample, @ls-urs-keller, it helped us verify that the current Framework branches are not affected by this bug.
Glad it helped.