Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F15422072
D19156.id45891.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
18 KB
Referenced Files
None
Subscribers
None
D19156.id45891.diff
View Options
diff --git a/resources/celerity/map.php b/resources/celerity/map.php
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -10,7 +10,7 @@
'conpherence.pkg.css' => 'e68cf1fa',
'conpherence.pkg.js' => '15191c65',
'core.pkg.css' => '2fa91e14',
- 'core.pkg.js' => '7aa5bd92',
+ 'core.pkg.js' => 'e7ce7bba',
'darkconsole.pkg.js' => '1f9a31bc',
'differential.pkg.css' => '113e692c',
'differential.pkg.js' => 'f6d809c0',
@@ -255,7 +255,7 @@
'rsrc/externals/javelin/lib/URI.js' => 'c989ade3',
'rsrc/externals/javelin/lib/Vector.js' => '2caa8fb8',
'rsrc/externals/javelin/lib/WebSocket.js' => '3ffe32d6',
- 'rsrc/externals/javelin/lib/Workflow.js' => '1e911d0f',
+ 'rsrc/externals/javelin/lib/Workflow.js' => '0eb1db0c',
'rsrc/externals/javelin/lib/__tests__/Cookie.js' => '5ed109e8',
'rsrc/externals/javelin/lib/__tests__/DOM.js' => 'c984504b',
'rsrc/externals/javelin/lib/__tests__/JSON.js' => '837a7d68',
@@ -757,7 +757,7 @@
'javelin-workboard-card' => 'c587b80f',
'javelin-workboard-column' => '758b4758',
'javelin-workboard-controller' => '26167537',
- 'javelin-workflow' => '1e911d0f',
+ 'javelin-workflow' => '0eb1db0c',
'maniphest-report-css' => '9b9580b7',
'maniphest-task-edit-css' => 'fda62a9b',
'maniphest-task-summary-css' => '11cc5344',
@@ -977,6 +977,17 @@
'javelin-dom',
'javelin-router',
),
+ '0eb1db0c' => array(
+ 'javelin-stratcom',
+ 'javelin-request',
+ 'javelin-dom',
+ 'javelin-vector',
+ 'javelin-install',
+ 'javelin-util',
+ 'javelin-mask',
+ 'javelin-uri',
+ 'javelin-routable',
+ ),
'0f764c35' => array(
'javelin-install',
'javelin-util',
@@ -1035,17 +1046,6 @@
'javelin-request',
'javelin-uri',
),
- '1e911d0f' => array(
- 'javelin-stratcom',
- 'javelin-request',
- 'javelin-dom',
- 'javelin-vector',
- 'javelin-install',
- 'javelin-util',
- 'javelin-mask',
- 'javelin-uri',
- 'javelin-routable',
- ),
'1f6794f6' => array(
'javelin-behavior',
'javelin-stratcom',
diff --git a/src/aphront/response/AphrontRedirectResponse.php b/src/aphront/response/AphrontRedirectResponse.php
--- a/src/aphront/response/AphrontRedirectResponse.php
+++ b/src/aphront/response/AphrontRedirectResponse.php
@@ -8,6 +8,7 @@
private $uri;
private $stackWhenCreated;
private $isExternal;
+ private $closeDialogBeforeRedirect;
public function setIsExternal($external) {
$this->isExternal = $external;
@@ -37,6 +38,15 @@
return PhabricatorEnv::getEnvConfig('debug.stop-on-redirect');
}
+ public function setCloseDialogBeforeRedirect($close) {
+ $this->closeDialogBeforeRedirect = $close;
+ return $this;
+ }
+
+ public function getCloseDialogBeforeRedirect() {
+ return $this->closeDialogBeforeRedirect;
+ }
+
public function getHeaders() {
$headers = array();
if (!$this->shouldStopForDebugging()) {
diff --git a/src/aphront/response/AphrontResponse.php b/src/aphront/response/AphrontResponse.php
--- a/src/aphront/response/AphrontResponse.php
+++ b/src/aphront/response/AphrontResponse.php
@@ -147,6 +147,13 @@
// Block relics of the old world: Flash, Java applets, and so on.
$csp[] = "object-src 'none'";
+ // Don't allow forms to submit offsite.
+
+ // This can result in some trickiness with file downloads if applications
+ // try to start downloads by submitting a dialog. Redirect to the file's
+ // download URI instead of submitting a form to it.
+ $csp[] = "form-action 'self'";
+
$csp = implode('; ', $csp);
return $csp;
diff --git a/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php b/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php
--- a/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php
+++ b/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php
@@ -24,10 +24,12 @@
$keys = PhabricatorSSHKeyGenerator::generateKeypair();
list($public_key, $private_key) = $keys;
+ $key_name = $default_name.'.key';
+
$file = PhabricatorFile::newFromFileData(
$private_key,
array(
- 'name' => $default_name.'.key',
+ 'name' => $key_name,
'ttl.relative' => phutil_units('10 minutes in seconds'),
'viewPolicy' => $viewer->getPHID(),
));
@@ -62,25 +64,33 @@
->setContentSourceFromRequest($request)
->applyTransactions($key, $xactions);
- // NOTE: We're disabling workflow on submit so the download works. We're
- // disabling workflow on cancel so the page reloads, showing the new
- // key.
+ $download_link = phutil_tag(
+ 'a',
+ array(
+ 'href' => $file->getDownloadURI(),
+ ),
+ array(
+ id(new PHUIIconView())->setIcon('fa-download'),
+ ' ',
+ pht('Download Private Key (%s)', $key_name),
+ ));
+ $download_link = phutil_tag('strong', array(), $download_link);
+
+ // NOTE: We're disabling workflow on cancel so the page reloads, showing
+ // the new key.
return $this->newDialog()
->setTitle(pht('Download Private Key'))
- ->setDisableWorkflowOnCancel(true)
- ->setDisableWorkflowOnSubmit(true)
- ->setSubmitURI($file->getDownloadURI())
->appendParagraph(
pht(
'A keypair has been generated, and the public key has been '.
- 'added as a recognized key. Use the button below to download '.
- 'the private key.'))
+ 'added as a recognized key.'))
+ ->appendParagraph($download_link)
->appendParagraph(
pht(
'After you download the private key, it will be destroyed. '.
'You will not be able to retrieve it if you lose your copy.'))
- ->addSubmitButton(pht('Download Private Key'))
+ ->setDisableWorkflowOnCancel(true)
->addCancelButton($cancel_uri, pht('Done'));
}
diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php
--- a/src/applications/base/controller/PhabricatorController.php
+++ b/src/applications/base/controller/PhabricatorController.php
@@ -298,6 +298,7 @@
->setContent(
array(
'redirect' => $response->getURI(),
+ 'close' => $response->getCloseDialogBeforeRedirect(),
));
}
}
diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php
--- a/src/applications/diffusion/controller/DiffusionServeController.php
+++ b/src/applications/diffusion/controller/DiffusionServeController.php
@@ -1046,7 +1046,7 @@
// <https://github.com/github/git-lfs/issues/1088>
$no_authorization = 'Basic '.base64_encode('none');
- $get_uri = $file->getCDNURI();
+ $get_uri = $file->getCDNURI('data');
$actions['download'] = array(
'href' => $get_uri,
'header' => array(
diff --git a/src/applications/files/application/PhabricatorFilesApplication.php b/src/applications/files/application/PhabricatorFilesApplication.php
--- a/src/applications/files/application/PhabricatorFilesApplication.php
+++ b/src/applications/files/application/PhabricatorFilesApplication.php
@@ -101,7 +101,7 @@
private function getResourceSubroutes() {
return array(
- 'data/'.
+ '(?P<kind>data|download)/'.
'(?:@(?P<instance>[^/]+)/)?'.
'(?P<key>[^/]+)/'.
'(?P<phid>[^/]+)/'.
@@ -132,7 +132,7 @@
public function getQuicksandURIPatternBlacklist() {
return array(
- '/file/data/.*',
+ '/file/(data|download)/.*',
);
}
diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php
--- a/src/applications/files/controller/PhabricatorFileDataController.php
+++ b/src/applications/files/controller/PhabricatorFileDataController.php
@@ -26,6 +26,9 @@
$req_domain = $request->getHost();
$main_domain = id(new PhutilURI($base_uri))->getDomain();
+ $request_kind = $request->getURIData('kind');
+ $is_download = ($request_kind === 'download');
+
if (!strlen($alt) || $main_domain == $alt_domain) {
// No alternate domain.
$should_redirect = false;
@@ -50,7 +53,7 @@
if ($should_redirect) {
return id(new AphrontRedirectResponse())
->setIsExternal(true)
- ->setURI($file->getCDNURI());
+ ->setURI($file->getCDNURI($request_kind));
}
$response = new AphrontFileResponse();
@@ -71,34 +74,30 @@
}
$is_viewable = $file->isViewableInBrowser();
- $force_download = $request->getExists('download');
-
$request_type = $request->getHTTPHeader('X-Phabricator-Request-Type');
$is_lfs = ($request_type == 'git-lfs');
- if ($is_viewable && !$force_download) {
+ if ($is_viewable && !$is_download) {
$response->setMimeType($file->getViewableMimeType());
} else {
- $is_public = !$viewer->isLoggedIn();
$is_post = $request->isHTTPPost();
- // NOTE: Require POST to download files from the primary domain if the
- // request includes credentials. The "Download File" links we generate
- // in the web UI are forms which use POST to satisfy this requirement.
-
- // The intent is to make attacks based on tags like "<iframe />" and
- // "<script />" (which can issue GET requests, but can not easily issue
- // POST requests) more difficult to execute.
-
- // The best defense against these attacks is to use an alternate file
- // domain, which is why we strongly recommend doing so.
+ // NOTE: Require POST to download files from the primary domain. If the
+ // request is not a POST request but arrives on the primary domain, we
+ // render a confirmation dialog. For discussion, see T13094.
- $is_safe = ($is_alternate_domain || $is_lfs || $is_post || $is_public);
+ $is_safe = ($is_alternate_domain || $is_lfs || $is_post);
if (!$is_safe) {
- // This is marked as "external" because it is fully qualified.
- return id(new AphrontRedirectResponse())
- ->setIsExternal(true)
- ->setURI(PhabricatorEnv::getProductionURI($file->getBestURI()));
+ return $this->newDialog()
+ ->setSubmitURI($file->getDownloadURI())
+ ->setTitle(pht('Download File'))
+ ->appendParagraph(
+ pht(
+ 'Download file %s (%s)?',
+ phutil_tag('strong', array(), $file->getName()),
+ phutil_format_bytes($file->getByteSize())))
+ ->addCancelButton($file->getURI())
+ ->addSubmitButton(pht('Download File'));
}
$response->setMimeType($file->getMimeType());
diff --git a/src/applications/files/controller/PhabricatorFileInfoController.php b/src/applications/files/controller/PhabricatorFileInfoController.php
--- a/src/applications/files/controller/PhabricatorFileInfoController.php
+++ b/src/applications/files/controller/PhabricatorFileInfoController.php
@@ -137,11 +137,10 @@
$curtain->addAction(
id(new PhabricatorActionView())
->setUser($viewer)
- ->setRenderAsForm($can_download)
->setDownload($can_download)
->setName(pht('Download File'))
->setIcon('fa-download')
- ->setHref($file->getViewURI())
+ ->setHref($file->getDownloadURI())
->setDisabled(!$can_download)
->setWorkflow(!$can_download));
}
diff --git a/src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php b/src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php
--- a/src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php
+++ b/src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php
@@ -144,7 +144,7 @@
$existing_xform = $file->getTransform($preview_key);
if ($existing_xform) {
- $xform_uri = $existing_xform->getCDNURI();
+ $xform_uri = $existing_xform->getCDNURI('data');
} else {
$xform_uri = $file->getURIForTransform($xform);
}
diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -810,16 +810,24 @@
pht('You must save a file before you can generate a view URI.'));
}
- return $this->getCDNURI();
+ return $this->getCDNURI('data');
}
- public function getCDNURI() {
+ public function getCDNURI($request_kind) {
+ if (($request_kind !== 'data') &&
+ ($request_kind !== 'download')) {
+ throw new Exception(
+ pht(
+ 'Unknown file content request kind "%s".',
+ $request_kind));
+ }
+
$name = self::normalizeFileName($this->getName());
$name = phutil_escape_uri($name);
$parts = array();
$parts[] = 'file';
- $parts[] = 'data';
+ $parts[] = $request_kind;
// If this is an instanced install, add the instance identifier to the URI.
// Instanced configurations behind a CDN may not be able to control the
@@ -861,9 +869,7 @@
}
public function getDownloadURI() {
- $uri = id(new PhutilURI($this->getViewURI()))
- ->setQueryParam('download', true);
- return (string)$uri;
+ return $this->getCDNURI('download');
}
public function getURIForTransform(PhabricatorFileTransform $transform) {
@@ -1469,6 +1475,16 @@
->setURI($uri);
}
+ public function newDownloadResponse() {
+ // We're cheating a little bit here and relying on the fact that
+ // getDownloadURI() always returns a fully qualified URI with a complete
+ // domain.
+ return id(new AphrontRedirectResponse())
+ ->setIsExternal(true)
+ ->setCloseDialogBeforeRedirect(true)
+ ->setURI($this->getDownloadURI());
+ }
+
public function attachTransforms(array $map) {
$this->transforms = $map;
return $this;
diff --git a/src/applications/harbormaster/controller/HarbormasterBuildLogDownloadController.php b/src/applications/harbormaster/controller/HarbormasterBuildLogDownloadController.php
--- a/src/applications/harbormaster/controller/HarbormasterBuildLogDownloadController.php
+++ b/src/applications/harbormaster/controller/HarbormasterBuildLogDownloadController.php
@@ -44,19 +44,7 @@
->addCancelButton($cancel_uri);
}
- $size = $file->getByteSize();
-
- return $this->newDialog()
- ->setTitle(pht('Download Build Log'))
- ->appendParagraph(
- pht(
- 'This log has a total size of %s. If you insist, you may '.
- 'download it.',
- phutil_tag('strong', array(), phutil_format_bytes($size))))
- ->setDisableWorkflowOnSubmit(true)
- ->addSubmitButton(pht('Download Log'))
- ->setSubmitURI($file->getDownloadURI())
- ->addCancelButton($cancel_uri, pht('Done'));
+ return $file->newDownloadResponse();
}
}
diff --git a/src/applications/harbormaster/view/HarbormasterBuildLogView.php b/src/applications/harbormaster/view/HarbormasterBuildLogView.php
--- a/src/applications/harbormaster/view/HarbormasterBuildLogView.php
+++ b/src/applications/harbormaster/view/HarbormasterBuildLogView.php
@@ -34,12 +34,14 @@
$download_uri = "/harbormaster/log/download/{$id}/";
+ $can_download = (bool)$log->getFilePHID();
+
$download_button = id(new PHUIButtonView())
->setTag('a')
->setHref($download_uri)
->setIcon('fa-download')
- ->setDisabled(!$log->getFilePHID())
- ->setWorkflow(true)
+ ->setDisabled(!$can_download)
+ ->setWorkflow(!$can_download)
->setText(pht('Download Log'));
$header->addActionLink($download_button);
diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php
--- a/src/applications/search/controller/PhabricatorApplicationSearchController.php
+++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php
@@ -512,15 +512,7 @@
->setURI($job->getMonitorURI());
} else {
$file = $export_engine->exportFile();
-
- return $this->newDialog()
- ->setTitle(pht('Download Results'))
- ->appendParagraph(
- pht('Click the download button to download the exported data.'))
- ->addCancelButton($cancel_uri, pht('Done'))
- ->setSubmitURI($file->getDownloadURI())
- ->setDisableWorkflowOnSubmit(true)
- ->addSubmitButton(pht('Download Data'));
+ return $file->newDownloadResponse();
}
}
}
@@ -535,12 +527,18 @@
->setValue($format_key)
->setOptions($format_options));
+ if ($is_large_export) {
+ $submit_button = pht('Continue');
+ } else {
+ $submit_button = pht('Download Data');
+ }
+
return $this->newDialog()
->setTitle(pht('Export Results'))
->setErrors($errors)
->appendForm($export_form)
->addCancelButton($cancel_uri)
- ->addSubmitButton(pht('Continue'));
+ ->addSubmitButton($submit_button);
}
private function processEditRequest() {
diff --git a/src/infrastructure/export/engine/PhabricatorExportEngineBulkJobType.php b/src/infrastructure/export/engine/PhabricatorExportEngineBulkJobType.php
--- a/src/infrastructure/export/engine/PhabricatorExportEngineBulkJobType.php
+++ b/src/infrastructure/export/engine/PhabricatorExportEngineBulkJobType.php
@@ -36,7 +36,6 @@
->setName(pht('Temporary File Expired'));
} else {
$actions[] = id(new PhabricatorActionView())
- ->setRenderAsForm(true)
->setHref($file->getDownloadURI())
->setIcon('fa-download')
->setName(pht('Download Data Export'));
diff --git a/webroot/rsrc/externals/javelin/lib/Workflow.js b/webroot/rsrc/externals/javelin/lib/Workflow.js
--- a/webroot/rsrc/externals/javelin/lib/Workflow.js
+++ b/webroot/rsrc/externals/javelin/lib/Workflow.js
@@ -276,6 +276,13 @@
// It is permissible to send back a falsey redirect to force a page
// reload, so we need to take this branch if the key is present.
if (r && (typeof r.redirect != 'undefined')) {
+ // Before we redirect to file downloads, we close the dialog. These
+ // redirects aren't real navigation events so we end up stuck in the
+ // dialog otherwise.
+ if (r.close) {
+ this._pop();
+ }
+
JX.$U(r.redirect).go();
} else if (r && r.dialog) {
this._push();
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Sun, Mar 23, 5:15 AM (2 w, 4 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7394762
Default Alt Text
D19156.id45891.diff (18 KB)
Attached To
Mode
D19156: Never generate file download forms which point to the CDN domain, tighten "form-action" CSP
Attached
Detach File
Event Timeline
Log In to Comment