1407675 - Unable to add cookie with an IP address IP

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[lucast1533]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

Attempt to add a cookie using an IP address for domain. More specifically, a cookie dictionary of:
{'path': '/', 'domain': '127.0.1.1', 'name': 'foo', 'value': 'bar', 'secure': False}

Deleting the 'domain' key works as intended and using a regular domain name seems to work fine.

This was discovered in the selenium python tests, starting on 04/10/2017. I believe it is due to the fix for https://bugzilla.mozilla.org/show_bug.cgi?id=1372595

Link to travis tests where it first started failing:
https://travis-ci.org/SeleniumHQ/selenium/jobs/283060112



Actual results:

Error is returned

"Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsICookieManager2.add]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://marionette/content/cookie.js :: cookie.add :: line 135"  data: no]

Trying to set the cookie with the port added results in:

Cookies may only be set for the current domain (127.0.1.1)

As expected.



Expected results:

Should be able to add a cookie for the IP address as the domain.

Comment 1

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

The actual bug Lukas is talking about here and which regressed this is bug 1402978.

Peter, could you have a look at it please?

Comment 2

[Peter Major]

Hmm, not quite sure how to fix this just yet, I'll take a more closer look..

Comment 3

[Andreas Tolfsen ❲:ato❳]

This happens because an "." is prefixed to restrictToHost in [1]:

>   if (restrictToHost) {
>     if (!restrictToHost.endsWith(newCookie.domain) &&
>         ("." + restrictToHost) !== newCookie.domain) {
>       throw new InvalidCookieDomainError(
>           `Cookies may only be set ` +
>           `for the current domain (${restrictToHost})`);
>     }
>   }

restrictToHost is new URL(GeckoDriver#currentURL).hostname from
GeckoDriver#addCookie [2].

  [1] https://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/testing/marionette/cookie.js#121-128
  [2] https://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/testing/marionette/driver.js#2626-2638

Comment 4

[Andreas Tolfsen ❲:ato❳]

It seems a prerequisite here to first determine if the provided
domain is an IP or a TLD before checking if the cookie can be set.
We might be able to make use of the nsIEffectiveTLDService for this.

There appears to be code in nsCookieService::CanSetCookie [1] that
is useful for understanding how to implement the correct check.

  [1] https://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/netwerk/cookie/nsCookieService.cpp#3464

Comment 5

[Peter Major]

Attachment: Bug 1407675 - Fix cookie creation via Marionette using IP address

The issue here was that the cookie domain was always prepended with
'.' character. To resolve this edge-case Marionette now first checks
whether the cookie domain is in fact an IP address.

Review commit: https://reviewboard.mozilla.org/r/188796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188796/

Comment 6

[Peter Major]

I had a look at this today and fired up a review for a potential fix.

Comment 7

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

Fantastic. Thank you a lot! Andreas or myself will have a look at the patch later.

Comment 8

[Andreas Tolfsen ❲:ato❳]

Just a head’s up before I look at the patch: I believe you have
commit access level 1 now?  This allows you to trigger try runs
on Treeherder.  You find a dialogue for doing this directly from
MozReview under ‘Automation’ → ‘Trigger a Try Build’.

A good try syntax for Marionette/geckodriver is:

> try: -b do -p linux64,macosx64,win64,android-api-16 -u marionette-e10s,marionette-headless-e10s,xpcshell,web-platform-tests,firefox-ui-functional-local-e10s,firefox-ui-functional-remote-e10s -t none

It’s generally a good idea to trigger a try run for your own build
because it reduces the turnaround time on getting patches through
review.

I triggered the try run for you now.

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8917855 [details]
Bug 1407675 - Fix cookie creation via Marionette using IP address

https://reviewboard.mozilla.org/r/188796/#review194460

Thanks for addressing the regression, this looks good to me.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51a852b856dc
Fix cookie creation via Marionette using IP address r=ato

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/51a852b856dc

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/025fd4ef0173

Comment 13

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

Peter, thanks a lot for fixing this regression!