1488225 - WebDriver:AddCookie fails for a singlepart domains

Status: NEW

(Remote Protocol :: Marionette, defect, P3)

Description

[Alexei Barantsev]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.106 Safari/537.36

Steps to reproduce:

Scenario in Selenium Java 3.14, geckodriver 0.21, Firefox 61, 62, 63:

WebDriver driver = new FirefoxDriver();
driver.get("http://localhost/");
Cookie cookie = new Cookie.Builder("xxx", "yyy").domain("localhost").build();
driver.manage().addCookie(cookie);


Actual results:

org.openqa.selenium.UnableToSetCookieException: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsICookieManager.add]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://marionette/content/cookie.js :: cookie.add :: line 162"  data: no]



Expected results:

It is expected to set a cookie.

Extra info: it works for domains with multipart names, like 'mozilla.org' or 'localhost.localdomain', but does not work for singlepart domains, like 'localhost' or 'mycomp'

Comment 1

[Alexei Barantsev]

Attachment: trace.log

Comment 2

[Andreas Tolfsen ❲:ato❳]

This one is a hard one to fix because I think we try to construct
a URL of the domain name, and IIRC nsIURI doesn’t like that.  Or
some variation on that.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Here the command which gets send from geckodriver to Marionette:

> 1535984243262	Marionette	TRACE	0 -> [0,3,"WebDriver:AddCookie",{"cookie":{"domain":"localhost","httpOnly":false,"name":"xxx","path":"/","secure":false,"value":"yyy"}}]

There is the domain explicitly specified as `localhost`. When I remove that it works. The problem lays here:

https://dxr.mozilla.org/mozilla-central/rev/c2e3be6a1dd352b969a45f0b85e87674e24ad284/testing/marionette/cookie.js#104-108

In the case of no domain specified we pick the `hostname`, and set `hostOnly` to true. But if a `hostname` is specified for the `AddCookie` command we don't enter this if block, and don't set `hostOnly` to true. Later in line 144 we prefix the domain with a dot:

>  if (!hostOnly && !isIpAddress) {
>    newCookie.domain = "." + newCookie.domain;
>  }

Without the leading . it works.

This let me have a look at MDN, where it is actually explained:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookieManager2#add()

> To be a domain cookie, the host must have at least two subdomain parts (e.g. '.foo.com', not '.com'), otherwise an exception will be thrown.

So as I read this we should only add the leading dot, when the domain has one or more subdomains, which is not the case for just the host, in this case `localhost`.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We could add code like the following after stripping of a leading dot to mark singlepart domains as hostOnly:

>  if (newCookie.domain.search(/[\.]/g) == -1) {
>    hostOnly = true;
>  }

Or did I misunderstood this `hostOnly` variable?

Comment 5

[Andreas Tolfsen ❲:ato❳]

Ideally we should be using a host resolver to identify what type
of remote the string represents, but as I understand it these don’t
support leading-dot domain cookies.

I don’t know the exact definition of hostOnly, but we currently
fail to identify that domain cookies have at least two subdomain
parts.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So we strip off the leading dot. Why can't we just check for any remaining dot in the domain afterward? If one exist, we could assume it's a host, or?

Comment 7

[Peter Major]

I did some digging around this issue, here's my understanding of the problem:
The cookie creation with a single component domain currently fails, because Marionette's internal processing determines that the cookie is meant to be a domain cookie, and as such it should have a leading dot character (because that's how the Firefox cookie manager differentiates domain cookies from host-only cookies). The browser's cookie manager then does its own processing of the domain value and determines that a single component domain is not valid for a domain cookie, and it throws an error.
A workaround to get things rolling with released versions of Firefox would be to not set the domain for the webdriver request, with Selenium and Java this would look something like:

driver.manage().addCookie(new Cookie("xxx", "yyy", null, "/", null));

The restriction to have at least two subdomain parts for domain cookies seems to be a Firefox design decision, but quite possibly there are other restrictions as well (like ensuring that the domain is not an exact match with a public suffix, like co.uk).
Marionette's current cookie implementation currently works like this:

• if the domain attribute is not present in the AddCookie request, treat the cookie as a host-only cookie (i.e. extract the domain from the current browsing context and ensure that it does not have a leading dot)
• if the domain attribute is present AND it's not an IP address, treat the cookie as a domain cookie (i.e. ensure the domain value has a leading dot)

The IP address caveat above was added to resolve Bug 1407675, which was quite similar to this issue really (not setting the domain attribute in that request could have been considered a valid fix for that issue just as well).

One possible way to resolve this inconsistency would be to update the hostOnly detection:

  if (typeof newCookie.domain == "undefined") {
    hostOnly = true;
    newCookie.domain = restrictToHost;
  } else if (newCookie.domain.indexOf(".") == -1) {
    hostOnly = true;
  }

This leaves behind only one edge-case: user submitting domain .localhost , which I'm not really sure we need to handle.
At the end of the day some of the pain around host only cookie handling is coming from the fact that the spec doesn't really explain how host only and domain cookies should be differentiated from each other in webdriver requests/responses, see:
https://github.com/w3c/webdriver/issues/1143