Bug 97689 – [chromium][Android] Add WebSecurityOrigin::grantLoadLocalResources()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Ben Murdoch]

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)

[Ben Murdoch]

Created attachment 165821 [details]
patch

Adds grantAccessToLocalFiles(bool) to WebKit::WebSecurityPolicy

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

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.

[Ben Murdoch]

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.

[Adam Barth]

> 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.

[Ben Murdoch]

(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 :)

[Ben Murdoch]

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 :)

[Adam Barth]

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.

[Adam Barth]

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.

[Ben Murdoch]

Created attachment 167787 [details]
patch.

ptal Adam.

Thanks, Ben

[Adam Barth]

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.

[Ben Murdoch]

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).

[WebKit Review Bot]

Comment on attachment 167846 [details]
updated comment

Clearing flags on attachment: 167846

Committed r130815: <http://trac.webkit.org/changeset/130815>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.