Bug 84886 - [Resource Timing] Implement cross-origin restrictions
Summary: [Resource Timing] Implement cross-origin restrictions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Simonsen
URL: http://dvcs.w3.org/hg/webperf/raw-fil...
Keywords:
Depends on: 84883
Blocks: 61138
  Show dependency treegraph
 
Reported: 2012-04-25 12:41 PDT by James Simonsen
Modified: 2012-12-02 03:24 PST (History)
5 users (show)

See Also:


Attachments
Patch (35.46 KB, patch)
2012-09-03 07:31 PDT, pdeng6
no flags Details | Formatted Diff | Diff
Patch (22.91 KB, patch)
2012-10-23 07:12 PDT, pdeng6
no flags Details | Formatted Diff | Diff
Patch (20.24 KB, patch)
2012-11-30 16:05 PST, James Simonsen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.