Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MockHttpServletRequest should not strip brackets from IPV6 address supplied via Host header #24916

Closed
jusmaki opened this issue Apr 16, 2020 · 3 comments
Assignees
Labels
in: test Issues in the test module status: backported An issue that has been backported to maintenance branches type: regression A bug that is also a regression
Milestone

Comments

@jusmaki
Copy link

jusmaki commented Apr 16, 2020

Affects: 5.0.2 - 5.2.5


The latest versions of spring-test since 5.0.2 don't include the IPV6 address in the getRequestURL() in correct []-quoting.

For example if the MockHttpServletRequest is constructed using "GET", "/" and then the server name is set with a call to setServerName("::ffff:abcd:abcd"), then getRequestURL() returns "http://[::ffff:abcd:abcd]/" in the old versions, but latest versions return "http://::ffff:abcd:abcd/" which cannot be parsed by java.net.URL.

The problem seems to happen in the getRequestURL() implementation which uses getServerName() and does not check if the serverName is actually IPV6 address which should be quoted with brackets.

See RFC 2732 for reference.

    @Test
    public void testIPV6() {
        String ipv6 = "::ffff:abcd:abcd";
        MockHttpServletRequest request = new MockHttpServletRequest("GET", "/");
        request.addHeader("Host", "["+ipv6+"]");
        request.setServerName("["+ipv6+"]");
        String urlString = request.getRequestURL().toString();
        System.out.println("#URL: \"" + urlString+"\"");
        // The URL should look like: "http://[::ffff:abcd:abcd]/"
        try {
            java.net.URL url = new java.net.URL(urlString);
        } catch (java.net.MalformedURLException e) {
            assertEquals(e, null);
        }
    }
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 16, 2020
@sbrannen sbrannen added the in: test Issues in the test module label Apr 16, 2020
@sbrannen
Copy link
Member

This is related to #16704 and #20686.

The work done in conjunction with #20686 appears to have resulted in this change of behavior. Note, however, that those changes were made in 5.0.2 but backported to 4.3.13.

@sbrannen
Copy link
Member

sbrannen commented Apr 16, 2020

After introducing the following two tests in MockHttpServletRequestTests on master, I can confirm that getRequestURLWithIpv6AddressViaHostHeader() fails; whereas, the other one passes.

Thus, the URL returned by getRequestURL() cannot be parsed by java.net.URL if an IPV6 Host header was set in the request.

	@Test
	void getRequestURLWithIpv6AddressViaServerName() throws Exception {
		String ipv6Address = "[::ffff:abcd:abcd]";
		request.setServerName(ipv6Address);
		URL url = new java.net.URL(request.getRequestURL().toString());
		assertThat(url).asString().isEqualTo("http://[::ffff:abcd:abcd]");
	}

	@Test
	void getRequestURLWithIpv6AddressViaHostHeader() throws Exception {
		String ipv6Address = "[::ffff:abcd:abcd]";
		request.addHeader("Host", ipv6Address);
		URL url = new java.net.URL(request.getRequestURL().toString());
		assertThat(url).asString().isEqualTo("http://[::ffff:abcd:abcd]");
	}

@sbrannen sbrannen added type: regression A bug that is also a regression and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 16, 2020
@sbrannen sbrannen self-assigned this Apr 16, 2020
@sbrannen
Copy link
Member

sbrannen commented Apr 16, 2020

@jusmaki, thanks for bringing this to our attention!

After further investigation, I have determined that the change I made in #16704 actually introduced a regression in Spring Framework 4.1 for the getServerName() method, and the changes in #20686 introduced a regression in getRequestURL() by delegating to the getServerName() method.

We will therefore ensure that enclosing brackets are no longer stripped from IPV6 host names when supplied via the Host header in a MockHttpServletRequest, and we'll backport the fix to all currently maintained branches.


For example, when using Tomcat as the Servlet container, a request sent to http://[0:0:0:0:0:0:0:1]:8080/events from a browser results in the following in the Servlet that processes the ServletRequest.

  • request.getHeader("Host"): [::1]:8080
  • request.getServerName(): [::1]
  • request.getRequestURL(): http://[::1]:8080/events

@sbrannen sbrannen added this to the 5.2.6 milestone Apr 16, 2020
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.1.x labels Apr 16, 2020
@sbrannen sbrannen changed the title Bug using literal IPV6 serverName for MockHttpServletRequest.getRequestURL() MockHttpServletRequest should not strip brackets from IPV6 address supplied via Host header Apr 16, 2020
sbrannen added a commit that referenced this issue Apr 17, 2020
According to the Javadoc for ServletRequest's getServerName() method,
when the `Host` header is set, the server name is "the value of the
part before ':' in the Host header value ...". For a value representing
an IPV6 address such as `[::ffff:abcd:abcd]`, the enclosing square
brackets should therefore not be stripped from the enclosed IPV6
address.

However, the changes made in conjunction with gh-16704 introduced a
regression in Spring Framework 4.1 for the getServerName() method in
MockHttpServletRequest by stripping the enclosing brackets from the
IPV6 address in the `Host` header. Similarly, the changes made in
conjunction with gh-20686 introduced a regression in Spring Framework
4.3.13 and 5.0.2 in the getRequestURL() method in
MockHttpServletRequest by delegating to the getServerName() method
which strips the enclosing brackets.

This commit fixes the implementation of getServerName() so that the
enclosing brackets are no longer stripped from an IPV6 address in the
`Host` header. The implementation of getRequestURL() is therefore also
fixed.

In addition, in order to avoid a NullPointerException, the
implementations of getServerName() and getServerPort() now assert that
an IPV6 address present in the `Host` header correctly contains an
opening and closing bracket and throw an IllegalStateException if that
is not the case.

Closes gh-24916
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Apr 17, 2020
According to the Javadoc for ServletRequest's getServerName() method,
when the `Host` header is set, the server name is "the value of the
part before ':' in the Host header value ...". For a value representing
an IPV6 address such as `[::ffff:abcd:abcd]`, the enclosing square
brackets should therefore not be stripped from the enclosed IPV6
address.

However, the changes made in conjunction with spring-projectsgh-16704 introduced a
regression in Spring Framework 4.1 for the getServerName() method in
MockHttpServletRequest by stripping the enclosing brackets from the
IPV6 address in the `Host` header. Similarly, the changes made in
conjunction with spring-projectsgh-20686 introduced a regression in Spring Framework
4.3.13 and 5.0.2 in the getRequestURL() method in
MockHttpServletRequest by delegating to the getServerName() method
which strips the enclosing brackets.

This commit fixes the implementation of getServerName() so that the
enclosing brackets are no longer stripped from an IPV6 address in the
`Host` header. The implementation of getRequestURL() is therefore also
fixed.

In addition, in order to avoid a NullPointerException, the
implementations of getServerName() and getServerPort() now assert that
an IPV6 address present in the `Host` header correctly contains an
opening and closing bracket and throw an IllegalStateException if that
is not the case.

Closes spring-projectsgh-24916
sbrannen added a commit that referenced this issue Apr 17, 2020
According to the Javadoc for ServletRequest's getServerName() method,
when the `Host` header is set, the server name is "the value of the
part before ':' in the Host header value ...". For a value representing
an IPV6 address such as `[::ffff:abcd:abcd]`, the enclosing square
brackets should therefore not be stripped from the enclosed IPV6
address.

However, the changes made in conjunction with gh-16704 introduced a
regression in Spring Framework 4.1 for the getServerName() method in
MockHttpServletRequest by stripping the enclosing brackets from the
IPV6 address in the `Host` header. Similarly, the changes made in
conjunction with gh-20686 introduced a regression in Spring Framework
4.3.13 and 5.0.2 in the getRequestURL() method in
MockHttpServletRequest by delegating to the getServerName() method
which strips the enclosing brackets.

This commit fixes the implementation of getServerName() so that the
enclosing brackets are no longer stripped from an IPV6 address in the
`Host` header. The implementation of getRequestURL() is therefore also
fixed.

In addition, in order to avoid a NullPointerException, the
implementations of getServerName() and getServerPort() now assert that
an IPV6 address present in the `Host` header correctly contains an
opening and closing bracket and throw an IllegalStateException if that
is not the case.

Closes gh-24916
zx20110729 pushed a commit to zx20110729/spring-framework that referenced this issue Feb 18, 2022
According to the Javadoc for ServletRequest's getServerName() method,
when the `Host` header is set, the server name is "the value of the
part before ':' in the Host header value ...". For a value representing
an IPV6 address such as `[::ffff:abcd:abcd]`, the enclosing square
brackets should therefore not be stripped from the enclosed IPV6
address.

However, the changes made in conjunction with spring-projectsgh-16704 introduced a
regression in Spring Framework 4.1 for the getServerName() method in
MockHttpServletRequest by stripping the enclosing brackets from the
IPV6 address in the `Host` header. Similarly, the changes made in
conjunction with spring-projectsgh-20686 introduced a regression in Spring Framework
4.3.13 and 5.0.2 in the getRequestURL() method in
MockHttpServletRequest by delegating to the getServerName() method
which strips the enclosing brackets.

This commit fixes the implementation of getServerName() so that the
enclosing brackets are no longer stripped from an IPV6 address in the
`Host` header. The implementation of getRequestURL() is therefore also
fixed.

In addition, in order to avoid a NullPointerException, the
implementations of getServerName() and getServerPort() now assert that
an IPV6 address present in the `Host` header correctly contains an
opening and closing bracket and throw an IllegalStateException if that
is not the case.

Closes spring-projectsgh-24916
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module status: backported An issue that has been backported to maintenance branches type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

3 participants