Reconsider lenient URI parsing fallback in ServletServerHttpRequest

[alexisgayte]

Linked to #25274 , #30475

url with relaxed char are not allowed even with the option set. Url example : /test?id={64aaa32-3f4e-93b0-9cd9-986a0a34a650}

using reactive and tomcat with TomcatHttpHandlerAdapter, this url call will fail down the route. as ServletHttpHandlerAdapter creates a ServletServerHttpRequest that tries to parse the url with java.net.URI which doesn't allow relaxed-query-chars.

The issue is here : https://github.com/spring-projects/spring-framework/blob/c227fbfdf27d6ef28af66e1d0cc78fcc6a009a13/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpRequest.java#L131

the URI parser doesn't allow relaxed char.

Error thrown is : ServletHttpHandlerAdapter - Failed to get request URL: Illegal character in query at

[alexisgayte]

For anyone facing this issue. Here my work around.

Work around is to monkey patch the java.net.URI class

Uncomment lowMask and highMask.

replacing L_MARK and H_MARK to add your relaxed characters. for example in my case :

    private static final long L_MARK = lowMask("-_.!~*'(){}");
    private static final long H_MARK = highMask("-_.!~*'(){}");

[sreeraksha]

Hi @alexisgayte,

Could you please ellaborate on how I can monkey patch java.net.RMI class by giving a more detailed example.

[alexisgayte]

with Java 8, just add the tweaked java.net.URI into your project. It will override it. With java 9 + you will need to patch the java.base module.

something like that : javac --patch-module java.base=src -d build/classes/java/patches //src/patches/java/java/net/URI.java java --patch-module java.base=build/classes/java/patches

cf https://openjdk.org/projects/jigsaw/

Then you need to integrate it with your project builder. With gradle I ended up with something like that :

sourceSets {
    patches
    main {
        compileClasspath += sourceSets.patches.output
        runtimeClasspath += sourceSets.patches.output
    }
}

bootRun {
    jvmArgs = ['--patch-module', 'java.base=build/classes/java/patches']
}

compilePatchesJava {
    options.fork = true
    options.forkOptions.executable = 'javac'
    options.compilerArgs.addAll(['--patch-module', 'java.base=src'])
}

with your URI class in it.

[MangalaEkanayake]

Does this have fixed in a recent Spring framework?

[alexisgayte]

I don't think so.

[ahoka]

It should just pass it as a String, IMHO.

[snicoll]

I am a little bit confused by the report. You're talking about reactive and tomcat and the code that you link is from Spring MVC. I took the time to rebuild a small sample based on your description and it works fine.

If you want support, please provide a small sample that we can run ourselves that reproduces the issue. You can attach it as a zip or you can push the code to a GitHub repository. Please note that while Spring Framework 5.3.x is still supported in OSS, Spring Boot 2.x is not.

[alexisgayte]

Hi Shephane, Thanks for your time over the festive holiday. This report came after I use spring cloud gateway. It is definitely reactive the path in the link shows reactive ( thinking, I might miss something on what you mean by reactive ). I am not able to create a case right now. But the URI class that is used don't allow relaxed char in the parsing process so I doubt that's work.

This was with spring boot 3.1.7 and I believe it hasn't changed with 3.2.1

BTW I am a big fan of your work. Thanks for all your work.

[snicoll]

It is definitely reactive the path in the link shows reactive

Sorry, it happens quite often I get caught by this. There are two ServletServerHttpRequest and my brain didn't read the reactive package in your link.

I am not able to create a case right now. But the URI class that is used don't allow relaxed char in the parsing process so I doubt that's work.

Yeah, that's why I was surprised. I didn't know about the Spring Cloud Gateway bit and I guess it might be related. I've pushed my little sample that only uses framework https://github.com/snicoll-scratches/spring-framework-30489. Perhaps you can have a look to it and see what's missing to reproduce the issue?

[alexisgayte]

Hi Stéphane, I hope you had a good break. I had a look at your sample, thanks for your hard work.

your sample is correct, but the test is not, probably due to webClient. I haven't digged into it, (my guess is that somehow it converts the special char).

But if you run it as an app and use your browser you will get a 400.

Side note:

• your test is running with netty not tomcat so I am not sure if it is exactly the same.
• You need to add the debug log in the property:

  logging.level.root=debug

You will get this log :

2024-01-02T16:27:21.005Z DEBUG 60660 --- [ctor-http-nio-2] r.n.http.server.HttpServerOperations     : [4f9b0144, L:/[0:0:0:0:0:0:0:1]:8080 - R:/[0:0:0:0:0:0:0:1]:60951] Increasing pending responses, now 1
2024-01-02T16:27:21.005Z DEBUG 60660 --- [ctor-http-nio-2] reactor.netty.http.server.HttpServer     : [4f9b0144-2, L:/[0:0:0:0:0:0:0:1]:8080 - R:/[0:0:0:0:0:0:0:1]:60951] Handler is being applied: org.springframework.http.server.reactive.ReactorHttpHandlerAdapter@194b3e93
2024-01-02T16:27:21.005Z DEBUG 60660 --- [ctor-http-nio-2] o.s.h.s.r.ReactorHttpHandlerAdapter      : Failed to get request URI: Illegal character in query at index 34: http://localhost:8080/test?id=test}
2024-01-02T16:27:21.006Z DEBUG 60660 --- [ctor-http-nio-2] r.n.http.server.HttpServerOperations     : [4f9b0144-2, L:/[0:0:0:0:0:0:0:1]:8080 - R:/[0:0:0:0:0:0:0:1]:60951] Last HTTP response frame
2024-01-02T16:27:21.006Z DEBUG 60660 --- [ctor-http-nio-2] r.n.http.server.HttpServerOperations     : [4f9b0144-2, L:/[0:0:0:0:0:0:0:1]:8080 - R:/[0:0:0:0:0:0:0:1]:60951] No sendHeaders() called before complete, sending zero-length header
2024-01-02T16:27:21.006Z DEBUG 60660 --- [ctor-http-nio-2] r.n.http.server.HttpServerOperations     : [4f9b0144-2, L:/[0:0:0:0:0:0:0:1]:8080 - R:/[0:0:0:0:0:0:0:1]:60951] Decreasing pending responses, now 0
2024-01-02T16:27:21.006Z DEBUG 60660 --- [ctor-http-nio-2] r.n.http.server.HttpServerOperations     : [4f9b0144-2, L:/[0:0:0:0:0:0:0:1]:8080 - R:/[0:0:0:0:0:0:0:1]:60951] Last HTTP packet was sent, terminating the channel
2024-01-02T16:27:21.007Z DEBUG 60660 --- [ctor-http-nio-2] r.netty.channel.ChannelOperations        : [4f9b0144-2, L:/[0:0:0:0:0:0:0:1]:8080 - R:/[0:0:0:0:0:0:0:1]:60951] [HttpServer] Channel inbound receiver cancelled (subscription disposed).
2024-01-02T16:27:21.007Z DEBUG 60660 --- [ctor-http-nio-2] r.n.channel.ChannelOperationsHandler     : [4f9b0144, L:/[0:0:0:0:0:0:0:1]:8080 - R:/[0:0:0:0:0:0:0:1]:60951] No ChannelOperation attached.

the important one is : o.s.h.s.r.ReactorHttpHandlerAdapter : Failed to get request URI: Illegal character in query at index 34: http://localhost:8080/test?id=test}

I believe you will be back to this initial report then, I haven't digged into it but that seems to be the issue.

[bclozel]

I think the main difference between WebFlux and MVC is that org.springframework.http.server.ServletServerHttpRequest is more lenient than org.springframework.http.server.reactive.ServletServerHttpRequest.

The MVC variant has a fallback and only uses servletRequest.getRequestURL().toString() which in this case does not contain the query string; see https://github.com/spring-projects/spring-framework/blob/f846d9484ce0813e9ed40933d5cc474feecac49f/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpRequest.java#L129-L132

The WebFlux variant does not have such fallback and throws the URI exception, see https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpRequest.java#L131

Note that Spring MVC doesn't alwaus uses this URI information, but will do in our filters and CORS support infrastructure.

I don't think we should expand the lenient fallback in WebFlux but rather reconsider it in MVC, as the URI information is plain wrong in those cases. The URI type is exposed in public APIs so we can't use a different implementation for that.

I'll repurpose this issue to discuss that change with @rstoyanchev .

[rstoyanchev]

I agree that proceeding without the query is not ideal. Looking at #20960 it was added to ignore an invalid query, but as a measure it's rather imprecise, and could create new issues, while also masking the original one. I'm guessing that in the case of the example URI, dropping the query would not solve the issue as the request probably won't succeed without the id parameter.

Ideally the client should encode the query, but when that's not possible something else would have to do it. We could consider a similar property to Tomcat's relaxed query chars, and create a fallback where we iterate over the query and encode any such configured chars, which would make it possible to create a URI.