Spring-security-oauth: DefaultRedirectResolver ignores port

[sfussenegger]

I think ports should be checked by default. In theory, an attacker could be able to run a process on the same host and redirect users to it.

Implementation should be somewhat like this:

protected boolean portMatches(String protocol, int registered, int requested) {
  int defaultPort = "http".equals(protocol) ? 80 : ("https".equals(protocol) ? 443 : -1);
  return (registered == -1 ? defaultPort : registered) == (requested == -1 ? defaultPort : requested);
}

However I'm not sure if this is intentional or not. After all there is ExactMatchRedirectResolver as an alternative. If it is, it should at least be documented in the Javadoc of `DefaultRedirectResolver.redirectMatches(String,String).

[jgrandja]

Thanks again @sfussenegger for recommending port matching. Agreed. This has now been merged to master.

[sfussenegger]

Thanks for fixing. But are you sure you want to release this as part of a patch level release? It does have the potential to break things for some people. Most likely in non-production environments - as it would for us - but still. Especially since @dsyer documented the existing behavior in e30aefd which was released as part of 2.0.9.

[dsyer]

I think it's a potential vulnerability, so I would prefer to put it in 2.0.10 with some release notes about how to migrate if you rely on it. I suppose we could add a flag to restore the old behaviour (then affected users would only have to explicitly create a resolver and inject it)?

[jgrandja]

Agreed @dsyer. I will add a matchPorts property on DefaultRedirectResolver that will provide backwards compatibility. It will default to true and existing/affected users will need to explicity set this property to false and inject the configured DefaultRedirectResolver on the AuthorizationEndpoint.

I'll provide this migration info in the release notes as well.

[jgrandja]

DefaultRedirectResolver.matchPorts property has been added and merged to master.