From 281b0741f741a7df17efddf18bca067af0a8f506 Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Thu, 12 Dec 2019 09:18:51 +0900 Subject: [PATCH] fix: name and expirationDate should be optional when setting cookie (#21454) (#21481) * fix: correctly set cookie date * fix: name is not required for setting cookie * test: clear cookie after each cookie test * test: should test session property * chore: style fixes --- shell/browser/api/atom_api_cookies.cc | 42 ++++++++++----------------- spec-main/api-net-spec.ts | 3 +- spec-main/api-session-spec.js | 39 +++++++++++++++++++++++-- 3 files changed, 54 insertions(+), 30 deletions(-) diff --git a/shell/browser/api/atom_api_cookies.cc b/shell/browser/api/atom_api_cookies.cc index 9bc8081d8516a..ace10c9bc641a 100644 --- a/shell/browser/api/atom_api_cookies.cc +++ b/shell/browser/api/atom_api_cookies.cc @@ -141,6 +141,15 @@ void FilterCookieWithStatuses(const base::Value& filter, net::cookie_util::StripStatuses(list)); } +// Parse dictionary property to CanonicalCookie time correctly. +base::Time ParseTimeProperty(const base::Optional& value) { + if (!value) // empty time means ignoring the parameter + return base::Time(); + if (*value == 0) // FromDoubleT would convert 0 to empty Time + return base::Time::UnixEpoch(); + return base::Time::FromDoubleT(*value); +} + std::string InclusionStatusToString( net::CanonicalCookie::CookieInclusionStatus status) { if (status.HasExclusionReason( @@ -243,21 +252,6 @@ v8::Local Cookies::Set(const base::DictionaryValue& details) { const std::string* path = details.FindStringKey("path"); bool secure = details.FindBoolKey("secure").value_or(false); bool http_only = details.FindBoolKey("httpOnly").value_or(false); - base::Optional creation_date = details.FindDoubleKey("creationDate"); - base::Optional expiration_date = - details.FindDoubleKey("expirationDate"); - base::Optional last_access_date = - details.FindDoubleKey("lastAccessDate"); - - base::Time creation_time = creation_date - ? base::Time::FromDoubleT(*creation_date) - : base::Time::UnixEpoch(); - base::Time expiration_time = expiration_date - ? base::Time::FromDoubleT(*expiration_date) - : base::Time::UnixEpoch(); - base::Time last_access_time = last_access_date - ? base::Time::FromDoubleT(*last_access_date) - : base::Time::UnixEpoch(); GURL url(url_string ? *url_string : ""); if (!url.is_valid()) { @@ -268,18 +262,14 @@ v8::Local Cookies::Set(const base::DictionaryValue& details) { return handle; } - if (!name || name->empty()) { - promise.RejectWithErrorMessage( - InclusionStatusToString(net::CanonicalCookie::CookieInclusionStatus( - net::CanonicalCookie::CookieInclusionStatus:: - EXCLUDE_FAILURE_TO_STORE))); - return handle; - } - auto canonical_cookie = net::CanonicalCookie::CreateSanitizedCookie( - url, *name, value ? *value : "", domain ? *domain : "", path ? *path : "", - creation_time, expiration_time, last_access_time, secure, http_only, - net::CookieSameSite::NO_RESTRICTION, net::COOKIE_PRIORITY_DEFAULT); + url, name ? *name : "", value ? *value : "", domain ? *domain : "", + path ? *path : "", + ParseTimeProperty(details.FindDoubleKey("creationDate")), + ParseTimeProperty(details.FindDoubleKey("expirationDate")), + ParseTimeProperty(details.FindDoubleKey("lastAccessDate")), secure, + http_only, net::CookieSameSite::NO_RESTRICTION, + net::COOKIE_PRIORITY_DEFAULT); if (!canonical_cookie || !canonical_cookie->IsCanonical()) { promise.RejectWithErrorMessage( InclusionStatusToString(net::CanonicalCookie::CookieInclusionStatus( diff --git a/spec-main/api-net-spec.ts b/spec-main/api-net-spec.ts index f0fcd57c96be1..8d17b6533e590 100644 --- a/spec-main/api-net-spec.ts +++ b/spec-main/api-net-spec.ts @@ -591,7 +591,8 @@ describe('net module', () => { customSession.cookies.set({ url: `${serverUrl}`, name: 'test', - value: '11111' + value: '11111', + expirationDate: 0 }).then(() => { // resolved const urlRequest = net.request({ method: 'GET', diff --git a/spec-main/api-session-spec.js b/spec-main/api-session-spec.js index 4471d42f0d86f..10946f4c72fb1 100644 --- a/spec-main/api-session-spec.js +++ b/spec-main/api-session-spec.js @@ -59,6 +59,15 @@ describe('session module', () => { const value = '0' afterEach(closeAllWindows) + // Clear cookie of defaultSession after each test. + afterEach(async () => { + const { cookies } = session.defaultSession + const cs = await cookies.get({ url }) + for (const c of cs) { + await cookies.remove(url, c.name) + } + }) + it('should get cookies', async () => { const server = http.createServer((req, res) => { res.setHeader('Set-Cookie', [`${name}=${value}`]) @@ -79,9 +88,33 @@ describe('session module', () => { const name = '1' const value = '1' - await cookies.set({ url, name, value, expirationDate: (+new Date) / 1000 + 120 }) - const cs = await cookies.get({ url }) - expect(cs.some(c => c.name === name && c.value === value)).to.equal(true) + await cookies.set({ url, name, value, expirationDate: (+new Date()) / 1000 + 120 }) + const c = (await cookies.get({ url }))[0] + expect(c.name).to.equal(name) + expect(c.value).to.equal(value) + expect(c.session).to.equal(false) + }) + + it('sets session cookies', async () => { + const { cookies } = session.defaultSession + const name = '2' + const value = '1' + + await cookies.set({ url, name, value }) + const c = (await cookies.get({ url }))[0] + expect(c.name).to.equal(name) + expect(c.value).to.equal(value) + expect(c.session).to.equal(true) + }) + + it('sets cookies without name', async () => { + const { cookies } = session.defaultSession + const value = '3' + + await cookies.set({ url, value }) + const c = (await cookies.get({ url }))[0] + expect(c.name).to.be.empty() + expect(c.value).to.equal(value) }) it('gets cookies without url', async () => {