From 9356a993a8a0e7138c9ce8fb5878f6f7f7581841 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Mar 2018 10:25:05 -0800
Subject: [PATCH] (stable) Perform a client-side redirect after OAuth server
 authorization

Summary:
Ref T13099. See that task for discussion. Chrome is unhappy with an MFA form submitting to an endpoint which redirects you to an OAuth URI.

Instead, do the redirect entirely on the client.

Chrome's rationale here isn't obvious, so we may be able to revert this at some point.

Test Plan: Went through the OAuth flow locally, was redirected on the client. Will verify in production.

Maniphest Tasks: T13099

Differential Revision: https://secure.phabricator.com/D19177
---
 resources/celerity/map.php                    |  6 ++++
 .../PhabricatorOAuthServerAuthController.php  | 30 +++++++++++++++++--
 webroot/rsrc/js/core/behavior-redirect.js     |  9 ++++++
 3 files changed, 42 insertions(+), 3 deletions(-)
 create mode 100644 webroot/rsrc/js/core/behavior-redirect.js

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index f601b5cec3..f90e3bd511 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -502,6 +502,7 @@ return array(
     'rsrc/js/core/behavior-phabricator-nav.js' => '836f966d',
     'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => 'acd29eee',
     'rsrc/js/core/behavior-read-only-warning.js' => 'ba158207',
+    'rsrc/js/core/behavior-redirect.js' => '0213259f',
     'rsrc/js/core/behavior-refresh-csrf.js' => 'ab2f381b',
     'rsrc/js/core/behavior-remarkup-preview.js' => '4b700e9e',
     'rsrc/js/core/behavior-reorder-applications.js' => '76b9fc3e',
@@ -686,6 +687,7 @@ return array(
     'javelin-behavior-project-create' => '065227cc',
     'javelin-behavior-quicksand-blacklist' => '7927a7d3',
     'javelin-behavior-read-only-warning' => 'ba158207',
+    'javelin-behavior-redirect' => '0213259f',
     'javelin-behavior-refresh-csrf' => 'ab2f381b',
     'javelin-behavior-releeph-preview-branch' => 'b2b4fbaf',
     'javelin-behavior-releeph-request-state-change' => 'a0b57eb8',
@@ -934,6 +936,10 @@ return array(
       'javelin-dom',
       'phabricator-keyboard-shortcut',
     ),
+    '0213259f' => array(
+      'javelin-behavior',
+      'javelin-uri',
+    ),
     '04b2ae03' => array(
       'javelin-install',
       'javelin-util',
diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
index b9d916e82b..8fdd6fdb75 100644
--- a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
+++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
@@ -172,9 +172,33 @@ final class PhabricatorOAuthServerAuthController
         ));
 
       if ($client->getIsTrusted()) {
-        return id(new AphrontRedirectResponse())
-          ->setIsExternal(true)
-          ->setURI((string)$full_uri);
+        // NOTE: See T13099. We currently emit a "Content-Security-Policy"
+        // which includes a narrow "form-action". At the time of writing,
+        // Chrome applies "form-action" to redirects following form submission.
+
+        // This can lead to a situation where a user enters the OAuth workflow
+        // and is prompted for MFA. When they submit an MFA response, the form
+        // can redirect here, and Chrome will block the "Location" redirect.
+
+        // To avoid this, render an interstitial. We only actually need to do
+        // this in Chrome (but do it everywhere for consistency) and only need
+        // to do it if the request is a redirect after a form submission (but
+        // we can't tell if it is or not).
+
+        Javelin::initBehavior(
+          'redirect',
+          array(
+            'uri' => (string)$full_uri,
+          ));
+
+        return $this->newDialog()
+          ->setTitle(pht('Authenticate: %s', $name))
+          ->setRedirect(true)
+          ->appendParagraph(
+            pht(
+              'Authorization for "%s" confirmed, redirecting...',
+              phutil_tag('strong', array(), $name)))
+          ->addCancelButton((string)$full_uri, pht('Continue'));
       }
 
       // TODO: It would be nice to give the user more options here, like
diff --git a/webroot/rsrc/js/core/behavior-redirect.js b/webroot/rsrc/js/core/behavior-redirect.js
new file mode 100644
index 0000000000..4f6e234c0a
--- /dev/null
+++ b/webroot/rsrc/js/core/behavior-redirect.js
@@ -0,0 +1,9 @@
+/**
+ * @provides javelin-behavior-redirect
+ * @requires javelin-behavior
+ *           javelin-uri
+ */
+
+JX.behavior('redirect', function(config) {
+  JX.$U(config.uri).go();
+});