Bug 60066 – [chromium] Go through WebPermissionClient for local storage access. Also cleanup left over code from previous WebPermissionClient change.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[John Abd-El-Malek]

[chromium] Go through WebPermissionClient for local storage access.  Also cleanup left over code from previous WebPermissionClient change.

[John Abd-El-Malek]

Created attachment 92137 [details]
Patch

[John Abd-El-Malek]

ccing Darin since it's WebKit API, although I don't think we need to wait for his ok for this since it's just adding a similar function to an API he just reviewed.

[John Abd-El-Malek]

chrome side change: http://codereview.chromium.org/6915017/

[James Robinson]

Comment on attachment 92137 [details]
Patch

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

> Source/WebKit/chromium/public/WebPermissionClient.h:49
> +    virtual bool allowLocalStorage(WebFrame*, const WebURL&) { return true; }

the URL can be generated from the WebFrame, so it's redundant here

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:89
> +    if (webView->permissionClient()
> +        && !webView->permissionClient()->allowLocalStorage(webFrame, url)) {

nit: don't line wrap this

[James Robinson]

Comment on attachment 92137 [details]
Patch

According to Dr Barth the allowLocalStorage() implementation should be using securityOrigin() and not URL.  There are case where the two can differ and we definitely don't want to let a page get around the user's local storage settings by faking out the url.

[John Abd-El-Malek]

(In reply to comment #5)
> (From update of attachment 92137 [details])
> According to Dr Barth the allowLocalStorage() implementation should be using securityOrigin() and not URL.  There are case where the two can differ and we definitely don't want to let a page get around the user's local storage settings by faking out the url.


So the old code was calling frame->document()->url(), hence why I moved it to do the same.  Does securityOrigin() return the whole url, or only scheme+host+port?  If it's the latter, then that wouldn't work if the user has a content setting for a url that has path for example.

[John Abd-El-Malek]

Created attachment 92179 [details]
Patch

[Adam Barth]

> So the old code was calling frame->document()->url(), hence why I moved it to do the same.  Does securityOrigin() return the whole url, or only scheme+host+port?  If it's the latter, then that wouldn't work if the user has a content setting for a url that has path for example.

Security restrictions at a finer granularity than a scheme+host+port don't work.

[John Abd-El-Malek]

ok so I tried to set content settings policies for a url with a path, and that didn't work.  so it looks like it's just for hostnames then, in which case using securityOrigin should be fine.  however, that should be done in a separate cl, in case that has any side-effects.  this change has others that depend on it, and once they're committed, reverting this won't be possible without reverting the chorme side.

[James Robinson]

Comment on attachment 92179 [details]
Patch

R=me. I agree that changing the url/securityOrigin behavior should be a separate patch.

[John Abd-El-Malek]

Created attachment 92182 [details]
Patch

[John Abd-El-Malek]

I filed bug 60108 for the url issue.

i updated the patch to include the chromium revision with the chrome side change.

[John Abd-El-Malek]

Created attachment 92184 [details]
Patch

[John Abd-El-Malek]

Committed r85703: <http://trac.webkit.org/changeset/85703>