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

@@ -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;
   }

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.

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 {