SEC-1648: Require explicit enabling of targetUrlParameter parameter in AbstractAuthenticationTargetUrlRequestHandler #1889

Closed
spring-issuemaster opened this Issue Jan 4, 2011 · 6 comments

1 participant

@spring-issuemaster

Luke Taylor (Migrated from SEC-1648) said:

It's possible that an AuthenticationSuccessHandler or other class extending from this class may be used in scenarios where it isn't desirable to have a parameter used to determine the redirect location, though when it is invoked during the login request, this shouldn't be a problem.

A user might create a subclass or use SimpleUrlAuthenticationSuccessHandler without realising that the parameter-based functionality exists. It would therefore probably be preferable to require that the functionality is explicitly enabled (as with the use of the "referer" header).

@spring-issuemaster

Luke Taylor said:

I've added a useTargetUrlParameter property which defaults to false, thus requiring that the user explicitly enable this functionality.

@spring-issuemaster

Rob Winch said:

Might we consider just flexing this if targetUrlParameter is null and defaulting it to null? This would reduce the state in the class. This does cause some additional passivity concerns, since targetUrlParameter has a getter. However, changing the default behaviour of the class (i.e. not using the parameter) is also a non-passive change. Another observation is that subclasses will likely have to inspect something (useTargetUrlParameter or targetUrlParameter!=null) in order to use AbstractAuthenticationTargetUrlRequestHandler without surprising results. This means that a getter for useTargetUrlParameter may need to be exposed if it is kept around. Just some thoughts (that may have already been considered).

@spring-issuemaster

Luke Taylor said:

Yes, I think that sounds like a better option.

@spring-issuemaster

Luke Taylor said:

Re-implemented following Rob's suggestion. The default value is now null and the functionality is disabled unless the targetUrlParameter property is set.

@spring-issuemaster

Piotr Jagielski said:

It took me a while to figure out. There are a lot of tutorials and blog entries on the web that mention the parameter "spring-security-redirect". Is the fact that it needs to be explicitly enabled now and how to enable it in the configuration covered somewhere in the reference documentation?

@spring-issuemaster

Clint Parham said:

I agree with Piotr...it would have been helpful to have this documented in the Spring docs. I ran right into this when upgrading from 3.0.2 to 3.2.5 and had to debug to figure out what was happening. I reviewed the "What's New" sections in the security reference docs for 3.1 and 3.2 and did not see any mention of it. Is there somewhere else I should have looked?

@spring-issuemaster spring-issuemaster added this to the 3.1.0.RC1 milestone Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment