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

ForwardedHeaderFilter does not respect Servlet forwarding [SPR-16983] #21521

Closed
spring-issuemaster opened this issue Jun 28, 2018 · 11 comments
Closed
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Jun 28, 2018

Eric Sirianni opened SPR-16983 and commented

ForwardedHeaderFilter captures the requestUri on initial request (e.g. /foo).  That request URI can be altered due to a server-side forward - for example:

request.getRequestDispatcher("/bar").forward(request, response)

When the request is re-dispatched, code calling HttpServletRequest.getRequestURI() receives the old value (e.g. /foo) instead of the new value (e.g. /bar). Even if the filter is registered with DispatcherType.FORWARD, the issue remains that the filter implements OncePerRequestFilter.


Affects: 5.0.7

Issue Links:

  • #21954 ResourceUrlEncodingFilter does not work with HttpServletRequestWrapper

Referenced from: commits feeec34

2 votes, 4 watchers

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 29, 2018

Rossen Stoyanchev commented

Can you provide the details of the initial request, i.e. URI and headers, before the ForwadedHeaderFilter?

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 29, 2018

Eric Sirianni commented

GET /foo HTTP/1.1
x-forwarded-host: www.mycompany.com
x-forwarded-proto: https
x-forwarded-port: 443
x-forwarded-for: ::ffff:127.0.0.1
accept-language: en-US,en;q=0.9
accept-encoding: gzip, deflate, br
accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8
user-agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.99 Safari/537.36
upgrade-insecure-requests: 1
connection: close
host: www.mycompany.com

The issue is that the requestURI is assigned from the initial request the first time the filter runs. It is never re-assigned when the request is re-dispatched via the RequestDispatcher.forward. The issue occurs regardless of the x-forwarded-* values.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 22, 2018

Alfred Staflinger commented

Please increase the priority of this error, because it is a serious problem if HttpServletRequest.getRequestURI() returns a wrong value.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 22, 2018

Rossen Stoyanchev commented

Sorry for the delay. I'll look into this right away.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 22, 2018

johann-sonntagbauer commented

Some additional information. 

Deployed Spring WebMVC 5.1 on Tomcat 9.0.11 with ForwardedHeaderFilter.

 

The org.springframework.security.web.access.AccessDeniedHandlerImpl#handle will forward the request to an errorPage.

// Set the 403 status code.
response.setStatus(HttpStatus.FORBIDDEN.value());

// forward to error page.
RequestDispatcher dispatcher = request.getRequestDispatcher(errorPage);
dispatcher.forward(request, response); 

 

During the forward the Request gets wrapped in org.apache.catalina.core.ApplicationDispatcher#wrapRequest.

There the logic is very complicated and some crazy stuff is going on. But at the end

((ServletRequestWrapper) previous).setRequest(wrapper); 

previous is in that case the ForwardedHeaderFilter$ForwardedHeaderExtractingRequest which does not allow to set another Request because all properties of the ForwardedHeader Request are already final.

So maybe overriding #setRequest in  ForwardedHeaderExtractingRequest and applying the same funcionality like in the constructor could solve that problem.

 

Hope this helps somehow with the analysis.

Stack trace

wrapRequest:925, ApplicationDispatcher (org.apache.catalina.core)
doForward:358, ApplicationDispatcher (org.apache.catalina.core)
forward:312, ApplicationDispatcher (org.apache.catalina.core)
handle:73, AccessDeniedHandlerImpl (org.springframework.security.web.access)
handle:40, AccessDeniedHandler (com.infoniqa.common.authorization.web)
handleSpringSecurityException:199, ExceptionTranslationFilter (org.springframework.security.web.access)
doFilter:141, ExceptionTranslationFilter (org.springframework.security.web.access)
doFilter:334, FilterChainProxy$VirtualFilterChain (org.springframework.security.web)
doFilter:137, SessionManagementFilter (org.springframework.security.web.session)
....
doFilterInternal:156, ForwardedHeaderFilter (org.springframework.web.filter)
doFilter:107, OncePerRequestFilter (org.springframework.web.filter)
internalDoFilter:193, ApplicationFilterChain (org.apache.catalina.core)
doFilter:166, ApplicationFilterChain (org.apache.catalina.core)
doFilter:71, Log4jServletFilter (org.apache.logging.log4j.web)
internalDoFilter:193, ApplicationFilterChain (org.apache.catalina.core)
doFilter:166, ApplicationFilterChain (org.apache.catalina.core)
invoke:199, StandardWrapperValve (org.apache.catalina.core)
invoke:96, StandardContextValve (org.apache.catalina.core)
invoke:490, AuthenticatorBase (org.apache.catalina.authenticator)
invoke:139, StandardHostValve (org.apache.catalina.core)
invoke:92, ErrorReportValve (org.apache.catalina.valves)
invoke:668, AbstractAccessLogValve (org.apache.catalina.valves)
invoke:74, StandardEngineValve (org.apache.catalina.core)
service:343, CoyoteAdapter (org.apache.catalina.connector)
service:408, Http11Processor (org.apache.coyote.http11)
process:66, AbstractProcessorLight (org.apache.coyote)
process:770, AbstractProtocol$ConnectionHandler (org.apache.coyote)
doRun:1415, NioEndpoint$SocketProcessor (org.apache.tomcat.util.net)
run:49, SocketProcessorBase (org.apache.tomcat.util.net)
runWorker:1128, ThreadPoolExecutor (java.util.concurrent)
run:628, ThreadPoolExecutor$Worker (java.util.concurrent)
run:61, TaskThread$WrappingRunnable (org.apache.tomcat.util.threads)
run:834, Thread (java.lang) 

 

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 22, 2018

johann-sonntagbauer commented

Rossen Stoyanchev Thank you very much. Please let me know if you need additional information. 

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 23, 2018

Rossen Stoyanchev commented

So maybe overriding #setRequest in ForwardedHeaderExtractingRequest and applying the same funcionality like in the constructor could solve that problem.

Yes I was considering something similar to Tomcat, or Jetty for that matter which mutates the underlying request, but that's more complicated that it needs to be. I'll experiment with the way Tomcat's RemoteItFilter allows dynamic recalculatation of the requestURL.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 23, 2018

Rossen Stoyanchev commented

There is a fix now in master (snapshot build running). It would be great to confirm it works in applications that were affected.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 23, 2018

johann-sonntagbauer commented

That is great news! I will verify the snapshot build tomorrow. Thank you very much for your effort and the immediate action.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 24, 2018

johann-sonntagbauer commented

Rossen Stoyanchev I am currently testing the SNAPSHOT 5.1.2 build and it looks promising so far. The main problem is solved as far as I can tell.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 24, 2018

Rossen Stoyanchev commented

Thanks for checking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.