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

SEC-2227: AbstractAuthenticationProcessingFilter (and similar filters) should match on exact URLs by default #2404

Closed
spring-issuemaster opened this issue Jul 24, 2013 · 4 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link

@spring-issuemaster spring-issuemaster commented Jul 24, 2013

Adam Gent (Migrated from SEC-2227) said:

AbstractAuthenticationProcessingFilter handles the filterProcessUrl(j_spring_security_check) inconsistently for the root context compared to other contexts.

See:

https://github.com/SpringSource/spring-security/blob/master/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java#L235

Based on this code if you had a context path of "/development" ONLY "/development/j_spring_security_check" will match BUT if you have a context path of "" (ie root which is what most people deploy to production as) then "/**/j_spring_security_check" will work.

This is an issue if you switch contexts and is IMHO consistent. I think safest solution is to make all contexts act like the root context ("").

@spring-issuemaster

This comment has been minimized.

Copy link
Author

@spring-issuemaster spring-issuemaster commented Jul 24, 2013

Rob Winch said:

I'm not sure I understand the inconsistency. The method requiresAuthentication will consistently match an URL that ends with HttpServletRequest.getContextRoot() + filterProcessUrl. If the context root is "", then it matches any URL ending in "" + "/j_spring_security_check". If the the context root is "/development", then it matches any URL ending with "/development" + "/j_spring_security_check". As you pointed out, it does mean that there are a lot more URLs that are matched when the context root is "", but this is consistently matching URLs that ends with + filterProcessUrl.

This is an issue if you switch contexts and is IMHO consistent.

When you are creating the URLs to submit your login form, you should be sure to create the URL dynamically with the context root so this should not be a problem. If you are using JSPs, this is done automatically for you when using the <c:url> and spring:url JSP tags.

If you have special requirements you can override the requiresAuthentication method. Note that this is also being reworked for Spring Security 3.2 in SEC-2042 so that you can inject a RequestMatcher. However, to remain passive the same functionality is preserved by default.

I think safest solution is to make all contexts act like the root context ("").

Do you feel the current behavior is "unsafe"? If so, why? I think the safest behavior is to leave it as since this is a passivity concern and poses security concerns to users. Additionally, there are easy ways to customize the behavior if it doesn't meet your needs.

@spring-issuemaster

This comment has been minimized.

Copy link
Author

@spring-issuemaster spring-issuemaster commented Jul 24, 2013

Adam Gent said:

Do you feel the current behavior is "unsafe"? If so, why? I think the safest behavior is to leave it as since this is a passivity concern and poses security concerns to users. Additionally, there are easy ways to customize the behavior if it doesn't meet your needs.

Safest in terms of backwards compatibility (ironically not because of security) to keep the root context working and non root context more flexible.

Its still bizarre that by switching the context path to root (getContextPath() == "") any URL that ends in /j_spring_security_check will match the security filter. I honestly think that was not the intention and thus a bug. The intention I believe was an explicit match for root context.

Think about it.

Lets say I test an API REST endpoint of /development/api/j_spring_security_check where j_spring_security_check happens to be a @PathParam and the context path is: /development which will work fine. But if I switch to the root context /api/j_spring_security_check ... the security filter will intercept it.

In fact I found this out by accident because I generally develop apps on the root context path and thus thought that by adding "/j_spring_security_check" on the end of any URL it would work but it does not when you switch the context path.

If there is a reason for this the JavaDoc should at least note the difference.

@spring-issuemaster

This comment has been minimized.

Copy link
Author

@spring-issuemaster spring-issuemaster commented Jul 24, 2013

Rob Winch said:

I see your point about the fact that Spring Security intercepting URLs that are unintentional. I think the same could be said about the other Filter's that use this logic (i.e. LogoutFilter).

I may be misunderstanding your suggestion on how to resolve the issue (as I don't think we are on the same page). It sounds like you think it should always match on /**/. Perhaps that is because you were concerned with passivity. However, I think if we are to resolve this we should do an exact match rather than endsWith. Otherwise, Spring Security will continue to intercept URLs that were not intended for it. Since this is fairly significant change in behavior, we will need to wait till Spring Security 4 (which will be after 3.2 is finalized).

@spring-issuemaster

This comment has been minimized.

Copy link
Author

@spring-issuemaster spring-issuemaster commented Dec 4, 2014

Rob Winch said:

As part of SEC-2781 the filters now use AntPathMatcher and thus match on the exact URL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.