spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.76k stars 5.89k forks source link

SEC-2981: AbstractRetryEntryPoint infinitive redirection loop when secured channel is required. #3190

Open spring-projects-issues opened 9 years ago

spring-projects-issues commented 9 years ago

Jakub Narloch (Migrated from SEC-2981) said:

Steps to reproduce:

 http.requiresChannel()
                    .anyRequest()
                    .requiresSecure();

As a result the client is being endlessly redirected to relative path resulting in the path being consecutive appended to current url e.g. : '/webapp/webapp/webapp/webapp/webapp/webapp/webapp/webapp/webapp/webapp/webapp'

The reason why this happens is the handling of the port mappings in AbstractRetryEntryPoint: https://github.com/spring-projects/spring-security/blob/master/web/src/main/java/org/springframework/security/web/access/channel/AbstractRetryEntryPoint.java#L54

if (redirectPort != null) {
    boolean includePort = redirectPort.intValue() != standardPort;

    redirectUrl = scheme + request.getServerName()
            + ((includePort) ? (":" + redirectPort) : "") + redirectUrl;
}

Proposition would be to return http 400 or 406 status code in cases that there is no valid port.

Yes I agree that in general this problem would be caused by incorrect port mappings between HTTP and HTTPS and this is only a matter of configuration in the PortMapper, although in our cases the client is connecting through proxy on non default port and is redirected to an invalid path without protocol switch.

spring-projects-issues commented 9 years ago

Jakub Narloch said:

I will try to provide the unit test to reproduce this problem.

spring-projects-issues commented 9 years ago

Jakub Narloch said:

I added extra test for that: https://github.com/jmnarloch/spring-security/commit/a6cae61e9175915d7bb05c4e1380c951f21e5e99

public void testOperationWhenTargetPortIsUnknown() throws Exception {
        MockHttpServletRequest request = new MockHttpServletRequest("GET", "/bigWebApp");
        request.setQueryString("open=true");
        request.setScheme("http");
        request.setServerName("www.example.com");
        request.setServerPort(8768);

        MockHttpServletResponse response = new MockHttpServletResponse();

        RetryWithHttpsEntryPoint ep = new RetryWithHttpsEntryPoint();
        ep.setPortMapper(new PortMapperImpl());
        ep.setPortResolver(new MockPortResolver(8768, 1234));

        ep.commence(request, response);
        assertEquals("/bigWebApp?open=true", response.getRedirectedUrl());
    }

The problem is with those implementation is that it returns the relative path that does not either change the scheme or port and what more is that the browser is trying resolve it based from the current request url making it looping on those redirections.