From b034bf9ae629bffe10b5fc47213957c7335c7704 Mon Sep 17 00:00:00 2001
From: Erick Zhao <erickzhao@github.com>
Date: Fri, 14 Jun 2019 10:54:32 -0700
Subject: [PATCH] fix: throw error on invalid URLs when setting cookie (#18756)

With this PR, invalid inputs to the url parameter will throw an error when using cookie.set(). This is done by checking if the URL is parseable using GURL rather than checking if the URL string being passed in is empty.

Previously, invalid URLs would be able to be added as a cookie, but you would not be able to filter for them or remove them.
---
 atom/browser/api/atom_api_cookies.cc | 23 +++++++++++++----------
 docs/api/cookies.md                  |  2 +-
 spec/api-session-spec.js             | 14 ++++++++++++++
 3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/atom/browser/api/atom_api_cookies.cc b/atom/browser/api/atom_api_cookies.cc
index a77de4c08670c..fb2b9e6ac35a7 100644
--- a/atom/browser/api/atom_api_cookies.cc
+++ b/atom/browser/api/atom_api_cookies.cc
@@ -226,13 +226,13 @@ void FlushCookieStoreOnIOThread(
 void SetCookieOnIO(scoped_refptr<net::URLRequestContextGetter> getter,
                    std::unique_ptr<base::DictionaryValue> details,
                    util::Promise promise) {
-  std::string url, name, value, domain, path;
+  std::string url_string, name, value, domain, path;
   bool secure = false;
   bool http_only = false;
   double creation_date;
   double expiration_date;
   double last_access_date;
-  details->GetString("url", &url);
+  details->GetString("url", &url_string);
   details->GetString("name", &name);
   details->GetString("value", &value);
   details->GetString("domain", &domain);
@@ -261,21 +261,24 @@ void SetCookieOnIO(scoped_refptr<net::URLRequestContextGetter> getter,
                            : base::Time::FromDoubleT(last_access_date);
   }
 
-  std::unique_ptr<net::CanonicalCookie> canonical_cookie(
-      net::CanonicalCookie::CreateSanitizedCookie(
-          GURL(url), name, value, domain, path, creation_time, expiration_time,
-          last_access_time, secure, http_only,
-          net::CookieSameSite::DEFAULT_MODE, net::COOKIE_PRIORITY_DEFAULT));
   auto completion_callback = base::BindOnce(OnSetCookie, std::move(promise));
-  if (!canonical_cookie || !canonical_cookie->IsCanonical()) {
+  GURL url(url_string);
+  if (!url.is_valid()) {
     std::move(completion_callback).Run(false);
     return;
   }
-  if (url.empty()) {
+
+  if (name.empty()) {
     std::move(completion_callback).Run(false);
     return;
   }
-  if (name.empty()) {
+
+  std::unique_ptr<net::CanonicalCookie> canonical_cookie(
+      net::CanonicalCookie::CreateSanitizedCookie(
+          url, name, value, domain, path, creation_time, expiration_time,
+          last_access_time, secure, http_only,
+          net::CookieSameSite::DEFAULT_MODE, net::COOKIE_PRIORITY_DEFAULT));
+  if (!canonical_cookie || !canonical_cookie->IsCanonical()) {
     std::move(completion_callback).Run(false);
     return;
   }
diff --git a/docs/api/cookies.md b/docs/api/cookies.md
index ea1ada143309a..9455f8ac9f40b 100644
--- a/docs/api/cookies.md
+++ b/docs/api/cookies.md
@@ -104,7 +104,7 @@ with `callback(error, cookies)` on complete.
 #### `cookies.set(details)`
 
 * `details` Object
-  * `url` String - The url to associate the cookie with.
+  * `url` String - The url to associate the cookie with. The promise will be rejected if the url is invalid.
   * `name` String (optional) - The name of the cookie. Empty by default if omitted.
   * `value` String (optional) - The value of the cookie. Empty by default if omitted.
   * `domain` String (optional) - The domain of the cookie; this will be normalized with a preceding dot so that it's also valid for subdomains. Empty by default if omitted.
diff --git a/spec/api-session-spec.js b/spec/api-session-spec.js
index 13a88bef78d15..e8b359f89165f 100644
--- a/spec/api-session-spec.js
+++ b/spec/api-session-spec.js
@@ -145,6 +145,20 @@ describe('session module', () => {
       expect(error).to.have.property('message').which.equals('Setting cookie failed')
     })
 
+    it('yields an error when setting a cookie with an invalid URL', async () => {
+      let error
+      try {
+        const { cookies } = session.defaultSession
+        const name = '1'
+        const value = '1'
+        await cookies.set({ url: 'asdf', name, value })
+      } catch (e) {
+        error = e
+      }
+      expect(error).is.an('Error')
+      expect(error).to.have.property('message').which.equals('Setting cookie failed')
+    })
+
     it('should overwrite previous cookies', async () => {
       let error
       try {