Closed Bug 1377689 Opened 6 years ago Closed 6 years ago

webRequest API should merge multiple headers with the same name rather than using only the last

Categories

(WebExtensions :: Request Handling, defect, P2)

Product:
WebExtensions
Component:
Request Handling
Version:
54 Branch
Type:
defect

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: giwayume, Assigned: psnyde2)

References

Details

(Whiteboard: [webRequest])

Attachments

(1 file, 2 obsolete files)

Bug 1377689 - merge identical headers in set{Request,Response}Header,
59 bytes, text/x-review-board-request
mixedpuppy
: review+
bzbarsky
: review+
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628145605

Steps to reproduce:

browser.webRequest.onHeadersReceived.addListener(
    function(e) {
        e.responseHeaders.push({
            name: "Set-Cookie",
            value: "mycookie=test; path=/"
        });
        return {responseHeaders: e.responseHeaders};
    },
    {urls: ['<all_urls>']},
    ["responseHeaders"]
);



Actual results:

"mycookie" was set, but cookies in other "Set-Cookie" headers provided by the server were not.


Expected results:

Both "mycookie" and other cookies provided by the "Set-Cookie" header from the original response headers should be set.

This code works correctly in Chrome.
Component: Untriaged → WebExtensions: Request Handling
Product: Firefox → Toolkit
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Appending "Set-Cookie" header in "webRequest.onHeadersReceived" tramples over the original "Set-Cookie" header provided by the server. → webRequest API should merge multiple headers with the same name rather than using only the last
Priority: -- → P2
Whiteboard: [webRequest]
Comment on attachment 8927966 [details]
Bug 1377689 - merge identical headers in set{Request,Response}Header,

https://reviewboard.mozilla.org/r/199178/#review204260

The first value for a header set by a given listener needs to override all previous values. Subsequent values from the same listener can be merged.
Attachment #8927966 - Flags: review-
Comment on attachment 8927966 [details]
Bug 1377689 - merge identical headers in set{Request,Response}Header,

https://reviewboard.mozilla.org/r/199178/#review204262

::: toolkit/components/extensions/webrequest/ChannelWrapper.cpp:303
(Diff revision 1)
>        if (NS_SUCCEEDED(rv)) {
>          mContentTypeHdr = aValue;
>        }
>      } else {
> -      rv = chan->SetResponseHeader(aHeader, aValue, false);
> +      bool shouldMergeHeaders = aHeader.LowerCaseEqualsLiteral("set-cookie");
> +      rv = chan->SetResponseHeader(aHeader, aValue, shouldMergeHeaders);

nice.  just need tests per irc
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #2)
> Comment on attachment 8927966 [details]
> Bug 1377689 - merge set-cookie headers set in webRequest.onHeadersReceived,
> 
> https://reviewboard.mozilla.org/r/199178/#review204260
> 
> The first value for a header set by a given listener needs to override all
> previous values. Subsequent values from the same listener can be merged.

Why is that?  Do you mean the same listener across all extensions, or the same listener within an extension?  If the latter, why wouldn't we let extensions be able to simply add cookies?
forgot ni? for comment 4
Flags: needinfo?(kmaglione+bmo)
I get it now.
Flags: needinfo?(kmaglione+bmo)
You don't even need to check the header name. The first time you set a given header for a given listener, pass merge=false. After that, pass true. The network code knows which headers can be merged.
Comment on attachment 8927966 [details]
Bug 1377689 - merge identical headers in set{Request,Response}Header,

https://reviewboard.mozilla.org/r/199178/#review204262

> nice.  just need tests per irc

I've added tests and altered the merge-vs-overwrite logic as discussed in IRC / bug comments.  Hows it look now?
Comment on attachment 8927966 [details]
Bug 1377689 - merge identical headers in set{Request,Response}Header,

https://reviewboard.mozilla.org/r/199178/#review206112

Looking at the last iteration, I think that is not going to work.  It is checking against the channel for the existence of the header, so in the case where the response had the header, it will always merge rather than overwrite then merge.  The test wont catch the since it is not doing a request that would return a set-cookie header (something that should also be addressed, see our use of .sjs files in our tests).

I think the approach is going to have to be a bit more like:

Update ChannelWrapper setResponseHeader and setRequestHeader to accept a third param for merge, defaulted to false.

In webrequest.jsm, update RequestHeaderChanger.setHeader and ResponseHeaderChanger.setHeader to receive/pass the merge argument.

Update HeaderChanger.applyChanges to handle the merge value.  My thought is to do something like:

setHeaders
for name, value in headers
   merge = setHeaders.has(name)
   setHeader(name, value, merge)
   setHeaders.add(name)


This will fix both request and response as well as address comment #7.
Attachment #8927966 - Flags: review?(mixedpuppy) → review-
I see, sounds good, I'll incorporate these changes into a different patch and submit that.  Can I add you for review to that one as well?
Assignee: nobody → psnyde2
I think this latest attempt addresses the previous concerns, and the new test now correctly triggers the issue (and shows the correction).  Apologies if I submitted the new version incorrectly, still learning the tools :)
Comment on attachment 8927966 [details]
Bug 1377689 - merge identical headers in set{Request,Response}Header,

https://reviewboard.mozilla.org/r/199178/#review208666

Looking good.  I want Kris to give a thumbs up on the ChannelWrapper change.  We'll also need a final review on the webidl change from a peer in that group.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:17
(Diff revision 3)
> +
> +<script type="text/javascript">
> +"use strict";
> +
> +let extension;
> +add_task(async function setup() {

the function name should be test_something

Also, add a second test that is basically what you had before (ie. no server set cookie)

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:33
(Diff revision 3)
> +      // This tab is opened for two purposes:
> +      // 1. To allow tests to run content scripts in the context of a tab,
> +      //    for fetching the value of document.cookie.
> +      // 2. To work around bug 1309637, based on the analysis in comment 8.

Are you going to add tests related to this comment?

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:61
(Diff revision 3)
> +    browser.webRequest.onHeadersReceived.addListener(
> +      onHeadersReceived,
> +      requestFilter,
> +      extraInfoSpec
> +    );

just inline everything here

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:72
(Diff revision 3)
> +    if (browser.windows) {
> +      await browser.browsingData.removeCookies({});
> +      let childTabDetails = await openWindowAndTab("http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/set_cookies.sjs");
> +
> +      let testCookies = await browser.cookies.getAll({});
> +      browser.test.assertEq(2, testCookies.length, "3 cookies were set (two by the request, one by the extension)");

You're testing for 2 but comment says 3

::: toolkit/modules/addons/WebRequest.jsm:132
(Diff revision 3)
>        if (binaryValue) {
>          value = String.fromCharCode(...binaryValue);
>        }

seems we should only bother doing this if we're actually going to call setHeader below.

::: toolkit/modules/addons/WebRequest.jsm:144
(Diff revision 3)
>        if (!original || value !== original.value) {
> -        this.setHeader(name, value);
> +        let shouldMerge = headersAlreadySet.has(lowerCaseHeaderName);
> +        this.setHeader(name, value, shouldMerge);
>        }
> +
> +      headersAlreadySet.add(lowerCaseHeaderName);

move this to the line just after setHeader
Comment on attachment 8927966 [details]
Bug 1377689 - merge identical headers in set{Request,Response}Header,

r? specifically for approach and channelwrapper changes
Attachment #8927966 - Flags: review?(kmaglione+bmo)
Comment on attachment 8927966 [details]
Bug 1377689 - merge identical headers in set{Request,Response}Header,

https://reviewboard.mozilla.org/r/199178/#review208666

> seems we should only bother doing this if we're actually going to call setHeader below.

bad comment by me.
Comment on attachment 8927966 [details]
Bug 1377689 - merge identical headers in set{Request,Response}Header,

https://reviewboard.mozilla.org/r/199178/#review208666

> move this to the line just after setHeader

I was full of fail on this as well.  Sorry, dropping.
Comment on attachment 8927966 [details]
Bug 1377689 - merge identical headers in set{Request,Response}Header,

https://reviewboard.mozilla.org/r/199178/#review209124

Close. Just some minor issues to address this time.

::: dom/webidl/ChannelWrapper.webidl:338
(Diff revision 3)
>     * removing it.
>     *
>     * For non-HTTP requests, throws NS_ERROR_UNEXPECTED.
>     */
>    [Throws]
> -  void setRequestHeader(ByteString header, ByteString value);
> +  void setRequestHeader(ByteString header, ByteString value, optional boolean merge = false);

Nit: Please move the new argument to its own line. Same below.

Also, please document what this argument means.

::: toolkit/components/extensions/test/mochitest/mochitest-common.ini:86
(Diff revision 3)
> +[test_ext_webrequestblocking_set_cookie.html]
>  skip-if = os == 'android' # bug 1369440

Please swap these two lines. As it is, this test will be skipped on android and test_ext_contentscript_about_blank will now run on android.

::: toolkit/components/extensions/test/mochitest/set_cookies.sjs:2
(Diff revision 3)
> +const returnedHtml = `
> +<!doctype html>

Nit: DOCTYPE should be upper-case.

::: toolkit/components/extensions/test/mochitest/set_cookies.sjs:14
(Diff revision 3)
> +function handleRequest(request, response) {
> +
> +  response.setHeader("Content-Type", "text/html;charset=utf-8");
> +  response.setHeader("Cache-Control", "no-cache");
> +  response.setHeader("Set-Cookie", "cookie1=value1");
> +  response.write(returnedHtml);

Nit: No need for an sjs script just to set response headers. Just create a headers file by appending `^headers^` to the HTML filename: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest#How_do_I_change_the_HTTP_headers_or_status_sent_with_a_file_used_in_a_Mochitest.3F

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:16
(Diff revision 3)
> +<body>
> +
> +<script type="text/javascript">
> +"use strict";
> +
> +let extension;

This shouldn't be a global.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:18
(Diff revision 3)
> +<script type="text/javascript">
> +"use strict";
> +
> +let extension;
> +add_task(async function setup() {
> +

Nit: No newlines at the start of a block. Our ESLint rules enforce this.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:23
(Diff revision 3)
> +
> +  async function background() {
> +
> +    async function openWindowAndTab(url) {
> +
> +      let tabReadyPromise = new Promise((resolve) => {

Nit: No parens around argument name in single-argument arrow function.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:44
(Diff revision 3)
> +      return {windowId, tabId};
> +    }
> +
> +    const requestFilter = {
> +      urls: ["<all_urls>"],
> +      types: ["main_frame", "sub_frame"]

Nit: Please add trailing comma. Our ESLint rules enforce this.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:49
(Diff revision 3)
> +      types: ["main_frame", "sub_frame"]
> +    };
> +
> +    const extraInfoSpec = ["blocking", "responseHeaders"];
> +
> +    const onHeadersReceived = function (details) {

Nit: Please use arrow function.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:53
(Diff revision 3)
> +
> +    const onHeadersReceived = function (details) {
> +
> +      details.responseHeaders.push({
> +        name: "Set-Cookie",
> +        value: "cookie2=value2"

Please add tailing comma.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:57
(Diff revision 3)
> +        name: "Set-Cookie",
> +        value: "cookie2=value2"
> +      });
> +
> +      return {
> +        responseHeaders: details.responseHeaders

Trailing comma.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:74
(Diff revision 3)
> +      let childTabDetails = await openWindowAndTab("http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/set_cookies.sjs");
> +
> +      let testCookies = await browser.cookies.getAll({});
> +      browser.test.assertEq(2, testCookies.length, "3 cookies were set (two by the request, one by the extension)");
> +
> +      for (let i = 1; i <= 2; i += 1) {

Nit: `i++`

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:75
(Diff revision 3)
> +
> +      let testCookies = await browser.cookies.getAll({});
> +      browser.test.assertEq(2, testCookies.length, "3 cookies were set (two by the request, one by the extension)");
> +
> +      for (let i = 1; i <= 2; i += 1) {
> +        let cookieName =`cookie${i}`;

Nit: Missing space after the `=` (our ESLint rules will reject this).

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:93
(Diff revision 3)
> +  extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      permissions: [
> +        "webRequest",
> +        "cookies",
> +        "browsingData",

This permission shouldn't be necessary. Also, please keep permissions sorted.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:99
(Diff revision 3)
> +        "webNavigation",
> +        "webRequestBlocking",
> +        "<all_urls>",
> +      ],
> +    },
> +    background

Nit: Trailing comma.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:105
(Diff revision 3)
> +  });
> +
> +  await extension.startup();
> +  await extension.awaitFinish("cookie modifying extension");
> +  await extension.unload();
> +});

We also need to test that multiple listeners don't wind up merging header values from previous listeners.

::: toolkit/modules/addons/WebRequest.jsm:127
(Diff revision 3)
>        if (!newHeaders.has(name)) {
>          this.setHeader(name, "");
>        }
>      }
>  
> -    // Set new or changed headers.
> +    // Set new or changed headers.  If there are two headers with the same

Nit: s/two/multiple/

::: toolkit/modules/addons/WebRequest.jsm:128
(Diff revision 3)
>          this.setHeader(name, "");
>        }
>      }
>  
> -    // Set new or changed headers.
> +    // Set new or changed headers.  If there are two headers with the same
> +    // name (e.g. Set-Cookie), tell setHeader to merge them, instead of having

Nit: s/tell setHeader to//

::: toolkit/modules/addons/WebRequest.jsm:136
(Diff revision 3)
>      for (let {name, value, binaryValue} of headers) {
>        if (binaryValue) {
>          value = String.fromCharCode(...binaryValue);
>        }
> -      let original = origHeaders.get(name.toLowerCase());
> +
> +      let lowerCaseHeaderName = name.toLowerCase();

Nit: s/lowerCaseHeaderName/lowerCaseName/ or something like that, please.
Attachment #8927966 - Flags: review?(kmaglione+bmo) → review-
I think this most recent commit fixes all the outstanding issues (and that I submitted the revision correctly…)

Comments regarding kmag's comments:

re: "Are you going to add tests related to this comment?"
I just removed the comment, since it was more confusing than clarifying, and something I brought over from `test_ext_cookies.html`.

re: "This permission [browsingData] shouldn't be necessary. Also, please keep permissions sorted."
I think its necessary for the `await browser.browsingData.removeCookies({});` call, no?

re: "Also, add a second test that is basically what you had before (ie. no server set cookie)"
I added in tests to this (ie fetch file_sample.html)

re: "We also need to test that multiple listeners don't wind up merging header values from previous listeners."
I added in a third test there that makes sure the results of multiple listeners are set correctly.  i.e. if there are two listeners that add their own cookies, and the initial request sets a cookie, the end result is 3 cookies.
A couple of those were my comments, but you did miss one of Kris':

"We also need to test that multiple listeners don't wind up merging header values from previous listeners."

I'd like some clarity on that in case I'm not catching something, because I think they *should* merge unless they explicitly remove the original set-cookie headers.

I'm also going to ask for one more test in there, do just that ^.  Make the request, remove the req cookie, add the ext cookie.
Flags: needinfo?(kmaglione+bmo)
I see, I added a test (the third test in there, line 90 of test_ext_webrequestblocking_set_cookie.html) that makes sure that if multiple listeners add cookies, the final result is the sum of all the cookies that were set (either from listeners or the original request).

If that is incorrect (or if im misunderstanding), please let me know and I can change the test to expect something else.

The new change set has a fourth test, which tests exactly what you described.
(In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> I'd like some clarity on that in case I'm not catching something, because I
> think they *should* merge unless they explicitly remove the original
> set-cookie headers.

Original headers:

  {"Set-Cookie": "foo=bar"}

Listener 1:

  return {responseHeaders: {"name": "Set-Cookie", value: "bar=baz"}

Listener 2:

  responseHeaders.push({name: "Set-Cookie", value: "a=b"})

Listener 3:

  responseHeaders.push({name: "Set-Cookie", value: "b=c"})

Result:

  {"Set-Cookie": "foo=bar; b=c"}

*or*:

  {"Set-Cookie": "foo=bar; a=b"}

*or*

  {"Set-Cookie": "bar=baz"}

But *never*:

  {"Set-Cookie": "foo=bar; a=b; foo=bar; b=c; bar=baz"}
Flags: needinfo?(kmaglione+bmo)
If the listeners fired in the order you listed, I would expect {"Set-Cookie": "bar=baz; a=b; b=c"}.  Regardless of order, I would never expect to see foo=bar because listener #1 is overwriting responseHeaders, which should remove foo=bar.

I would also expect that the underlying merge handling in httpchannel would never allow foo=bar twice, but we do check the value in HeaderChanger.applyChanges so that shouldn't be possible.
The ordering of listeners is, at least in principle, nondeterministic. If multiple listeners try to change the same header, one of them will succeed, but there's no guarantee which.

The merge handling doesn't care about duplicated values. There's nothing that makes them illegal. It just concatenates the values and separates them with semicolons.

bar=baz should never appear with the other header values. The value of a header after a given listener is processed is entirely dependent on the values returned by that listener, and they never see the bar=baz value that was set by a parallel listener.
Ok, the part I forgot was that a listener does not get the prior listeners changes.  That makes it impossible for two extensions to set different cookies for the same request, one of them will get dropped.  I'm not sure I care for that, its rolling the dice to see if you'll win.
The entire header change API is always a game of rolling dice.

But, as it happens, I suspect with the current code, and the above listeners, we'll wind up with one of:


  {"Set-Cookie": "bar=baz; b=c; a=b"}

  {"Set-Cookie": "bar=baz; a=b; b=c"}

  {"Set-Cookie": "bar=baz"}

  {"Set-Cookie": "bar=baz; b=c"}

  {"Set-Cookie": "bar=baz; a=b"}

depending on the order of the listeners, since we only actually try to change a header when it's different from its initial value. The results would be different if the new value was unshifted rather than pushed, though.

That's probably generally more or less what people would want in this circumstance, but it's a bit DWIMmy for my liking, and doesn't really match the API contract...
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #28)

> That's probably generally more or less what people would want in this
> circumstance, but it's a bit DWIMmy for my liking, and doesn't really match
> the API contract...

Who's api contract?  I mentioned in irc...Chrome says only one extension can change the headers.  MDN currently says each subsequent listener will see prior changes and can make more changes, which is not what we do at this point, each overwrites the prior now.  

Since I've no good idea on a fix for that, lets get the header merge code in and move this issue to another bug.
I've created bug 1421725 to address the conflict issues discussed.
See Also: → 1421725
Comment on attachment 8932647 [details]
Bug 1377689 - linting corrections, added tests for server not setting cookies and multiple listiners,

https://reviewboard.mozilla.org/r/203702/#review209506

The hardest part of this is obviously the tests :)  Koodos for doing this.

::: dom/webidl/ChannelWrapper.webidl:336
(Diff revision 1)
>     * Sets the given request header to the given value, overwriting any
>     * previous value. Setting a header to a null string has the effect of
> -   * removing it.
> +   * removing it.  If merge is true, then the passed value will be merged
> +   * to any existing value that exists for the header.  Otherwise, any
> +   * prior value for the header will be overwritten (e.g. handling multiple
> +   * Set-Cookie headers).

add: Merge is ignored for headers that cannot be merged.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:16
(Diff revision 1)
>  <body>
>  
>  <script type="text/javascript">
>  "use strict";
>  
> +add_task(async function test_modifying_cookies_from_onHeadersReceived() {

Lets also add one simple test for modifying headers in onBeforeSendHeaders.  The header merging code is shared between the two so just keep it short and simple.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:76
(Diff revision 1)
> -    );
> -
>      if (browser.windows) {
> +      // First, perform a request that should not set any cookies, and check
> +      // that the cookie the extension sets is the only cookie in the
> +      // cookie jar.

Also add a test with a onHeadersReceived listener that overwrites the server header.  Something like:

webRequest.onHeadersReceived.addListener(details =>{
  return {responseHeaders: "Set-Cookie: blah"}
}, requestFilter, extraInfoSpec);

That should remove the server set cookie and add the blah cookie.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:93
(Diff revision 1)
> +      await browser.windows.remove(cookieSettingTabDetails.windowId);
> +
> +      // Third, register another onHeadersReceived handler that also
> +      // sets a cookie (thirdcookie=thirdvalue), to make sure modifications from
> +      // multiple onHeadersReceived listeners are merged correctly.
> +      const secondOnHeadersRecievedListiner = details => {

I'm curious, is the ext cookie in responseHeaders here?

::: toolkit/components/extensions/test/mochitest/test_ext_webrequestblocking_set_cookie.html:107
(Diff revision 1)
> -        browser.test.assertEq(expectedCookieValue, aCookie[0] && aCookie[0].value, `Cookie ${cookieName} has expected value of ${expectedCookieValue}`);
> -      }
>  
> -      await browser.windows.remove(childTabDetails.windowId);
> +      await browser.browsingData.removeCookies({});
> +      const secondCookieSettingTabDetails = await openWindowAndTab("file_webrequestblocking_set_cookie.html");
> +      await checkCookies("ext", "req", "third");

One more test after this.  Load a second extension with a listener.  The first extension setting ext, the second setting ext2.  

We should get one of either:

{set-cookie: "req; ext"} or {set-cookie: "req; ext2"}.  

Based on the conversation in the bug, if we end up with req+ext+ext2 there should be something wrong.
Comment on attachment 8932661 [details]
Bug 1377689 - add test for  replacing request set cookie with an extension set cookie,

https://reviewboard.mozilla.org/r/203712/#review209512

Ah, I requested this test in the prior patch review.  If you can merge these into one patch that would be great.  Thanks!
Comment on attachment 8927966 [details]
Bug 1377689 - merge identical headers in set{Request,Response}Header,

I'm assuming this patch is obsoleted by the following patch.
Attachment #8927966 - Attachment is obsolete: true
Attachment #8927966 - Flags: review?(mixedpuppy)
I'm not sure how the code review system handles these patches, but as of `2f311f84c5c8` all the above changes are in places.

So `hg diff -r 4b1257fe22a2` from `2f311f84c5c8` lists all the needed changes.

Apologies for the confusion, I wasn't sure if all the relevant changes (`75982c9b0899`, `8aff48ba6e7d` and `2f311f84c5c8`) shoudl be squashed into one commit, or to keep adding commits to address each round of issues.
Comment on attachment 8932647 [details]
Bug 1377689 - linting corrections, added tests for server not setting cookies and multiple listiners,

https://reviewboard.mozilla.org/r/203702/#review209506

> Lets also add one simple test for modifying headers in onBeforeSendHeaders.  The header merging code is shared between the two so just keep it short and simple.

I can add this in, but could you say a bit more about what you're looking for here?  Would something like this cover whats needed?

Create 1 extension that registers two onBeforeSendHeaders listiners, both add cookies to the request (ext1cookie=ext1value, ext2cookie=ext2value).  The page reports in JS what cookies it recieves.  Check to make sure that the page sees both ext1cookie and ext2cookie?
(In reply to psnyde2 from comment #35)
> Comment on attachment 8932647 [details]
> Bug 1377689 - linting corrections, added tests for server not setting
> cookies and multiple listiners,
> 
> https://reviewboard.mozilla.org/r/203702/#review209506
> 
> > Lets also add one simple test for modifying headers in onBeforeSendHeaders.  The header merging code is shared between the two so just keep it short and simple.
> 
> I can add this in, but could you say a bit more about what you're looking
> for here?  Would something like this cover whats needed?

I'd like some test coverage for requestHeader changing, all the tests are focused on responseHeader.  The base class is shared between the two, but there is still some minor difference.  There's enough tests on responseHeader that I don't think we need to be as complete.

> Create 1 extension that registers two onBeforeSendHeaders listiners, both
> add cookies to the request (ext1cookie=ext1value, ext2cookie=ext2value). 
> The page reports in JS what cookies it recieves.  Check to make sure that
> the page sees both ext1cookie and ext2cookie?

Basically, but even one listener and push two cookie headers into requestHeaders would be enough to make a merge happen.
Comment on attachment 8932647 [details]
Bug 1377689 - linting corrections, added tests for server not setting cookies and multiple listiners,

https://reviewboard.mozilla.org/r/203702/#review209506

> I'm curious, is the ext cookie in responseHeaders here?

Nope, it only sees the server set cookies, not the changes from other listeners.
Attachment #8932647 - Attachment is obsolete: true
Attachment #8932647 - Flags: review?(mixedpuppy)
Attachment #8932647 - Flags: review?(kmaglione+bmo)
Attachment #8932661 - Attachment is obsolete: true
Attachment #8932661 - Flags: review?(mixedpuppy)
re: Lets also add one simple test for modifying headers in onBeforeSendHeaders.  The header merging code is shared between the two so just keep it short and simple.
Punting on this, as discussed in IRC

re: Also add a test with a onHeadersReceived listener that overwrites the server header
This is now the fifth test in `test_ext_webrequestblocking_set_cookie.html`

re: Load a second extension with a listener…
Dropped, as per IRC convo
Comment on attachment 8927966 [details]
Bug 1377689 - merge identical headers in set{Request,Response}Header,

bz, can you give the webidl changes a review? In general a second look at the ChannelWrapper.* changes would be great.
Attachment #8927966 - Flags: review?(bzbarsky)
Comment on attachment 8927966 [details]
Bug 1377689 - merge identical headers in set{Request,Response}Header,

https://reviewboard.mozilla.org/r/199178/#review210380

This looks good.  Assuming the try I initiated is clean, we'll just wait on the review for webidl and this should be done.
Attachment #8927966 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8927966 [details]
Bug 1377689 - merge identical headers in set{Request,Response}Header,

https://reviewboard.mozilla.org/r/199178/#review210696

r=me with the minor nits below picked.

::: dom/webidl/ChannelWrapper.webidl:335
(Diff revision 4)
>    /**
>     * Sets the given request header to the given value, overwriting any
>     * previous value. Setting a header to a null string has the effect of
> -   * removing it.
> +   * removing it.  If merge is true, then the passed value will be merged
> +   * to any existing value that exists for the header.  Otherwise, any
> +   * prior value for the header will be overwritten (e.g. handling multiple

This "e.g" parenthetical seems out of place.  Shouldn't it be after the "if merge is true" bits?

Also, Set-Cookie is odd for a _request_ header.

::: dom/webidl/ChannelWrapper.webidl:350
(Diff revision 4)
>    /**
>     * Sets the given response header to the given value, overwriting any
>     * previous value. Setting a header to a null string has the effect of
> -   * removing it.
> +   * removing it.  If merge is true, then the passed value will be merged
> +   * to any existing value that exists for the header.  Otherwise, any
> +   * prior value for the header will be overwritten (e.g. handling multiple

Again, the parentherical seems misplaced.

::: toolkit/components/extensions/webrequest/ChannelWrapper.cpp:280
(Diff revision 4)
>      aRv.Throw(NS_ERROR_UNEXPECTED);
>    }
>  }
>  
>  void
> -ChannelWrapper::SetRequestHeader(const nsCString& aHeader, const nsCString& aValue, ErrorResult& aRv)
> +ChannelWrapper::SetRequestHeader(const nsCString& aHeader, const nsCString& aValue, bool merge, ErrorResult& aRv)

aMerge, like the other args.

::: toolkit/components/extensions/webrequest/ChannelWrapper.cpp:292
(Diff revision 4)
>      aRv.Throw(rv);
>    }
>  }
>  
>  void
> -ChannelWrapper::SetResponseHeader(const nsCString& aHeader, const nsCString& aValue, ErrorResult& aRv)
> +ChannelWrapper::SetResponseHeader(const nsCString& aHeader, const nsCString& aValue, bool merge, ErrorResult& aRv)

aMerge

::: toolkit/modules/addons/WebRequest.jsm:144
(Diff revision 4)
>        if (!original || value !== original.value) {
> -        this.setHeader(name, value);
> +        let shouldMerge = headersAlreadySet.has(lowerCaseName);
> +        this.setHeader(name, value, shouldMerge);
>        }
> +
> +      headersAlreadySet.add(lowerCaseName);

Moght be worth documenting that this done even if value == original.value, so that the behavior of setting to "a=b" then "c=d" doesn't depend on whether the original value is "a=b" and leads to "a=b, c=d" in both cases.
Attachment #8927966 - Flags: review?(bzbarsky) → review+
I think these changes should address the issues you mentioned. The one exception is for the documentation of the setRequestHeader method in ChannelWrapper.webidl.  I didn't want to have a parenthetical for "Cookie" there because of the problem I just documented in Bug 1422973.  Maybe it'd make more sense to add a specific mention (Cookie or otherwise) once that was resolved?
The comment in WebRequest.jsm is not quite right, I think...  If the values are equal, the set is just ignored, if I understand that code right.  It's _later_ (not equal) values that get merged.
Hows about this then?

    // Set new or changed headers.  If there are multiple headers with the same
    // name (e.g. Set-Cookie), merge them, instead of having new values
    // overwrite previous ones.
    //
    // When the new value of a header is equal the existing value of the header
    // (e.g. the initial response set "Set-Cookie: examplename=examplevalue",
    // and an extension also added the header
    // "Set-Cookie: examplename=examplevalue") then the header value is not
    // re-set, but subsequent headers of the same type will be merged in.

Its a little bit clunky, but its the most concise / unambiguous I could come up with.

If that sounds a-ok, I can re-submit right away
That sounds good to me!
eggcellent, resubmitted :)
brilliant.  lets land this!
Keywords: checkin-needed
hi, you have to mark the issues in mozreview as fixed, so we can land the patch.
Flags: needinfo?(psnyde2)
Keywords: checkin-needed
Ah, whoops, marked all as fixed now.  Thanks!
Flags: needinfo?(psnyde2)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fc3d68852d40
merge identical headers in set{Request,Response}Header, r=bz,mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fc3d68852d40
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Awesome! Thanks, psnyde2! Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#December_2017

Would you be interested in creating an account on mozillians.org? I'd love to vouch for you. 

Welcome onboard! I hope to see you around.
Sure, that would be great, thank you for the suggestion.  I just created account.  Thank you for the kindness!
Also, apologies r,mixedpuppy but I see I left a line of debugging code in the test

browser.test.log(JSON.stringify(details.responseHeaders))

Whats the best way to scrub that back out?  Open another issue?

Apologies for the hassle!
Flags: needinfo?(mixedpuppy)
You can do a followup bug, or just remove it when we get a test sorted out in bug 1422973
Flags: needinfo?(mixedpuppy)
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(psnyde2)
No manual testing is needed for this bug.  There is an attached test case that triggers the issue too.

I am not sure where to set the "qe-verify-" flag though.  I see a "qe‑verify" option under the "Tracking" section of the, but the element is disabled
Flags: needinfo?(psnyde2)
Flags: qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.