Browser 'view' activity handler doesn't clear background correctly for data: or javascript: URIs

RESOLVED FIXED

Status

Firefox OS
Gaia::Browser
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: pauljt, Assigned: vingtetun)

Tracking

unspecified
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: QARegressExclude)

Attachments

(1 attachment, 1 obsolete attachment)

THe browser handles the view activity when the type is URL. If the supplied URL parameter (ie the URL to be opened) is a data: or javascript: uri then the resulting content is overlayed over the page which called the mozActivity. This might lead to spoofing cases, but I can think of anything particularly compelling at the moment.
For example, the call might be:

new MozActivity({
      name : 'view',
      data : {
        type : 'url',
        url : "data:text/html;,<button onclick=alert(1)>test</button>"
      }
    });

This creates a button overlayed on the calling page.  Not really a high priority issue I think, but still a bug.
Depends on: 822152
No longer depends on: 822232
Blocks: 822152
No longer depends on: 822152
z
Assignee: nobody → 21
Created attachment 695996 [details] [diff] [review]
Patch

Let's use the new regexp filter that will be introduced by bug 816730.
Attachment #695996 - Flags: review?(dflanagan)
Comment on attachment 695996 [details] [diff] [review]
Patch

With this patch, invoking the system would look for a property named 'regexp' in the activity request.

I think the correct syntax will be something like:

"type": "url",
"url": { "required":true, "regexp":"/^https?:/" }

I don't have a m-i tree to test this against, though, so I'm not sure.

Also: is http and https enough or do we need to whitelist the app:// protocol here?  I don't know at all whether that is necessary.
Attachment #695996 - Flags: review?(dflanagan) → review-
(In reply to David Flanagan [:djf] from comment #3)
> Comment on attachment 695996 [details] [diff] [review]
> Patch
> 
> With this patch, invoking the system would look for a property named
> 'regexp' in the activity request.
> 
> I think the correct syntax will be something like:
> 
> "type": "url",
> "url": { "required":true, "regexp":"/^https?:/" }
> 
> I don't have a m-i tree to test this against, though, so I'm not sure.
> 

I'm rebuilding now.

> Also: is http and https enough or do we need to whitelist the app://
> protocol here?  I don't know at all whether that is necessary.

I don't see why someone wants to open something using the app:// protocol with the browser.
Created attachment 696070 [details] [diff] [review]
Patch v2
Attachment #695996 - Attachment is obsolete: true
Attachment #696070 - Flags: review?(dflanagan)
Comment on attachment 696070 [details] [diff] [review]
Patch v2

This looks good to me, but as noted, I'm not able to test it myself.

If it works in your testing against m-i, then r+, but coordinate the landing in gaia with the landing of the gecko patch. I don't know how things will break with this new filter in a gecko that doesn't support the syntax.

If this filter does not work against m-i, then you should ask Andrea or Mounir, I think.
Attachment #696070 - Flags: review?(dflanagan) → review+

Updated

6 years ago
Whiteboard: QARegressExclude
You need to log in before you can comment on or make changes to this bug.