963322 - [B2G][Browser] Multiple "Unable to connect" errors in browser app have %20 in place of spaces

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect, P1)

Description

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot - 1

Description:
Multiple "Unable to connect" errors in the browser app appear to have parsing issues where "%20" appears in place of character spaces.

Errors include:

"The%20URL%20is%20not%20valid%20and%20cannot%20be%20loaded." ("The URL is not valid and cannot be loaded." - Received on trying to navigate to http://:5000)

"This%20address%20uses%20a%20network%20port%20which%20is%20normally%20used%20for%20purposes%20other%20than%20Web%20browsing.%20Firefox%20has%20cancelled%20the%20request%20for%20your%20protection" ("This address uses a network port which is normally used for purposes other than Web browsing. Firefox has cancelled the request for your protection" - Received on trying to navigate to http://::1/)

Repro Steps:
1) Updated Buri to BuildID: 20140121004137
2) Open browser app
3) Type "::1" into URL bar and tap enter

Actual:
Incorrect string parsing causes error text to be difficult to read.

Expected:
All error messages easily encountered by end user are readily readable.

Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20140121004137
Gaia: 47049555282a9a01fb60d1e1421b57e2810c96f5
Gecko: 6f7dfe36ab6c
Version: 28.0a2
Firmware Version: V1.2-device.cfg

Notes:

Repro frequency: 3/3, 100%
See attached: screenshot

Comment 1

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot - 2

Comment 2

[Jason Smith [:jsmith]]

Those characters shouldn't even be possible to get into that error message. Is this vulnerable to XSS?

Paul - Do you know?

Comment 3

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #2)
> Those characters shouldn't even be possible to get into that error message.
> Is this vulnerable to XSS?
> 
> Paul - Do you know?

This btw is referring to the app error page itself, as I'm surprised that %20 could be rendered here in a spot of the app error page doesn't support custom input.

Comment 4

[Michael Henretty [:mikehenrty][:mhenretty]]

This is not an XSS vulnerability. That message string is coming from gecko, but even if someone could hijack that message, we use .textContent to render the message:

https://github.com/mozilla-b2g/gaia/blob/4347460dfda2c6a75c39164e389293bb73591122/apps/system/js/net_error.js#L128

However, I think this is still a bug. We should be decoding those characters before setting them as textContent.

Comment 5

[Jason Smith [:jsmith]]

When this happens, this is bad, as it makes reading the error page incredibly difficult to understand. However, I'm not sure of what the probability is of reproducing this.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #5)
> When this happens, this is bad, as it makes reading the error page
> incredibly difficult to understand. However, I'm not sure of what the
> probability is of reproducing this.

Probability meaning the likelihood an end-user would actually run into this bug.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #6)
> (In reply to Jason Smith [:jsmith] from comment #5)
> > When this happens, this is bad, as it makes reading the error page
> > incredibly difficult to understand. However, I'm not sure of what the
> > probability is of reproducing this.
> 
> Probability meaning the likelihood an end-user would actually run into this
> bug.

Nailed down the probability - this reproduces with any app error that deals with unknown protocol error. That's realistic for an end-user to hit on the web, so this is worth nominating.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #7)
> (In reply to Jason Smith [:jsmith] from comment #6)
> > (In reply to Jason Smith [:jsmith] from comment #5)
> > > When this happens, this is bad, as it makes reading the error page
> > > incredibly difficult to understand. However, I'm not sure of what the
> > > probability is of reproducing this.
> > 
> > Probability meaning the likelihood an end-user would actually run into this
> > bug.
> 
> Nailed down the probability - this reproduces with any app error that deals
> with unknown protocol error. That's realistic for an end-user to hit on the
> web, so this is worth nominating.

This looks like a regression too - I'm actually hitting this with all fallback errors. I don't recall hitting this previously, so I think this is a regression.

Comment 9

[Jason Smith [:jsmith]]

At a minimum, I know this worked when bug 952305 originally landed (which was on 1/9/2014).

Comment 10

[Jason Smith [:jsmith]]

Also - this is being seen on the iframe app error UX as well. So the regression here might be in gecko.

Comment 11

[Peter Bylenga [:PBylenga]]

The regression window is 1/9/2014 to 1/10/2014.  Also on 1/9/2014 and earlier "no network" error message is given instead of the actual results in Comment 0.

First build issue reproduces on:
v1.3 Environmental Variables:
Device: buri v1.3 MOZ
BuildID: 20140110004009
Gaia: c606b129a2d1647c0fc7bfb197555026e9b27f9e
Gecko: c5109884ae3a
Version: 28.0a2
Firmware Version: v1.2-device.cfg

Last build issue did not reproduce on:
v1.3 Environmental Variables:
Device: buri v1.3 MOZ
BuildID: 20140109004002
Gaia: 22bc6be5b76cdc6d4e9667ff070979041a20ce2f
Gecko: 2c8f8683bd0d
Version: 28.0a2
Firmware Version: v1.2-device.cfg

Comment 12

[Jason Smith [:jsmith]]

(In reply to Peter Bylenga from comment #11)
> The regression window is 1/9/2014 to 1/10/2014.  Also on 1/9/2014 and
> earlier "no network" error message is given instead of the actual results in
> Comment 0.
> 
> First build issue reproduces on:
> v1.3 Environmental Variables:
> Device: buri v1.3 MOZ
> BuildID: 20140110004009
> Gaia: c606b129a2d1647c0fc7bfb197555026e9b27f9e
> Gecko: c5109884ae3a
> Version: 28.0a2
> Firmware Version: v1.2-device.cfg
> 
> Last build issue did not reproduce on:
> v1.3 Environmental Variables:
> Device: buri v1.3 MOZ
> BuildID: 20140109004002
> Gaia: 22bc6be5b76cdc6d4e9667ff070979041a20ce2f
> Gecko: 2c8f8683bd0d
> Version: 28.0a2
> Firmware Version: v1.2-device.cfg

Guess that implies this isn't a regression. Maybe this patch just worked locally, but failed upon checkin?

Comment 13

[Aus Lacroix [:aus]]

I'll have a look at this today. Should be an easy fix.

Comment 14

[Aus Lacroix [:aus]]

Attachment: Pull request - Mainline 1.3

Comment 15

[Aus Lacroix [:aus]]

Attachment: Pull Request - Tarako 1.3

Same changes, but for 1.3t

Comment 16

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8366957 [details] [review]
Pull request - Mainline 1.3

Code looks good. Verified on 1.3 Keon that this patch fixes the bug. Let's wait for travis to go green, then merge (I restarted the build since it looks like NPM was down again yesterday).

Comment 17

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8366958 [details] [review]
Pull Request - Tarako 1.3

Same as above, although, I don't have an actual device to test the code.

Comment 18

[Aus Lacroix [:aus]]

Fixed on 1.3:  Commit = https://github.com/mozilla-b2g/gaia/commit/8defa5bf0cbce290c649e564b7f3ebe708e19b23
Fixed on 1.3t: Commit = https://github.com/mozilla-b2g/gaia/commit/7c68649158ec610665ff5e6b75e7ec5d483a91ce

Marking fixed.

Comment 19

[Jason Smith [:jsmith]]

lgtm on 1.3 with some sanity checking with content errors in browser & app.

Why wasn't this landed on master?

Comment 20

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Jason Smith [:jsmith] from comment #19)
> lgtm on 1.3 with some sanity checking with content errors in browser & app.
> 
> Why wasn't this landed on master?

I can answer that. Between aurora and m-c, there was a change in gecko that did the decoding for us (that's why the screenshot in original bug was correct). Sadly, this change never made it into 1.3's gecko.