Bug 209900 – [WPE][GTK] Public API should not allow trying to register a special URI scheme

• Component: WebKit API
• Status: RESOLVED FIXED

[Adrian Perez]

Calling webkit_web_context_register_uri_scheme() with a “special”
URI scheme name (like “file” or “http”) which is not supposed to
be overriden should do a check at the API entry, instead of trying
to pass that down to WebKit unchecked and cause a failed assertion:

ASSERTION FAILED: !WTF::URLParser::isSpecialScheme(canonicalizedScheme.value())

Backtrace:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x0000007fafb8fb54 in __GI_abort () at abort.c:79
#2  0x0000007fb0d4a4c0 in CRASH_WITH_INFO(...) () at /home/buildroot/buildroot/rpi3/output/build/wpewebkit-2.28.0/DerivedSources/ForwardingHeaders/wtf/Assertions.h:660
#3  WebKit::WebPageProxy::setURLSchemeHandlerForScheme (this=this@entry=0x7fa37f6000, handler=..., scheme=...)
    at /home/buildroot/buildroot/rpi3/output/build/wpewebkit-2.28.0/Source/WebKit/UIProcess/WebPageProxy.cpp:9114
#4  0x0000007fb0e5f7dc in webkitWebContextCreatePageForWebView (context=0x287691e0, webView=<optimized out>, webView@entry=0x287ac100, userContentManager=<optimized out>, relatedView=<optimized out>)
    at /home/buildroot/buildroot/rpi3/output/build/wpewebkit-2.28.0/Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1828
#5 ...

[Adrian Perez]

Created attachment 395254 [details]
Patch

[EWS Watchlist]

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

[EWS]

Committed r259382: <https://trac.webkit.org/changeset/259382>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395254 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61200217>

[Michael Catanzaro]

Note this makes it impossible to implement support for ftp. Probably only schemes that are actually supported by WebKit should be blacklisted.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #5)
> Note this makes it impossible to implement support for ftp. Probably only
> schemes that are actually supported by WebKit should be blacklisted.

It was already impossible to implement it outside of WebKit: without
the early check added by this at the GTK/WPE API level, the assertion
would be triggered anyway inside WebKit when trying to register an URI
scheme handler for FTP.

If we want to allow implementing FTP via a custom URI scheme handler,
then that would be a separate issue, and we will need to do a number
of changes inside WebKit. If anybody knows whether any application was
(or intends to) do that, please go ahead and open a new bug :)

[Adrian Perez]

(In reply to Adrian Perez from comment #6)
> (In reply to Michael Catanzaro from comment #5)
> > Note this makes it impossible to implement support for ftp. Probably only
> > schemes that are actually supported by WebKit should be blacklisted.
> 
> It was already impossible to implement it outside of WebKit: without
> the early check added by this at the GTK/WPE API level, the assertion
> would be triggered anyway inside WebKit when trying to register an URI
> scheme handler for FTP.

By the way, there is an “ENABLE_FTPDIR” build option in WebKit, enabled
by default. I think there is no support for returning documents of type
“application/x-ftp-directory” from libsoup, and that's why even when it's
enabled trying to load e.g. ftp://ftp.es.debian.org results in an error
saying “The URL cannot be shown” ;-)

[Michael Catanzaro]

(In reply to Adrian Perez from comment #6)
> It was already impossible to implement it outside of WebKit: without
> the early check added by this at the GTK/WPE API level, the assertion
> would be triggered anyway inside WebKit when trying to register an URI
> scheme handler for FTP.

That can't be true, because Epiphany registered this scheme unconditionally on startup until 3.36. That code has been there for years and was only removed for 3.36 because we noticed Chrome had decided to no longer handle ftp and decided to just give up on it.

> If we want to allow implementing FTP via a custom URI scheme handler,
> then that would be a separate issue, and we will need to do a number
> of changes inside WebKit. If anybody knows whether any application was
> (or intends to) do that, please go ahead and open a new bug :)

So yeah, Epiphany prior to 3.36 implemented FTP via a custom URI scheme handler.

If you think it would result in an assertion, then maybe I need to check to see if Epiphany is crashing on F31. We might have busted older Epiphany when upgrading to 2.28 if this assertion is new.

[Michael Catanzaro]

I'm reopening because I'm worried about adding criticals for old Ephy versions. Maybe we should switch to g_warning() instead?

I don't think the code was previously invalid.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #9)
> I'm reopening because I'm worried about adding criticals for old Ephy
> versions. Maybe we should switch to g_warning() instead?

The ASSERT was added in r213686, but didn't affect us because we used legacy custom protocol implementation until r250597. I think r250597 is when the regression was introduced.

Introducing criticals in previously-valid applications is not good, so let's switch to g_warning(). Also, let's not hardcode the list of protocols here.

[Michael Catanzaro]

Created attachment 402386 [details]
Patch

[Michael Catanzaro]

Created attachment 402391 [details]
Patch

[Adrian Perez]

Comment on attachment 402391 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1280
> +        g_critical("Cannot register invalid URI scheme %s", scheme);

I would probably write this as:

   auto isValidScheme = !!WTF::URLParser::maybeCanonicalizeScheme(scheme);
   g_return_if_fail(iValidScheme);

So the error message follows the usual format emitted by the g_return[_val]_if_fail()
macros.

[Michael Catanzaro]

I need to use canonicalizedScheme below, and I can avoid !! by using C++ bool:

auto canonicalizedScheme = WTF::URLParser::maybeCanonicalizeScheme(scheme);
bool isValidScheme = canonicalizedScheme;
g_return_if_fail(isValidScheme);

OK? Honestly I prefer to write out the critical as I did originally, but you're the reviewer. ;)

[Adrian Perez]

(In reply to Michael Catanzaro from comment #14)
> I need to use canonicalizedScheme below, and I can avoid !! by using C++
> bool:
> 
> auto canonicalizedScheme = WTF::URLParser::maybeCanonicalizeScheme(scheme);
> bool isValidScheme = canonicalizedScheme;
> g_return_if_fail(isValidScheme);
> 
> OK? Honestly I prefer to write out the critical as I did originally, but
> you're the reviewer. ;)

I don't have a very strong preference, to be honest… The reason why I was
suggesting ending up using g_return_if_fail() would be to make sure it is
GLib the one formatting the message, to make all the checks on API input
parameters output messages with the same format.

[Michael Catanzaro]

(In reply to Adrian Perez from comment #15)
> I don't have a very strong preference, to be honest…

OK, I'm going to commit the original version of the patch, because I'd rather not expose a local variable in the error message. I like the error message g_return_if_fail() gives when used directly on public API function parameters, but when more complex decisions are needed, like in this case, I think it's nicer to write out a custom error message.

[EWS]

Committed r263369: <https://trac.webkit.org/changeset/263369>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402391 [details].