Closed
Bug 1321528
Opened 6 years ago
Closed 6 years ago
Changes to Content-Disposition response header are too late to have an effect
Categories
(WebExtensions :: Request Handling, defect)
Tracking
(firefox52 fixed, firefox53 verified)
VERIFIED
FIXED
mozilla53
People
(Reporter: lcz970, Assigned: kmag)
References
Details
Attachments
(4 files)
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
Assignee | ||
Comment 1•6 years ago
|
||
This is an intentional change, for security reasons. Sorry.
Status: UNCONFIRMED → RESOLVED
Closed: 6 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.
Assignee | ||
Comment 3•6 years ago
|
||
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?
Assignee | ||
Comment 5•6 years ago
|
||
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.
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 → ---
Assignee | ||
Updated•6 years ago
|
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•6 years ago
|
||
mozreview-review |
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+
![]() |
||
Updated•6 years ago
|
Assignee: nobody → kmaglione+bmo
![]() |
||
Comment 12•6 years ago
|
||
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)
Assignee | ||
Comment 13•6 years ago
|
||
(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 14•6 years ago
|
||
mozreview-review |
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+
![]() |
||
Comment 15•6 years ago
|
||
(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!
Assignee | ||
Comment 16•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 17•6 years ago
|
||
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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f7a10b7c1217 https://hg.mozilla.org/mozilla-central/rev/95bb4ca0e110
Status: NEW → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Comment 19•6 years ago
|
||
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?
Assignee | ||
Comment 20•6 years ago
|
||
Can you verify that this issue is fixed in the latest nightly? Thanks.
Flags: needinfo?(lcz970)
Reporter | ||
Comment 21•6 years ago
|
||
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 22•6 years ago
|
||
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+
Comment 23•6 years ago
|
||
(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•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/1c4863a4ff98 https://hg.mozilla.org/releases/mozilla-aurora/rev/4cdf04be5d8e
Reporter | ||
Comment 26•6 years ago
|
||
And, Aurora Dec-22 build finally got the fix. Thanks for all the works again! :)
Comment 27•6 years ago
|
||
The nightly 53 build fixes the problem we reported in Bug 1324979.
Updated•5 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•