Bug 97689

Summary: [chromium][Android] Add WebSecurityOrigin::grantLoadLocalResources()
Product: WebKit Reporter: Ben Murdoch <benm>
Component: WebKit APIAssignee: Ben Murdoch <benm>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fishd, jamesr, mnaganov, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Android   
OS: Android   
Attachments:
Description Flags
patch
abarth: review-
patch.
none
patch.
abarth: review+, abarth: commit-queue-
updated comment none

Description Ben Murdoch 2012-09-26 08:49:27 PDT
To correctly support the Android WebView#loadDataWithBaseUrl API[1], we would like to expose the WebCore::SecurityPolicy::setLocalLoadPolicy function to Chromium through the API.

For more information, please see the chromium bug: https://code.google.com/p/chromium/issues/detail?id=152223

Patch coming.

[1]http://developer.android.com/reference/android/webkit/WebView.html#loadDataWithBaseURL(java.lang.String, java.lang.String, java.lang.String, java.lang.String, java.lang.String)
Comment 1 Ben Murdoch 2012-09-26 09:17:08 PDT
Created attachment 165821 [details]
patch

Adds grantAccessToLocalFiles(bool) to WebKit::WebSecurityPolicy
Comment 2 WebKit Review Bot 2012-09-26 09:20:46 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 Adam Barth 2012-09-26 10:07:23 PDT
Comment on attachment 165821 [details]
patch

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

Do you really want to this globally for the whole process or just for an individual WebView?

> Source/WebKit/chromium/public/WebSecurityPolicy.h:98
> +    // Grants access to file:// URLs.
> +    WEBKIT_EXPORT static void grantAccessToLocalFiles(bool);

Also, this boolean parameter is really vague.  Perhaps this should be two functions, one that grants local access for all URIs and one that grants access only for local URIs.
Comment 4 Ben Murdoch 2012-09-26 12:06:23 PDT
Thanks for looking Adam!

(In reply to comment #3)
> (From update of attachment 165821 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165821&action=review
> 
> Do you really want to this globally for the whole process or just for an individual WebView?

Ah, good call. I suppose what I really want is to get the SecurityOrigin for the page and call it's grantLoadLocalResources() method, and it's actually that that I want to expose to Chromium via the WebKit::WebSecurityOrigin. Does that sounds along the right lines to you?

I see however that SecurityOrigin::grantLoadLocalResources is only around for backwards compatibility... can we consider implementing the Android WebView API as implementing backwards compatibility? These checks didn't exist when the original API was implemented inside Android's old WebKit fork... :/

> 
> > Source/WebKit/chromium/public/WebSecurityPolicy.h:98
> > +    // Grants access to file:// URLs.
> > +    WEBKIT_EXPORT static void grantAccessToLocalFiles(bool);
> 
> Also, this boolean parameter is really vague.  Perhaps this should be two functions, one that grants local access for all URIs and one that grants access only for local URIs.

This should go away if I head in the direction mentioned above.
Comment 5 Adam Barth 2012-09-26 12:08:29 PDT
> Ah, good call. I suppose what I really want is to get the SecurityOrigin for the page and call it's grantLoadLocalResources() method, and it's actually that that I want to expose to Chromium via the WebKit::WebSecurityOrigin. Does that sounds along the right lines to you?

Yes, precisely.

> I see however that SecurityOrigin::grantLoadLocalResources is only around for backwards compatibility... can we consider implementing the Android WebView API as implementing backwards compatibility? These checks didn't exist when the original API was implemented inside Android's old WebKit fork... :/

We might need to update the comment to explain what it is used for.
Comment 6 Ben Murdoch 2012-09-26 12:19:54 PDT
(In reply to comment #5)
> > Ah, good call. I suppose what I really want is to get the SecurityOrigin for the page and call it's grantLoadLocalResources() method, and it's actually that that I want to expose to Chromium via the WebKit::WebSecurityOrigin. Does that sounds along the right lines to you?
> 
> Yes, precisely.
> 
> > I see however that SecurityOrigin::grantLoadLocalResources is only around for backwards compatibility... can we consider implementing the Android WebView API as implementing backwards compatibility? These checks didn't exist when the original API was implemented inside Android's old WebKit fork... :/
> 
> We might need to update the comment to explain what it is used for.

OK :)
Comment 7 Ben Murdoch 2012-10-08 10:31:12 PDT
Created attachment 167556 [details]
patch.

Adam, here's v2 of the patch.

I didn't have time to create a decent ChangeLog this evening so I appreciate that it's unfit for landing (hence not requesting r?). But I'd be very grateful if you could comment in the context of the chromium side patch (http://codereview.chromium.org/10990056/), and then I can try and fix it up tomorrow UK time.

Many thanks :)
Comment 8 Adam Barth 2012-10-08 11:13:32 PDT
Comment on attachment 167556 [details]
patch.

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

> Source/WebKit/chromium/public/WebSecurityPolicy.h:97
> +    // Updates the current WebCore::SecurityPolicy to allow local loads from both

There is no such thing as WebCore from the point of view of the API.  I would have written this as "Updates the current security policy to allow..."

> Source/WebKit/chromium/src/WebSecurityOrigin.cpp:163
> +void WebSecurityOrigin::grantLocalLoads() const
> +{
> +    get()->grantLoadLocalResources();
> +}

Any particular reason why you picked a different function name?  These functions should have the same name.  If you don't like the name in WebCore, we can change it.

> Source/WebKit/chromium/src/WebSecurityPolicy.cpp:130
> +void WebSecurityPolicy::allowLocalLoadsForLocalAndSubstituteData()
> +{
> +    SecurityPolicy::setLocalLoadPolicy(SecurityPolicy::AllowLocalLoadsForLocalAndSubstituteData);
> +}
> +
> +void WebSecurityPolicy::allowLocalLoadsForLocalOnly()
> +{
> +    SecurityPolicy::setLocalLoadPolicy(SecurityPolicy::AllowLocalLoadsForLocalOnly);
> +}

It's not clear from the API that these functions twiddle the same state.
Comment 9 Adam Barth 2012-10-08 11:19:32 PDT
I commented on the Chromium review as well.  It seems very unlikely that you should be manipulating WebKit-global state based on what's happening in a single RenderView.
Comment 10 Ben Murdoch 2012-10-09 11:01:09 PDT
Created attachment 167787 [details]
patch.

ptal Adam.

Thanks, Ben
Comment 11 Adam Barth 2012-10-09 14:12:03 PDT
Comment on attachment 167787 [details]
patch.

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

> Source/WebCore/page/SecurityOrigin.cpp:433
>      // To be backwards compatible with older versions of WebKit, we also use
>      // this function to grant the ability to load local resources to documents
>      // loaded with SubstituteData.

This part of the comment is outdated and should be removed.

> Source/WebKit/chromium/public/WebSecurityOrigin.h:100
> +    // Allows this WebSecurityOrigin access to local resources.
> +    WEBKIT_EXPORT void grantLoadLocalResources() const;

I have some reservations about exposing this API since its easy to use it in an insecure way, but I guess we'll just need to be careful.
Comment 12 Ben Murdoch 2012-10-09 14:17:43 PDT
Created attachment 167846 [details]
updated comment

Thanks Adam!

New patch with the updated comment; I'd like the CQ to land for me (I don't have an svn checkout to hand that I can commit from).
Comment 13 WebKit Review Bot 2012-10-09 14:50:38 PDT
Comment on attachment 167846 [details]
updated comment

Clearing flags on attachment: 167846

Committed r130815: <http://trac.webkit.org/changeset/130815>
Comment 14 WebKit Review Bot 2012-10-09 14:50:42 PDT
All reviewed patches have been landed.  Closing bug.