Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cookies.get orders cookies according to RFC-6265 #36193

Merged

Conversation

rebloor
Copy link
Contributor

@rebloor rebloor commented Oct 4, 2024

Description

Addresses the documentation needs for Bug 1818968 cookies.get() should return the cookie with the closest matching path but also makes grammar and format corrections across all the cookies API docs.

Related issues and pull requests

Related BCD changes in mdn/browser-compat-data#24634

@rebloor rebloor added the Content:WebExt WebExtensions docs label Oct 4, 2024
@rebloor rebloor self-assigned this Oct 4, 2024
@rebloor rebloor requested review from a team as code owners October 4, 2024 17:43
@rebloor rebloor requested review from pepelsbey and removed request for a team October 4, 2024 17:43
@github-actions github-actions bot added Content:Firefox Content in the Mozilla/Firefox subtree size/m [PR only] 51-500 LoC changed labels Oct 4, 2024
Copy link
Contributor

github-actions bot commented Oct 4, 2024

Preview URLs (12 pages)
Flaws (1)

Note! 11 documents with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Mozilla/Add-ons/WebExtensions/API/cookies/SameSiteStatus
Title: cookies.SameSiteStatus
Flaw count: 1

  • bad_bcd_queries:
    • No BCD data for query: webextensions.api.cookies.SameSiteStatus
External URLs (3)

URL: /en-US/docs/Mozilla/Firefox/Releases/133
Title: Firefox 133 for developers


URL: /en-US/docs/Mozilla/Add-ons/WebExtensions/API/cookies/CookieStore
Title: cookies.CookieStore

(comment last updated: 2024-11-22 02:45:40)

@@ -51,10 +51,10 @@ Trackers use third-party cookies, that is, cookies set by a website other than t
1. You visit `a-shopping-site.com`, which uses `ad-tracker.com` to deliver its adverts on the web. `ad-tracker.com` sets a cookie associated with the `ad-tracker.com` domain. While you are on `a-shopping-site.com`, `ad-tracker.com` receives information about the products you browse.
2. You now visit `a-news-site.com` that uses `ad-tracker.com` to deliver adverts. `ad-tracker.com` read its cookie and use the information collected from `a-shopping-site.com` to decide which adverts to display to you.

Firefox includes features to prevent tracking. These features separate cookies so that trackers cannot make an association between websites visited. So, in the preceding example, `ad-tracker.com` cannot see the cookie created on `a-news-site.com` when visiting `a-shopping-site.com`. The first iteration of this protection was first-party isolation which is now being superseded by [dynamic partitioning](/en-US/docs/Web/Privacy/State_Partitioning#dynamic_partitioning).
Firefox includes features to prevent tracking. These features separate cookies so that trackers cannot make an association between websites visited. So, in the preceding example, `ad-tracker.com` cannot see the cookie created on `a-news-site.com` when visiting `a-shopping-site.com`. The first iteration of this protection was first-party isolation, which is being superseded by [dynamic partitioning](/en-US/docs/Web/Privacy/State_Partitioning#dynamic_partitioning).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rob--W for this and the next para, has first-party isolation to dynamic partitioning been completed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both features are present.

In the Tor browser that is based on Firefox, first party isolation is enabled.

In Firefox, dFPI is enabled by default, but the behavior can be changed by users. This is documented at https://developer.mozilla.org/en-US/docs/Web/Privacy/State_Partitioning#status_of_partitioning_in_firefox

Interestingly, the fact that it is enabled by default is not documented on that section of MDN.

The bug where it was enabled by default (in Firefox 103) is https://bugzilla.mozilla.org/show_bug.cgi?id=1776760

Tagging pbz - @Trikolon - do you know why the support status of dFPI is not reflected in the MDN documentation at https://developer.mozilla.org/en-US/docs/Web/Privacy/State_Partitioning#status_of_partitioning_in_firefox ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MDN article is simply outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trikolon could you take a look at my changes and let me know if they are sufficient and accurate? See e7329a3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Generally, while still supported via the extension API, we don't want to encourage users to use first party isolation in Firefox. It doesn't hurt to mention in in a technical article though.

@rebloor rebloor requested a review from a team as a code owner October 11, 2024 01:09
@github-actions github-actions bot added the Content:Other Any docs not covered by another "Content:" label label Oct 11, 2024
Copy link
Member

@pepelsbey pepelsbey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update! I have a question about the Firefox version and a suggestion about the RFC wording below.

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Nov 7, 2024
Copy link
Contributor

github-actions bot commented Nov 7, 2024

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Nov 8, 2024
Copy link
Collaborator

@dotproto dotproto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only see one relatively minor issue. Nice work, @rebloor!

Copy link
Member

@pepelsbey pepelsbey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my perspective. Thanks!

@rebloor
Copy link
Contributor Author

rebloor commented Nov 19, 2024

@dotproto a reminder that I'm blocked until you've cleared your request for changes.

@@ -9,7 +9,12 @@ browser-compat: webextensions.api.cookies.remove

The **`remove()`** method of the {{WebExtAPIRef("cookies")}} API deletes a cookie, given its name and URL.

The call succeeds only if you include the "cookies" [API permission](/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/permissions#api_permissions) in your [manifest.json](/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json) file, as well as [host permissions](/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/permissions#host_permissions) for the given URL specified in its manifest.
To use this method, in its [manifest.json](/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json) file, an add-on must specify the "cookies" [API permission](/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/permissions#api_permissions) and have the url of the site whose cookie it wants to remove in [host permissions](/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/permissions#host_permissions).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a continuation of another thread.

I don't want to let perfect be the enemy of good, so consider this non-blocking feedback.

Given the overarching statement, should we also add this to get(), getAll(), and getAllCookieStores()? Or indeed - remove from here and 'set'?

Whether we keep a note about permissions or delete it from remove() and set(), I agree that we should use a consistent approach across all methods in the namespace. Between the two, I'd prefer to include mention of the permissions requirements on each method's page.

How do you feel about to replace this line in with a link to the permissions section of the cookies page? For example:

To use this method, an extension must have the "cookies" permission and relevant host permissions. See cookie permissions for additional details.

If you don't like that direction, I'd suggest just removing the permissions paragraph for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, there's no guarantee that someone who arrives on those method pages will have read the overview (or even if they have remembered the content), which is why I asked the question :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dotproto I've made some changes including edits on the cookies permissions section. There shouldn't be any issues, but in case you get the chance to have a look, I'll hold merging for 24 hours.

@dotproto
Copy link
Collaborator

Thanks for the ping, @rebloor. It has been a busy week 😓

@rebloor rebloor merged commit 6f58b8a into mdn:main Nov 22, 2024
8 checks passed
@rebloor rebloor deleted the cookies.get-orders-cookies-according-to-RFC-6265 branch November 22, 2024 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:Firefox Content in the Mozilla/Firefox subtree Content:Other Any docs not covered by another "Content:" label Content:WebExt WebExtensions docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants