An unexpected error occurred with browser.cookies.set() with domain and url key for ip addresses

RESOLVED FIXED in Firefox 68

Status

defect
P5
normal
RESOLVED FIXED
7 months ago
2 months ago

People

(Reporter: mrdokenny, Assigned: myeongjun.ko, Mentored)

Tracking

({good-first-bug})

65 Branch
mozilla68

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(2 attachments)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

1. Have an extension with cookies and all urls permission
2. Run this in either the extension code or in the console

await browser.cookies.set({
  "domain": "192.168.1.1",
  "httpOnly": true,
  "name": "",
  "path": "/",
  "sameSite": "lax",
  "secure": true,
  "storeId": "firefox-default",
  "url": "https://192.168.1.1/",
  "value": ""
})


Actual results:

Error: An unexpected error occurred


Expected results:

A cookie should have been set. The above works in Chrome though.

This call without the domain key works in Firefox.

await browser.cookies.set({
  "httpOnly": true,
  "name": "",
  "path": "/",
  "sameSite": "lax",
  "secure": true,
  "storeId": "firefox-default",
  "url": "https://192.168.1.1/",
  "value": ""
})

Another thing to note is that this only happens for IP domains.
Component: Untriaged → General
Product: Firefox → WebExtensions
Component: General → Compatibility
Flags: needinfo?(rob)

This call without the domain key works in Firefox.

This is how the cookies API is supposed to be used. When the "domain" key is passed to the details parameter of browser.cookies.set, the extension API will treat it as a domain cookie (opposed to a host-only cookie). IP addresses are not domain names (they cannot have subdomains), so only host-only cookies are supported for these addresses.

Our implementation unconditionally prepends a period (".") [1], which causes the cookie to be interpreted as a domain cookie. To force the cookie to be treated as a host-only cookie, check if the host is an IP address and avoid prepending a period (e.g. by returning early).

[1] https://searchfox.org/mozilla-central/rev/c21d6620d384dfb13ede6054015da05a6353b899/toolkit/components/extensions/parent/ext-cookies.js#130-132

Mentor: rob
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(rob)
Keywords: good-first-bug
Priority: -- → P5

Note: Do not forget to account for IPv6 addresses.

See Also: → 1520058

For those who want to fix this bug, I described the issue and how to fix it in more detail at https://phabricator.services.mozilla.com/D21828#752489

Note: The browser.cookies.set implementation currently also has an issue with brackets: https://searchfox.org/mozilla-central/rev/2738efcf98d746c8810819e7c0634b2c57212a8d/toolkit/components/extensions/parent/ext-cookies.js#86-100,130-132

  • Line 86-88 : If domain is not passed to browser.cookies.set (i.e. cookie.host is empty), the parsed URI's host is used, which does not have brackets - OK.
  • Line 100 : The cookie.host (should be with brackets) is compared with uri.host (which doesn't have brackets). This is wrong. At that point, cookie.host should be normalized with dropBracketsIfIPv6.
  • Line 130-132 : This is the cause of https://bugzilla.mozilla.org/show_bug.cgi?id=1517993

Together with comment 1, the path to a patch should be quite doable.

If you are new here and interested in picking up this bug, see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Could I ask you this issue?
I want to check contents I understand.
Because IP address are not domain names.
If host(i.e. cookie.host) is IP address(IPv6 or IPv4), the host(i.e. cookie.host) be not added a leading ".".

If I understood right, I would try to solve this issue.

Flags: needinfo?(rob)

I believe that you have understood the issue, and have assigned the bug to you.

Assignee: nobody → myeongjun.ko
Status: NEW → ASSIGNED
Flags: needinfo?(rob)

Thanks for checking my code:) I appreciate it.
I revised again.
If It's wrong or need correction, please tell me again.

Flags: needinfo?(rob)

(Already replied in Phabricator)

Flags: needinfo?(rob)
Keywords: checkin-needed

Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f69f765ef51
An unexpected error occured with browser.cookies.set() with domain and url key for ip address. r=robwu,rpl,mixedpuppy

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.