From 7065093869e2942ba9a7f90f2f96b6466df07157 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Sun, 16 Sep 2018 01:42:43 +1000 Subject: [PATCH] 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 --- atom/browser/api/atom_api_web_contents.cc | 18 +++++-- atom/browser/api/atom_api_web_contents.h | 5 ++ atom/browser/atom_browser_client.cc | 9 ++++ atom/browser/atom_browser_client.h | 3 ++ atom/browser/atom_navigation_throttle.cc | 44 ++++++++++++++++ atom/browser/atom_navigation_throttle.h | 27 ++++++++++ docs/api/web-contents.md | 38 ++++++++++++++ filenames.gni | 2 + spec/api-browser-window-spec.js | 63 +++++++++++++++++++++++ spec/static/main.js | 16 ++++++ 10 files changed, 222 insertions(+), 3 deletions(-) create mode 100644 atom/browser/atom_navigation_throttle.cc create mode 100644 atom/browser/atom_navigation_throttle.h diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 4fadf30545a6..d9cdc980d615 100644 --- 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 index 558a1a27adf1..294a82e76193 100644 --- 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, 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, 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 index 5a795bbd2e2f..576da03427ee 100644 --- 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> +AtomBrowserClient::CreateThrottlesForNavigation( + content::NavigationHandle* handle) { + std::vector> throttles; + throttles.push_back(std::make_unique(handle)); + return throttles; +} + } // namespace atom diff --git a/atom/browser/atom_browser_client.h b/atom/browser/atom_browser_client.h index df955ec99c7f..4f28bddb89fa 100644 --- 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& schemes); + std::vector> + 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 new file mode 100644 index 000000000000..bcb0dfe56118 --- /dev/null +++ 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 new file mode 100644 index 000000000000..779258aa14f4 --- /dev/null +++ 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 index f9bc61131774..9570ac904167 100644 --- 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 index 29fc65b92895..1b104ef6ad40 100644 --- 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 index 2d612ba48b01..2e4ebc8130f8 100644 --- 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(``) } 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 index 1948eb8e710c..c1cd975cb2bf 100644 --- 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