Skip to content

Commit

Permalink
[1.x] fix: Logout controller allows open redirects (#3948)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
imorland authored Jan 5, 2024
1 parent 45a8b57 commit 7d70328
Showing 1 changed file with 47 additions and 8 deletions.
55 changes: 47 additions & 8 deletions framework/core/src/Forum/Controller/LogOutController.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

namespace Flarum\Forum\Controller;

use Flarum\Foundation\Config;
use Flarum\Http\Exception\TokenMismatchException;
use Flarum\Http\Rememberer;
use Flarum\Http\RequestUtil;
Expand All @@ -20,6 +21,7 @@
use Illuminate\Support\Arr;
use Laminas\Diactoros\Response\HtmlResponse;
use Laminas\Diactoros\Response\RedirectResponse;
use Laminas\Diactoros\Uri;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface as Request;
use Psr\Http\Server\RequestHandlerInterface;
Expand Down Expand Up @@ -51,25 +53,33 @@ class LogOutController implements RequestHandlerInterface
*/
protected $url;

/**
* @var Config
*/
protected $config;

/**
* @param Dispatcher $events
* @param SessionAuthenticator $authenticator
* @param Rememberer $rememberer
* @param Factory $view
* @param UrlGenerator $url
* @param Config $config
*/
public function __construct(
Dispatcher $events,
SessionAuthenticator $authenticator,
Rememberer $rememberer,
Factory $view,
UrlGenerator $url
UrlGenerator $url,
Config $config
) {
$this->events = $events;
$this->authenticator = $authenticator;
$this->rememberer = $rememberer;
$this->view = $view;
$this->url = $url;
$this->config = $config;
}

/**
Expand All @@ -81,29 +91,29 @@ public function handle(Request $request): ResponseInterface
{
$session = $request->getAttribute('session');
$actor = RequestUtil::getActor($request);
$base = $this->url->to('forum')->base();

$url = Arr::get($request->getQueryParams(), 'return', $this->url->to('forum')->base());
$returnUrl = Arr::get($request->getQueryParams(), 'return');
$return = $this->sanitizeReturnUrl((string) $returnUrl, $base);

// If there is no user logged in, return to the index.
// If there is no user logged in, return to the index or the return url if it's set.
if ($actor->isGuest()) {
return new RedirectResponse($url);
return new RedirectResponse($return);
}

// If a valid CSRF token hasn't been provided, show a view which will
// allow the user to press a button to complete the log out process.
$csrfToken = $session->token();

if (Arr::get($request->getQueryParams(), 'token') !== $csrfToken) {
$return = Arr::get($request->getQueryParams(), 'return');

$view = $this->view->make('flarum.forum::log-out')
->with('url', $this->url->to('forum')->route('logout').'?token='.$csrfToken.($return ? '&return='.urlencode($return) : ''));
->with('url', $this->url->to('forum')->route('logout') . '?token=' . $csrfToken . ($returnUrl ? '&return=' . urlencode($return) : ''));

return new HtmlResponse($view->render());
}

$accessToken = $session->get('access_token');
$response = new RedirectResponse($url);
$response = new RedirectResponse($return);

$this->authenticator->logOut($session);

Expand All @@ -113,4 +123,33 @@ public function handle(Request $request): ResponseInterface

return $this->rememberer->forget($response);
}

protected function sanitizeReturnUrl(string $url, string $base): Uri
{
if (empty($url)) {
return new Uri($base);
}

try {
$parsedUrl = new Uri($url);
} catch (\InvalidArgumentException $e) {
return new Uri($base);
}

if (in_array($parsedUrl->getHost(), $this->getAllowedRedirectDomains())) {
return $parsedUrl;
}

return new Uri($base);
}

protected function getAllowedRedirectDomains(): array
{
$forumUri = $this->config->url();

return array_merge(
[$forumUri->getHost()],
$this->config->offsetGet('redirectDomains') ?? []
);
}
}

0 comments on commit 7d70328

Please sign in to comment.