Login redirect param doesn't prevent absolute/external URLs
LordSimal opened this issue · 5 comments
The Problem
The redirect param used to save the desired URL a user wants to visit can be an external URL.
How to reproduce
- Go to a protected URL without being logged in
- You will get redirected to something like https://%my-domain%/login?redirect=%url-encoded-desired-url%
- Change the %url-encoded-desired-url% to google.com
- Login with a valid user
- You get redirected to google.com
A bit more context
As we just discussed in the CakePHP Slack support channel the cakephp/authentication plugin basically prevents external OR absolute URLs if the functiongetLoginRedirect() is used
https://github.com/cakephp/authentication/blob/de989c759937406f514a5a31313b36578005b07c/src/AuthenticationService.php#L400
I haven't gone that deep in the cakedc/users plugin to check where this needs to be adjusted but I guess there should be at least a config option to enable this "security feature"
I agree, I think the best approach here would be adding a test case to check this redirect is actually happening and then enforce the same behavior we have in cakephp/authentication
Well, I just tried to get a better understanding of what is happening here.
So first I started of checking the afterIdentifyUser function in the LoginComponent.
https://github.com/CakeDC/users/blob/master/src/Controller/Component/LoginComponent.php#L154
Here I realised, that there is already a check for if the current user can access the redirect Url so I went in there.
So from here we call isAuthorized($url)
https://github.com/CakeDC/auth/blob/6.next-cake4/src/Traits/IsAuthorizedTrait.php
which then calls _checkCanAccess($url)
https://github.com/CakeDC/auth/blob/6.next-cake4/src/Traits/IsAuthorizedTrait.php#L50
which then calls $service->can($identity, $action, $targetRequest)
https://github.com/CakeDC/auth/blob/6.next-cake4/src/Traits/IsAuthorizedTrait.php#L63
which uses the SuperuserPolicy (because I login with a superuser) and therefore the access is granted to an external URL.
So the question now is where should this fix be implemented?
Should external URLs be denied to be checked in general?
@LordSimal I've actually started working on a fix to:
- Define an array of trusted hostnames in configuration
- Check refactor LoginComponent::afterIdentifyUser to check for safe redirects
I think that would cover our security needs and fix any other edge cases when you really need to redirect to another domain in multi-tenant apps.
I would guess the creation of the redirect query param doesn't need to be adjusted, only when the query param is being parsed and used to redirect after successfull login.
otherwhise your solution seems awesome 😄