User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.101 Safari/537.36

Before Resource Timing will be prefed on, the "timing allow check algorithm" algorithm must be implemented. This will allow tracking of cross-origin resources.

Comment on attachment 8417772 [details] [diff] [review] Implement the _timing allow check algorithm_ for cross-origin Resource Timing This doesn't implement the redirect handling the spec calls for, as far as I can tell...

And also, using the referrer for the origin is almost certainly wrong. But the spec's language here is pretty insane anyway; I sent in a spec comment.

(In reply to Boris Zbarsky [:bz] from comment #4) > Comment on attachment 8417772 [details] [diff] [review] > Implement the _timing allow check algorithm_ for cross-origin Resource Timing > > This doesn't implement the redirect handling the spec calls for, as far as I > can tell... Could you briefly explain what the redirect handling should do? I don't seem to understand it from the spec.

So here's an example. Say site A does a request to site B, which redirects to site C. Site C sends the requisite Timing-Allow-Origin headers. Per spec, redirectStart/End need to end up as 0 in this case. Does that happen with your patch?

Indeed, (In reply to Boris Zbarsky [:bz] from comment #7) > Per spec, redirectStart/End need to end up as 0 in this case. Does that > happen with your patch? Indeed, it didn't really do that. I also found a related issue and filed Bug 1006575.

Comment on attachment 8422918 [details] [diff] [review] Implement the _timing allow check algorithm_ for cross-origin Resource Timing Relevant changes: Moved the timing-allow-check implementation to HttpBaseChannel Renamed several methods in nsPerformance to better illustrate what they do The redirect chain is checked in HttpBaseChannel::SetupReplacementChannel (except for the last redirect, which is still checked in nsPerformance)

Comment on attachment 8422918 [details] [diff] [review] Implement the _timing allow check algorithm_ for cross-origin Resource Timing Why do we need the mRedirectedFrom thing? Why are we representing origins as URIs instead of either principals or actual origin strings? Why do we need all this state on channels in general? As far as I can tell, this is not implementing the intent of the spec, which is that we only expose timing info for channels which are either same-origin with the page or whose Timing-Allow-Origin lists the origin of the _page_ (afaict). But good to have that part clarified before implementing this...

Comment on attachment 8425595 [details] [diff] [review] Part 2: Fix old tests for resource_timing > +++ b/dom/tests/mochitest/general/mochitest.ini How does that work right now? We're running the paste_selection test on b2g? >+++ b/dom/tests/mochitest/general/resource_timing_iframe.html If you just want to make sure these are same-origin loads, please document that. And maybe use location.host to generate the hostname... I guess this is ok, though. r=me

>r=me Modulo sorting out that paste_selection thing!

Comment on attachment 8425598 [details] [diff] [review] Part 3: Add tests for cross origin resource timing check Let's decide on the behavior we want before we worry about the tests.

(In reply to Boris Zbarsky [:bz] from comment #16) > Comment on attachment 8425595 [details] [diff] [review] > Part 2: Fix old tests for resource_timing > > > +++ b/dom/tests/mochitest/general/mochitest.ini > > How does that work right now? We're running the paste_selection test on b2g? At the top of the file there's: [DEFAULT] skip-if = e10s From what I know, b2g is running with e10s, so I think that's the reason this still worked.

(In reply to Boris Zbarsky [:bz] from comment #15) > Comment on attachment 8422918 [details] [diff] [review] > Implement the _timing allow check algorithm_ for cross-origin Resource Timing > > Why do we need the mRedirectedFrom thing? Why are we representing origins > as URIs instead of either principals or actual origin strings? Why do we > need all this state on channels in general? > mRedirectedFrom and the other bits I added to mTimedChannel are needed because I need to do the timing-allow-check for every redirect. I can't do it at the end, because I don't have access to the redirect chain (Bug 974018 will probably fix that). I also can't do it when I create the new channel, because I need the response headers for the request. So the way I do it is: Lets say site A redirects to B which redirects to C 1. mRedirectedFrom saves the last URI (I could save the origin string instead, if you think that's better) 2. When we setup channel B, we don't check anything yet, because redirectedFrom is null. 3. When we setup channel C, we check that B and A are same origin, or if the timing header is there. 4. When we create the nsPerformanceTiming object, we check that C and B (last redirect) are same origin All of these checks are passed on by passesTimingAllowCheck attribute. This determines if we report redirectStart/End. 5. Also when we create the nsPerformanceTiming object, we check that C and A are same origin, which determines if we report all of the other timing info. > As far as I can tell, this is not implementing the intent of the spec, which > is that we only expose timing info for channels which are either same-origin > with the page or whose Timing-Allow-Origin lists the origin of the _page_ > (afaict). But good to have that part clarified before implementing this... Is this something I misunderstood in the spec? Or are you objecting to adding stuff to nsITimedChannel? I guess we don't need all of all of the extra attributes in nsITimedChannel once bug 974018 lands. Thanks

> I can't do it at the end, because I don't have access to the redirect chain (Bug 974018 > will probably fix that). I also can't do it when I create the new channel, because I need > the response headers for the request. It seems like you should be able to do it on redirects for the old channel and at the end for the last channel in the chain.... Am I just not understanding the spec? > mRedirectedFrom saves the last URI Why is this something we care about? > 2. When we setup channel B, we don't check anything yet, because redirectedFrom is null. We should be checking the headers for A, unless it's same-origin with the page, I'd think. > 3. When we setup channel C, we check that B and A are same origin, or if the timing > header is there. Afaict, that's not what the spec says to do. Or at least it's certainly not what the proposed spec clarification at http://lists.w3.org/Archives/Public/public-web-perf/2014May/0053.html says to do. > Is this something I misunderstood in the spec? I think so...

(In reply to Boris Zbarsky [:bz] from comment #21) > > 3. When we setup channel C, we check that B and A are same origin, or if the timing > > header is there. > > Afaict, that's not what the spec says to do. Or at least it's certainly not > what the proposed spec clarification at > http://lists.w3.org/Archives/Public/public-web-perf/2014May/0053.html says > to do. > > > Is this something I misunderstood in the spec? > > I think so... I understand now. I was under the impression that we should check each redirect against the hostname of the previous channel. I will post an updated patch checking each redirect against the referrer.

Not against the referrer. Against the principal that started the loads, please. (Whatever that means for images, btw, since those are shared across documents.)

Will do. Thanks!

Comment on attachment 8422918 [details] [diff] [review] Implement the _timing allow check algorithm_ for cross-origin Resource Timing Review of attachment 8422918 [details] [diff] [review]: ----------------------------------------------------------------- I give up. This is underdocumented and overcoplicated and probably well wrong patch. Please update with comments and explanation how this has to work. I also checked the Boris r- comments and I have to fully agree with him. ::: dom/base/nsPerformance.h @@ +220,5 @@ > // TimeStamp (results are absolute timstamps - wallclock); (2) "0" (results > // are relative to the navigation start). > DOMHighResTimeStamp mZeroTime; > + bool mTimingAllowed; > + bool mReportCrossOriginRedirect; comments! ::: netwerk/base/public/nsITimedChannel.idl @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #include "nsISupports.idl" > +#include "nsIURI.idl" just interface nsIURI; will do @@ +48,5 @@ > [noscript] attribute boolean allRedirectsSameOrigin; > + // This flag is set to false if the timing allow check fails > + [noscript] attribute boolean passesTimingAllowCheck; > + // Throws error if the check fails > + [noscript] void timingAllowCheck(in nsIURI origin); I'd rather return bool ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +1946,5 @@ > + nsCOMPtr<nsIURI> previousURL; > + GetRedirectedFrom(getter_AddRefs(previousURL)); > + if (!previousURL) { > + previousURL = mReferrer; > + } Isn't mReferrer (that is carried forward in the redirect chain) good enough for you? I'd like to avoid the "redirected from" thing if possible. If it's not possible to go w/o it, then explain. @@ +1949,5 @@ > + previousURL = mReferrer; > + } > + > + bool passesTimingCheck = mPassesTimingAllowCheck && > + NS_SUCCEEDED(oldTimedChannel->TimingAllowCheck(previousURL)); Isn't oldTimedChannel == this? should you rather check on the newchannel? http://c1/doc refers http://c1/css that redirs as: http://c1/css -> http://c2/css2 -> http://c2/css3 on the first redir from c1 to c2 in TimingAllowCheck: aOrigin = http://c1(/doc) = mReferrer resourceURI = http://c1/css = the old channel mURI passes, but I think shouldn't! on the second redir from css2 to css3: aOrigin = http://c1(/css) = oldchannel (css2) redirected from resourceURI = http://c2/css3 correctly fails then I also think this should rather work with principals and not just urls if possible. @@ +2105,5 @@ > + } > + > + nsAutoCString headerValue; > + rv = GetResponseHeader(nsAutoCString("Timing-Allow-Origin"), headerValue); > + if (NS_SUCCEEDED(rv)) { I'd prefer if (NS_FAILED(rv)) return NS_ERROR_BAD_.... @@ +2139,5 @@ > + first = second; > + } > + if (Substring(first, second) == origin) { // Last token > + return NS_OK; > + } needs a test

(In reply to Boris Zbarsky [:bz] from comment #23) > Not against the referrer. Against the principal that started the loads, > please. > > (Whatever that means for images, btw, since those are shared across > documents.) (In reply to Honza Bambas (:mayhemer) from comment #25) > Isn't oldTimedChannel == this? should you rather check on the newchannel? > I used oldTimedChannel (instead of the implicit this) to be consistent with the rest of the code in SetupReplacementChannel. Also, I can't do the check for newTimedChannel at this point because it hasn't been opened so we don't have the response headers. > > then I also think this should rather work with principals and not just urls > if possible. > Do you think it's possible to get _the principal that started the loads_ from the channel? Would it be correct to use mOriginalURI instead?

> Do you think it's possible to get _the principal that started the loads_ from the channel? After bug 965413 gets fixed and we propagate that stuff to all loads... > Would it be correct to use mOriginalURI instead? No.

Thanks!

(In reply to Valentin Gosu [:valentin] from comment #30) > Created attachment 8456147 [details] [diff] [review] > Implement the _timing allow check algorithm_ for cross-origin Resource Timing Valentin, few words/points on what the patch is doing, what has changed from the last version would be really nice. Just throwing patches like this makes reviewer's life harder.

(In reply to Honza Bambas (:mayhemer) from comment #31) > (In reply to Valentin Gosu [:valentin] from comment #30) > > Created attachment 8456147 [details] [diff] [review] > > Implement the _timing allow check algorithm_ for cross-origin Resource Timing > > Valentin, few words/points on what the patch is doing, what has changed from > the last version would be really nice. Just throwing patches like this > makes reviewer's life harder. Sure thing. I will address Jason's comments, and upload a new version with appropriate comments and interdiff. Thanks!

Comment on attachment 8456147 [details] [diff] [review] Implement the _timing allow check algorithm_ for cross-origin Resource Timing Dropping the review, as I understand, a newer patch should come.

Comment on attachment 8460682 [details] [diff] [review] Part 1: Implement the _timing allow check algorithm_ for cross-origin Resource Timing The nsPerformanceTiming::CheckAllowedOrigin and HttpBaseChannel::SetupReplacementChannel methods now call to HttpBaseChannel::TimingAllowCheck with the URI that started the load which we get from loadInfo->LoadingPrincipal() mPassesTimingAllowCheck is true if all redirects pass the check. ReportCrossOriginRedirect will return true if all redirects pass the check and the final response passes the check.

Comment on attachment 8456145 [details] [diff] [review] Part 3: Add tests for cross origin resource timing check Updated the tests to check for correct behaviour and new usecases.

I'm sorry for the terrible lag here. I will make sure to do this ASAP when I get back. :(

Comment on attachment 8460682 [details] [diff] [review] Part 1: Implement the _timing allow check algorithm_ for cross-origin Resource Timing Review of attachment 8460682 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsPerformance.cpp @@ +45,5 @@ > // is being used for the navigation timing (document) and has a non-null > // value for the resource timing (any resources within the page). > if (aHttpChannel) { > + mTimingAllowed = CheckAllowedOrigin(aHttpChannel); > + bool redirectsPassCheck = true; assume false? @@ +96,5 @@ > + nsCOMPtr<nsIPrincipal> principal = loadInfo->LoadingPrincipal(); > + nsCOMPtr<nsIURI> initialURI; > + principal->GetURI(getter_AddRefs(initialURI)); > + > + return mChannel->TimingAllowCheck(initialURI); add comment what this actually does (what all kind of checks) ::: dom/base/nsPerformance.h @@ +140,5 @@ > > uint16_t GetRedirectCount() const; > + bool TimingAllowed() const; > + bool CheckAllowedOrigin(nsIHttpChannel* aResourceChannel); > + bool ReportCrossOriginRedirect() const; comments... @@ +221,5 @@ > // TimeStamp (results are absolute timstamps - wallclock); (2) "0" (results > // are relative to the navigation start). > DOMHighResTimeStamp mZeroTime; > + bool mTimingAllowed; > + bool mReportCrossOriginRedirect; comments... ::: netwerk/base/public/nsITimedChannel.idl @@ +46,5 @@ > > // This flag should be set to false only if a cross-domain redirect occurred > [noscript] attribute boolean allRedirectsSameOrigin; > + // This flag is set to false if the timing allow check fails > + [noscript] attribute boolean passesTimingAllowCheck; please be more decriptive, also the name doesn't seem to be the best @@ +48,5 @@ > [noscript] attribute boolean allRedirectsSameOrigin; > + // This flag is set to false if the timing allow check fails > + [noscript] attribute boolean passesTimingAllowCheck; > + // Throws error if the check fails > + [noscript] boolean timingAllowCheck(in nsIURI origin); throws or returns false? what "the check" is? ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +2083,5 @@ > + nsCOMPtr<nsIPrincipal> principal = loadInfo->LoadingPrincipal(); > + nsCOMPtr<nsIURI> initialURI; > + principal->GetURI(getter_AddRefs(initialURI)); > + newTimedChannel->SetPassesTimingAllowCheck(mPassesTimingAllowCheck && > + oldTimedChannel->TimingAllowCheck(initialURI)); don't you want newTimedChannel->TimingAllowCheck() ? this is all weird. I'd assume you want to check the new URI (the one we are redirecting to) against the old channel's principal's origin and then check the old (or rather always just the first? - not sure about what the spec says) channel's Timing-Allow-Origin header against the origin we are redirecting to. or not? what does the spec says about redirects exactly? @@ +2245,5 @@ > + aOrigin->GetScheme(origin); > + origin.Append("://"); > + nsAutoCString host; > + aOrigin->GetHostPort(host); > + origin.Append(host); am not sure this code is the best. Hmm.. we should consider to move/copy http://hg.mozilla.org/mozilla-central/annotate/d697d649c765/content/base/src/nsContentUtils.cpp#l5739 @@ +2250,5 @@ > + > + nsACString::const_iterator first, end; > + headerValue.BeginReading(first); > + headerValue.EndReading(end); > + nsACString::const_iterator second(first); please at least name these some more descriptively... @@ +2264,5 @@ > + } > + if (Substring(first, second) == origin) { // Last token > + *_retval = true; > + return NS_OK; > + } the parser needed!!! ::: netwerk/protocol/http/HttpBaseChannel.h @@ +334,5 @@ > uint32_t mLoadUnblocked : 1; > uint32_t mResponseTimeoutEnabled : 1; > // A flag that should be false only if a cross-domain redirect occurred > uint32_t mAllRedirectsSameOrigin : 1; > + uint32_t mPassesTimingAllowCheck : 1; comment what this is, how does it change, what influence it has... more the better (I still have to repeat myself on and on on adding comments....)

(In reply to Honza Bambas (:mayhemer) from comment #38) > > ::: netwerk/protocol/http/HttpBaseChannel.cpp > @@ +2083,5 @@ > > + nsCOMPtr<nsIPrincipal> principal = loadInfo->LoadingPrincipal(); > > + nsCOMPtr<nsIURI> initialURI; > > + principal->GetURI(getter_AddRefs(initialURI)); > > + newTimedChannel->SetPassesTimingAllowCheck(mPassesTimingAllowCheck && > > + oldTimedChannel->TimingAllowCheck(initialURI)); > > don't you want newTimedChannel->TimingAllowCheck() ? > > this is all weird. I'd assume you want to check the new URI (the one we are > redirecting to) against the old channel's principal's origin and then check > the old (or rather always just the first? - not sure about what the spec > says) channel's Timing-Allow-Origin header against the origin we are > redirecting to. or not? > > what does the spec says about redirects exactly? > We can't do the check on the newChannel at this point, as the request hasn't been made yet, and there are no response headers to check against.

Can you please look into the spec, and outline here what this has to do? Maybe I just don't follow and the code is correct.

Relevant chapters would be http://www.w3.org/TR/resource-timing/#timing-allow-check point 3, and http://www.w3.org/TR/resource-timing/#processing-model point 19. When a redirect occurs, we need to check the headers for that redirect, and if the check fails, we need to set redirectStart/End to 0. We do this check in SetupReplacementChannel (which coresponds to point 19), and then we do another check in the nsPerformanceTiming constructor - for the final resource.

(In reply to Valentin Gosu [:valentin] from comment #41) > Relevant chapters would be > http://www.w3.org/TR/resource-timing/#timing-allow-check point 3, and > http://www.w3.org/TR/resource-timing/#processing-model point 19. > > When a redirect occurs, we need to check the headers for that redirect, and Check what against which response headers? > if the check fails, we need to set redirectStart/End to 0. We do this check > in SetupReplacementChannel (which coresponds to point 19), and then we do > another check in the nsPerformanceTiming constructor - for the final > resource.

So lets say Page A.com loads Resource B from a different origin (B.com). Loading Resource B - we get a 3XX redirect to C.com, and a 'Timing-Allow-Origin: domain.com' response header. In SetupReplacementChannel, if the domain doesn't match to A.com, mPassesTimingAllowCheck will be set to false (which will lead to redirectStart/End being 0 in the resourceTiming object). When we load the final resource from C.com, we add a new PerformanceTiming object, and we do another check, if C.com's response headers include A.com, and we set mTimingAllowed accordingly.

Addressed all of the issues in comment 38. Better comments and naming.

Rebased on top of m-c. Reviewed by :bz in comment 17

Rebased on top of m-c. Better description for the tests

Comment on attachment 8481731 [details] [diff] [review] Part 1: Implement the _timing allow check algorithm_ for cross-origin Resource Timing Review of attachment 8481731 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/public/nsITimedChannel.idl @@ +49,5 @@ > + // This flag is set to false if the timing allow check fails > + [noscript] attribute boolean allRedirectsPassTimingAllowCheck; > + // Implements the timing-allow-check to determine if we should report > + // timing info for the resourceTiming object. > + [noscript] boolean timingAllowCheck(in nsIURI origin); it's a custom to describe the arguments as well. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +2096,5 @@ > + nsCOMPtr<nsIURI> initialURI; > + principal->GetURI(getter_AddRefs(initialURI)); > + newTimedChannel->SetAllRedirectsPassTimingAllowCheck( > + mAllRedirectsPassTimingAllowCheck && > + oldTimedChannel->TimingAllowCheck(initialURI)); so, if I understand, here you are checking the redirect response is either the same origin resource (the request URL) or has set the Timing-Allow-Origin to something the check algo passes for. is that so? It happens here since there is no other place in the timing code where check on a redirect response might happen, right? @@ +2259,5 @@ > + > + nsACString::const_iterator firstIterator, end; > + headerValue.BeginReading(firstIterator); > + headerValue.EndReading(end); > + nsACString::const_iterator secondIterator(firstIterator); :) I was more thinking about originStart, originEnd or so... "Iterator" is really not adding any more description.

Comment on attachment 8481731 [details] [diff] [review] Part 1: Implement the _timing allow check algorithm_ for cross-origin Resource Timing Boris, could you also take a look at the dom parts? Thanks!

Comment on attachment 8481731 [details] [diff] [review] Part 1: Implement the _timing allow check algorithm_ for cross-origin Resource Timing >+ bool TimingAllowed() const; >+ bool CheckAllowedOrigin(nsIHttpChannel* aResourceChannel); These need documentation. >+ bool ReportCrossOriginRedirect() const; Maybe ShouldReportCrossOriginRedirect? >+ [noscript] boolean timingAllowCheck(in nsIURI origin); Why is this taking nsIURI, not nsIPrincipal? >+ nsresult rv = ssm->CheckSameOriginURI(resourceURI, aOrigin, false); So I think this should take an nsIPrincipal for aOrigin. For now, you could then do a CheckMayLoad against our channel's URI. But once the GetChannelURIPrincipal bits in bug 1038756 land (which might be tomorrow morning; I asked people to expedite those) you could use that and then just nsIPrincipal::Equals. >+ rv = GetResponseHeader(nsAutoCString("Timing-Allow-Origin"), headerValue); NS_LITERAL_CSTRING, please. >+ if (headerValue == "null" || headerValue == "") { >+ *_retval = false; That's not what the spec says. Why do this check at all? >+ // Search through the list for the origin what list? Spec says to just compare the header value to the origin; it doesn't allow putting a list in Timing-Allow-Origin. Am I just missing something? The rest seems ok, I think.

Comment on attachment 8481733 [details] [diff] [review] Part 3: Add tests for cross origin resource timing check Not going to worry about this until the "list of origins" thing is sorted out, though do please add all those missing trailing newlines.

(In reply to Boris Zbarsky [:bz] from comment #49) > Comment on attachment 8481731 [details] [diff] [review] > > >+ // Search through the list for the origin > > what list? Spec says to just compare the header value to the origin; it > doesn't allow putting a list in Timing-Allow-Origin. Am I just missing > something? > I had noticed the note on http://www.w3.org/TR/resource-timing/#timing-allow-origin , but wasn't sure whether that meant there would always be only one origin and why. Chrome allows for several origins in the list. But you're right. This goes against what the spec says.

Please file a bug on Chrome!

Comment on attachment 8485285 [details] [diff] [review] Part 1: Implement the _timing allow check algorithm_ for cross-origin Resource Timing >+ nsCOMPtr<nsIPrincipal> resourcePrincipal = GetPrincipal(false); This will do the wrong thing for things like cross-origin XHR. This really needs to do GetChannelURIPrincipal() on the security manager. r=me with that fixed.

Comment on attachment 8485464 [details] [diff] [review] Part 3: Add tests for cross origin resource timing check >+SpecialPowers.setBoolPref("dom.enable_resource_timing", true); You should probably use pushPrefEnv instead... That might work in b2g too, if the real issue this test has there is the sync pref set, no? And then run the rest of the test in a subframe created after the pref is set, because what the test is doing now (assuming no one touches Performance before the pref is set) is really fragile. So fragile, in fact, that I'm pretty sure the fix for bug 1017425 violated that assumption. >+ makeXhr("test-data.json", firstCheck); Why is this XHR needed? Worth documenting. >+ if (entry == undefined) >+ return; Document why this is needed? And perhaps hoist it out of the loop over properties? This is only testing <object>. Can you at least test XHR as well? That would have caught the bug in how you were getting the channel principal, I think... r=me with those fixes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/17f69581d758 https://hg.mozilla.org/integration/mozilla-inbound/rev/8a4f92aad7fc https://hg.mozilla.org/integration/mozilla-inbound/rev/315b12a63b13

https://hg.mozilla.org/mozilla-central/rev/17f69581d758 https://hg.mozilla.org/mozilla-central/rev/8a4f92aad7fc https://hg.mozilla.org/mozilla-central/rev/315b12a63b13