Generate a 403 page with a nice dialog when a file token is invalid
Summary:
Ref T5685. Currently we just 403 on an invalid token, but we can be a little more helpful.
The issues here are:
- If we **do** redirect you on this page and something goes wrong, you might get stuck in a redirect loop.
- If we **don't** redirect you, copy/pasting the link to someone (or reloading the page) gives them a pretty confusing result, since the link doesn't work any more. Prior to this diff, they get a 403.
To mitigate this, do a little better than a bare 403: give them a link to auth and generate a new URI for the file.
If this is still confusing, the next best thing I can come up with is something like this:
- Put some modulous of the timestamp in the URI.
- If the current time is within 2 seconds of the generation time, show this dialog.
- Otherwise, redirect.
That seems like it would be okay, but I worry that "2" has to be small (so links you copy/paste -> chat -> click still work) and a small value means that a small amount of clock skew breaks things. We could use the database clock, but ehhh.
Other ideas:
- Put a hash of the remote IP in the URI, redirect if it doesn't match. Fails for companies behind a NAT gateway but should work in a lot of other cases.
- Just redirect always, there's no reason it should ever loop and browsers don't really do anything bad when there's a loop (they'll show an error after too many redirects).
I'm leaning toward letting this stabilize in the wild for a bit, then trying "always redirect".
Test Plan: {F188914}
Reviewers: btrahan, 20after4
Reviewed By: 20after4
Subscribers: epriestley
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10215
This commit is contained in:
@@ -2,17 +2,6 @@
|
|||||||
|
|
||||||
abstract class PhabricatorFileController extends PhabricatorController {
|
abstract class PhabricatorFileController extends PhabricatorController {
|
||||||
|
|
||||||
public function buildApplicationCrumbs() {
|
|
||||||
$crumbs = parent::buildApplicationCrumbs();
|
|
||||||
$crumbs->addAction(
|
|
||||||
id(new PHUIListItemView())
|
|
||||||
->setName(pht('Upload File'))
|
|
||||||
->setIcon('fa-upload')
|
|
||||||
->setHref($this->getApplicationURI('/upload/')));
|
|
||||||
|
|
||||||
return $crumbs;
|
|
||||||
}
|
|
||||||
|
|
||||||
protected function buildSideNavView() {
|
protected function buildSideNavView() {
|
||||||
$menu = $this->buildMenu($for_devices = false);
|
$menu = $this->buildMenu($for_devices = false);
|
||||||
return AphrontSideNavFilterView::newFromMenu($menu);
|
return AphrontSideNavFilterView::newFromMenu($menu);
|
||||||
|
|||||||
@@ -90,23 +90,46 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
|
|||||||
return $error_response;
|
return $error_response;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
$acquire_token_uri = id(new PhutilURI($file->getViewURI()))
|
||||||
|
->setDomain($main_domain);
|
||||||
|
|
||||||
|
|
||||||
if ($this->token) {
|
if ($this->token) {
|
||||||
// validate the token, if it is valid, continue
|
// validate the token, if it is valid, continue
|
||||||
$validated_token = $file->validateOneTimeToken($this->token);
|
$validated_token = $file->validateOneTimeToken($this->token);
|
||||||
|
|
||||||
if (!$validated_token) {
|
if (!$validated_token) {
|
||||||
return new Aphront403Response();
|
$dialog = $this->newDialog()
|
||||||
|
->setShortTitle(pht('Expired File'))
|
||||||
|
->setTitle(pht('File Link Has Expired'))
|
||||||
|
->appendParagraph(
|
||||||
|
pht(
|
||||||
|
'The link you followed to view this file is invalid or '.
|
||||||
|
'expired.'))
|
||||||
|
->appendParagraph(
|
||||||
|
pht(
|
||||||
|
'Continue to generate a new link to the file. You may be '.
|
||||||
|
'required to log in.'))
|
||||||
|
->addCancelButton(
|
||||||
|
$acquire_token_uri,
|
||||||
|
pht('Continue'));
|
||||||
|
|
||||||
|
// Build an explicit response so we can respond with HTTP/403 instead
|
||||||
|
// of HTTP/200.
|
||||||
|
$response = id(new AphrontDialogResponse())
|
||||||
|
->setDialog($dialog)
|
||||||
|
->setHTTPResponseCode(403);
|
||||||
|
|
||||||
|
return $response;
|
||||||
}
|
}
|
||||||
// return the file data without cache headers
|
// return the file data without cache headers
|
||||||
$cache_response = false;
|
$cache_response = false;
|
||||||
} else if (!$file->getCanCDN()) {
|
} else if (!$file->getCanCDN()) {
|
||||||
// file cannot be served via cdn, and no token given
|
// file cannot be served via cdn, and no token given
|
||||||
// redirect to the main domain to aquire a token
|
// redirect to the main domain to aquire a token
|
||||||
$file_uri = id(new PhutilURI($file->getViewURI()))
|
|
||||||
->setDomain($main_domain);
|
|
||||||
|
|
||||||
return id(new AphrontRedirectResponse())
|
return id(new AphrontRedirectResponse())
|
||||||
->setURI($file_uri);
|
->setURI($acquire_token_uri);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -22,4 +22,15 @@ final class PhabricatorFileListController extends PhabricatorFileController {
|
|||||||
return $this->delegateToController($controller);
|
return $this->delegateToController($controller);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
public function buildApplicationCrumbs() {
|
||||||
|
$crumbs = parent::buildApplicationCrumbs();
|
||||||
|
$crumbs->addAction(
|
||||||
|
id(new PHUIListItemView())
|
||||||
|
->setName(pht('Upload File'))
|
||||||
|
->setIcon('fa-upload')
|
||||||
|
->setHref($this->getApplicationURI('/upload/')));
|
||||||
|
|
||||||
|
return $crumbs;
|
||||||
|
}
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user