feat: add will-redirect (#13866)

* feat: add will-redirect to allow people to prevent 30X redirects * spec: add tests for the will-redirect event * refactor: implement will-redirect using NavigationThrottle instead of PostTask This avoids a potential race condition and immediately cancels the navigation * docs: add docs for did-redirect-navigation * refactor: move AtomNavigationThrottle out of net folder * refactor: update header guard for atom_navigation_throttle.h * refactor: fix chromium style errors in the GN build * refactor: update throttle impl to NOTREACHED and std::make_unqique
electron · Sep 15, 2018 · 7065093 · 7065093
1 parent 6ad8583
commit 7065093
Show file tree

Hide file tree

Showing 10 changed files with 222 additions and 3 deletions.
diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc
@@ -16,6 +16,7 @@
 #include "atom/browser/atom_browser_context.h"
 #include "atom/browser/atom_browser_main_parts.h"
 #include "atom/browser/atom_javascript_dialog_manager.h"
+#include "atom/browser/atom_navigation_throttle.h"
 #include "atom/browser/child_web_contents_tracker.h"
 #include "atom/browser/lib/bluetooth_chooser.h"
 #include "atom/browser/native_window.h"
@@ -845,7 +846,8 @@ void WebContents::DidStopLoading() {
   Emit("did-stop-loading");
 }
 
-void WebContents::DidStartNavigation(
+bool WebContents::EmitNavigationEvent(
+    const std::string& event,
     content::NavigationHandle* navigation_handle) {
   bool is_main_frame = navigation_handle->IsInMainFrame();
   int frame_tree_node_id = navigation_handle->GetFrameTreeNodeId();
@@ -866,8 +868,18 @@ void WebContents::DidStartNavigation(
   }
   bool is_same_document = navigation_handle->IsSameDocument();
   auto url = navigation_handle->GetURL();
-  Emit("did-start-navigation", url, is_same_document, is_main_frame,
-       frame_process_id, frame_routing_id);
+  return Emit(event, url, is_same_document, is_main_frame, frame_process_id,
+              frame_routing_id);
+}
+
+void WebContents::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  EmitNavigationEvent("did-start-navigation", navigation_handle);
+}
+
+void WebContents::DidRedirectNavigation(
+    content::NavigationHandle* navigation_handle) {
+  EmitNavigationEvent("did-redirect-navigation", navigation_handle);
 }
 
 void WebContents::DidFinishNavigation(

diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h
@@ -266,6 +266,9 @@ class WebContents : public mate::TrackableObject<WebContents>,
     observers_.RemoveObserver(obs);
   }
 
+  bool EmitNavigationEvent(const std::string& event,
+                           content::NavigationHandle* navigation_handle);
+
  protected:
   WebContents(v8::Isolate* isolate,
               content::WebContents* web_contents,
@@ -366,6 +369,8 @@ class WebContents : public mate::TrackableObject<WebContents>,
   void DidStopLoading() override;
   void DidStartNavigation(
       content::NavigationHandle* navigation_handle) override;
+  void DidRedirectNavigation(
+      content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
   bool OnMessageReceived(const IPC::Message& message) override;

diff --git a/atom/browser/atom_browser_client.cc b/atom/browser/atom_browser_client.cc
@@ -16,6 +16,7 @@
 #include "atom/browser/api/atom_api_web_contents.h"
 #include "atom/browser/atom_browser_context.h"
 #include "atom/browser/atom_browser_main_parts.h"
+#include "atom/browser/atom_navigation_throttle.h"
 #include "atom/browser/atom_quota_permission_context.h"
 #include "atom/browser/atom_resource_dispatcher_host_delegate.h"
 #include "atom/browser/atom_speech_recognition_manager_delegate.h"
@@ -613,4 +614,12 @@ bool AtomBrowserClient::HandleExternalProtocol(
   return true;
 }
 
+std::vector<std::unique_ptr<content::NavigationThrottle>>
+AtomBrowserClient::CreateThrottlesForNavigation(
+    content::NavigationHandle* handle) {
+  std::vector<std::unique_ptr<content::NavigationThrottle>> throttles;
+  throttles.push_back(std::make_unique<AtomNavigationThrottle>(handle));
+  return throttles;
+}
+
 }  // namespace atom
diff --git a/atom/browser/atom_browser_client.h b/atom/browser/atom_browser_client.h
@@ -47,6 +47,9 @@ class AtomBrowserClient : public brightray::BrowserClient,
   static void SetCustomServiceWorkerSchemes(
       const std::vector<std::string>& schemes);
 
+  std::vector<std::unique_ptr<content::NavigationThrottle>>
+  CreateThrottlesForNavigation(content::NavigationHandle* handle) override;
+
  protected:
   // content::ContentBrowserClient:
   void RenderProcessWillLaunch(

diff --git a/atom/browser/atom_navigation_throttle.cc b/atom/browser/atom_navigation_throttle.cc
@@ -0,0 +1,44 @@
+// Copyright (c) 2015 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#include "atom/browser/atom_navigation_throttle.h"
+
+#include "atom/browser/api/atom_api_web_contents.h"
+#include "content/public/browser/navigation_handle.h"
+
+namespace atom {
+
+AtomNavigationThrottle::AtomNavigationThrottle(
+    content::NavigationHandle* navigation_handle)
+    : content::NavigationThrottle(navigation_handle) {}
+
+AtomNavigationThrottle::~AtomNavigationThrottle() {}
+
+const char* AtomNavigationThrottle::GetNameForLogging() {
+  return "AtomNavigationThrottle";
+}
+
+content::NavigationThrottle::ThrottleCheckResult
+AtomNavigationThrottle::WillRedirectRequest() {
+  auto* handle = navigation_handle();
+  auto* contents = handle->GetWebContents();
+  if (!contents) {
+    NOTREACHED();
+    return PROCEED;
+  }
+
+  auto api_contents =
+      atom::api::WebContents::CreateFrom(v8::Isolate::GetCurrent(), contents);
+  if (api_contents.IsEmpty()) {
+    NOTREACHED();
+    return PROCEED;
+  }
+
+  if (api_contents->EmitNavigationEvent("will-redirect", handle)) {
+    return CANCEL;
+  }
+  return PROCEED;
+}
+
+}  // namespace atom
diff --git a/atom/browser/atom_navigation_throttle.h b/atom/browser/atom_navigation_throttle.h
@@ -0,0 +1,27 @@
+// Copyright (c) 2018 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#ifndef ATOM_BROWSER_ATOM_NAVIGATION_THROTTLE_H_
+#define ATOM_BROWSER_ATOM_NAVIGATION_THROTTLE_H_
+
+#include "content/public/browser/navigation_throttle.h"
+
+namespace atom {
+
+class AtomNavigationThrottle : public content::NavigationThrottle {
+ public:
+  explicit AtomNavigationThrottle(content::NavigationHandle* handle);
+  ~AtomNavigationThrottle() override;
+
+  AtomNavigationThrottle::ThrottleCheckResult WillRedirectRequest() override;
+
+  const char* GetNameForLogging() override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AtomNavigationThrottle);
+};
+
+}  // namespace atom
+
+#endif  // ATOM_BROWSER_ATOM_NAVIGATION_THROTTLE_H_
diff --git a/docs/api/web-contents.md b/docs/api/web-contents.md
@@ -170,6 +170,7 @@ Calling `event.preventDefault()` will prevent the navigation.
 
 Returns:
 
+* `event` Event
 * `url` String
 * `isInPlace` Boolean
 * `isMainFrame` Boolean
@@ -179,6 +180,43 @@ Returns:
 Emitted when any frame (including main) starts navigating. `isInplace` will be
 `true` for in-page navigations.
 
+#### Event: 'will-redirect'
+
+Returns:
+
+* `event` Event
+* `url` String
+* `isInPlace` Boolean
+* `isMainFrame` Boolean
+* `frameProcessId` Integer
+* `frameRoutingId` Integer
+
+Emitted as a server side redirect occurs during navigation.  For example a 302
+redirect.
+
+This event will be emitted after `did-start-navigation` and always before the
+`did-redirect-navigation` event for the same navigation.
+
+Calling `event.preventDefault()` will prevent the navigation (not just the
+redirect).
+
+#### Event: 'did-redirect-navigation'
+
+Returns:
+
+* `event` Event
+* `url` String
+* `isInPlace` Boolean
+* `isMainFrame` Boolean
+* `frameProcessId` Integer
+* `frameRoutingId` Integer
+
+Emitted after a server side redirect occurs during navigation.  For example a 302
+redirect.
+
+This event can not be prevented, if you want to prevent redirects you should
+checkout out the `will-redirect` event above.
+
 #### Event: 'did-navigate'
 
 Returns:

diff --git a/filenames.gni b/filenames.gni
@@ -209,6 +209,8 @@ filenames = {
     "atom/browser/atom_browser_main_parts_posix.cc",
     "atom/browser/atom_javascript_dialog_manager.cc",
     "atom/browser/atom_javascript_dialog_manager.h",
+    "atom/browser/atom_navigation_throttle.h",
+    "atom/browser/atom_navigation_throttle.cc",
     "atom/browser/atom_permission_manager.cc",
     "atom/browser/atom_permission_manager.h",
     "atom/browser/atom_quota_permission_context.cc",

diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js
@@ -84,6 +84,12 @@ describe('BrowserWindow module', () => {
               }
             })
           })
+        } else if (req.url === '/302') {
+          res.setHeader('Location', '/200')
+          res.statusCode = 302
+          res.end()
+        } else if (req.url === '/navigate-302') {
+          res.end(`<html><body><script>window.location='${server.url}/302'</script></body></html>`)
         } else {
           res.end()
         }
@@ -344,6 +350,63 @@ describe('BrowserWindow module', () => {
     })
   })
 
+  describe('will-redirect event', () => {
+    it('is emitted on redirects', (done) => {
+      w.webContents.on('will-redirect', (event, url) => {
+        done()
+      })
+      w.loadURL(`${server.url}/302`)
+    })
+
+    it('is emitted after will-navigate on redirects', (done) => {
+      let navigateCalled = false
+      w.loadURL(`${server.url}/navigate-302`)
+      w.webContents.on('will-navigate', () => {
+        navigateCalled = true
+      })
+      w.webContents.on('will-redirect', (event, url) => {
+        expect(navigateCalled).to.equal(true, 'should have called will-navigate first')
+        done()
+      })
+    })
+
+    it('is emitted before did-stop-loading on redirects', (done) => {
+      let stopCalled = false
+      w.webContents.on('did-stop-loading', () => {
+        stopCalled = true
+      })
+      w.webContents.on('will-redirect', (event, url) => {
+        expect(stopCalled).to.equal(false, 'should not have called did-stop-loading first')
+        done()
+      })
+      w.loadURL(`${server.url}/302`)
+    })
+
+    it('allows the window to be closed from the event listener', (done) => {
+      ipcRenderer.send('close-on-will-redirect', w.id)
+      ipcRenderer.once('closed-on-will-redirect', () => { done() })
+      w.loadURL(`${server.url}/302`)
+    })
+
+    it('can be prevented', (done) => {
+      ipcRenderer.send('prevent-will-redirect', w.id)
+      w.webContents.on('will-navigate', (e, url) => {
+        expect(url).to.equal(`${server.url}/302`)
+      })
+      w.webContents.on('did-stop-loading', () => {
+        expect(w.webContents.getURL()).to.equal(
+          `${server.url}/navigate-302`,
+          'url should not have changed after navigation event'
+        )
+        done()
+      })
+      w.webContents.on('will-redirect', (e, url) => {
+        expect(url).to.equal(`${server.url}/200`)
+      })
+      w.loadURL(`${server.url}/navigate-302`)
+    })
+  })
+
   describe('BrowserWindow.show()', () => {
     before(function () {
       if (isCI) {

diff --git a/spec/static/main.js b/spec/static/main.js
@@ -265,6 +265,22 @@ ipcMain.on('close-on-will-navigate', (event, id) => {
   })
 })
 
+ipcMain.on('close-on-will-redirect', (event, id) => {
+  const contents = event.sender
+  const window = BrowserWindow.fromId(id)
+  window.webContents.once('will-redirect', (event, input) => {
+    window.close()
+    contents.send('closed-on-will-redirect')
+  })
+})
+
+ipcMain.on('prevent-will-redirect', (event, id) => {
+  const window = BrowserWindow.fromId(id)
+  window.webContents.once('will-redirect', (event) => {
+    event.preventDefault()
+  })
+})
+
 ipcMain.on('create-window-with-options-cycle', (event) => {
   // This can't be done over remote since cycles are already
   // nulled out at the IPC layer