Open Bug 1422973 Opened 7 years ago Updated 2 years ago

Test header merging in webRequest.onBeforeSendHeaders

Categories

(WebExtensions :: General, defect, P3)

Product:
WebExtensions
Component:
General
Version:
57 Branch
Type:
defect

Tracking

(Not tracked)

Status:
REOPENED

People

(Reporter: psnyde2, Unassigned)

Details

Attachments

(1 file)

Bug 1422973 - add test for merging Cookie header in onBeforeSendHeaders,
59 bytes, text/x-review-board-request
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171128222554

Steps to reproduce:

browser.webRequest.onHeadersReceived.addListener(
    details => {
        details.requestHeaders.push({
            name: "Cookie",
            value: "cookiea=valuea"
        });

        // optionally
        details.requestHeaders.push({
            name: "Cookie",
            value: "cookieb=valueb"
        });

        return {
            requestHeaders: details.requestHeaders,
        };
    },
    {urls: ['<all_urls>']},
    ["responseHeaders", "blocking"]
);

This may be one or two bugs (I don't know if they are related)

1) Pages don't see cookies set through webRequest.onBeforeSendHeaders
2) Cookies set in webRequest.onBeforeSendHeaders do not merge correctly (similar to Bug 1377689)





Actual results:

using the above extension, pages do not see either cookiea=valuea or cookieb=valueb.

If you just set one cookie, the page does not see that either (ie if you just do 

browser.webRequest.onHeadersReceived.addListener(
    details => {
        details.requestHeaders.push({
            name: "Cookie",
            value: "cookiea=valuea"
        });

        return {
            requestHeaders: details.requestHeaders,
        };
    },
    {urls: ['<all_urls>']},
    ["responseHeaders", "blocking"]
);

pages don't see cookiea=valuea either



Expected results:

document.cookie should be populated with cookiea=valuea and cookieb=valueb
The code in your example uses onHeadersReceived, wasn't this about onBeforeRequest and onBeforeSendHeaders?
Flags: needinfo?(psnyde2)
Ah, that is a dumb mistake, that should be a s/onHeadersReceived/onBeforeSendHeaders/g in both the examples above.  I don't see a way to edit that.  Sorry for the going-too-fast goof!
Flags: needinfo?(psnyde2)
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
The Cookie headers sent to the server have nothing to do with the cookies visible to page scripts. If you want to change the latter, please use the cookies API.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
I understand that that is the current implementation, but if a page sends a Cookie header, then the page should respect that, no?  I'm not saying that this is the simplest or most direct way to set a cookie, just that the page state should reflect the headers sent to it.

:mixedpuppy this was my understanding from discussing Bug 1377689 with you (in addition to seeming like expected behavior).

If the page's DOM / state should not reflect the headers, its worth documenting this in the onHeadersReceived documentation I think.  Happy to contribute text there if thats the most useful option
Flags: needinfo?(mixedpuppy)
The page doesn't set Cookie headers. The browser sends Cookie headers to the server so it knows what cookies the browser has. The server sends Set-Cookie headers, and if you add one of those to the response headers, it will be visible to page scripts. But you'd still be much better off using the cookie API in that case, since it isn't prone to conflicts.
The bug is about setting Cookie on the request to the page using webRequest.onBeforeSendHeaders, not about setting a cookie in the cookie jar. The expectation is that sending a Cookie header in the request will make the page see that cookie.  

This bug was created from a failed test case that was requested of me in Bug 1377689, comment 34, that said sending two "Cookie" headers in the request headers to a page should result in the page seeing those cookies.  

Not trying to pick a fight or argue or nothing, just that (i guess along with mixedpuppy) expected things to operate that way, and when they didn't it seemed like a bug.  If both of our expectations are wrong (or, at least mine, maybe I misunderstood MP), then it might just be worth adding a mention in the docs to keep other people from being similarly confused is all :)
(In reply to psnyde2 from comment #6)
> The bug is about setting Cookie on the request to the page using
> webRequest.onBeforeSendHeaders, not about setting a cookie in the cookie
> jar. The expectation is that sending a Cookie header in the request will
> make the page see that cookie.  

That is not a valid expectation. Setting a Cookie header in onBeforeSendHeaders will make the *server* see the cookie. Setting a Set-Cookie header in onHeadersReceived will make the page see it.
Ah, i see your point.  Derp, thank you for correcting :)

I'll check to see if Cookie headers are merged correctly and seen correctly by the server and open a new bug if needed.
I keep trying to reply and you two keep getting in front of me :)

I had been thinking already about the cookie api along the same lines Kris mentioned, doing this via webRequest is kind of redundant.  That makes this particular side of the equation low priority or wontfix if it is just about cookies.

However, in general, I think merge-able headers should be possible (assuming there are headers that should be merge-able) during onBeforeSendHeaders and Cookie is a reasonable test case (assuming Cookie is among the merge-able headers).

The test should then use onSendHeaders to verify that the headers were merged.  After that we really don't care because that is dependent on the server.

So a) what headers are merge-able on the request and, b) how was this tested?
Flags: needinfo?(mixedpuppy)
Summary: Page does not see cookies set in webRequest.onBeforeSendHeaders → Test header merging in webRequest.onBeforeSendHeaders
I *do* want to test merging here which was my original intention, so re-opening.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INVALID → ---
I've created a test that… tests this, and it seems like merging "Cookie" headers on request does not work.  I wanted to check the test in for review first, to make sure I wasn't missing something very obvious. 

If the test looks sane, I'll start looking into solutions / fixes.
I think you could simplify the test by setting Cookie headers in onBeforeSendHeaders, then looking at what the headers are in onSendHeaders, but your test seems like it should be ok.  What is actually in "tabContent"?

The logic that needs to be examined for this case is here:

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpHeaderArray.cpp#46

and

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpHeaderArray.h#256

From that, it looks like merged headers would be deliminated with ", " rather than "; ".
Attachment #8935666 - Flags: review?(mixedpuppy)
tabContent ends up being JSON of the headers that were sent with the request (its just calling to `return_headers.sjs`, which was a test script that was already in the repo)

After poking at this all afternoon though, I think the issue isn't in the merging logic (or, at least, not mostly.  nsHttp::Cookie will need to be added to https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpHeaderArray.h#268)

The main issue is still back in WebRequest.jsm, and the model that listeners should not see the results of other listeners.  So if the original request has no Cookies, and then two listeners both set Cookie, the merging code thinks that neither Cookie header should be merged (since neither listener sees the Cookie header the other sent).

In other words, if the initial request sends init=init, and then the two listeners set l1=l1 and l2=l2, the end result will be (assuming no bugs somewhere else) "init=init; l1=l1; l2=l2" (b/c each listener sees "init=init", and so merges)

However, if there initial request sends no Cookie header, and the two listeners set l1=l1 and l2=l2, the end result will be "l1=l1" or "l2=l2", but never "l1=l1; l2=l2".

Not sure how you all would like to proceed on this.  For my two cents, the best approach would be to let the listeners see the results of prior listeners (even if that ordering is not guaranteed). 

Happy to keep hacking on this, but just want to make sure its in a direction thats compatible with you all.
Lets not worry about the multiple listener situation right now (Bug 1421725 for that), lets just be sure merge works where it should.

I'm not clear if Cookie needs the alternate (ie newline) delimiter, but that can also be a separate bug.

Based on my reading of the code I pointed to, something like this should result in a merge happening:

onBeforeSendHeaders.addListener(
    details => {
        details.requestHeaders.push({
            name: "foobar",
            value: "a=1"
        });
        details.requestHeaders.push({
            name: "foobar",
            value: "b=2"
        });

        return {
            requestHeaders: details.requestHeaders,
        };
    },
    {urls: ['<all_urls>']},
    ["responseHeaders", "blocking"]
);
So the test fails now.  The cookies are merged incorrectly, as "firstcookie=firstvalue, secondcookie=secondvalue".  The title suggests that this bug is just for the test, but if its useful, I can start working on a fix (for this bug, or for a new one)
I've gone back and examined the specs[1][2] this morning, the value you're seeing is technically correct (4.3.4 in rfc2109).  The cookie spec does define the separator as (";" | ",").

A couple things I'm not clear about:

- Is it better to use ; anyway?
- Should the array code also insert new lines as it does for Set-Cookie?
- Is server support better with multiple cookie headers or merged headers?

And a thought on the api:

- Should we support a merge flag in the webrequest header object when setting headers?  that might provide more control over this to extensions.  ie. merge=false to prevent header merging

I think we should get input from someone a touch more in tune with real world workings of this part of the spec.  Dragana has done some work in the headers code, so ni?

[1] https://tools.ietf.org/html/rfc2616
[2] https://tools.ietf.org/html/rfc2109
Flags: needinfo?(dd.mozilla)
Comment on attachment 8935666 [details]
Bug 1422973 - add test for merging Cookie header in onBeforeSendHeaders,

https://reviewboard.mozilla.org/r/206570/#review212674

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_merge_request_headers.html:82
(Diff revisions 1 - 2)
>      const foundCookies = parsedHeaders.cookie
>        .trim()
>        .split("; ")
>        .reduce((prev, next) => {
>          const [cookieName, cookieValue] = next.trim().split("=");
>          prev[cookieName] = cookieValue;
>          return prev;
>        }, {});

The parsing here is basic and doesn't really parse cookies the way the browser would or necessarily per spec.  For example, cookies can be seperated by a comma or semicolon.
I can make the cookie parsing code in the test more robust, but for my two cents, I think it should be sufficient (if the cookies merged from onHeadersReceived are merged the same way as they are for request cookies).  I'm not aware of any time Firefox uses ", "  to separate values in "Cookie".  So even if its standards compliant, it seems like unexpected behavior.

Or, at least unexpected to me!
(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> I've gone back and examined the specs[1][2] this morning, the value you're
> seeing is technically correct (4.3.4 in rfc2109).  The cookie spec does
> define the separator as (";" | ",").
> 
> A couple things I'm not clear about:
> 
> - Is it better to use ; anyway?
> - Should the array code also insert new lines as it does for Set-Cookie?
> - Is server support better with multiple cookie headers or merged headers?
> 
> And a thought on the api:
> 
> - Should we support a merge flag in the webrequest header object when
> setting headers?  that might provide more control over this to extensions. 
> ie. merge=false to prevent header merging
> 
> I think we should get input from someone a touch more in tune with real
> world workings of this part of the spec.  Dragana has done some work in the
> headers code, so ni?
> 
> [1] https://tools.ietf.org/html/rfc2616
> [2] https://tools.ietf.org/html/rfc2109

rfc2109 is obsoleted by https://tools.ietf.org/html/rfc6265
and rfc2616 is obsoleted by rfc-s 7230, 7231, 7232, 7233, 7234, 7235

So please take a look at one of them.

rfc 6265 suggests using ";" as separator.

When a cookie header is created from cookiejar we use ";" :
https://searchfox.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#3277
and
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#3317

Although if cookie headers are set using setrequestheader and merge==true we will use "," as a separator.
We could add that special case in:
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpHeaderArray.h#256

I do not see a reason why not, but let's double check. Patrick, what do you think?

How realistic is this usecase? Using cookie api is more realistic?

(If we do change nsHttpHeaderArray.h#256, please do it in a separate bug)
Flags: needinfo?(dd.mozilla) → needinfo?(mcmanus)
a special case for cookies would not surprise me - they commonly are treated as an exception.

Tanvi is probably the right person to ask.
Flags: needinfo?(mcmanus) → needinfo?(tanvi)
(In reply to Patrick McManus [:mcmanus] from comment #22)
> a special case for cookies would not surprise me - they commonly are treated
> as an exception.
> 
> Tanvi is probably the right person to ask.

Actually, Dan has more experience with the cookie code and infrastructure.
Flags: needinfo?(tanvi) → needinfo?(dveditz)
Priority: -- → P3
If you're talking about the Cookie: header (sent TO the server) then use ';' between name/value pairs. HTTP in general merges multiple headers with ',' so you might think that's OK too, but the spec says "the user agent MUST NOT attach more than one Cookie header field."

If you're talking about the Set-Cookie: header (sent BY the server) then do NOT use ';' as a delimiter between cookies, because ';' already delimits attributes. You can send multiple individual Set-Cookie: headers. You should NOT try to merge set-cookie headers.

   Origin servers SHOULD NOT fold multiple Set-Cookie header fields into
   a single header field.  The usual mechanism for folding HTTP headers
   fields (i.e., as defined in [RFC2616]) might change the semantics of
   the Set-Cookie header field because the %x2C (",") character is used
   by Set-Cookie in a way that conflicts with such folding.

Sadly this means you can't create a "generic merge http headers" mechanism, cookies are always an exception.
Flags: needinfo?(dveditz)
Necko doesn't support multiple headers with the same name. It already had its own logic for merging headers, though. We don't need to implement our own.
(But, if necko doesn't handle this case correctly, it sounds like we should fix it)
Product: Toolkit → WebExtensions
Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958
Component: Untriaged → General
Attachment #8935666 - Flags: review?(mixedpuppy)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: