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

VERIFIED FIXED in Firefox 52

Status

defect
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: lcz970, Assigned: kmag)

Tracking

52 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox53 verified)

Details

Attachments

(4 attachments)

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.
Summary: onHeadersReceived with blocking permission not working on Firefox 52+ → WebExtension onHeadersReceived with blocking permission not working on Firefox 52+
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
Component: WebExtensions: General → WebExtensions: Request Handling
This is an intentional change, for security reasons. Sorry.
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
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.
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.
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?
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.
That's great! 
Thanks for the works.
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".
Status: RESOLVED → UNCONFIRMED
Resolution: WONTFIX → ---
Summary: WebExtension onHeadersReceived with blocking permission not working on Firefox 52+ → Changes to Content-Disposition response header are too late to have an effect
Comment on attachment 8817230 [details]
Bug 1321528: Part 2 - Add tests for response header modification timing.

https://reviewboard.mozilla.org/r/97602/#review98008
Attachment #8817230 - Flags: review?(mixedpuppy) → review+
Assignee: nobody → kmaglione+bmo
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.
Flags: needinfo?(kmaglione+bmo)
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(kmaglione+bmo)
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.
Attachment #8817229 - Flags: review?(honzab.moz) → review+
(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 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.
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
https://hg.mozilla.org/mozilla-central/rev/f7a10b7c1217
https://hg.mozilla.org/mozilla-central/rev/95bb4ca0e110
Status: NEW → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.
Attachment #8817229 - Flags: approval-mozilla-aurora?
Can you verify that this issue is fixed in the latest nightly?

Thanks.
Flags: needinfo?(lcz970)
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.
Flags: needinfo?(lcz970)
Comment on attachment 8817229 [details]
Bug 1321528: Part 1 - Support suspending requests when processing response headers.

webextensions request processing fix, for aurora52
Attachment #8817229 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1324979
And, Aurora Dec-22 build finally got the fix. 
Thanks for all the works again! :)
The nightly 53 build fixes the problem we reported in Bug 1324979.
Depends on: 1401516
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.