From 49096c23591ea88b3bba465c405281c526a9492b Mon Sep 17 00:00:00 2001
From: Shelley Vohr <shelley.vohr@gmail.com>
Date: Thu, 25 Jul 2019 10:20:02 -0700
Subject: [PATCH] fix: throw on invalid webRequest filters (#19337)

Closes #11371.

Previously, we didn't consider the return value of the webRequest URLPattern mate converter, which meant that when the pattern wasn't correctly parsed owing to invalid filter specification users would not be made aware of that fact and would just think that the filtering itself had failed. This corrects that error by moving the business logic of url pattern parsing out of the converter and into the function itself so that granular and specific errors can be thrown.

There's also no real reason that i'm aware of not to allow wider breadth of filters by letting users use a wildcard for effective TLD, so I also overrode that (default for the 1-arg Parse is not to allow that).

Finally, I added some examples of url filter types for users to reference.
---
 docs/api/web-request.md                   | 15 +++++++++
 shell/browser/api/atom_api_web_request.cc | 38 ++++++++++++-----------
 spec-main/api-net-spec.ts                 | 25 +++++++++++++++
 3 files changed, 60 insertions(+), 18 deletions(-)

diff --git a/docs/api/web-request.md b/docs/api/web-request.md
index 31e9e3649451..502da81301d5 100644
--- a/docs/api/web-request.md
+++ b/docs/api/web-request.md
@@ -68,6 +68,21 @@ The `uploadData` is an array of `UploadData` objects.
 
 The `callback` has to be called with an `response` object.
 
+Some examples of valid `urls`:
+
+```js
+'http://foo:1234/'
+'http://foo.com/'
+'http://foo:1234/bar'
+'*://*/*'
+'*://example.com/*'
+'*://example.com/foo/*'
+'http://*.foo:1234/'
+'file://foo:1234/bar'
+'http://foo:*/'
+'*://www.foo.com/'
+```
+
 #### `webRequest.onBeforeSendHeaders([filter, ]listener)`
 
 * `filter` Object (optional)
diff --git a/shell/browser/api/atom_api_web_request.cc b/shell/browser/api/atom_api_web_request.cc
index 7fb60bba4f8a..762b9cb91185 100644
--- a/shell/browser/api/atom_api_web_request.cc
+++ b/shell/browser/api/atom_api_web_request.cc
@@ -4,6 +4,7 @@
 
 #include "shell/browser/api/atom_api_web_request.h"
 
+#include <set>
 #include <string>
 #include <utility>
 
@@ -20,23 +21,6 @@
 
 using content::BrowserThread;
 
-namespace mate {
-
-template <>
-struct Converter<URLPattern> {
-  static bool FromV8(v8::Isolate* isolate,
-                     v8::Local<v8::Value> val,
-                     URLPattern* out) {
-    std::string pattern;
-    if (!ConvertFromV8(isolate, val, &pattern))
-      return false;
-    *out = URLPattern(URLPattern::SCHEME_ALL);
-    return out->Parse(pattern) == URLPattern::ParseResult::kSuccess;
-  }
-};
-
-}  // namespace mate
-
 namespace electron {
 
 namespace api {
@@ -84,7 +68,25 @@ void WebRequest::SetListener(Method method, Event type, mate::Arguments* args) {
   // { urls }.
   URLPatterns patterns;
   mate::Dictionary dict;
-  args->GetNext(&dict) && dict.Get("urls", &patterns);
+  std::set<std::string> filter_patterns;
+
+  if (args->GetNext(&dict) && !dict.Get("urls", &filter_patterns)) {
+    args->ThrowError(
+        "onBeforeRequest parameter 'filter' must have property 'urls'.");
+    return;
+  }
+
+  URLPattern pattern(URLPattern::SCHEME_ALL);
+  for (const std::string& filter_pattern : filter_patterns) {
+    const URLPattern::ParseResult result = pattern.Parse(filter_pattern);
+    if (result == URLPattern::ParseResult::kSuccess) {
+      patterns.insert(pattern);
+    } else {
+      const char* error_type = URLPattern::GetParseResultString(result);
+      args->ThrowError("Invalid url pattern " + filter_pattern + ": " +
+                       error_type);
+    }
+  }
 
   // Function or null.
   v8::Local<v8::Value> value;
diff --git a/spec-main/api-net-spec.ts b/spec-main/api-net-spec.ts
index c1863225d8a7..d196b98b7708 100644
--- a/spec-main/api-net-spec.ts
+++ b/spec-main/api-net-spec.ts
@@ -633,6 +633,31 @@ describe('net module', () => {
         session.defaultSession.webRequest.onBeforeRequest(null)
       })
 
+      it('Should throw when invalid filters are passed', () => {
+        expect(() => {
+          session.defaultSession.webRequest.onBeforeRequest(
+            { urls: ['*://www.googleapis.com'] },
+            (details, callback) => { callback({ cancel: false }) }
+          )
+        }).to.throw('Invalid url pattern *://www.googleapis.com: Empty path.')
+
+        expect(() => {
+          session.defaultSession.webRequest.onBeforeRequest(
+            { urls: [ '*://www.googleapis.com/', '*://blahblah.dev' ] },
+            (details, callback) => { callback({ cancel: false }) }
+          )
+        }).to.throw('Invalid url pattern *://blahblah.dev: Empty path.')
+      })
+
+      it('Should not throw when valid filters are passed', () => {
+        expect(() => {
+          session.defaultSession.webRequest.onBeforeRequest(
+            { urls: ['*://www.googleapis.com/'] },
+            (details, callback) => { callback({ cancel: false }) }
+          )
+        }).to.not.throw()
+      })
+
       it('Requests should be intercepted by webRequest module', (done) => {
         const requestUrl = '/requestUrl'
         const redirectUrl = '/redirectUrl'