Closed Bug 1224446 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed
fennec 44+ ---

People

(Reporter: ally, Assigned: ally)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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://+16311234567?body=testmessage
	final bundle becomes
	mData smsto://+16311234567
	mExtras Bundle[{sms_body=testmessage}]

smsto:
	comes in as smsto:+16311234567?body=testmessage
	Intent { act=android.intent.action.VIEW dat=smsto:xxxxxxxxxxxxxxxxxxxxxxxxxxxxx (has extras) }
        raw bundle
	mData smsto:+16311234567?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.
Assignee: nobody → ally
breaking this down further,

uri.buildUpon() seems to be where the number vanishes
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.
Attached patch v0-ensure '//'Splinter Review
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.
also relevant to review: we'll be modifying the variable uri in bug 1209133 as well.
Comment on attachment 8687472 [details] [diff] [review]
v0-ensure '//'

See above comments for context.
Attachment #8687472 - Flags: review?(s.kaspari)
Blocks: 1209133
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.
(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.
(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 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://?
Attachment #8687472 - Flags: review?(s.kaspari) → feedback+
(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?
Flags: needinfo?(s.kaspari)
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. :)
Flags: needinfo?(s.kaspari)
Attachment #8687472 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/22b421afcf73
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
By Finkle's word on irc, we land in 44.
tracking-fennec: --- → 44+
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.
Attachment #8687472 - Flags: approval-mozilla-aurora?
Comment on attachment 8687472 [details] [diff] [review]
v0-ensure '//'

Aurora44+
Attachment #8687472 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1334850
You need to log in before you can comment on or make changes to this bug.