1417828 - chrome.cookies.getAll url filter is broken wrt port

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P2)

Description

[romain]

When trying to search for a given cookie in current active tab, we got no result, and couldn't figure out why.

Here are the steps to reproduce the bug:

1. We opened page for url: http://localhost:8080/nuxeo/view_documents.faces?conversationId=0NXMAIN` and now have a cookie named 'JSESSIONID'
2. We search for all cookies named 'JSESSIONID' with url 'http://localhost:8080/nuxeo/view_documents.faces?conversationId=0NXMAIN3' it returns undefined (!):

chrome.cookies.getAll({name: 'JSESSIONID', url: 'http://localhost:8080/nuxeo/view_documents.faces?conversationId=0NXMAIN3'}, function(cookies){cookies.forEach(console.log)});

We tested this behavior on other urls as well, it worked well. Then we noticed the port part of the url and tried this:

chrome.cookies.getAll({name: 'JSESSIONID', url: 'http://localhost/nuxeo/view_documents.faces?conversationId=0NXMAIN3'}, function(cookies){cookies.forEach(console.log)});

And it returned the cookie we wanted to find (in a list of one).

So it seems that the port component of the url is not used correctly to look for cookies.

We found that it works well in Firefox 57, but is broken starting from Firefox 58.

Comment 1

[Rob Wu [:robwu]]

Are you sure that this works in Firefox 57?
I believe that bug 1398630 causes this issue (https://hg.mozilla.org/mozilla-central/rev/b5c96a5d7b54), and that first landed in Firefox 57

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1417828 - Ignore ports in cookies API

Fixes regression from bug 1398630, where nsIURI was replaced with DOM
URL. In nsIURI, "host" does not include a port number, but in a DOM URL,
"host" does include a port number and "hostname" has to be used to
obtain the original result.

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

Comment 3

[ktouchie]

It does, actually. I reproduced the error in FF57.

Comment 4

[Tomislav Jovanovic :zombie]

> It does, actually. 
Wait, it does or it doesn't work?

> I reproduced the error in FF57.
"reproduced the error" usually means "doesn't work".

Can you please confirm?

Comment 5

[Tomislav Jovanovic :zombie]

Comment on attachment 8933848 [details]
Bug 1417828 - Ignore ports in cookies API

https://reviewboard.mozilla.org/r/204760/#review212884

Thanks, and sorry for the delay.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:137
(Diff revision 1)
> +
> +    cookie = await browser.cookies.get({url: changePort(TEST_URL, 65535), name: "name1"});
> +    assertExpected(expected, cookie);
> +
> +    cookies = await browser.cookies.getAll({url: TEST_URL});
> +    browser.test.assertEq(cookies.length, 1, "Found cookie in getAll without port");

nit: "Found cookie using..." or something.  Also below.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:147
(Diff revision 1)
> +    assertExpected(expected, cookies[0]);
> +
> +    // .remove should return the URL of the API call, so the port is included in the return value.
> +    const TEST_URL_TO_REMOVE = changePort(TEST_URL, 1023);
> +    details = await browser.cookies.remove({url: TEST_URL_TO_REMOVE, name: "name1"});
> +    assertExpected({url: TEST_URL_TO_REMOVE, name: "name1", storeId: STORE_ID}, details);

Hm, I guess this makes more sense than the alternative. _Infelicitous_ indeed.

Comment 6

[ktouchie]

(In reply to Tomislav Jovanovic :zombie from comment #4)
> > It does, actually. 
> Wait, it does or it doesn't work?
> 
> > I reproduced the error in FF57.
> "reproduced the error" usually means "doesn't work".
> 
> Can you please confirm?

I meant the error does indeed occur in FF57... I reproduced the error in FF57.

Comment 7

[Rob Wu [:robwu]]

Comment on attachment 8933848 [details]
Bug 1417828 - Ignore ports in cookies API

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204760/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb72a70b524f
Ignore ports in cookies API r=zombie

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/eb72a70b524f