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

Description James Simonsen 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.
Comment 1 pdeng6 2012-09-03 07:31:14 PDT
Created attachment 161925 [details]
Patch
Comment 2 pdeng6 2012-10-23 07:12:16 PDT
Created attachment 170155 [details]
Patch
Comment 3 James Simonsen 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.
Comment 4 James Simonsen 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.
Comment 5 James Simonsen 2012-11-30 16:05:42 PST
Created attachment 177042 [details]
Patch
Comment 6 James Simonsen 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.
Comment 7 WebKit Review Bot 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>
Comment 8 WebKit Review Bot 2012-12-02 03:24:01 PST
All reviewed patches have been landed.  Closing bug.