mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-24 15:52:41 +01:00
5b1d9c935a
Summary: Fixes T3793. There's a lot of history here, see D4012, T2102. Basically, the problem is that things used to work like this: - User is logged out and accesses `/xyz/`. After they login, we'd like to send them back to `/xyz/`, so we set a `next_uri` cookie. - User's browser has a bunch of extensions and now makes a ton of requests for stuff that doesn't exist, like `humans.txt` and `apple-touch-icon.png`. We can't distinguish between these requests and normal requests in a general way, so we write `next_uri` cookies, overwriting the user's intent (`/xyz/`). To fix this, we made the 404 page not set `next_uri`, in D4012. So if the browser requests `humans.txt`, we 404 with no cookie, and the `/xyz/` cookie is preserved. However, this is bad because an attacker can determine if objects exist and applications are installed, by visiting, e.g., `/T123` and seeing if they get a 404 page (resource really does not exist) or a login page (resource exists). We'd rather not leak this information. The comment in the body text describes this in more detail. This diff sort of tries to do the right thing most of the time: we write the cookie only if we haven't written it in the last 2 minutes. Generally, this should mean that the original request to `/xyz/` writes it, all the `humans.txt` requests don't write it, and things work like users expect. This may occasionally do the wrong thing, but it should be very rare, and we stop leaking information about applications and objects. Test Plan: Logged out, clicked around / logged in, used Charles to verify that cookies were set in the expected way. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T3793 Differential Revision: https://secure.phabricator.com/D8047
73 lines
1.9 KiB
PHP
73 lines
1.9 KiB
PHP
<?php
|
|
|
|
final class PhabricatorAuthValidateController
|
|
extends PhabricatorAuthController {
|
|
|
|
public function shouldRequireLogin() {
|
|
return false;
|
|
}
|
|
|
|
public function processRequest() {
|
|
$request = $this->getRequest();
|
|
$viewer = $request->getUser();
|
|
|
|
$failures = array();
|
|
|
|
if (!strlen($request->getStr('expect'))) {
|
|
return $this->renderErrors(
|
|
array(
|
|
pht(
|
|
'Login validation is missing expected parameter ("%s").',
|
|
'phusr')));
|
|
}
|
|
|
|
$expect_phusr = $request->getStr('expect');
|
|
$actual_phusr = $request->getCookie(PhabricatorCookies::COOKIE_USERNAME);
|
|
if ($actual_phusr != $expect_phusr) {
|
|
if ($actual_phusr) {
|
|
$failures[] = pht(
|
|
"Attempted to set '%s' cookie to '%s', but your browser sent back ".
|
|
"a cookie with the value '%s'. Clear your browser's cookies and ".
|
|
"try again.",
|
|
'phusr',
|
|
$expect_phusr,
|
|
$actual_phusr);
|
|
} else {
|
|
$failures[] = pht(
|
|
"Attempted to set '%s' cookie to '%s', but your browser did not ".
|
|
"accept the cookie. Check that cookies are enabled, clear them, ".
|
|
"and try again.",
|
|
'phusr',
|
|
$expect_phusr);
|
|
}
|
|
}
|
|
|
|
if (!$failures) {
|
|
if (!$viewer->getPHID()) {
|
|
$failures[] = pht(
|
|
"Login cookie was set correctly, but your login session is not ".
|
|
"valid. Try clearing cookies and logging in again.");
|
|
}
|
|
}
|
|
|
|
if ($failures) {
|
|
return $this->renderErrors($failures);
|
|
}
|
|
|
|
$next = PhabricatorCookies::getNextURICookie($request);
|
|
$request->clearCookie(PhabricatorCookies::COOKIE_NEXTURI);
|
|
|
|
if (!PhabricatorEnv::isValidLocalWebResource($next)) {
|
|
$next = '/';
|
|
}
|
|
|
|
return id(new AphrontRedirectResponse())->setURI($next);
|
|
}
|
|
|
|
private function renderErrors(array $messages) {
|
|
return $this->renderErrorPage(
|
|
pht('Login Failure'),
|
|
$messages);
|
|
}
|
|
|
|
}
|