feat: add will-redirect #13866

MarshallOfSound · 2018-07-30T04:39:46Z

Example usage:

mainWindow.webContents.on('will-redirect', (e, url) => {
  if (!testIsValidUrl(url))
    e.preventDefault()
})

TODO:

Tests

Notes: added a will-redirect event on webContents to capture, handle and prevent redirects during navigations

MarshallOfSound · 2018-07-30T05:21:21Z

Please do not merge until after the meeting on Monday 30th

felixrieseberg · 2018-07-30T05:35:39Z

Well that was easy - I had no idea Chrome already has an event, but of course it does. 👏 for sprinting into action, this seems like a very clean solution.

nornagon · 2018-07-30T23:42:52Z

atom/browser/api/atom_api_web_contents.cc

+  if (EmitNavigationEvent("will-redirect", navigation_handle)) {
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner(
+        base::ThreadTaskRunnerHandle::Get());
+    task_runner->PostTask(FROM_HERE, base::BindOnce(&OnStopSoon, this));


Does the asynchronous nature of this leave a window of opportunity for the redirect to start happening? It'd be awesome if there was a way to guarantee that not a single byte of network traffic leaves to the domain of the redirect's target.

It'd be awesome if there was a way to guarantee that not a single byte of network traffic leaves to the domain of the redirect's target.

Its possible by registering navigation throttlers https://cs.chromium.org/chromium/src/content/public/browser/navigation_throttle.h

MarshallOfSound · 2018-08-01T07:54:55Z

@nornagon @deepak1556 I re-implemented this event and the prevention logic using NavigationThrottle. Should be no chance for race conditions now 👍

deepak1556

LGTM, with some minor changes. Thanks!

deepak1556 · 2018-08-01T15:36:40Z

atom/browser/atom_browser_client.cc

+AtomBrowserClient::CreateThrottlesForNavigation(
+    content::NavigationHandle* handle) {
+  std::vector<std::unique_ptr<content::NavigationThrottle>> throttles;
+  throttles.push_back(base::WrapUnique(new AtomNavigationThrottle(handle)));


Prefer std::make_unique

Call me crazy, but std::make_unique didn't compile ❓ and the equivalent CreateThrottlesForNavigation inside chromium uses the base helper 🤔 I can look into changing it tomorrow if it's a big thing

Call me crazy, but std::make_unique didn't compile

Hmm it should work.

and the equivalent CreateThrottlesForNavigation inside chromium uses the base helper

yeah in this case its fine using either, but prefer make_unique for creation of unique_ptr, More context from chromium style guide https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer-std::make_unique-to-base::WrapUnique

Would still like to see this resolved

deepak1556 · 2018-08-01T15:39:22Z

atom/browser/net/atom_navigation_throttle.cc

+  auto* handle = navigation_handle();
+  auto* contents = handle->GetWebContents();
+  if (!contents)
+    return PROCEED;


Should we let the navigation proceed if there are no web contents ? This case should definitely not happen, but in the rarest chance it occurs we should terminate the navigation otherwise potential for use-after-free bugs.

Yeah this is more like a NOREACHED() but I thought I'd handle it for safety. Current behavior where we have no throttles would be for the navigation to proceed so I'd like to maintain that behavior. Might be worth adding a DCHECK or something there so that it would cause failures in debug mode though?

Current behavior where we have no throttles would be for the navigation to proceed so I'd like to maintain that behavior

👍

deepak1556 · 2018-08-01T15:40:03Z

atom/browser/net/atom_navigation_throttle.cc

+  auto api_contents =
+      atom::api::WebContents::CreateFrom(v8::Isolate::GetCurrent(), contents);
+  if (api_contents.IsEmpty())
+    return PROCEED;


Navigation should be terminated for the same reason mentioned above.

deepak1556 · 2018-08-01T15:40:40Z

atom/browser/net/atom_navigation_throttle.h

+
+#include "content/public/browser/navigation_throttle.h"
+
+class URLPattern;


Nit: Unused declaration.

deepak1556 · 2018-08-01T16:57:45Z

atom/browser/net/atom_navigation_throttle.h

+
+  AtomNavigationThrottle::ThrottleCheckResult WillRedirectRequest() override;
+
+  const char* GetNameForLogging() { return "AtomNavigationThrottle"; }


const char* GetNameForLogging() override { }

deepak1556 · 2018-08-01T17:00:38Z

atom/browser/api/atom_api_web_contents.cc

@@ -27,6 +27,7 @@
 #include "atom/browser/child_web_contents_tracker.h"
 #include "atom/browser/lib/bluetooth_chooser.h"
 #include "atom/browser/native_window.h"
+#include "atom/browser/net/atom_navigation_throttle.h"


Can you move the atom_navigation_throttle under atom/browser, the throttler is entirely used on the UI thread and it doesn't directly handle any component from //net of chromium.

nornagon

I think the DCHECKs should be CHECKs. It's scary to me that those supposedly-never-reached conditions result in allowing the navigation to occur. Safer to CHECK, if they're truly never reached.

nornagon · 2018-08-08T21:56:47Z

atom/browser/atom_browser_client.cc

+AtomBrowserClient::CreateThrottlesForNavigation(
+    content::NavigationHandle* handle) {
+  std::vector<std::unique_ptr<content::NavigationThrottle>> throttles;
+  throttles.push_back(base::WrapUnique(new AtomNavigationThrottle(handle)));


Would still like to see this resolved

nornagon · 2018-08-08T22:01:32Z

atom/browser/atom_navigation_throttle.cc

+  auto* handle = navigation_handle();
+  auto* contents = handle->GetWebContents();
+  if (!contents) {
+    DCHECK(false);  // This should be unreachable


CHECK(false) then?

The reason they are DCHECK is to maintain existing behavior in release builds (see the comment chain above). Although this should be unreachable, in it's current state Electron would PROCEED with the navigation.

ok. BTW NOTREACHED() exists and should be used here (it just DCHECK(false)es, but it documents itself and is greppable)

nornagon · 2018-08-08T22:01:38Z

atom/browser/atom_navigation_throttle.cc

+  auto api_contents =
+      atom::api::WebContents::CreateFrom(v8::Isolate::GetCurrent(), contents);
+  if (api_contents.IsEmpty()) {
+    DCHECK(false);  // This should be unreachable


… PostTask This avoids a potential race condition and immediately cancels the navigation

MarshallOfSound · 2018-09-15T14:54:10Z

@nornagon This has been updated and I even made std::make_unique work, idk why it wasn't before but it does now 😆

Assuming CI is green I'm gonna merge this in 👍

release-clerk · 2018-09-15T15:42:45Z

Release Notes Persisted

added a will-redirect event on webContents to capture, handle and prevent redirects during navigations

MarshallOfSound requested review from a team and felixrieseberg July 30, 2018 04:39

MarshallOfSound force-pushed the feat/will-redirect branch from 47055b2 to f483586 Compare July 30, 2018 04:39

MarshallOfSound requested a review from a team July 30, 2018 04:39

MarshallOfSound changed the title ~~feat: add will-redirect~~ [wip] feat: add will-redirect Jul 30, 2018

MarshallOfSound changed the title ~~[wip] feat: add will-redirect~~ feat: add will-redirect Jul 30, 2018

ckerr added the semver/minor backwards-compatible functionality label Jul 30, 2018

nornagon requested changes Jul 30, 2018

View reviewed changes

MarshallOfSound force-pushed the feat/will-redirect branch from 97d4dc6 to aba6d24 Compare August 1, 2018 07:51

deepak1556 reviewed Aug 1, 2018

View reviewed changes

nornagon requested changes Aug 8, 2018

View reviewed changes

nornagon approved these changes Aug 10, 2018

View reviewed changes

MarshallOfSound added 6 commits September 16, 2018 00:34

feat: add will-redirect to allow people to prevent 30X redirects

424477f

spec: add tests for the will-redirect event

48a16de

refactor: implement will-redirect using NavigationThrottle instead of…

c8eedd9

… PostTask This avoids a potential race condition and immediately cancels the navigation

docs: add docs for did-redirect-navigation

82ea3b9

refactor: move AtomNavigationThrottle out of net folder

f96dad4

refactor: update header guard for atom_navigation_throttle.h

4b9e1e4

MarshallOfSound force-pushed the feat/will-redirect branch from e8e9cb3 to 4b9e1e4 Compare September 15, 2018 14:46

MarshallOfSound added 2 commits September 16, 2018 00:47

refactor: fix chromium style errors in the GN build

d01e84e

refactor: update throttle impl to NOTREACHED and std::make_unqique

4b86058

MarshallOfSound merged commit 7065093 into master Sep 15, 2018

MarshallOfSound deleted the feat/will-redirect branch September 15, 2018 15:42

ckerr mentioned this pull request Dec 18, 2018

Draft 4.0.0 Release Notes #16117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add will-redirect #13866

feat: add will-redirect #13866

MarshallOfSound commented Jul 30, 2018 •

edited

MarshallOfSound commented Jul 30, 2018

felixrieseberg commented Jul 30, 2018

nornagon Jul 30, 2018

deepak1556 Jul 31, 2018 •

edited

MarshallOfSound commented Aug 1, 2018

deepak1556 left a comment

deepak1556 Aug 1, 2018

MarshallOfSound Aug 1, 2018

deepak1556 Aug 1, 2018

nornagon Aug 8, 2018

deepak1556 Aug 1, 2018

MarshallOfSound Aug 1, 2018

deepak1556 Aug 1, 2018

deepak1556 Aug 1, 2018

deepak1556 Aug 1, 2018

deepak1556 Aug 1, 2018

deepak1556 Aug 1, 2018

nornagon left a comment

nornagon Aug 8, 2018

nornagon Aug 8, 2018

MarshallOfSound Aug 10, 2018

nornagon Aug 10, 2018 •

edited

nornagon Aug 8, 2018

MarshallOfSound commented Sep 15, 2018

release-clerk bot commented Sep 15, 2018


		#include "content/public/browser/navigation_throttle.h"

		class URLPattern;


		AtomNavigationThrottle::ThrottleCheckResult WillRedirectRequest() override;

		const char* GetNameForLogging() { return "AtomNavigationThrottle"; }

feat: add will-redirect #13866

feat: add will-redirect #13866

Conversation

MarshallOfSound commented Jul 30, 2018 • edited

MarshallOfSound commented Jul 30, 2018

felixrieseberg commented Jul 30, 2018

Choose a reason for hiding this comment

deepak1556 Jul 31, 2018 • edited

Choose a reason for hiding this comment

MarshallOfSound commented Aug 1, 2018

deepak1556 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nornagon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nornagon Aug 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarshallOfSound commented Sep 15, 2018

release-clerk bot commented Sep 15, 2018

MarshallOfSound commented Jul 30, 2018 •

edited

deepak1556 Jul 31, 2018 •

edited

nornagon Aug 10, 2018 •

edited