[GTK] Expose AllowUniversalAccessFromFileURLs preference now that calling localStorage.getItem() results in SecurityError: DOM Exception 18

• Component: WebKitGTK
• Status: REOPENED ---

[Dustin Falgout]

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.

[Michael Catanzaro]

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.

[Brent Fulgham]

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

[Dustin Falgout]

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

[Brent Fulgham]

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

[Michael Catanzaro]

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

[Dustin Falgout]

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?

[Brent Fulgham]

(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

[Brent Fulgham]

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

[Brent Fulgham]

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

[Dustin Falgout]

Cool. Thanks for the info. 

@Michael, please let me know if there's anything I can do to help get this fixed.

[Dustin Falgout]

Created attachment 276573 [details]
Patch

[WebKit Commit Bot]

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

[Dustin Falgout]

Created attachment 276576 [details]
Patch

[Dustin Falgout]

Created attachment 276577 [details]
Patch

[Dustin Falgout]

Created attachment 276578 [details]
Patch

[Dustin Falgout]

Created attachment 276579 [details]
Patch

[Dustin Falgout]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Dustin Falgout]

Created attachment 276593 [details]
Patch

[Dustin Falgout]

Created attachment 276594 [details]
Patch

[Michael Catanzaro]

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

[Dustin Falgout]

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

[Michael Catanzaro]

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

[Brent Fulgham]

(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?

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

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

[Dustin Falgout]

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

[Carlos Garcia Campos]

(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:/.

[Dustin Falgout]

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

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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

[Michael Catanzaro]

I don't understand, are you proposing that we permanently alter the semantics of AllowFileAccessFromFileURLs for GTK only to allow access to localStorage?

[Carlos Garcia Campos]

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

[Michael Catanzaro]

(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?

[Carlos Garcia Campos]

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

[Michael Catanzaro]

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

[Dustin Falgout]

Created attachment 276852 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 276852 [details]
Patch

Clearing flags on attachment: 276852

Committed r199795: <http://trac.webkit.org/changeset/199795>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

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

[Michael Catanzaro]

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