Closed Bug 1032577 Opened 8 years ago Closed 8 years ago

[Rocketbar] URL scheme like data: will not do anything; need some sort of UX around this?

Categories

(Firefox OS Graveyard :: Gaia::Search, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog, b2g-v2.0 affected, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S5 (4july)
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- affected
b2g-v2.1 --- fixed

People

(Reporter: nhirata, Assigned: daleharvey)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files)

Attached file 21201.html
1. run the test case from PR : 21201
2. TEST_FILES="apps/verticalhome/test/marionette/search_string_test.js" ./bin/gaia-marionette

Expected: some sort of error message stating that the url is not allowed
Actual: does not switch to browser, there are no error messages.

You can run this manually:
1. tap on the rocketbar
2. type in the rocketbar : data:text/html;base64,PGh0bWw+DQo8c2NyaXB0PndpbmRvdy5hbGVydCgiSGVsbG8gVGhlcmUiKTs8L3NjcmlwdD4NCjwvaHRtbD4=
3. hit return

note:
Gaia      c0c8ad187c0466285f2580531e09f8322996f561
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/d4dc609bcc8a
BuildID   20140630000201
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
B1TC00011220
flame
UX: needed around different URI; still exploring other URIs.
Flags: needinfo?(fdjabri)
Flags: needinfo?(kgrandon)
This shouldnt error, it should open a tab / frame with the image shown

+1000 for already written integration tests, I can take a look
Ah ok.  Thanks, Dale!

I think part of the concern though is security.
Theres an open bug for app:// urls, not sure whats happening there, but for data urls there is no security issue, chances are if firefox desktop handles it, we should
Assignee: nobody → dale
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking-]
(In reply to Dale Harvey (:daleharvey) from comment #4)
> Theres an open bug for app:// urls, not sure whats happening there, but for
> data urls there is no security issue, chances are if firefox desktop handles
> it, we should

This data URL contains an HTML page with an alert message, i.e.: 
<html><script>window.alert("Hello There");</script></html>

If the JS executes in a sandbox process (i.e. a browser tab) you're fine. If that JS occurs in the system process, then we might have a security issue.
+1 for the test, nice work. Seems like Dale has this under control. For exactly the reasons stated in comment 5, I think we should try to avoid surfacing these in the browser, especially since the browser runs in the system process.
Flags: needinfo?(kgrandon)
(In reply to Clint Talbert ( :ctalbert ) from comment #5)
> (In reply to Dale Harvey (:daleharvey) from comment #4)
> > Theres an open bug for app:// urls, not sure whats happening there, but for
> > data urls there is no security issue, chances are if firefox desktop handles
> > it, we should
> 
> This data URL contains an HTML page with an alert message, i.e.: 
> <html><script>window.alert("Hello There");</script></html>
> 
> If the JS executes in a sandbox process (i.e. a browser tab) you're fine. If
> that JS occurs in the system process, then we might have a security issue.

Paul - Can you check this to see if there's a security issue here?
Flags: needinfo?(ptheriault)
Dale - since you took this one, can you integrate with the test case here, or land the patch from bug 1032539 with yours? Blocking so we don't lose track. Thanks!
Depends on: 1032539
(In reply to Clint Talbert ( :ctalbert ) from comment #5)
> (In reply to Dale Harvey (:daleharvey) from comment #4)
> > Theres an open bug for app:// urls, not sure whats happening there, but for
> > data urls there is no security issue, chances are if firefox desktop handles
> > it, we should
> 
> This data URL contains an HTML page with an alert message, i.e.: 
> <html><script>window.alert("Hello There");</script></html>
> 
> If the JS executes in a sandbox process (i.e. a browser tab) you're fine. If
> that JS occurs in the system process, then we might have a security issue.

data: URIs inherit the origin from the document that loaded it (at least in Firefox,  webkit always treats data: uris as a separate option). E.g. if foo.com links to a data: URI, the origin is foo.com. 

However if you just paste a data: URI into the address bar, it should get a null principal (same as about:blank). See bug 656433 which implemented this change i think (3 years ago).

The main thing we need to be careful of here is to make sure that entering data: uris into the rocketbar, doesn't introduce script into the page currently loaded. That is definitely not on spec, and dangerous. (it is for this reason that we forbid javascript: uris with the browser.urlbar.filter.javascript, after a rash of facebook/other site attacks convincing users to own themselves iirc)
Flags: needinfo?(ptheriault)
er I obviously meant "...webkit always treats data: uris as a separate ORIGIN..."
(In reply to Clint Talbert ( :ctalbert ) from comment #5)
> (In reply to Dale Harvey (:daleharvey) from comment #4)
> > Theres an open bug for app:// urls, not sure whats happening there, but for
> > data urls there is no security issue, chances are if firefox desktop handles
> > it, we should
> 
> This data URL contains an HTML page with an alert message, i.e.: 
> <html><script>window.alert("Hello There");</script></html>
> 
> If the JS executes in a sandbox process (i.e. a browser tab) you're fine. If
> that JS occurs in the system process, then we might have a security issue.

By the way - if there is EVER anyway to influence the browser app code from the URL bar that is really super dangerous bad. But this should be possible - iirc the browser apps URL loading  by setting the location of a remote (OOP) iframe. 

I think the bigger risk here is that when you enter a data: uri into the browser app navigation bar (or we receive a data URI from an activity handler etc), you inherit the origin of the currently loaded page, which is definitely not the behavior that we want.
(In reply to Dale Harvey (:daleharvey) from comment #4)
> Theres an open bug for app:// urls, not sure whats happening there, but for
> data urls there is no security issue, chances are if firefox desktop handles
> it, we should

We deliberately prevent loading or linking app: urls between apps (see bug 773886). I think this is mainly to enforce segregation between apps, and to stop apps just loading other app's content. 

Eventually we want a way to link to packaged apps, but there are some threats we probably need to think about (e.g. any app that using url fragments to maintain state might have issues if webpages/apps can start loading apps at a specific URL).
The javascript executes in the remote process, not the system browser, we just set the .src on a remote iframe

> I think the bigger risk here is that when you enter a data: uri 
> into the browser app navigation bar (or we receive a data URI from 
> an activity handler etc), you inherit the origin of the currently 
> loaded page, which is definitely not the behavior that we want.

How do we avoid this? when we recieve a uri from an activity we always open a new tab which will have a null principal, but when you enter a data uri thats not true, also relying on opening new tabs via activities seems somewhat unreliable
Flags: needinfo?(ptheriault)
So actually theres a bit more of the problem than we would expect in the browser

The activity has a regex on it

  "required":true, "pattern":"https?:.{1,16384}", "patternFlags":"i"

This is so we dont provide the browser as an option to open tel: urls etc, we need to use that regex to differentiate whether we fire an activity or not, anything that doesnt match that regex becomes a search, and add data uris to that regex
Dale, fyi I did see that the only one that works is emailto ; bug 1031030
Initial investigation was a bit off, we cant match the regex since we should support urls that the browser doesnt support, so we need to test this on a case by case bases (tel / sms / mailto should be supported, ftp: and other (valid urls with no receivers) will need to be handled seperately)

This opens data: urls in the browser including Naoki's test
Attachment #8449912 - Flags: review?(kgrandon)
Naoki I made a few minor ammendments to the test in the follow up, just flagging so you notice them, I think its easier for me to follow up changes with the fix than to have the test go through seperate review

The current test wouldnt launch anything, you need to press enter so added that @ https://github.com/daleharvey/gaia/commit/0e67eddab63bc192ed946d85e43534fcaaca048b#diff-befceef5444c1284c8b8c895316397c3R49

There isnt any specific reason to use base64 so switched to just html @ https://github.com/daleharvey/gaia/commit/0e67eddab63bc192ed946d85e43534fcaaca048b#diff-befceef5444c1284c8b8c895316397c3R56, and took out the alert as that will make follow up tests harder

There isnt any need for the server setup @ https://github.com/daleharvey/gaia/commit/28348936aece91bddc415579a1d8dd28f912979e#diff-befceef5444c1284c8b8c895316397c3R20 but I suspect we will need it at some point so I just left it
Flags: needinfo?(nhirata.bugzilla)
Duplicate of this bug: 1032539
Oops!  Rather embarassing, I forgot to include the carriage return that Kevin told me about.  Thanks for making the changes. I'll review them and hopefully make better test cases for the next time around.
Flags: needinfo?(nhirata.bugzilla)
Comment on attachment 8449912 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21314

So the code generally looks fine to me, and I could probably R+ it, but I do think it's worth it here to get some more eyes on this before I leave an R+. I read through this bug, and while I think I have an understanding, I would like to have a few more people look at it first.

FYI - it is possible to enter the same data URI as in the description, and the user will see an alert in the browser app: data:text/html;base64,PGh0bWw+DQo8c2NyaXB0PndpbmRvdy5hbGVydCgiSGVsbG8gVGhlcmUiKTs8L3NjcmlwdD4NCjwvaHRtbD4= I'm not really sure if this is something we should be worried about, or if it's a realistic use-case to get into.

I think it's worth getting some feedback on this patch from Paul and Fabrice.
Attachment #8449912 - Flags: review?(kgrandon)
Attachment #8449912 - Flags: feedback?(ptheriault)
Attachment #8449912 - Flags: feedback?(fabrice)
(In reply to Dale Harvey (:daleharvey) from comment #13)
> The javascript executes in the remote process, not the system browser, we
> just set the .src on a remote iframe
> 
> > I think the bigger risk here is that when you enter a data: uri 
> > into the browser app navigation bar (or we receive a data URI from 
> > an activity handler etc), you inherit the origin of the currently 
> > loaded page, which is definitely not the behavior that we want.
> 
> How do we avoid this? when we recieve a uri from an activity we always open
> a new tab which will have a null principal, but when you enter a data uri
> thats not true, also relying on opening new tabs via activities seems
> somewhat unreliable

Why do we even want to support entering data: URIs directly into the rocketbar? I am not at all familiar enough with rocketbar, but I can't think of how or why a user would ever want to do this. (especially without copy + paste)

As for the way its handled with activities? Are you worried that there might be a case when you don't open a fresh window?
Flags: needinfo?(ptheriault)
(In reply to Kevin Grandon :kgrandon from comment #20)
> Comment on attachment 8449912 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/21314
> 
> So the code generally looks fine to me, and I could probably R+ it, but I do
> think it's worth it here to get some more eyes on this before I leave an R+.
> I read through this bug, and while I think I have an understanding, I would
> like to have a few more people look at it first.
> 
> FYI - it is possible to enter the same data URI as in the description, and
> the user will see an alert in the browser app:
> data:text/html;base64,
> PGh0bWw+DQo8c2NyaXB0PndpbmRvdy5hbGVydCgiSGVsbG8gVGhlcmUiKTs8L3NjcmlwdD4NCjwva
> HRtbD4= I'm not really sure if this is something we should be worried about,
> or if it's a realistic use-case to get into.
> 
> I think it's worth getting some feedback on this patch from Paul and Fabrice.

I'd like to test this, but as far as I know, so long as we create a new tab (and new remote mozbrowser iframe) I think we are ok. Actually even if you re-use a tab, I think its fine. 

There is nothing wrong with seeing an alert, I'd only worry if we saw something other than a data URL with an example like:

data:text/html,<script>alert(window.top.location)</script>

If you embed that in a regular iframe inside a page, the location of the parent frame will be shown. But in this case, we replace the frames location so I think its fine.
Attachment #8449912 - Flags: feedback?(ptheriault) → feedback+
The rocketbar is going to become analogous to the awesomebar in that it will be the primary interface for accessing urls, although I dont think there is particular need for the current browser to support it, having support and tests in place for future will help (since we have bugs to file on tel: support etc as well.
Comment on attachment 8449912 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21314

Ok, happy leaving R+ here since Paul has provided feedback. Thanks!
Attachment #8449912 - Flags: review+
There was a genuine build ftest failure which I fixed, landed in

https://github.com/mozilla-b2g/gaia/commit/9f0f26b7f70cd1fd50f58027e8c612803dd7145d
https://github.com/mozilla-b2g/gaia/commit/7d5ecb4ff78cbade6e0bcd58d4e2d6b194d0184a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #8449912 - Flags: feedback?(fabrice)
Target Milestone: --- → 2.0 S5 (4july)
Whiteboard: [systemsfe]
blocking-b2g: --- → 2.0?
I don't think that we should block on this for Rocketbar unless there is some critical use case that I'm not familiar with.
blocking-b2g: 2.0? → backlog
Flags: needinfo?(fdjabri)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.