Closed
Bug 1441336
Opened 6 years ago
Closed 6 years ago
PerformanceObserver CORS restrictions in WebExtensions Content scripts despite appropriate Host permissions
Categories
(WebExtensions :: General, defect, P2)
Tracking
(firefox61 verified)
VERIFIED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | verified |
People
(Reporter: mozilla+bugzilla, Assigned: zombie)
Details
Attachments
(3 files)
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0 Build ID: 20180208175058 Steps to reproduce: Created fresh profile. Added WebExtension: manifest.json: ---- "permissions": [ "<all_urls>" ], "content_scripts": [ { "matches": ["<all_urls>"], "js": ["contentscript.js"], "run_at": "document_start", "all_frames": true } ] ---- contentscript.js ---- const observer = new PerformanceObserver((list) => { list.getEntries().map(entry => { if (!entry.name.startsWith('https://jsfiddle.net')) { console.log(entry.transferSize); } }); }); observer.observe({entryTypes: ['resource']}); --- Now navigate to https://jsfiddle.net/ Actual results: The console logs only 0 Expected results: The console should log the actual transferSize Note: This is probably because of the CORS restrictions for some properties provided by PerformanceObserver, including transferSize: https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/transferSize Since Host permissions should overwrite CORS restrictions I would assume that in this case the actual transferSize is provided.
Updated•6 years ago
|
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Updated•6 years ago
|
Status: UNCONFIRMED → NEW
Component: WebExtensions: Untriaged → WebExtensions: General
Ever confirmed: true
Priority: -- → P2
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8965418 -
Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8965884 -
Flags: review?(kmaglione+bmo)
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8965418 [details] Bug 1441336 - Use addon permissions for PerformanceTiming properties https://reviewboard.mozilla.org/r/234176/#review240418 ::: dom/performance/PerformanceResourceTiming.h:76 (Diff revision 5) > - return mTimingData && mTimingData->ShouldReportCrossOriginRedirect() > - ? mTimingData->RedirectStartHighRes(mPerformance) > - : 0; > + bool allowed = mTimingData && > + (mTimingData->ShouldReportCrossOriginRedirect() || > + AllowedForCaller(aSubjectPrincipal)); This is repeated a lot. Maybe add a private helper method for it? Also, please remove trailing space :) ::: dom/performance/PerformanceResourceTiming.h:95 (Diff revision 5) > + return allowed ? mTimingData->RedirectEndHighRes(mPerformance) : 0; > } > > - DOMHighResTimeStamp DomainLookupStart() const { > - return mTimingData && mTimingData->TimingAllowed() > - ? mTimingData->DomainLookupStartHighRes(mPerformance) > + DOMHighResTimeStamp DomainLookupStart(Maybe<nsIPrincipal*>& aSubjectPrincipal) const { > + bool allowed = mTimingData && > + (mTimingData->TimingAllowed() || AllowedForCaller(aSubjectPrincipal)); Probably make this `TimingAllowed(aSubjectPrincipal)` ::: dom/performance/PerformanceResourceTiming.cpp:36 (Diff revision 5) > , mTimingData(Move(aPerformanceTiming)) > , mPerformance(aPerformance) > { > MOZ_ASSERT(aPerformance, "Parent performance object should be provided"); > + > + NS_NewURI(getter_AddRefs(mURI), aName); Can only do this on the main thread. But we don't have principals on worker threads, anyway, so maybe just check IsMainThread, and leave mURI null otherwise.
Attachment #8965418 -
Flags: review?(kmaglione+bmo)
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8965884 [details] Bug 1441336 - Test addon permissions for PerformanceTiming properties https://reviewboard.mozilla.org/r/234708/#review240420 ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_perf_observers.js:20 (Diff revision 1) > + }], > + }, > + files: { > + "cs.js"() { > + let obs = new window.PerformanceObserver(list => { > + list.getEntries().map(e => { Nit: s/map/forEach/ ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_perf_observers.js:30 (Diff revision 1) > + > + let w = window; > + w.eval(` > + let b = document.createElement("link"); > + b.rel = "stylesheet"; > + b.href = "http://b.example.com/file_download.txt"; No eval. `w.wrappedJSObject.href = "...";`; ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_perf_observers.js:32 (Diff revision 1) > + w.eval(` > + let b = document.createElement("link"); > + b.rel = "stylesheet"; > + b.href = "http://b.example.com/file_download.txt"; > + document.head.appendChild(b); > + Nit: Trailing space.
Attachment #8965884 -
Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8965884 [details] Bug 1441336 - Test addon permissions for PerformanceTiming properties https://reviewboard.mozilla.org/r/234708/#review240420 > No eval. `w.wrappedJSObject.href = "...";`; Nice, didn't know that works.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8965418 [details] Bug 1441336 - Use addon permissions for PerformanceTiming properties https://reviewboard.mozilla.org/r/234176/#review240436 Thanks. This also needs review from a DOM peer, of course. ::: dom/performance/PerformanceResourceTiming.h:174 (Diff revision 7) > > size_t > SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const override; > > + bool > + TimingAllowedForCaller(Maybe<nsIPrincipal*>& aCaller, bool aRedirect = false) const Please document this, particularly the aRedirect parameter. ::: dom/performance/PerformanceResourceTiming.h:180 (Diff revision 7) > + if ((aRedirect && mTimingData->ShouldReportCrossOriginRedirect()) || > + (!aRedirect && mTimingData->TimingAllowed())) { This should probably be a ternery or a nested if `aRedirect ? ShouldReport() : TimingAllowed()` ::: dom/performance/PerformanceResourceTiming.cpp:36 (Diff revision 7) > , mTimingData(Move(aPerformanceTiming)) > , mPerformance(aPerformance) > { > MOZ_ASSERT(aPerformance, "Parent performance object should be provided"); > + if (NS_IsMainThread()) { > + NS_NewURI(getter_AddRefs(mURI), aName); Please add a comment about why it's OK for this to be null off-main-thread.
Attachment #8965418 -
Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8965418 -
Flags: review?(bzbarsky)
Comment 17•6 years ago
|
||
mozreview-review |
Comment on attachment 8965418 [details] Bug 1441336 - Use addon permissions for PerformanceTiming properties https://reviewboard.mozilla.org/r/234176/#review240678 ::: dom/performance/PerformanceResourceTiming.h:75 (Diff revision 8) > : 0; > } > > - DOMHighResTimeStamp RedirectStart() const { > + DOMHighResTimeStamp RedirectStart(Maybe<nsIPrincipal*>& aSubjectPrincipal) const { > // We have to check if all the redirect URIs had the same origin (since > // there is no check in RedirectEndHighRes()) This comment should be talking about RedirectStartHighRes (preexisting, I know). ::: dom/performance/PerformanceResourceTiming.h:189 (Diff revision 8) > + > nsString mInitiatorType; > UniquePtr<PerformanceTimingData> mTimingData; > RefPtr<Performance> mPerformance; > + > + nsCOMPtr<nsIURI> mURI; What is this member, exactly? Which URI? ::: dom/performance/PerformanceResourceTiming.cpp:38 (Diff revision 8) > { > MOZ_ASSERT(aPerformance, "Parent performance object should be provided"); > + if (NS_IsMainThread()) { > + // Used to check if an addon content script has access to this timing. > + // We don't need it in workers, and ignore mURI if null. > + NS_NewURI(getter_AddRefs(mURI), aName); OK, so this looks like the initial pre-all-redirects URI, right? ::: dom/performance/PerformanceResourceTiming.cpp:106 (Diff revision 8) > + : mTimingData->TimingAllowed()) { > + return true; > + } > + > + // Check if the addon has permission to access the cross-origin resource. > + return mURI && aCaller.isSome() && Why do we not need to check that the addon can access _all_ URIs in the redirect chain? ::: dom/webidl/PerformanceResourceTiming.webidl:20 (Diff revision 8) > { > readonly attribute DOMString initiatorType; > readonly attribute DOMString nextHopProtocol; > > readonly attribute DOMHighResTimeStamp workerStart; > + readonly attribute DOMHighResTimeStamp fetchStart; Please don't change the order here. The old order matched the spec IDL, and we should continue doing so. If nothing else, changing the order here leads to web-observably different enumeration behavior!
Attachment #8965418 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8965418 [details] Bug 1441336 - Use addon permissions for PerformanceTiming properties https://reviewboard.mozilla.org/r/234176/#review241046 ::: dom/performance/PerformanceResourceTiming.cpp:38 (Diff revision 8) > { > MOZ_ASSERT(aPerformance, "Parent performance object should be provided"); > + if (NS_IsMainThread()) { > + // Used to check if an addon content script has access to this timing. > + // We don't need it in workers, and ignore mURI if null. > + NS_NewURI(getter_AddRefs(mURI), aName); Yes, per the spec for the `name` property, that's `originalURI` from: https://searchfox.org/mozilla-central/rev/b55e1a1c/dom/performance/PerformanceTiming.cpp#56-64 I can rename to make that more explicit. ::: dom/performance/PerformanceResourceTiming.cpp:106 (Diff revision 8) > + : mTimingData->TimingAllowed()) { > + return true; > + } > + > + // Check if the addon has permission to access the cross-origin resource. > + return mURI && aCaller.isSome() && This needs a longer explanation: (from my reading and current understanding of Performance Timing code) There is a big mismatch between the current Performance Timing implementation, and what we would want for this bug. Understandably, all of cross-origin decisions are performed upon creation of `PerformanceTimingData`[1], which is later passed to any and all observer callbacks. And in the world without extensions, this makes sense, since all performance observers that can see a specific TimingData run with the same origin. Extension content scripts obviously mess with this assumption, and since they can be added at any point, that whole approach is invalid, and to get the exact correct behavior would require a bigger refactoring of performance timing code -- something that would be above my head (or at least, which would take me too much time to be worth it). So I figured, perhaps naively, since this is only exposing redirects _timing_ data, and for resources that an extension would otherwise have access to according to the originalURI, this would be acceptable (and extensions might have other ways to get those timings anyway). I can drop that logic for redirect timings if you disagree with that, or if you see an easy way to work around this problem (without major rewrites), I'd be happy to do it. 1) https://searchfox.org/mozilla-central/rev/b55e1a1c/dom/performance/PerformanceTiming.cpp#208-213 ::: dom/webidl/PerformanceResourceTiming.webidl:20 (Diff revision 8) > { > readonly attribute DOMString initiatorType; > readonly attribute DOMString nextHopProtocol; > > readonly attribute DOMHighResTimeStamp workerStart; > + readonly attribute DOMHighResTimeStamp fetchStart; Oops, sorry.
Comment 19•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8965418 [details] Bug 1441336 - Use addon permissions for PerformanceTiming properties https://reviewboard.mozilla.org/r/234176/#review241046 > Yes, per the spec for the `name` property, that's `originalURI` from: > > https://searchfox.org/mozilla-central/rev/b55e1a1c/dom/performance/PerformanceTiming.cpp#56-64 > > I can rename to make that more explicit. Documenting in addition to that would be even better. > This needs a longer explanation: > > (from my reading and current understanding of Performance Timing code) > > There is a big mismatch between the current Performance Timing implementation, and what we would want for this bug. Understandably, all of cross-origin decisions are performed upon creation of `PerformanceTimingData`[1], which is later passed to any and all observer callbacks. And in the world without extensions, this makes sense, since all performance observers that can see a specific TimingData run with the same origin. > > Extension content scripts obviously mess with this assumption, and since they can be added at any point, that whole approach is invalid, and to get the exact correct behavior would require a bigger refactoring of performance timing code -- something that would be above my head (or at least, which would take me too much time to be worth it). > > So I figured, perhaps naively, since this is only exposing redirects _timing_ data, and for resources that an extension would otherwise have access to according to the originalURI, this would be acceptable (and extensions might have other ways to get those timings anyway). > > I can drop that logic for redirect timings if you disagree with that, or if you see an easy way to work around this problem (without major rewrites), I'd be happy to do it. > > > 1) https://searchfox.org/mozilla-central/rev/b55e1a1c/dom/performance/PerformanceTiming.cpp#208-213 > and for resources that an extension would otherwise have access to according to the originalURI That's the question. How do extensions handle access there when redirects are involved?
Comment 20•6 years ago
|
||
God, I hate the quoting behavior of mozreview, and the fact that there's no way to make it sane. :(
Comment 21•6 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #19) > That's the question. How do extensions handle access there when redirects > are involved? In general, for each phase of the request, we check permissions against the final channel URI for that phase, and also against the loading principal. So, even if they don't have permissions for previous phases, they can see phases they do have permissions for.
Comment 22•6 years ago
|
||
OK. So the point is webextensions try to hide information about the no-permissions phases, and this change would allow them for timing information for those phases... I would prefer we not expose stuff to not exposing too much.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8965418 [details] Bug 1441336 - Use addon permissions for PerformanceTiming properties https://reviewboard.mozilla.org/r/234176/#review241502 ::: dom/performance/PerformanceResourceTiming.h:176 (Diff revisions 8 - 9) > - * @return True if the caller should have access to this timing data. > - */ > bool > - TimingAllowedForCaller(Maybe<nsIPrincipal*>& aCaller, bool aCheckRedirects = false) const; > + TimingAllowedForCaller(Maybe<nsIPrincipal*>& aCaller) const; > + > + bool This should probably be documented somewhat in terms of what it's trying to enforce.
Attachment #8965418 -
Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 28•6 years ago
|
||
Pushed by tomica@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c294177f4ec9 Use addon permissions for PerformanceTiming properties r=bz,kmag https://hg.mozilla.org/integration/autoland/rev/0f1e4fe1eb4c Test addon permissions for PerformanceTiming properties r=kmag
Comment 29•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c294177f4ec9 https://hg.mozilla.org/mozilla-central/rev/0f1e4fe1eb4c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 30•6 years ago
|
||
Verified as fixed in latest Nightly. I will attach a before(FF59.02)\after(61.0a1) screenshot where the fix can be seen
Status: RESOLVED → VERIFIED
Comment 31•6 years ago
|
||
Updated•6 years ago
|
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•