Closed
Bug 1407675
Opened 7 years ago
Closed 7 years ago
Unable to add cookie with an IP address IP
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(firefox-esr52 unaffected, firefox56 unaffected, firefox57 fixed, firefox58 fixed)
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | --- | fixed |
firefox58 | --- | fixed |
People
(Reporter: lucast1533, Assigned: majorpetya)
References
Details
(Keywords: regression)
Attachments
(1 file)
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•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?
Blocks: 1402978
status-firefox57:
--- → affected
status-firefox58:
--- → affected
Component: Untriaged → Marionette
Flags: needinfo?(majorpetya)
Keywords: regression
Product: Firefox → Testing
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 2•7 years ago
|
||
Hmm, not quite sure how to fix this just yet, I'll take a more closer look..
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
OS: Unspecified → All
Hardware: Unspecified → All
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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
I had a look at this today and fired up a review for a potential fix.
Flags: needinfo?(majorpetya)
Comment 7•7 years ago
|
||
Fantastic. Thank you a lot! Andreas or myself will have a look at the patch later.
Updated•7 years ago
|
Assignee: nobody → majorpetya
Status: NEW → ASSIGNED
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.
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.
Attachment #8917855 -
Flags: review?(ato) → review+
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
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/51a852b856dc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•7 years ago
|
status-firefox56:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Whiteboard: [checkin-needed-beta]
Comment 12•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/025fd4ef0173
Updated•7 years ago
|
Attachment #8917855 -
Flags: review?(hskupin)
Comment 13•7 years ago
|
||
Peter, thanks a lot for fixing this regression!
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•