Unable to add cookie with an IP address IP

RESOLVED FIXED in Firefox 57

Status

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: lucast1533, Assigned: majorpetya)

Tracking

({regression})

Trunk
mozilla58
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 fixed, firefox58 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year 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.
The actual bug Lukas is talking about here and which regressed this is bug 1402978.

Peter, could you have a look at it please?
Blocks: 1402978
status-firefox57: --- → affected
status-firefox58: --- → affected
Component: Untriaged → Marionette
Flags: needinfo?(majorpetya)
Keywords: regression
Product: Firefox → Testing
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

a year ago
Hmm, not quite sure how to fix this just yet, I'll take a more closer look..
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
OS: Unspecified → All
Hardware: Unspecified → All
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 hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
I had a look at this today and fired up a review for a potential fix.
Flags: needinfo?(majorpetya)
Fantastic. Thank you a lot! Andreas or myself will have a look at the patch later.
Assignee: nobody → majorpetya
Status: NEW → ASSIGNED
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

a year 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.
Attachment #8917855 - Flags: review?(ato) → review+

Comment 10

a year ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51a852b856dc
Fix cookie creation via Marionette using IP address r=ato
https://hg.mozilla.org/mozilla-central/rev/51a852b856dc
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
status-firefox56: --- → unaffected
status-firefox-esr52: --- → unaffected
Whiteboard: [checkin-needed-beta]

Comment 12

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/025fd4ef0173
status-firefox57: affected → fixed
Flags: in-testsuite+
Whiteboard: [checkin-needed-beta]
Peter, thanks a lot for fixing this regression!
You need to log in before you can comment on or make changes to this bug.