Bug 84886

Summary: [Resource Timing] Implement cross-origin restrictions
Product: WebKit Reporter: James Simonsen <simonjam>
Component: PlatformAssignee: James Simonsen <simonjam>
Status: RESOLVED FIXED    
Severity: Normal CC: levin+threading, pan.deng, sullivan, tonyg, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://dvcs.w3.org/hg/webperf/raw-file/tip/specs/ResourceTiming/Overview.html
Bug Depends on: 84883    
Bug Blocks: 61138    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

James Simonsen
Reported 2012-04-25 12:41:09 PDT
Resources fetched from another origin should not appear in detail on the Performance Timeline. Instead, only the startTime and duration attributes should be populated. Optionally, hosts can supply a Timing-Allow-Origin header to override this behavior.
Attachments
Patch (35.46 KB, patch)
2012-09-03 07:31 PDT, pdeng6 		no flags
DetailsFormatted DiffDiff
Patch (22.91 KB, patch)
2012-10-23 07:12 PDT, pdeng6 		no flags
DetailsFormatted DiffDiff
Patch (20.24 KB, patch)
2012-11-30 16:05 PST, James Simonsen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
pdeng6
Comment 1 2012-09-03 07:31:14 PDT
Created attachment 161925 [details] Patch
pdeng6
Comment 2 2012-10-23 07:12:16 PDT
Created attachment 170155 [details] Patch
James Simonsen
Comment 3 2012-11-30 13:58:04 PST
Comment on attachment 170155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170155&action=review > Source/WebCore/page/Performance.cpp:189 > +static bool passesTimingAllowCheck(const ResourceResponse& response, SecurityOrigin* securityOrigin) I think this should happen in the PerformanceResourceTiming constructor. It's really only applicable to that class. > Source/WebCore/page/Performance.cpp:201 > + if (timingAllowOriginString != securityOrigin->toString()) The string may be a space separated list. We should check each item in it. > Source/WebCore/page/Performance.cpp:207 > +void Performance::addResourceTiming(const ResourceRequest& request, const ResourceResponse& response, double finishTime, Document* requestingDocument, SecurityOrigin* initiatorOrigin) We don't need to pass in the origin. You can get it from requestingDocument->securityOrigin(). > Source/WebCore/page/Performance.cpp:213 > + bool allowTiming = WebCore::SecurityOrigin::create(response.url())->equal(initiatorOrigin) || passesTimingAllowCheck(response, initiatorOrigin); allowTiming -> shouldReportDetails > Source/WebCore/page/PerformanceResourceTiming.h:52 > + static PassRefPtr<PerformanceResourceTiming> create(const ResourceRequest& request, const ResourceResponse& response, double finishTime, Document* requestingDocument, bool isAllowTiming) isAllowTiming -> shouldReportDetails > Source/WebCore/page/PerformanceResourceTiming.h:74 > + PerformanceResourceTiming(const ResourceRequest&, const ResourceResponse&, double finishTime, Document*, bool); This needs a name. You can only omit the name if it's clear what it is. > LayoutTests/http/tests/w3c/webperf/submission/Intel/resource-timing/test_resource_timing_timing_allow_cross_origin_resource_request.html:51 > + requestUrl += '?origin=http://' + pageOrigin; We should have a test where it's allowed by this parameter.
James Simonsen
Comment 4 2012-11-30 15:55:49 PST
(In reply to comment #3) > > LayoutTests/http/tests/w3c/webperf/submission/Intel/resource-timing/test_resource_timing_timing_allow_cross_origin_resource_request.html:51 > > + requestUrl += '?origin=http://' + pageOrigin; > > We should have a test where it's allowed by this parameter. Sorry, I misread the code. I saw the 0's but didn't notice the greater_than. I think we're good here.
James Simonsen
Comment 5 2012-11-30 16:05:42 PST
Created attachment 177042 [details] Patch
James Simonsen
Comment 6 2012-11-30 16:06:33 PST
In the interest of getting this landed soon, I made the suggestions. Someone else (Tony?) will have to review it still.
WebKit Review Bot
Comment 7 2012-12-02 03:23:58 PST
Comment on attachment 177042 [details] Patch Clearing flags on attachment: 177042 Committed r136329: <http://trac.webkit.org/changeset/136329>
WebKit Review Bot
Comment 8 2012-12-02 03:24:01 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.