Bug 156651 - [GTK] Expose AllowUniversalAccessFromFileURLs preference now that calling localStorage.getItem() results in SecurityError: DOM Exception 18
Summary: [GTK] Expose AllowUniversalAccessFromFileURLs preference now that calling loc...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Linux
: P1 Critical
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-15 18:12 PDT by Dustin Falgout
Modified: 2017-06-28 12:17 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.84 KB, patch)
2016-04-16 22:08 PDT, Dustin Falgout
no flags Details | Formatted Diff | Diff
Patch (4.84 KB, patch)
2016-04-16 22:36 PDT, Dustin Falgout
no flags Details | Formatted Diff | Diff
Patch (5.44 KB, patch)
2016-04-16 22:41 PDT, Dustin Falgout
no flags Details | Formatted Diff | Diff
Patch (6.37 KB, patch)
2016-04-16 22:48 PDT, Dustin Falgout
no flags Details | Formatted Diff | Diff
Patch (7.17 KB, patch)
2016-04-16 22:55 PDT, Dustin Falgout
no flags Details | Formatted Diff | Diff
Patch (7.00 KB, patch)
2016-04-17 10:21 PDT, Dustin Falgout
no flags Details | Formatted Diff | Diff
Patch (9.40 KB, patch)
2016-04-17 10:34 PDT, Dustin Falgout
no flags Details | Formatted Diff | Diff
Patch (9.45 KB, patch)
2016-04-20 14:31 PDT, Dustin Falgout
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dustin Falgout 2016-04-15 18:12:08 PDT
This issue appeared with the latest release of webkit2gtk (2.12.1). It does not occur with the previous version (2.12.0). Please let me know if there is any further information I can provide that might be helpful. Thanks.
Comment 1 Michael Catanzaro 2016-04-16 11:46:12 PDT
I guess you are using local file URIs, in which case this is expected behavior after r197858. You can access sessionStorage instead. It's unfortunate to have a compatibility break in a stable release, but this one looks unavoidable.

(If this is happening for http/https URIs, then we would need to reopen this.)

You miiight be able to access localStorage by using webkit_settings_set_allow_file_access_from_file_urls, but I haven't tested to see if that would work.
Comment 2 Brent Fulgham 2016-04-16 11:59:27 PDT
(In reply to comment #1)
> I guess you are using local file URIs, in which case this is expected
> behavior after r197858. You can access sessionStorage instead. It's
> unfortunate to have a compatibility break in a stable release, but this one
> looks unavoidable.
> 
> (If this is happening for http/https URIs, then we would need to reopen
> this.)
> 
> You miiight be able to access localStorage by using
> webkit_settings_set_allow_file_access_from_file_urls, but I haven't tested
> to see if that would work.

(In reply to comment #0)
> This issue appeared with the latest release of webkit2gtk (2.12.1). It does
> not occur with the previous version (2.12.0). Please let me know if there is
> any further information I can provide that might be helpful. Thanks.

The equivalent setting on iOS/OS X is how we deal with this for the WebInspector and other applications seeking to access local storage from local file content.

The point of the earlier security fix is to avoidance case of downloaded HTML content (such as e-mail attachments or JS injected into local contexts) from having access to your local file system and arbitrary local storage.

If you are serving local files in your applications, you can use the suggested preference to tell Webkit that you are approve of these kinds of interactions.
Comment 3 Dustin Falgout 2016-04-16 14:48:02 PDT
The "allow_file_access_from_file_urls" setting does not have any affect on this. We've always enabled that setting. I implemented cookies as a fallback for now but it seems like there should be an option to allow access to local storage when the files are served locally (and not downloaded from the web).
Comment 4 Brent Fulgham 2016-04-16 17:51:00 PDT
(In reply to comment #3)
> The "allow_file_access_from_file_urls" setting does not have any affect on
> this. We've always enabled that setting. I implemented cookies as a fallback
> for now but it seems like there should be an option to allow access to local
> storage when the files are served locally (and not downloaded from the web).

That seems like a bug. I'll try to give better notes when I'm at my computer.
Comment 5 Michael Catanzaro 2016-04-16 18:36:39 PDT
(In reply to comment #4)
> That seems like a bug.

I wonder, because there are two settings here, AllowFileAccessFromFileURLs and AllowUniversalAccessFromFileURLs. Only AllowFileAccessFromFileURLs is exposed by WebKitGTK+, but perhapsAllowUniversalAccessFromFileURLs is the setting that's needed here?

I'd hate to add new API in a point release, but might be needed here.
Comment 6 Dustin Falgout 2016-04-16 18:39:04 PDT
It was my understanding that "AllowUniversalAccessFromFileURLs" was part of the webkit1 API and not webkit2. Do you think its worth trying to enable both settings being that we are using only webkit2?
Comment 7 Brent Fulgham 2016-04-16 19:58:11 PDT
(In reply to comment #6)
> It was my understanding that "AllowUniversalAccessFromFileURLs" was part of
> the webkit1 API and not webkit2. Do you think its worth trying to enable
> both settings being that we are using only webkit2?

Yes -- that's the setting that controls it. We use the UniversalAccessFromFileURLs setting to gate access.

This is definitely a WK2 supported setting.

-Brent
Comment 8 Brent Fulgham 2016-04-16 20:13:40 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > It was my understanding that "AllowUniversalAccessFromFileURLs" was part of
> > the webkit1 API and not webkit2. Do you think its worth trying to enable
> > both settings being that we are using only webkit2?
> 
> Yes -- that's the setting that controls it. We use the
> UniversalAccessFromFileURLs setting to gate access.

See 'SecurityOrigin::canAccessStorage' for the details. Local storage access is gated (for file:// URLs) on m_universalAccess.

This gets set up in Document::initSecurityContext():

    if (settings->allowUniversalAccessFromFileURLs()
        || m_frame->loader().client().shouldForceUniversalAccessFromLocalURL(m_url)) {
        // Some clients want local URLs to have universal access, but that setting is dangerous for other clients.
        securityOrigin()->grantUniversalAccess();
    }

So, you might be able to do something with the frame loader client's "shouldForceUniversalAccessFromLocalURL".

You might be able to use WKBundlePageShouldForceUniversalAccessFromLocalURLCallback to set a function that either always says "Yes, go ahead and use it", or check for specific URLs that you approve of.
Comment 9 Brent Fulgham 2016-04-16 20:16:04 PDT
(In reply to comment #8)
> 
> So, you might be able to do something with the frame loader client's
> "shouldForceUniversalAccessFromLocalURL".
> 
> You might be able to use
> WKBundlePageShouldForceUniversalAccessFromLocalURLCallback to set a function
> that either always says "Yes, go ahead and use it", or check for specific
> URLs that you approve of.

Also:

WKBundleSetAllowUniversalAccessFromFileURLs might be all you need. This is a WKWebViewConfigurationPrivate.h method.
Comment 10 Dustin Falgout 2016-04-16 20:24:56 PDT
Cool. Thanks for the info. 

@Michael, please let me know if there's anything I can do to help get this fixed.
Comment 11 Dustin Falgout 2016-04-16 22:08:50 PDT
Created attachment 276573 [details]
Patch
Comment 12 WebKit Commit Bot 2016-04-16 22:10:22 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 13 Dustin Falgout 2016-04-16 22:36:18 PDT
Created attachment 276576 [details]
Patch
Comment 14 Dustin Falgout 2016-04-16 22:41:08 PDT
Created attachment 276577 [details]
Patch
Comment 15 Dustin Falgout 2016-04-16 22:48:50 PDT
Created attachment 276578 [details]
Patch
Comment 16 Dustin Falgout 2016-04-16 22:55:19 PDT
Created attachment 276579 [details]
Patch
Comment 17 Dustin Falgout 2016-04-16 23:07:10 PDT
I don't see my commit message on the patch (not sure if that's normal-I used the webkit-patch script to upload it). Anyway, here is my commit message:


GTK: Expose WebKitAllowUniversalAccessFromFileURLs preference key in WebKitGTK+

As of r197858 JavaScript loaded in the context of a file scheme url cannot access local storage. That is a major breaking change as many applications that serve files locally rely on having access to local storage. The point of the that security fix is to avoid cases of downloaded HTML content (such as e-mail attachments or JS injected into local contexts) from having access to your local file system and arbitrary local storage. If you are serving local files in your applications, you can use the WebKitAllowUniversalAccessFromFileURLs preference key to tell Webkit that you are approve of these kinds of interactions.

https://bugs.webkit.org/show_bug.cgi?id=156651
Comment 18 Michael Catanzaro 2016-04-17 08:48:05 PDT
Comment on attachment 276579 [details]
Patch

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

Please look under Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitSettings.cpp, and add a test at the bottom of testWebKitSettings just to make sure changing the value of the setting actually works. You can just copy the test for AllowFileAccessFromFileURLs and change the name of the setting.

Anyway, this looks good to me; I presume you've tested and confirmed it solves your problem. This requires approval from another GTK+ reviewer as it adds new API, though, and from Carlos specifically as the Since tags assume a backport to 2.12.2.

> Source/WebKit2/ChangeLog:7
> +

You should add your commit message here, underneath the reviewed by line, then it will get added to the SVN commit message.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1268
> +            _("Allow local storage access from the context of file scheme URLs"),

Maybe we should say "Allow universal access" here, in case the setting is overloaded for something other than local storage in the future (or if that already is the case).
Comment 19 Michael Catanzaro 2016-04-17 08:50:11 PDT
(In reply to comment #18)
> Please look under
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitSettings.cpp, and add a test
> at the bottom of testWebKitSettings just to make sure changing the value of
> the setting actually works. You can just copy the test for
> AllowFileAccessFromFileURLs and change the name of the setting.

Be sure to re-run the prepare-ChangeLog script to create a new changelog under Tools after you do this.
Comment 20 Dustin Falgout 2016-04-17 10:21:30 PDT
Created attachment 276593 [details]
Patch
Comment 21 Dustin Falgout 2016-04-17 10:34:28 PDT
Created attachment 276594 [details]
Patch
Comment 22 Michael Catanzaro 2016-04-17 11:04:49 PDT
Comment on attachment 276594 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        breaking change as many applications that serve files locally rely on having access to local storage. The point  

Out of curiosity, did this break some free software application? Are you aware of any applications typically included in Linux distributions that would be affected by this? We might need to make an announcement regarding this....
Comment 23 Dustin Falgout 2016-04-17 12:04:11 PDT
(In reply to comment #22)
> Comment on attachment 276594 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276594&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        breaking change as many applications that serve files locally rely on having access to local storage. The point  
> 
> Out of curiosity, did this break some free software application? Are you
> aware of any applications typically included in Linux distributions that
> would be affected by this? We might need to make an announcement regarding
> this....

I maintain lightdm-webkit2-greeter. We use it as the default greeter for Antergos. This issue broke the greeter for everyone because it never occurred to me that it was in the realm of possibility for local storage to be unavailable. Definitely a lesson learned. I've already implemented some changes to prevent failures like this one from breaking the login screen in the future. Back to your question, (sorry), I don't know of any other affected applications off the top of my head, but I'm sure there are some.
Comment 24 Michael Catanzaro 2016-04-17 14:05:59 PDT
Sorry you got hit by this. I put out an warning on webkit-gtk list and GNOME distributor-list.

Do consider that WebKit is probably not a good dependency for a display manager; if you happen to have unlimited time and money, an automated test (e.g. using OpenQA) to make sure your distro boots to the login screen would be a good investment.

(Anyway, I wonder if the security semantics of localStorage with respect to local applications are properly understood. As far as I can tell, the same database under ~/.local/share/webkit/databases is shared by all applications. All installed applications are necessarily trusted, but this still seems non-ideal.)
Comment 25 Brent Fulgham 2016-04-17 18:43:20 PDT
(In reply to comment #18)
> Comment on attachment 276579 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276579&action=review

Note that there are some local and session storage tests that I think should have showed this on GTK before it was noticed by and end-user. Perhaps they aren't turned on?
Comment 26 Carlos Garcia Campos 2016-04-18 00:09:28 PDT
Comment on attachment 276594 [details]
Patch

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

We have always tried to avoid exposing this setting, because it's considered dangerous. My main concern is that this looks like the easy solution to fix any cross-origin issue in web apps, but it's not necessarily the right one. But anyway, if we can't avoid exposing this, let's just do it, but not in a stable branch. We released 2.12.0 with a behavior and we should stay with it. So, I see two possibilities here:

1.- Patch WebKit in 2.12 branch only to allow access to local storage from file URLs when AllowFileAccessFromFileURLS is set.
2.- Roll out the patch that introduced the issue. It doesn't look like a so serious issue in the end, and it has always been that way.

Then expose this setting in trunk for 2.14.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1257
> +     * should be allowed to access content from any origin.  By default, when

Extra space between origin. and By default.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1261
> +     * it would be possible to use local storage, for example.

I think we should explain here the differences between allow-file-access and allow-universal-access, to make sure people who actually need allow-file-access don't use this setting. We should probably warn that this could be dangerous.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1263
> +     * Since: 2.12.2

I don't think we should add new API to 2.12 branch, applications shouldn't need any change to properly work between 2.12 and 2.12.x
Comment 27 Carlos Garcia Campos 2016-04-18 00:11:02 PDT
cq- is only because of the Since: tags, the patch looks to me for trunk. Remember to add the index of new symbols for 2.14 in the docs too.
Comment 28 Michael Catanzaro 2016-04-18 07:56:06 PDT
(In reply to comment #26) 
> 1.- Patch WebKit in 2.12 branch only to allow access to local storage from
> file URLs when AllowFileAccessFromFileURLS is set.

No please, let's not change the semantics of this setting just for a stable branch.

> 2.- Roll out the patch that introduced the issue. It doesn't look like a so
> serious issue in the end, and it has always been that way.

I dunno about that either; arbitrary access to localStorage seems pretty serious.
Comment 29 Dustin Falgout 2016-04-18 15:31:47 PDT
(In reply to comment #28)
> 
> I dunno about that either; arbitrary access to localStorage seems pretty
> serious.

IMO, an API breaking change making it into a point release undetected is more serious than anything else. (IJS)
Comment 30 Carlos Garcia Campos 2016-04-18 23:50:05 PDT
(In reply to comment #28)
> (In reply to comment #26) 
> > 1.- Patch WebKit in 2.12 branch only to allow access to local storage from
> > file URLs when AllowFileAccessFromFileURLS is set.
> 
> No please, let's not change the semantics of this setting just for a stable
> branch.

It's not actually changing the semantics, it's just assuming that if you enabled that setting is because your app is serving file URIs and you also want to allow access to the local storage, for backwards compatibility only.

> > 2.- Roll out the patch that introduced the issue. It doesn't look like a so
> > serious issue in the end, and it has always been that way.
> 
> I dunno about that either; arbitrary access to localStorage seems pretty
> serious.

That's why my initial proposal was to limit it to apps already enabling file access. We could also check the document URL to see if it's a local file, but it could be a custom URI scheme or a resource:/.
Comment 31 Dustin Falgout 2016-04-18 23:58:35 PDT
(In reply to comment #30)
> (In reply to comment #28)
> > (In reply to comment #26) 
> > > 1.- Patch WebKit in 2.12 branch only to allow access to local storage from
> > > file URLs when AllowFileAccessFromFileURLS is set.
> > 
> > No please, let's not change the semantics of this setting just for a stable
> > branch.
> 
> It's not actually changing the semantics, it's just assuming that if you
> enabled that setting is because your app is serving file URIs and you also
> want to allow access to the local storage, for backwards compatibility only.

For what its worth, this sounds reasonable to me.
Comment 32 Michael Catanzaro 2016-04-19 09:58:46 PDT
I still kinda think the best option is just to add the new API, even in a stable release, because I think very few apps will be affected by this.

(In reply to comment #30)
> It's not actually changing the semantics, it's just assuming that if you
> enabled that setting is because your app is serving file URIs and you also
> want to allow access to the local storage, for backwards compatibility only.

I guess it's OK to do this for 2.12 only, to unblock this, but then apps are going to have to change twice: once now to use AllowFileAccessFromFileURLs if they didn't already have that set (does this lightdm greeter use that?), and again in five months to switch to AllowUniversalAccessFromFileURLs because AllowFileAccessFromFileURLs won't work anymore in 2.14. (Surely we do not want the semantics of our setting to differ from Apple's going forward.)

> That's why my initial proposal was to limit it to apps already enabling file
> access. We could also check the document URL to see if it's a local file,
> but it could be a custom URI scheme or a resource:/.

I don't think this wins much; if apps using AllowFileAccessFromFileURLs allow opening untrusted files, then that's already more serious than allowing access to localStorage.
Comment 33 Carlos Garcia Campos 2016-04-19 10:09:36 PDT
(In reply to comment #32)
> I still kinda think the best option is just to add the new API, even in a
> stable release, because I think very few apps will be affected by this.
> 
> (In reply to comment #30)
> > It's not actually changing the semantics, it's just assuming that if you
> > enabled that setting is because your app is serving file URIs and you also
> > want to allow access to the local storage, for backwards compatibility only.
> 
> I guess it's OK to do this for 2.12 only, to unblock this, but then apps are
> going to have to change twice: once now to use AllowFileAccessFromFileURLs
> if they didn't already have that set (does this lightdm greeter use that?),
> and again in five months to switch to AllowUniversalAccessFromFileURLs
> because AllowFileAccessFromFileURLs won't work anymore in 2.14. (Surely we
> do not want the semantics of our setting to differ from Apple's going
> forward.)

No, that's not the point at all. Apps should not need to adapt anything, that's why I don't want to add a new setting, I'm assuming that aps already use AllowFileAccessFromFileURLs, which I think is the case lightdm, the only app we know that is affected by this.

> > That's why my initial proposal was to limit it to apps already enabling file
> > access. We could also check the document URL to see if it's a local file,
> > but it could be a custom URI scheme or a resource:/.
> 
> I don't think this wins much; if apps using AllowFileAccessFromFileURLs
> allow opening untrusted files, then that's already more serious than
> allowing access to localStorage.

The point is simply that apps don't need to do anything.
Comment 34 Michael Catanzaro 2016-04-19 10:19:53 PDT
I don't understand, are you proposing that we permanently alter the semantics of AllowFileAccessFromFileURLs for GTK only to allow access to localStorage?
Comment 35 Carlos Garcia Campos 2016-04-19 22:56:38 PDT
(In reply to comment #34)
> I don't understand, are you proposing that we permanently alter the
> semantics of AllowFileAccessFromFileURLs for GTK only to allow access to
> localStorage?

Not at all. I'm proposing that we assume that apps already setting AllowFileAccessFromFileURLs are serving local file URIs and allow access to local storage for them for backwards compatibility. We won't document, nor encourage anybody to use that setting for that particular purpose.
Comment 36 Michael Catanzaro 2016-04-20 07:59:04 PDT
(In reply to comment #35)
> (In reply to comment #34)
> > I don't understand, are you proposing that we permanently alter the
> > semantics of AllowFileAccessFromFileURLs for GTK only to allow access to
> > localStorage?
> 
> Not at all. I'm proposing that we assume that apps already setting
> AllowFileAccessFromFileURLs are serving local file URIs and allow access to
> local storage for them for backwards compatibility. We won't document, nor
> encourage anybody to use that setting for that particular purpose.

So you proposing that we permanently alter AllowFileAccessFromFileURLs to allow access to local storage, for all ports? (Brent?)

What, then, would be the value in the AllowUniversalAccessFromFileURLs setting?
Comment 37 Carlos Garcia Campos 2016-04-20 08:33:47 PDT
(In reply to comment #36)
> (In reply to comment #35)
> > (In reply to comment #34)
> > > I don't understand, are you proposing that we permanently alter the
> > > semantics of AllowFileAccessFromFileURLs for GTK only to allow access to
> > > localStorage?
> > 
> > Not at all. I'm proposing that we assume that apps already setting
> > AllowFileAccessFromFileURLs are serving local file URIs and allow access to
> > local storage for them for backwards compatibility. We won't document, nor
> > encourage anybody to use that setting for that particular purpose.
> 
> So you proposing that we permanently alter AllowFileAccessFromFileURLs to
> allow access to local storage, for all ports? (Brent?)

No, I haven't said that. I'm proposing that we allow access to local storage for apps already setting AllowFileAccessFromFileURLs in 2.12 branch only. Tha doesn't affect trunk nor any other port. Only for backwards compatibility, because that was the case already in 2.12.0.

> What, then, would be the value in the AllowUniversalAccessFromFileURLs
> setting?

For trunk we should expose the universal access setting or shouldForceUniversalAccessFromLocalURL() or both.
Comment 38 Michael Catanzaro 2016-04-20 13:30:53 PDT
(In reply to comment #37)
> No, I haven't said that. I'm proposing that we allow access to local storage
> for apps already setting AllowFileAccessFromFileURLs in 2.12 branch only.
> Tha doesn't affect trunk nor any other port. Only for backwards
> compatibility, because that was the case already in 2.12.0.

OK, I confirmed lightdm-webkit2-greeter is actually using this setting. Let's do it.

> For trunk we should expose the universal access setting or
> shouldForceUniversalAccessFromLocalURL() or both.

OK, so this patch would be good for trunk with altered Since tags.
Comment 39 Dustin Falgout 2016-04-20 14:31:17 PDT
Created attachment 276852 [details]
Patch
Comment 40 WebKit Commit Bot 2016-04-20 15:56:23 PDT
Comment on attachment 276852 [details]
Patch

Clearing flags on attachment: 276852

Committed r199795: <http://trac.webkit.org/changeset/199795>
Comment 41 WebKit Commit Bot 2016-04-20 15:56:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 42 Carlos Garcia Campos 2016-04-21 00:19:59 PDT
(In reply to comment #38)
> (In reply to comment #37)
> > No, I haven't said that. I'm proposing that we allow access to local storage
> > for apps already setting AllowFileAccessFromFileURLs in 2.12 branch only.
> > Tha doesn't affect trunk nor any other port. Only for backwards
> > compatibility, because that was the case already in 2.12.0.
> 
> OK, I confirmed lightdm-webkit2-greeter is actually using this setting.
> Let's do it.
> 
> > For trunk we should expose the universal access setting or
> > shouldForceUniversalAccessFromLocalURL() or both.
> 
> OK, so this patch would be good for trunk with altered Since tags.

I made a couple of comments in my review and suggested to add the index of new symbols to the docs, but still, I'll do that in a follow up.
Comment 43 Michael Catanzaro 2016-04-21 06:44:54 PDT
(In reply to comment #42)
> I made a couple of comments in my review and suggested to add the index of
> new symbols to the docs, but still, I'll do that in a follow up.

Sorry, I missed this in all the discussion.