Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1.x] fix: Logout controller allows open redirects #3948

Merged
merged 11 commits into from
Jan 5, 2024
Merged

Conversation

imorland
Copy link
Member

@imorland imorland commented Jan 4, 2024

Prevents open redirects on the LogoutController

By default, only return URL's on the forum host are permitted. Additional domains may be whitelisted using config.php:

...
  'redirectDomains' =>
  array (
    'trusted.com`,
    'another-trusted.org'
  )

@imorland imorland marked this pull request as ready for review January 4, 2024 22:08
@imorland imorland requested a review from a team as a code owner January 4, 2024 22:08
@imorland imorland changed the title Im/logout redirect fix: Logout controller allows open redirects Jan 5, 2024
@imorland imorland changed the title fix: Logout controller allows open redirects [1.x] fix: Logout controller allows open redirects Jan 5, 2024
Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Would be nice to see an integration test, but otherwise the code makes sense!

Copy link
Member

@luceos luceos left a comment

Choose a reason for hiding this comment

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

Haven't tested this, but left some comments for improvement.

framework/core/src/Forum/Controller/LogOutController.php Outdated Show resolved Hide resolved
framework/core/src/Forum/Controller/LogOutController.php Outdated Show resolved Hide resolved
framework/core/src/Forum/Controller/LogOutController.php Outdated Show resolved Hide resolved
@imorland imorland requested a review from luceos January 5, 2024 12:44
@imorland
Copy link
Member Author

imorland commented Jan 5, 2024

Would be nice to see an integration test, but otherwise the code makes sense!

I did try, however we need to make some changes to flarum/testing in order to pass the query string, as it looks like this is not currently supported.

For example:

    public function logout_with_forum_redirect(string $returnUrl)
    {
        $encodedReturnUrl = urlencode($returnUrl);

        $response = $this->send(
            $this->request('GET', '/logout?return=' . $encodedReturnUrl)
        );

        $this->assertEquals(302, $response->getStatusCode());
        $this->assertEquals($returnUrl, $response->getHeaderLine('location'));
    }

This is a test I attempted to write for this, but we currently don't have the ability to pass the query, due to

$request = new ServerRequest([], [], $path, $method);

@imorland imorland merged commit 7d70328 into 1.x Jan 5, 2024
312 checks passed
@imorland imorland deleted the im/logout-redirect branch January 5, 2024 15:33
SychO9 pushed a commit that referenced this pull request Oct 24, 2024
* fix: prevent open redirects on logout controller

* use clearer config key

* cast url as string, reinstate guest redirect

* clean up a little

* simplify

* return Uri

* resolve ternary always true

* simplify some more

* remove extra newline

* handle malformed uri

* chore: requested changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants