1224446 - Partner sms(to)/mms(to) links missing // lose phone number in Uri pruning

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Allison Naaktgeboren :ally]

The spec is that smsto links are formatted like so smsto://<NUMBER>  this was added in bug 1222116. However we have a partner whose smsto links are missing the all important //. This results in the phone number being pruned off the mData uri of the intent. 

Without the 'to' number, most sms apps, including the default one, behave like the intent was empty, looking broken.

After some digging the offending code is
   final String newQuery = resultQuery.length() > 0 ? "?" + resultQuery : "";
   final Uri pruned = uri.buildUpon().encodedQuery(newQuery).build();
   intent.setData(pruned);

raw bundle is intent before the above code, final bundle is after that code has run to prune

  smsto:// (working case)
  Intent { act=android.intent.action.VIEW dat=smsto:xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (has extras) }
        raw bundle is
        mExtras Bundle[{sms_body=testmessage}]
        mData smsto://[phone]?body=testmessage
	final bundle becomes
	mData smsto://[phone]
	mExtras Bundle[{sms_body=testmessage}]

smsto:
	comes in as smsto:[phone]?body=testmessage
	Intent { act=android.intent.action.VIEW dat=smsto:xxxxxxxxxxxxxxxxxxxxxxxxxxxxx (has extras) }
        raw bundle
	mData smsto:[phone]?body=testmessage
	mExtra Bundle[{sms_body=testmessage}]
	final bundle becomes
	mData smsto:
	mExtra Bundle[{sms_body=testmessage}]

While this particular partner did not request support for sms/mms links with body messages, this problem applies in those cases.

Comment 1

[Allison Naaktgeboren :ally]

breaking this down further,

uri.buildUpon() seems to be where the number vanishes

Comment 2

[Allison Naaktgeboren :ally]

also of note when the scheme is missing the '//'

        String temp2 = data.getAuthority();
        String temp3 = data.getUserInfo();
        String temp4 = data.getPath();
        String temp5 = data.getFragment();
        String temp6 = data.getHost();

all of these retun null instead of the expected values from the uri.

Comment 3

[Allison Naaktgeboren :ally]

Attachment: v0-ensure '//'

Unfortunately, it seems there is no way to extract & modify just the part we need to change when the uri is not quite right, so we're stuck for converting it to a string, fixing it, and parsing it back into the uri. 

Since this bug afaik only applies to the sms*/mms* family and we hit this function for common things like loading urls, I do this processing only if we actually reach the special handling for the sms family.

Patch for 1209133 depends on this one.

Comment 4

[Allison Naaktgeboren :ally]

also relevant to review: we'll be modifying the variable uri in bug 1209133 as well.

Comment 5

[Allison Naaktgeboren :ally]

Comment on attachment 8687472 [details] [diff] [review]
v0-ensure '//'

See above comments for context.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8687472 [details] [diff] [review]
v0-ensure '//'

Review of attachment 8687472 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoAppShell.java
@@ +1241,5 @@
> +        // android's Uri builder will interpret those as malformed and buildUpon() & getAuthority() will not work correctly
> +        String currentUri = uri.toString();
> +        if (!currentUri.contains("//")) {
> +            uri = Uri.parse(currentUri.replace(":", "://"));
> +        }

How do we currently handle tel: links? It seems like that would have the same issue, but somehow we don't need to do any special-casing for those links.

I suspect that we want to be treating these web URIs as their own thing, not attempt to pass them along directly as Android URIs.

Comment 7

[Allison Naaktgeboren :ally]

(In reply to :Margaret Leibovic from comment #6)
> Comment on attachment 8687472 [details] [diff] [review]
> v0-ensure '//'
> 
> Review of attachment 8687472 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoAppShell.java
> @@ +1241,5 @@
> > +        // android's Uri builder will interpret those as malformed and buildUpon() & getAuthority() will not work correctly
> > +        String currentUri = uri.toString();
> > +        if (!currentUri.contains("//")) {
> > +            uri = Uri.parse(currentUri.replace(":", "://"));
> > +        }
> 
> How do we currently handle tel: links? It seems like that would have the
> same issue, but somehow we don't need to do any special-casing for those
> links.

tel: URIs are returned[1] as-is before any of the extraction information from the URI and packaging up it up in Intent fields. Those uris may very well have problems in their own right, but since we don't try to extract uri.getHost() or do uri manipulation on those, the code doesn't notice a problem with either.

[1] https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java?rev=06c46075db46#1242

Also note that testing tel: vs tel:// handling outside of fx is complicated by Bug 1224295, which causes clicking on tel: links to crash fennec.

Comment 8

[Richard Newman [:rnewman]]

(In reply to Allison Naaktgeboren :ally from comment #7)

> > > +            uri = Uri.parse(currentUri.replace(":", "://"));

I think you want .replaceFirst instead of .replace; consider this test body:

  sms:+12345;body=hello:%20yes

But I would consider validating/parsing more explicitly to avoid producing garbage if the leading : is not present in a malformed URI.


> > How do we currently handle tel: links? It seems like that would have the
> > same issue, but somehow we don't need to do any special-casing for those
> > links.
> 
> tel: URIs are returned[1] as-is…

tel URIs aren't supposed to have slashes; see RFC3966. sip://...;user=phone URIs are the closest telephony analog that do have slashes, and I don't think we pretend to support them.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8687472 [details] [diff] [review]
v0-ensure '//'

Review of attachment 8687472 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like fixing the URI is our only way out when using the Uri class. What's odd though is that it seems like the non-slashes version should be the correct one according to the RFC?

::: mobile/android/base/GeckoAppShell.java
@@ +1237,5 @@
>              return intent;
>          }
>  
> +        // While it is common to see sms*/mms* uris on the web without '//' before the scheme,
> +        // android's Uri builder will interpret those as malformed and buildUpon() & getAuthority() will not work correctly

I'm a bit confused: We do not use getAuthority() here, do we?

I was curious to know what happens here and looked into the Code of Uri. It seems like this is actually intended behavior and Android strictly separates hierarchical (without scheme or starting with a /) and non-hierarchical Uris:
http://androidxref.com/4.2_r1/xref/frameworks/base/core/java/android/net/Uri.java#774

Looking at the sms URI scheme the version without slashes seems to be the correct one:
https://www.ietf.org/rfc/rfc5724.txt

So is this an Android bug?

@@ +1240,5 @@
> +        // While it is common to see sms*/mms* uris on the web without '//' before the scheme,
> +        // android's Uri builder will interpret those as malformed and buildUpon() & getAuthority() will not work correctly
> +        String currentUri = uri.toString();
> +        if (!currentUri.contains("//")) {
> +            uri = Uri.parse(currentUri.replace(":", "://"));

This will replace all colons. Is it possible that any of the fields contain colons? Maybe we should just replace the first. Or use regex to only replace ^scheme: with scheme://?

Comment 10

[Allison Naaktgeboren :ally]

(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> Comment on attachment 8687472 [details] [diff] [review]
> v0-ensure '//'
> 
> Review of attachment 8687472 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems like fixing the URI is our only way out when using the Uri class.
> What's odd though is that it seems like the non-slashes version should be
> the correct one according to the RFC?

I think the biggest problem is that Android's specification deliberately diverges from W3C's and we're caught in the middle.

I think that dropping the Uri class entirely would be a really bad idea, so we should stick with fixing the URI to conform to Android's spec.  

> 
> ::: mobile/android/base/GeckoAppShell.java
> @@ +1237,5 @@
> >              return intent;
> >          }
> >  
> > +        // While it is common to see sms*/mms* uris on the web without '//' before the scheme,
> > +        // android's Uri builder will interpret those as malformed and buildUpon() & getAuthority() will not work correctly
> 
> I'm a bit confused: We do not use getAuthority() here, do we?

Sorry, we will/do use getAuthority()/getHost() in 1209133. I was working on that when I ran into this and it blocked my work.
> 
> I was curious to know what happens here and looked into the Code of Uri. It
> seems like this is actually intended behavior and Android strictly separates
> hierarchical (without scheme or starting with a /) and non-hierarchical Uris:
> http://androidxref.com/4.2_r1/xref/frameworks/base/core/java/android/net/Uri.
> java#774
> 
> Looking at the sms URI scheme the version without slashes seems to be the
> correct one:
> https://www.ietf.org/rfc/rfc5724.txt
> 
> So is this an Android bug?

I don't think Android considers it a bug.

Their spec for the URI scheme is clearly stated to not be the same as w3c's.  Android's code and documentation are very clear about what they consider correct to be for this family of uris.

> 
> @@ +1240,5 @@
> > +        // While it is common to see sms*/mms* uris on the web without '//' before the scheme,
> > +        // android's Uri builder will interpret those as malformed and buildUpon() & getAuthority() will not work correctly
> > +        String currentUri = uri.toString();
> > +        if (!currentUri.contains("//")) {
> > +            uri = Uri.parse(currentUri.replace(":", "://"));
> 
> This will replace all colons. Is it possible that any of the fields contain
> colons? Maybe we should just replace the first. Or use regex to only replace
> ^scheme: with scheme://?

I think just replacing the first one attached to scheme: makes the most sense.

Thoughts?

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8687472 [details] [diff] [review]
v0-ensure '//'

(In reply to Allison Naaktgeboren :ally from comment #10)
> I think that dropping the Uri class entirely would be a really bad idea, so
> we should stick with fixing the URI to conform to Android's spec.  

Definitely! In the end we need an Uri object to put in the Intent anyways.

(In reply to Allison Naaktgeboren :ally from comment #10)
> I think just replacing the first one attached to scheme: makes the most
> sense.

I can't think of a way to break a valid uri with replacing the (wrong) first colon. So I'm okay with that. r+ with that little change. :)

Comment 12

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/fx-team/rev/22b421afcf73

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22b421afcf73

Comment 14

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

By Finkle's word on irc, we land in 44.

Comment 15

[:Margaret Leibovic]

Comment on attachment 8687472 [details] [diff] [review]
v0-ensure '//'

Approval Request Comment
[Feature/regressing bug #]: Partnership requirements, looking to pre-install Fx44.

[User impact if declined]: Clicking on MMS links won't work.

[Describe test coverage new/current, TreeHerder]: No automated test coverage, tested locally against partner example requirements.

[Risks and why]: Low-risk, adds additional case to our intent launching logic. The main risk would be regressing our SMS intent logic, but our testing should cover that.

[String/UUID change made/needed]: None.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8687472 [details] [diff] [review]
v0-ensure '//'

Aurora44+

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d39cb9d10fcf

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d39cb9d10fcf