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)
Created attachment 165821 [details] patch Adds grantAccessToLocalFiles(bool) to WebKit::WebSecurityPolicy
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 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.
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.
> 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.
(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 :)
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 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.
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.
Created attachment 167787 [details] patch. ptal Adam. Thanks, Ben
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.
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 on attachment 167846 [details] updated comment Clearing flags on attachment: 167846 Committed r130815: <http://trac.webkit.org/changeset/130815>
All reviewed patches have been landed. Closing bug.