1686808 - GeckoWebExecutor.fetch sometimes rejects http/https URLs as invalid

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Christian Sadilek [:csadilek]]

We see valid URLs being rejected due to "Unsupported URI scheme":
https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoWebExecutor.java#142

We see two cases of this in Fenix:

• For icons where we check the URL scheme is http or https
• For addon collections where the URL is configured as part of the build and definitely https

One problem here is the use of toLowerCase without specifying a locale (e.g. Locale.ROOT) which will fallback to the device locale and can have surprising results. However, I wasn't able to reproduce this.

Comment 1

[[ex-Mozilla] Agi Sferro | :agi]

Let's see if Bug 1686809 helps.

Comment 2

[[ex-Mozilla] Agi Sferro | :agi]

We landed the patch to change toLowerCase did this bug go away?

Comment 3

[Christian Sadilek [:csadilek]]

Unfortunately, no, we still see this happening for http urls we pass down to GeckoWebExecutor.fetch. This also happens for urls that are configured as part of the build e.g. the AMO endpoint to fetch collections.

But even for icons, we check if the url starts with http before we pass it down. So, it's strange that somehow the regex in GV doesn't find a match.

Comment 4

[Christian Sadilek [:csadilek]]

For Icons:
Sentry: https://sentry.prod.mozaws.net/operations/firefox/issues/9276162/
Our logic to check URL before fetching: https://github.com/mozilla-mobile/android-components/blob/master/components/browser/icons/src/main/java/mozilla/components/browser/icons/loader/HttpIconLoader.kt#L81 (checks if scheme is "http" or "https" which must pass before we fetch)

For AMO collection endpoint:
Sentry: https://sentry.prod.mozaws.net/operations/firefox-nightly/issues/9682088/ (it's suspicious that this one only happens in Nightly where we allow custom collection IDs but this shouldn't affect the URI scheme).

Comment 5

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1686808 - Add debugging information for invalid scheme in fetch().

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/724a7032c777
Add debugging information for invalid scheme in fetch(). r=droeh

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/724a7032c777

Comment 8

[[ex-Mozilla] Agi Sferro | :agi]

Copying from slack:

ok I got it
uri.matches("(http|blob).") matches the full uri string
. matches everything... except a newline chracter
so if the URL contains a new line character anywhere the uri.matches fails regardless of what it starts with
what I'm suspecting is happening is users are copying the collection name which sometimes happens to contain a new line
I tested it if I add a new line at the end of the collection name I get that error

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1686808 - Don't use regex to parse URIs in WebExecutor.

The harmless looking regex

uri.matches("(http|blob).*")

has a flaw: it will not accept URLs that contain a new line character. In that
case GeckoWebExecutor errors out with "Unsupported URI scheme." when in reality
that's not the problem at all.

Since it has caused a bunch of headaches to the frontend already, let's just
replace that regex with a plain startsWith which should work in all cases.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bfec8634b029
Don't use regex to parse URIs in WebExecutor. r=owlish

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/bfec8634b029

Comment 12

[[ex-Mozilla] Agi Sferro | :agi]

Looks like the patch in Comment #9 worked, the crashes are gone 🥳