Skip to content

Commit

Permalink
Fix issue where redirectURLs were not generated correctly.
Browse files Browse the repository at this point in the history
When the first path segment matches the base path an incorrect URL was
generated. Trimming slashes off makes Router normalize the URL correctly
as the leading / implies that the base is already prepended.

Fixes #3897
  • Loading branch information
markstory committed Jun 30, 2013
1 parent 0d76bfe commit 1d18a4f
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 2 deletions.
5 changes: 3 additions & 2 deletions lib/Cake/Controller/Component/AuthComponent.php
Expand Up @@ -651,8 +651,8 @@ public function redirect($url = null) {
* If no parameter is passed, gets the authentication redirect URL. The URL
* returned is as per following rules:
*
* - Returns the session Auth.redirect value if it is present and for the same
* domain the current app is running on.
* - Returns the normalized URL from session Auth.redirect value if it is
* present and for the same domain the current app is running on.
* - If there is no session value and there is a $loginRedirect, the $loginRedirect
* value is returned.
* - If there is no session and no $loginRedirect, / is returned.
Expand All @@ -666,6 +666,7 @@ public function redirectUrl($url = null) {
$this->Session->write('Auth.redirect', $redir);
} elseif ($this->Session->check('Auth.redirect')) {
$redir = $this->Session->read('Auth.redirect');
$redir = is_string($redir) ? ltrim($redir, '/') : $redir;
$this->Session->delete('Auth.redirect');

if (Router::normalize($redir) == Router::normalize($this->loginAction)) {
Expand Down
17 changes: 17 additions & 0 deletions lib/Cake/Test/Case/Controller/Component/AuthComponentTest.php
Expand Up @@ -1235,6 +1235,23 @@ public function testRedirectSessionRead() {
$this->assertFalse($this->Auth->Session->check('Auth.redirect'));
}

/**
* test redirectUrl with duplicate base.
*
* @return void
*/
public function testRedirectSessionReadDuplicateBase() {
$this->Auth->request->webroot = '/waves/';
$this->Auth->request->base = '/waves';

Router::setRequestInfo($this->Auth->request);

$this->Auth->Session->write('Auth.redirect', '/waves/add');

$result = $this->Auth->redirectUrl();
$this->assertEquals('/waves/add', $result);
}

/**
* test that redirect does not return loginAction if that is what's stored in Auth.redirect.
* instead loginRedirect should be used.
Expand Down

10 comments on commit 1d18a4f

@wouterverweirder
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ltrim introduces issues when working from a plugin (plugin name is appended again when redirecting). I fixed this by stripping the base path when setting the Auth.redirect in the session:

$this->Session->write('Auth.redirect', $request->here(false)); (line 323)

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be a better solution. I did not do that in the first place as I was worried about creating upgrade issues with data in the session. I will give this a try though as it might be a simpler solution.

@renan
Copy link
Contributor

@renan renan commented on 1d18a4f Jul 12, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change introduces a bug when the application is not running on /.

I have just setup a fresh 2.3 application at /cakephp/2.3, it tries to redirect to http://localhost/cakephp/2.3/cakephp/2.3/users/protected after login.

@jinhr
Copy link

@jinhr jinhr commented on 1d18a4f Jul 12, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with renansaddam, I reported as ticket #3916

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, twas a bad fix then. I'll take another look at correcting it.

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jinhr, @renansaddam Could you try again with d40c737 I tried with an app in a subdirectory that shared a name with a controller and the original issue stayed fixed. I also tried in a subdir with a different name, and it also worked.

@Phally
Copy link
Contributor

@Phally Phally commented on 1d18a4f Jul 13, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d40c737 seems to do the trick. The redirect url in the session looks good as well. With 1d18a4f it still had the base in it.

@renan
Copy link
Contributor

@renan renan commented on 1d18a4f Jul 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug introduced by the commit is fixed and all tests are green. Good fix!

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woot 👍

@jinhr
Copy link

@jinhr jinhr commented on 1d18a4f Jul 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markstory I think d40c737 fixed the redirect to '/'. Great job!

Please sign in to comment.