Closed Bug 1417828 Opened 2 years ago Closed 2 years ago

chrome.cookies.getAll url filter is broken wrt port

Categories

(WebExtensions :: Request Handling, defect, P2)

58 Branch
defect

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: romain, Assigned: robwu)

Details

Attachments

(1 file)

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.
Component: Extension Compatibility → WebExtensions: Request Handling
Product: Firefox → Toolkit
Priority: -- → P2
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
Flags: needinfo?(romain)
Assignee: nobody → rob
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
It does, actually. I reproduced the error in FF57.
> 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?
Flags: needinfo?(ktouchie)
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.
Attachment #8933848 - Flags: review?(tomica) → review+
(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.
Flags: needinfo?(ktouchie)
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/eb72a70b524f
Ignore ports in cookies API r=zombie
https://hg.mozilla.org/mozilla-central/rev/eb72a70b524f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Product: Toolkit → WebExtensions
Flags: needinfo?(romain)
You need to log in before you can comment on or make changes to this bug.