spring-framework icon indicating copy to clipboard operation
spring-framework copied to clipboard

Reparse URI when host is missing

Open bodograumann opened this issue 1 year ago • 3 comments

When a hostname contains an underscore, it is valid in RFC 3986, but only part of a registry-based authority under RFC 2396. Due to the fact, that URI only supports RFC 2396, we cannot simply use getHost() in these cases. Instead we need to parse the URI ourselves.

Only parsing the authority part seems too much effort, given the custom UrlParser we are using.

Closes https://github.com/spring-projects/spring-framework/issues/27774 Closes https://github.com/spring-projects/spring-security/issues/15852

bodograumann avatar Sep 30 '24 11:09 bodograumann

I think this is duplicating #33608, but I missed the comments in #27774. We will discuss this as a team, but my comment on #33608 my still stand.

bclozel avatar Sep 30 '24 11:09 bclozel

Thanks for taking this into consideration.

My main goal is to get https://github.com/spring-projects/spring-security/issues/15852 fixed, so simply not using URI with UriComponentsBuilder would also achieve that goal as suggested here: https://github.com/spring-projects/spring-security/pull/15853 Unfortunately I wasn't yet able to provide proper tests there...

In the other PR you mention https://github.com/spring-projects/spring-framework/issues/33542, but afaict it is concerned with the differences between RFC 3986 and the WhatWG living spec, not with the difference between RFC 3986 and RFC 2396.

I also found https://cr.openjdk.org/~dfuchs/writeups/updating-uri/, but I couldn't find any updates on further progress.

bodograumann avatar Sep 30 '24 12:09 bodograumann

I agree with Brian this could lead to surprises, and we should not quietly re-parse from within fromUri(URI). Spring Security can do that in fallback mode as I commented under https://github.com/spring-projects/spring-security/pull/15853.

rstoyanchev avatar Oct 01 '24 13:10 rstoyanchev

Closing per team feedback. Thanks for raising this!

bclozel avatar Feb 04 '25 14:02 bclozel