1321528 - Changes to Content-Disposition response header are too late to have an effect

Status: VERIFIED FIXED

(WebExtensions :: Request Handling, defect)

Description

[esterTion]

Attachment: firefox-onHeadersReceived-test.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20161130004019

Steps to reproduce:

WebExtension API [onHeadersReceived] seems have some problems. 
I have an example extension attached. 

The example code requested for permission of webRequest and blocking, and background script added a listener which adds header "Content-Disposition: attachment" to mozilla.org requests. 


Actual results:

On Firefox 51b5, web page did change to a file download, but nothing happend on Firefox 52.0a2 & 53.0a1. 


Expected results:

Header value should be successfully modified.

Comment 1

[Kris Maglione [:kmag]]

This is an intentional change, for security reasons. Sorry.

Comment 2

[esterTion]

I just changed component and saw #1273281. 
So there will be a new extension permission for changing sensitive header, or certain header in a list won't let any extension change? 
So far as I tested, add header like "Test-Add-Header: Test-Value" didn't works too, this is not a security header.

Comment 3

[Kris Maglione [:kmag]]

We currently don't allow modifying requests to certain Mozilla sites in any way. We may ease those restrictions in the future, but for now, this is not possible.

Comment 4

[esterTion]

I still have some questions if only Mozilla domain are not allowed. 

My original extension adds content-disposition to a video server that let MP4 files become download, not preview. 
Maybe the problem is on domain matching. 
The CDN video server used does a 302 redirect, like:
http://video.domain.com/file -> http://1.2.3.4/video.domain.com/file
So I set match pattern to
["*://video.domain.com/*","*://*/video.domain.com/*"]
And now it's not working. 
Is the second pattern blocked?

Comment 5

[Kris Maglione [:kmag]]

No, there was also a recent bug that prevented modifying requests for top-level page loads. That should be fixed in the next nightly and Aurora builds.

Comment 6

[esterTion]

That's great! 
Thanks for the works.

Comment 7

[esterTion]

After waited for two days and found two updates for Aurora (Dec 01 & Dec 02), still not working. 
And Nightly Dec 02 build does not work, too. 
Since Mozilla sites are protected, I changed test site to stackoverflow. 
Kind of same, adds "Content-Disposition: attachment" and "test-header: value". The console did print the log of "modified", but nothing really worked. 

Aurora can't even open the debug console for this test extension, it says "addonActor is undefined".

Comment 8

[esterTion]

Attachment: firefox-onHeadersReceived-test-stackoverflow.zip

Comment 9

[Kris Maglione [:kmag]]

Attachment: Bug 1321528: Part 1 - Support suspending requests when processing response headers.

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

Comment 10

[Kris Maglione [:kmag]]

Attachment: Bug 1321528: Part 2 - Add tests for response header modification timing.

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

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8817230 [details]
Bug 1321528: Part 2 - Add tests for response header modification timing.

https://reviewboard.mozilla.org/r/97602/#review98008

Comment 12

[Honza Bambas (:mayhemer)]

Kris, I need a good description what the patches are doing to do a proper review and understand the purpose of the changes.  Please provide it.

Comment 13

[Kris Maglione [:kmag]]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> Kris, I need a good description what the patches are doing to do a proper
> review and understand the purpose of the changes.  Please provide it.

The basic problem is this: Extension request listeners are all asynchronous. Since extension code is designed to run in a separate, sandboxed process, any request listener that needs to make changes needs to be registered as a blocking listener. When those listeners are triggered, we need to suspend the request, trigger the listener in the extension process, make any changes, and then resume the request.

In the case of response header changes, though, the effect of suspending the request happens too late, and the response headers are processed before the changes made by the listeners have been applied. In the particular case reported in this bug, the problem is that onStartRequest is being called before the header changes have been applied. There are other cases, nearer to the changed code, where cookie and auth headers are likewise handled too early.

This patch changes the response handling flow so that if the request is suspended by an http-on-examine-response observer, further processing is delayed until the request is resumed.

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 8817229 [details]
Bug 1321528: Part 1 - Support suspending requests when processing response headers.

https://reviewboard.mozilla.org/r/97600/#review99100

r=honzab with all the comments addressed and conditioned by a green try run (something I don't see a reference to in the bug so far).

::: netwerk/protocol/http/nsHttpChannel.h:293
(Diff revision 1)
>      nsresult SetupTransaction();
>      void     SetupTransactionRequestContext();
>      nsresult CallOnStartRequest();
>      nsresult ProcessResponse();
> +    void     AsyncContinueProcessResponse();
> +    nsresult ContinueProcessResponse0();

oh, please renumber :)  next time we can't add ContinueProcessResponse-1() ;)

::: netwerk/protocol/http/nsHttpChannel.cpp:2004
(Diff revision 1)
> +}
> +
> +void
> +nsHttpChannel::AsyncContinueProcessResponse()
> +{
> +    Unused << NS_WARN_IF(NS_FAILED(ContinueProcessResponse0()));

when ContinueProcessResponse0() fails, we must cancel the channel.

because w/o the patch when something fails in this part, it's being passed as a return value of the OnStartRequest call which, when failed, cancels the channel.

hence, AsyncContinueProcessResponse() should more look like:

nsresult rv = ContinueProcessResponse0();
if (NS_FAILED(rv)) {
  // a nice comment _why_ you have to do this
  Cancel(rv);
}

::: netwerk/protocol/http/nsHttpChannel.cpp:2012
(Diff revision 1)
> +nsresult
> +nsHttpChannel::ContinueProcessResponse0()
> +{
> +    if (mSuspendCount) {
> +        LOG(("Waiting until resume to finish processing response [this=%p]\n", this));
> +        mCallOnResume = &nsHttpChannel::AsyncContinueProcessResponse;

please assert that mCallOnResume is null prior your assignment.

::: netwerk/protocol/http/nsHttpChannel.cpp:2057
(Diff revision 1)
>          MOZ_ASSERT(!mOnStartRequestCalled);
>          nsCOMPtr<nsIURI> redirectTo;
>          mAPIRedirectToURI.swap(redirectTo);
>  
>          PushRedirectAsyncFunc(&nsHttpChannel::ContinueProcessResponse1);
> -        rv = StartRedirectChannelToURI(redirectTo, nsIChannelEventSink::REDIRECT_TEMPORARY);
> +        nsresult rv = StartRedirectChannelToURI(redirectTo, nsIChannelEventSink::REDIRECT_TEMPORARY);

if you want to redeclare, you must write a comment why!

if this is the first use of rv then put "nsresult rv;" as the very first declaration into the method.  despite there is the rule in our style to "declare on first use" I always do an exception for "rv" to prevent nasty and hard to detect rv redeclare bugs, since it's usually used (or may become used in future) more than once in a method.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Kris Maglione [:kmag] from comment #13)
> (In reply to Honza Bambas (:mayhemer) from comment #12)
> > Kris, I need a good description what the patches are doing to do a proper
> > review and understand the purpose of the changes.  Please provide it.
> 
> The basic problem is this: Extension request listeners are all asynchronous.

And thanks for this description, made the review way easier!

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8817229 [details]
Bug 1321528: Part 1 - Support suspending requests when processing response headers.

https://reviewboard.mozilla.org/r/97600/#review99100

Yup, I just took it for granted that a green try run was a precondition.

> oh, please renumber :)  next time we can't add ContinueProcessResponse-1() ;)

Sure, I just figured 0 was as good a starting point as any :) I'll renumber, though.

Comment 17

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7a10b7c12175551f87fa69d2a1bc7c1cd438cf5
Bug 1321528: Part 1 - Support suspending requests when processing response headers. r=honzab

https://hg.mozilla.org/integration/mozilla-inbound/rev/95bb4ca0e110cd8f07e7f355d71de8b7caab5121
Bug 1321528: Part 2 - Add tests for response header modification timing. r=mixedpuppy

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f7a10b7c1217
https://hg.mozilla.org/mozilla-central/rev/95bb4ca0e110

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8817229 [details]
Bug 1321528: Part 1 - Support suspending requests when processing response headers.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1305217

[User impact if declined]: This bug prevents certain response header modifications from WebExtensions from having the expected effect.

[Is this code covered by automated tests?]: Yes, the related code is covered by tests, and these patches add new tests for this issue.

[Has the fix been verified in Nightly?]: No, but I manually verified in a local build.
[Needs manual test from QE? If yes, steps to reproduce]: With the extension in attachment 8816653 [details] installed, visiting any page at stackoverflow.com should cause a download prompt rather than displaying the page.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: Low-risk.
[Why is the change risky/not risky?]: For the normal case, the behavior should be exactly the same as it was before. When a blocking response header listener is in use, the only difference in behavior is that we suspend the request slightly earlier in the response handling chain, so that its changes have the expected effect.

[String changes made/needed]: None.

Comment 20

[Kris Maglione [:kmag]]

Can you verify that this issue is fixed in the latest nightly?

Thanks.

Comment 21

[esterTion]

Yes, it's fixed now in nightly. 
Aurora build still have this bug, though. 
Will that be pushed in next version increase? 

Anyway, thanks for the works and have a nice day.

Comment 22

[Julien Cristau [:jcristau]]

Comment on attachment 8817229 [details]
Bug 1321528: Part 1 - Support suspending requests when processing response headers.

webextensions request processing fix, for aurora52

Comment 23

[Julien Cristau [:jcristau]]

(In reply to lcz970 from comment #21)
> Yes, it's fixed now in nightly. 
> Aurora build still have this bug, though. 

Setting status to verified for 53.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1c4863a4ff98
https://hg.mozilla.org/releases/mozilla-aurora/rev/4cdf04be5d8e

Comment 26

[esterTion]

And, Aurora Dec-22 build finally got the fix. 
Thanks for all the works again! :)

Comment 27

[github]

The nightly 53 build fixes the problem we reported in Bug 1324979.