-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Prevent caching of non-document requests in HttpSessionRequestCache #17687
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
base: main
Are you sure you want to change the base?
Conversation
When the browser requests a missing favicon.ico on the login page, it triggers an ERROR dispatch. This request was mistakenly saved and caused redirection to /error after login instead of the original URL. This change checks for DispatcherType.ERROR and Sec-Fetch-Dest header to avoid caching such requests. Closes spring-projects#17686 Signed-off-by: Andrey Litvitski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @therepanic! I've left my feedback inline.
@@ -61,6 +63,17 @@ public class HttpSessionRequestCache implements RequestCache { | |||
*/ | |||
@Override | |||
public void saveRequest(HttpServletRequest request, HttpServletResponse response) { | |||
boolean documentRequest = "document".equals(request.getHeader("Sec-Fetch-Dest")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at RequestCacheConfigurer
, which constructs a request matcher that skips /favicon.*
requests. If that is in place and you are still seeing this problem, then that's where the fix should go.
Let's first see if we can figure out why the existing request matcher provided in RequestCacheConfigurer
is not sufficient. It may be that the best fix is to check this header and the dispatcher type; however, since there is already logic for /favicon.ico
in place, I'm curious to first establish why that isn't working here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also try and keep the fix as small as possible to make it more likely to backport to earlier versions of Spring Security that may also have the bug. Adding support for request dispatcher type and the Sec-Fetch-Dest
headers are good ideas, but they are more of an improvement and will likely not be backported.
When the browser requests a missing favicon.ico on the login page, it triggers an ERROR dispatch.
This request was mistakenly saved and caused redirection to /error after login instead of the original URL.
This change checks for DispatcherType.ERROR and Sec-Fetch-Dest header to avoid caching such requests.
Closes #17686