Bug 89185 - AssociatedURLLoader should allow trusted clients to read all headers, not just exposed ones
Summary: AssociatedURLLoader should allow trusted clients to read all headers, not jus...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-15 01:22 PDT by Ami Fischman
Modified: 2012-06-15 13:49 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (4.97 KB, patch)
2012-06-15 07:10 PDT, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed patch (4.81 KB, patch)
2012-06-15 07:12 PDT, Bill Budge
abarth: review+
Details | Formatted Diff | Diff
Proposed Patch (4.85 KB, patch)
2012-06-15 11:03 PDT, Bill Budge
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ami Fischman 2012-06-15 01:22:51 PDT
Today AssociatedURLLoader::ClientAdapter::didReceiveResponse() copies the response for the benefit of its client, stripping out blocked headers.  This makes it impossible for any code, trusted or not, to inspect such headers.
An example problem this raises: chromium's media loading code specifies Range: in its request header, so it requests [origin, accept-encoding, referer, range] in the CORS pre-flight request (Access-Control-Request-Headers).  When it receives a 206 response, it wants to use the Content-Range, Content-Length, and Accept-Ranges headers, but it can't do so because these are not "simple headers" (in CORS-speak) and are not explicitly included in Access-Control-Expose-Headers because the server doesn't necessarily know the client requires them, so AssociatedURLLoader::ClientAdapter::didReceiveResponse() strips them out of the response copy handed to BufferedResourceLoader.

Instead, there should be an API for trusted code to side-step this header-stripping.
Comment 1 Bill Budge 2012-06-15 07:10:22 PDT
Created attachment 147805 [details]
Proposed patch
Comment 2 WebKit Review Bot 2012-06-15 07:11:58 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Bill Budge 2012-06-15 07:12:06 PDT
Created attachment 147806 [details]
Proposed patch
Comment 4 Bill Budge 2012-06-15 07:14:06 PDT
Ignore the first patch - it doesn't build.
Comment 5 Bill Budge 2012-06-15 07:21:24 PDT
Adam, this new WebURLLoaderOption field completely turns off response header filtering. Therefore this would expose "set-cookie" headers to the client. I'm assuming this is OK since it's trusted.
Comment 6 Adam Barth 2012-06-15 10:52:27 PDT
Comment on attachment 147806 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147806&action=review

Interesting approach.  I was expecting you to add another function for grabbing the headers, but I see why you've taken this approach.

> Source/WebKit/chromium/public/WebURLLoaderOptions.h:57
> +    bool allowResponseHeaders; // If policy is to use access control, whether to allow non-simple response headers.

I wonder if we should name this something like exposeAllResponseHeaders to mimic the name of Access-Control-Expose-Headers ?
Comment 7 Bill Budge 2012-06-15 11:03:16 PDT
Created attachment 147859 [details]
Proposed Patch

'exposeAllResponseHeaders' is a much better name. Thanks.
Comment 8 WebKit Review Bot 2012-06-15 13:49:11 PDT
Comment on attachment 147859 [details]
Proposed Patch

Clearing flags on attachment: 147859

Committed r120490: <http://trac.webkit.org/changeset/120490>
Comment 9 WebKit Review Bot 2012-06-15 13:49:17 PDT
All reviewed patches have been landed.  Closing bug.