Skip to content

Commit

Permalink
Fix login/logout redirect vulnerability
Browse files Browse the repository at this point in the history
  • Loading branch information
@Danielss89
Danielss89 committed Jun 8, 2014
1 parent db5b39d commit d1a6b3f
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 4 deletions.
4 changes: 2 additions & 2 deletions src/ZfcUser/Controller/UserController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ public function logoutAction()
$redirect = $this->params()->fromPost('redirect', $this->params()->fromQuery('redirect', false));

if ($this->getOptions()->getUseRedirectParameterIfPresent() && $redirect) {
return $this->redirect()->toUrl($redirect);
return $this->redirect()->toRoute($redirect);

This comment has been minimized.

Sign in to view

Copy link
@shipleyr

shipleyr Jul 1, 2014

Contributor

What happens if the route I need to redirect to requires extra parameters (e.g. segment route). How would that be specified in the query string? This was developed by myself explicitly to have the URL as the redirect route. I have a requirement to redirect someone to a specific page in a checkout process (/checkout/customer) and the route only specifies /checkout[/:step]. How do I redirect to that with only the route name and no params?

This comment has been minimized.

Sign in to view

Copy link
@Ocramius

Ocramius Jul 1, 2014

Contributor

@shipleyr this is being reworked by @Danielss89 so that a callback can be used instead.

This comment has been minimized.

Sign in to view

Copy link
@shipleyr

shipleyr Jul 1, 2014

Contributor

But in the meantime no one who uses this can upgrade at all. It's very restrictive update to make if it's being entirely re-worked. This change should only be included in the Major version update branch because it's a b/c break!
}

return $this->redirect()->toRoute($this->getOptions()->getLogoutRedirectRoute());
Expand Down Expand Up @@ -155,7 +155,7 @@ public function authenticateAction()
}

if ($this->getOptions()->getUseRedirectParameterIfPresent() && $redirect) {
return $this->redirect()->toUrl($redirect);
return $this->redirect()->toRoute($redirect);
}

return $this->redirect()->toRoute($this->getOptions()->getLoginRedirectRoute());
Expand Down
4 changes: 2 additions & 2 deletions tests/ZfcUserTest/Controller/UserControllerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ public function testLogoutAction($withRedirect, $post, $query)
->method('getUseRedirectParameterIfPresent')
->will($this->returnValue((bool) $withRedirect));
$redirect->expects($this->any())
->method('toUrl')
->method('toRoute')
->with($expectedLocation)
->will($this->returnValue($response));
} else {
Expand Down Expand Up @@ -510,7 +510,7 @@ public function testAuthenticateAction($wantRedirect, $post, $query, $prepareRes

} elseif ($wantRedirect && $hasRedirect) {
$redirect->expects($this->once())
->method('toUrl')
->method('toRoute')
->with(($post ?: $query ?: false))
->will($this->returnValue($response));
} else {
Expand Down

0 comments on commit d1a6b3f

Please sign in to comment.