lucast1533 Reporter
	Description • 7 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 1 • 7 years ago

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

Peter, could you have a look at it please?

	Peter Major Assignee
	Comment 2 • 7 years ago

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

	Andreas Tolfsen ❲:ato❳
	Comment 3 • 7 years ago

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

	Andreas Tolfsen ❲:ato❳
	Comment 4 • 7 years ago

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

	Peter Major Assignee
	Comment 6 • 7 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 7 • 7 years ago

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

	Andreas Tolfsen ❲:ato❳
	Comment 8 • 7 years ago

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.

	Andreas Tolfsen ❲:ato❳
	Comment 9 • 7 years ago
mozreview-review

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.

	Pulsebot
	Comment 10 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51a852b856dc
Fix cookie creation via Marionette using IP address r=ato

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 11 • 7 years ago
bugherder

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

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 7 years ago
bugherder uplift

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

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 13 • 7 years ago

Peter, thanks a lot for fixing this regression!