add domain to add item #103

JPedroBorges · 2023-08-25T22:26:41Z

Add domain field when creating a new cookie item.

Important

GET

If there are multiples match to a key, it will select based on the order that the browser sent it.
This is not a new behavior, but would be interesting to figure out how is the order affected.

SET

Retrieves information about a single cookie. If more than one cookie of the same name exists for the given URL, the one with the longest path will be returned. For cookies with the same path length, the cookie with the earliest creation time will be returned.
https://developer.chrome.com/docs/extensions/reference/cookies/#method-get

What this is actually saying is even if we request a cookie for a subdomain (subdomain.domain.com) if there are collisions with a same name cookie in the domain (domain.com) it will pick the most recent.

I'm unsure if we should retrieve all the cookies and select the correct subdomain cookie, or if this is a non issue.

Should we dig deeper?

screenshots

create page

view page

bmg13 · 2023-08-26T12:20:35Z

What this is actually saying is even if we request a cookie for a subdomain (subdomain.domain.com) if there are collisions with a same name cookie in the domain (domain.com) it will pick the most recent.

Would in any scenario the most recent not be the one you would want?

bmg13 · 2023-08-26T12:14:12Z

src/handler/operation/cookie.js

-function tabToStringDomain(tab) {
-  const url = new URL(tab.url);
+function tabToStringDomain(inputUrl) {
+  const url = new URL(inputUrl.startsWith("http") ? inputUrl : `http://${inputUrl}`);


just a suggestion of setting a const "http://" and use it twice here, allowing some (although minor) reuse. Would assume no issue when https is used

In this case, we are just concatenating HTTP to form a valid URL in order to validate it with the URL class, so no hard in just doing HTTP.
It makes sense, I will add it as a constant.

bmg13 · 2023-08-26T12:19:04Z

src/handler/operation/cookie.js

+function isDomainMatch(itemDomain, cookieDomain) {
+  return itemDomain === cookieDomain || "." + itemDomain === cookieDomain;
+}


possible misconception from my side, but is the next viable?

Suggested change

function isDomainMatch(itemDomain, cookieDomain) {

return itemDomain === cookieDomain || "." + itemDomain === cookieDomain;

}

function isDomainMatch(itemDomain, cookieDomain) {

return itemDomain === cookieDomain.substring(1);

}

From my experience, we still need to validate the .domain.comand domain.com since I've seen both cases. Although it seems that the extra dot is not necessary.
But using the substring seems a neater approach. Thank you for the suggestion 👍🏻

Marianarodrigues7

From my understanding of your PR, once we set a custom value for a cookie item (without a defined domain), we assume the cookie domain is the same as the current tab.

If so, we should also do something in the apply function to our store. From what I can see, in the setter functions for our store we are just updating the value and not applying the domain.
Although you are doing a domain validation in getCookie, we don't see/have this information in our badges since it's not stored in our storeItems list.

I don't know if this was intentional (not having this information attached to the cookieItem when we assume the domain) but we can discuss it offline 🐶

Marianarodrigues7 · 2023-09-01T19:45:10Z

src/view/items.js

    }

+    if (domain) {
+      const domainLabel = newLabelWithBadge({ label: "Domain", value: domain || "" });


Maybe not for this MVP, but we have to consider, in terms of design, big values here. Having 2 or 3 badges with medium values might break the badge text since we are fixing our extension max width.
Not sure if an ellipsis would be the best UX but let's discuss this offline.

#103 (comment)

Marianarodrigues7 · 2023-09-01T19:48:44Z

src/view/items.js

    }

+    if (domain) {
+      const domainLabel = newLabelWithBadge({ label: "Domain", value: domain || "" });


Probably my fault here from a previous PR (subkey) but we should remove this:

Suggested change

const domainLabel = newLabelWithBadge({ label: "Domain", value: domain || "" });

const domainLabel = newLabelWithBadge({ label: "Domain", value: domain});

We are already checking if the value exists to render the badge.

Please also update subkey above.

My bad on copy and past. Nice catch 👍

Marianarodrigues7 · 2023-09-01T19:52:27Z

src/view/items.js

  }

-  function itemFooter({ id, key, subKey }) {
+  function itemFooter({ id, domain, key, subKey }) {


Side note: Following our design review about the badge & big values issue, we should review this component itemFooter. It's getting bigger and messy with new features.

I understand your concern. Create a new issue where we can properly address it: #104

src/handler/operation/cookie.js

Marianarodrigues7 · 2023-09-01T20:05:21Z

src/handler/valueWriter.js

+async function saveCookieValue(tab, key, subKey = "", domain, value) {
  try {
-    return await chromeSaveCookieValue(tab, key, subKey, value);
+    return await chromeSaveCookieValue(tab, key, subKey, domain, value);


This chromeSaveCookieValue function is getting crowded in terms of args, so probably it would be easier to use having it grouped some of these by their context - tab, {context of cookie}:

Suggested change

return await chromeSaveCookieValue(tab, key, subKey, domain, value);

return await chromeSaveCookieValue(tab, {key, subKey, domain, value});

WDYT?

At this point, I'm strongly inclined to include typescript in the project.
Making this kind of changes is starting to get a bit scary 😅

I'm a bit resistant to make this change in this PR because by changing this, I feel like we would need to change local and session storage as well, which are out of scope for this PR.
What do you reckon if we move this change to a different issue?

src/handler/operation/cookie.js

JPedroBorges · 2023-09-03T01:16:10Z

@Marianarodrigues7

we assume the cookie domain is the same as the current tab.

Yes, on the initial implementation of the cookie flow, we were just looking at cookies from top level domain of the current tab.
I understand that this might be a bit limiting since we are excluding the “cross-domain cookies” as we often see in advertisement cookies.
Do you foresee the need of including those as well? I'm unsure how feasible it is to do.

we don't see/have this information in our badges

Assuming that we are not working with cookies outside of the top level domain, the cookie will be applied to the current domain. This means, whatever you are on localhost or, domain.com you are still able to use the same entry for both situations.
The whole point of the new domain field, was to control where the cookie was stored.

In this scenario, we have the same key being set on different domain levels. And the point of this change is for the user to be able to choose which one he wants to see.
Maybe we could “lock” the top level domain, and give the ability for the user to choose only the subdomain? “Maintaining the consistency between tabs”?

@bmg13

Would in any scenario the most recent not be the one you would want?

This is something that I'm really struggling with. I'm not going to bite the bullet, I'm changing the implementation to list all cookies and pick the one that is correct instead of relying on chrome cookies API.

Furthermore, contrary to my beliefs that domains always started with . to be retro compatible with older browsers, I've found some examples where this is not true for the same key.

Not sure if this is an edge case or a specific framework. Either way, since the domain are different we have 2 entries in the cookies list.
I really think there's a need to dig deeper into cookies before moving forward with this PR.

src/handler/operation/cookie.js

JPedroBorges · 2023-09-17T22:50:25Z

Due to a lack of direction on this topic, I'm removing the input field.
Focusing on #92 and on a later date, we can come back to this topic with a fresh eyes.

sonarqubecloud · 2025-06-25T23:46:08Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

JPedroBorges added 2 commits August 25, 2023 22:38

add domain to add item

4022544

set value

0450a64

JPedroBorges requested a review from Marianarodrigues7 August 25, 2023 22:26

JPedroBorges linked an issue Aug 25, 2023 that may be closed by this pull request

add domain to cookies #98

Open

Prettified Code!

0f67f5c

bmg13 approved these changes Aug 26, 2023

View reviewed changes

Marianarodrigues7 added the minor Just a minor change label Sep 1, 2023

Marianarodrigues7 reviewed Sep 1, 2023

View reviewed changes

Merge branch 'main' into 98-add-domain-to-cookies

82e264b

github-advanced-security bot found potential problems Sep 2, 2023

View reviewed changes

src/handler/operation/cookie.js Fixed Show fixed Hide fixed

src/handler/operation/cookie.js Fixed Show fixed Hide fixed

JPedroBorges mentioned this pull request Sep 3, 2023

Update badge component #104

Open

JPedroBorges added 3 commits September 3, 2023 01:35

fix add form with visible domain

f2c84b7

fix and improve readbility

ba8932d

remove unnecessary cheks

cb608bd

JPedroBorges added 2 commits September 17, 2023 00:15

fix cookie saver

43fdb54

Prettified Code!

67d8997

JPedroBorges requested a review from Marianarodrigues7 September 16, 2023 23:15

fix cookie saver with empty cookie

4aad1ee

github-advanced-security bot found potential problems Sep 17, 2023

View reviewed changes

src/handler/operation/cookie.js Fixed Show fixed Hide fixed

src/handler/operation/cookie.js Fixed Show fixed Hide fixed

remove cookie domain input

55f074a

Merge branch 'main' into 98-add-domain-to-cookies

4a88c09

	const domainLabel = newLabelWithBadge({ label: "Domain", value: domain \|\| "" });
	const domainLabel = newLabelWithBadge({ label: "Domain", value: domain});

	return await chromeSaveCookieValue(tab, key, subKey, domain, value);
	return await chromeSaveCookieValue(tab, {key, subKey, domain, value});

add domain to add item #103

Are you sure you want to change the base?

add domain to add item #103

Uh oh!

Conversation

JPedroBorges commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Important

GET

SET

screenshots

create page

view page

Uh oh!

bmg13 commented Aug 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JPedroBorges Sep 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marianarodrigues7 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marianarodrigues7 Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JPedroBorges commented Sep 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JPedroBorges commented Sep 17, 2023

Uh oh!

sonarqubecloud bot commented Jun 25, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JPedroBorges commented Aug 25, 2023 •

edited

Loading

JPedroBorges Sep 3, 2023 •

edited

Loading

Marianarodrigues7 left a comment •

edited

Loading

Marianarodrigues7 Sep 1, 2023 •

edited

Loading

JPedroBorges commented Sep 3, 2023 •

edited

Loading