From ed42f6010384e00bb975f2a5225bc457bb79682d Mon Sep 17 00:00:00 2001 From: Erick Zhao Date: Wed, 12 Jun 2019 14:10:26 -0700 Subject: [PATCH 1/6] fix: Check for invalid URLs with GURL --- atom/browser/api/atom_api_cookies.cc | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/atom/browser/api/atom_api_cookies.cc b/atom/browser/api/atom_api_cookies.cc index a77de4c08670c..2f44b9f9c5094 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 getter, std::unique_ptr 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 getter, : base::Time::FromDoubleT(last_access_date); } - std::unique_ptr 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_empty()) { 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 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)); + auto completion_callback = base::BindOnce(OnSetCookie, std::move(promise)); + if (!canonical_cookie || !canonical_cookie->IsCanonical()) { std::move(completion_callback).Run(false); return; } From ef325f6057f6ac5cbb2494097734387ac2c975df Mon Sep 17 00:00:00 2001 From: Erick Zhao Date: Wed, 12 Jun 2019 14:15:03 -0700 Subject: [PATCH 2/6] docs: Clarify that invalid URLs are no bueno --- docs/api/cookies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/api/cookies.md b/docs/api/cookies.md index ea1ada143309a..3314368605747 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. An error is thrown 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. From 1ce0f3d95a888c291d23a179798619ad246e56ce Mon Sep 17 00:00:00 2001 From: Erick Zhao Date: Wed, 12 Jun 2019 14:39:39 -0700 Subject: [PATCH 3/6] test: Add test to verify that invalid URL throws --- spec/api-session-spec.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) 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 { From c7239c74b8347671495cf2424c8796e494e53777 Mon Sep 17 00:00:00 2001 From: Erick Zhao Date: Wed, 12 Jun 2019 15:27:18 -0700 Subject: [PATCH 4/6] fix: Move variable declaration above usage --- atom/browser/api/atom_api_cookies.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atom/browser/api/atom_api_cookies.cc b/atom/browser/api/atom_api_cookies.cc index 2f44b9f9c5094..5407fa6f4451f 100644 --- a/atom/browser/api/atom_api_cookies.cc +++ b/atom/browser/api/atom_api_cookies.cc @@ -261,6 +261,7 @@ void SetCookieOnIO(scoped_refptr getter, : base::Time::FromDoubleT(last_access_date); } + auto completion_callback = base::BindOnce(OnSetCookie, std::move(promise)); GURL url(url_string); if (url.is_empty()) { std::move(completion_callback).Run(false); @@ -277,7 +278,6 @@ void SetCookieOnIO(scoped_refptr getter, 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()) { std::move(completion_callback).Run(false); return; From 746cdc9acfe8d06d00c0e0f074f0b94858656571 Mon Sep 17 00:00:00 2001 From: Erick Zhao Date: Thu, 13 Jun 2019 09:23:01 -0700 Subject: [PATCH 5/6] fix: is_empty -> is_valid Co-Authored-By: Samuel Attard --- atom/browser/api/atom_api_cookies.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atom/browser/api/atom_api_cookies.cc b/atom/browser/api/atom_api_cookies.cc index 5407fa6f4451f..fb2b9e6ac35a7 100644 --- a/atom/browser/api/atom_api_cookies.cc +++ b/atom/browser/api/atom_api_cookies.cc @@ -263,7 +263,7 @@ void SetCookieOnIO(scoped_refptr getter, auto completion_callback = base::BindOnce(OnSetCookie, std::move(promise)); GURL url(url_string); - if (url.is_empty()) { + if (!url.is_valid()) { std::move(completion_callback).Run(false); return; } From 8e45bff3ef73e91f98ced24317d956be382cbb4f Mon Sep 17 00:00:00 2001 From: Micha Hanselmann Date: Thu, 13 Jun 2019 10:39:50 -0700 Subject: [PATCH 6/6] Update docs/api/cookies.md Co-Authored-By: Samuel Attard --- docs/api/cookies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/api/cookies.md b/docs/api/cookies.md index 3314368605747..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. An error is thrown if the url is invalid. + * `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.