796729 - Sanitize Web Activities

Status: RESOLVED WONTFIX

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

Description

[GH to BZ]

[GitHub issue by mounirlamouri on 2012-09-25T15:44:32Z, https://github.com/mozilla-b2g/gaia/issues/5147]
I was working with Andrea to see how we could implement tel:, sms: and mailto: handling with activities and I saw that the dial activity requires a TYPE. That makes no sense. It requires every caller to add "type: "webtelephony/number"" while we *always* expect to get a number there. In addition, "webtelephony/number" means nothing.

We should sanitize how Web Activities are used in Gaia because this is going to be part of our API to developers and clearly, this hasn't been thought enough.

In a nutshell, type should only be used when it is possible that an activity can receive input in different type. To make sure the handler knows the actual type, 'type' can be used to specify it. Typically, an image editing activity will require a type to make sure it handles the image correctly. However, a dial activity shouldn't use a type because the number is always going to be a number. If by any chance, the activity is extended to allow passing a contact, a |contact| field should be used in data or another activity should be used.

After a quick glance, it feels like all activities require a type while, really, nearly none of them should.

This should definitely block IMO.

Comment 1

[GH to BZ]

[GitHub comment by etiennesegonzac on 2012-09-25T15:50:49Z]
So

[ ] Remove the type from the DIAL web activity

Looking at https://wiki.mozilla.org/Gaia/System/Activities#App_Interaction_Matrix the other web activity need a type (open, share, pick). Do you have any proposition for the types in question so we can list the changes needed here?

Comment 2

[GH to BZ]

[GitHub comment by fabricedesre on 2012-09-25T16:06:27Z]
I think that the gallery, video and music app have legitimate use case for 'type', but that we should rename it "mime-type", just to make it clear what it is.

Comment 3

[GH to BZ]

[GitHub comment by etiennesegonzac on 2012-09-25T16:44:38Z]
/cc @davidflanagan

Comment 4

[GH to BZ]

[GitHub comment by autonome on 2012-09-28T06:00:40Z]
It's a bit late to do rethinking of things like this for V1. Blocking-. Let's get the Brazil release out and make things like this correct in V2.

Comment 5

[GH to BZ]

[GitHub comment by mounirlamouri on 2012-09-28T08:57:06Z]
Doing that could break apps using Web Activities when upgrading to v2 and also would make us look like fools because we are going to provide badly designed Web Activities to applications.

Comment 6

[GH to BZ]

[GitHub comment by davidflanagan on 2012-09-28T16:28:00Z]
I've been meaning to follow up the big activites table on the wiki with documentation of all the activities we have now, with all of their inputs and outputs.  We have to do that so that someone like Sheppy can document them, anyway.

@mounirlamouri is right. These are public facing API, and we need to get them right before v1.  Right now we've got the features implemented.  Let's treat the bad API as implementation bugs to be fixed after feature freeze.

@autonome: I don't know whether this needs to block, but it is public API, not implementation detail, and getting it right now is much easier than creating upgrade headaches for our app developer community. Its also a matter of pride; we don't want the shoddy bits showing.  

We need to do this.  And given that we've only had working inline activities for a week or so, its not really something that could have happened earlier. 

Here's the work plan I propose, to begin ASAP after feature freeze

0) We appoint one person as Activities lead

1) That person documents the current state of all activities we're using.

2) That person proposes a minimal set of changes to make the activities consistent and sane.  When activities cannot be implemented correctly for v1, the proposal should change the activity name to make it clear that this is not the final version of the activity. (All the image picking activities, for example, pass data: URLs instead of blobs, so the name should be changed form "pick" to "pick-dataurl" or something so that we can easily deprecate it when we've got blob support. The bug for blob support is https://bugzilla.mozilla.org/show_bug.cgi?id=782766, but no one is assigned.  And it is not even nominated as a blocker... is it too late to do that?)

3) We discuss on dev-gaia

4) The lead updates the proposal based on the discussion and the updated proposal becomes our API specification for all Gaia activities

5) The lead files bugs for each activity.  Fixing each bug will require all apps that initiate or handle that one particular activity to be updated in parallel.

I nominate myself as Activities lead.

Comment 7

[GH to BZ]

[GitHub comment by davidflanagan on 2012-09-28T16:36:00Z]
Let me be clear about that "shoddy bits" comment.  I'm not saying anyone has done shoddy work!  Just that we've all been making activities up on the fly and there has not yet been time for coordination and sanity checking. It is time for that sanity check now, and we must not freeze the activities API in its current state.

@autonome I've renominated this as a blocker, though as I said, I think it is work that needs to happen even if it doesn't block.

Comment 8

[GH to BZ]

[GitHub comment by autonome on 2012-09-28T19:55:39Z]
Thanks for the clarifications everyone. While this does seem like cleanup/polish to make the set of implemented activities more cohesive and consistent, it's definitely blurry as to whether it's feature work or not.  Assuming this doesn't require re-architecture, we could possibly take changes for V1.

Also, please note that V1 isn't going to the whole world. It's going to Brazil and a couple of other countries. We have to think of these as something we can change moving forward - things that *will* change, actually.

Thanks for offering to lead this effort David. However, unless this is going to close within the next two weeks, I'm very doubtful that these changes will make V1. This needed to be done a long time ago if it was this important.

Comment 9

[GH to BZ]

[GitHub comment by davidflanagan on 2012-09-28T21:09:42Z]
v1 isn't going to the whole world, but developers around the world might write apps against it.  We don't want to lock in horrible activities and then have to keep supporting them for years.

Within 2 weeks is easy, assuming I have the authority to cut off debate and just make some changes.  The goal isn't perfection here, it is just avoiding publishing an embarassing API that we have to live with.

Marking LOE:S and, hearing no objections, assigning to myself.

Comment 10

[GH to BZ]

[GitHub comment by autonome on 2012-10-01T15:21:43Z]
Blocking-. We cannot hold the release for this. The workarounds, according to the triage group, are to support the V1 activities while shipping newer better ones in V2.

If a patch becomes available, please file a new bug in Bugzilla: https://bugzilla.mozilla.org/enter_bug.cgi?product=Boot2Gecko&component=Gaia 

And we can assess risk there.

Comment 11

[David Flanagan [:djf]]

As a first step, I've documented all the activities we use and have written up recommendations for changing them here: https://etherpad.mozilla.org/gaia-activities-audit

Comment 12

[David Flanagan [:djf]]

Jonas hopes that we can get blobs working in activities before v1 (but its not blocking). If that happens, it would alter the choices we make here. So marking this bug as depending on that one.

Comment 13

[David Flanagan [:djf]]

Now that web activities support blobs, the first step toward addressing this bug is making the share activity use blobs.  I've submitted https://github.com/mozilla-b2g/gaia/pull/6048 to do that.

Still need to find a reviewer, though.

Comment 14

[David Flanagan [:djf]]

Attachment: link to github pull request

The attachment links to a pull request on github which changes all apps that use the share image/* activity to use blobs instead of data urls

Comment 15

[David Flanagan [:djf]]

Comment on attachment 676208 [details]
link to github pull request

r=asuth over on github

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
User impact if declined: serious compatibilty headaches for v2
Testing completed: yes
Risk to taking this patch (and alternatives if risky): not risky

Comment 16

[David Flanagan [:djf]]

Attachment: link to github pull request

Comment 17

[David Flanagan [:djf]]

Comment on attachment 676263 [details]
link to github pull request

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): no regression; its just stupid to be using data URLs now that we can use blobs instead
User impact if declined: performace won't be as good
Testing completed: local tests
Risk to taking this patch (and alternatives if risky): there are blob lifetime issues when transferred through activities, and we need to watch out for those until #806503 is fixed.

Comment 18

[[e-mail], [e-mail]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6048

Pointer to Github pull-request

Comment 19

[[e-mail], [e-mail]]

The issue block Bug 806277 - [bluetooth] implement blue tooth file transfer now.
Could we let the blocking-basecamp to be + ?

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

It's also blocking closing out bug 799827, which is also blocking-basecamp.

Comment 21

[Mike Connor [:mconnor]]

Let's get this landed.

Comment 22

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 676208 [details]
link to github pull request

landed on gaia/master with merge:
https://github.com/mozilla-b2g/gaia/commit/e48360ff851924636a68544c4ec572dcc74b4cba

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 676263 [details]
link to github pull request

landed on gaia/master in merge:
https://github.com/mozilla-b2g/gaia/commit/412909862ec3aed0713c8a7bb6113e58f4516d7c

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

both patches have been landed, marking resolved.

Comment 25

[David Flanagan [:djf]]

Thanks for landing these, Andrew.  I was going to do that today, but wasn't able to retest them with the Gecko/Gaia version skew.

I'm reopening this, because there are some other activity insanity to be cleaned up under this bug as well.  The open activity is one in particular that I think you need.  But also some naming changes that Mounir wants.

Comment 26

[David Flanagan [:djf]]

Attachment: link to github pull request

I'm attaching a link to a github pull request that changes the open activity to use blobs.  This affects gallery, email and camera.  And the bluetooth app will use it too.

Andrew and Dale, would either or both of you review this?

Comment 27

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 677932 [details]
link to github pull request

Looks good.  I tested with the e-mail app, gallery app, and camera app on an unagi device, I believe covering all of the affected use cases.  I did experience a weird long-press home card view glitch, but I believe that to be unrelated.

While I'm not a camera app pro, the changes looked limited enough that I believe them to be correct.  (only the "data-type" to "data-filetype" change was notable, but it's clear from the other uses and the consumption by getAttribute that that was what was intended.)

Thanks very much for implementing the back-button mode for the 'open' app by default!

I'm clearing the review request from Dale, but feel free to review it if you like, Dale!

Comment 28

[David Flanagan [:djf]]

I've landed attachment #677932 [details].  Ian, this means that you can now use the open activity with a blob to get the Gallery to open an image from your Bluetooth app.

I'm keeping this bug open because there are still a few naming changes we've agreed to make to clean up the activities we're using. Mounir and Ben: please speak up here if there are particular activities you want to be sure we change.  Mounir: I'll be sure to remove the type from the dial activity, which was the issue that you opened this bug with.

Almost done here!

Comment 29

[David Flanagan [:djf]]

Here is the current state of proposals to clean up the public facing activities details, from https://etherpad.mozilla.org/gaia-activities-audit

1) Change "view" type="url" to "view" type="text/html"

2) Change 'pick' type="webcontacts/contact" to use type "webcontacts/number" since this activity actually only returns a phone number.

3) Change "new" type="webcontacts/contact" to "add-contact"

4) Change "new" type="websms/sms" to "compose-sms"

5) Change "new" type="mail" to "compose-mail"

6) Change "record" type="photos" to "capture" type="image/*" or just "launch-camera"?

7) Change "browse" type="photos" to "browse" type="image/*" or just "launch-gallery"

8) Change "dial" type="webtelephony/number" to just plain "dial"

9) Change "configure" target="device" to remove the target parameter

10) Change "configure target="simpin-unlock" to "private-simpin-unlock" or remove it completely.

11) Change "costcontrol/open" and "costcontrol/topup" to "private-costcontrol-open" and "private-costcontrol-topup" or remove them.

I'll work on a patch to make these changes, where I think it is possible to make them safely.

Comment 30

[David Flanagan [:djf]]

Attachment: link to github PR for activities cleanup

This patch does all of the things listed above. It doesn't remove any of the unnecessary activities, but just prefixes them with 'private'.

Comment 31

[David Flanagan [:djf]]

Comment on attachment 679074 [details]
link to github PR for activities cleanup

Mounir: this patch addesses the dial activity issue with which you opened this bug, and does a number of the other things we discussed on the etherpad.

Your feedback is welcome, but only if you send it before I get an r+ on the patch!

Comment 32

[David Flanagan [:djf]]

Note that the attachment here also fixes 801520

Comment 33

[Mounir Lamouri (:mounir)]

(In reply to David Flanagan [:djf] from comment #29)
> Here is the current state of proposals to clean up the public facing
> activities details, from https://etherpad.mozilla.org/gaia-activities-audit
> 
> 1) Change "view" type="url" to "view" type="text/html"

This is complex. More than simply setting the type to the correct value. When the type is "text/html", we will actually send an URL but in most other cases we will send a blob or a data-URI. We really need a real plan regarding the format of the data we send (will get back to that later).

> 2) Change 'pick' type="webcontacts/contact" to use type "webcontacts/number"
> since this activity actually only returns a phone number.

Using webcontacts/number isn't better than "webcontacts/contact". We shouldn't create fake MIME types but if we really need to, we should prefix them with "vnd.".
See: https://en.wikipedia.org/wiki/Internet_media_type
Also, have a look at what Android is doing in that area. They are creating a lot of new MIME types but they do a good job in prefixing them AFAICT.

> 7) Change "browse" type="photos" to "browse" type="image/*" or just
> "launch-gallery"

I prefer "browse". It's weird and I'm not sure how other types will be able to use this but it's better than "launch-gallery".

> 9) Change "configure" target="device" to remove the target parameter

I'm not a big fan of this but I guess it's not like we have any choice.
Is it really used? I don't remember seeing something like this.
Does the change consist on fully removing the target parameter or just for target="device"? I don't really understand what's the difference between "target" and "section". Or is "target" the actual section and "section" the sub-section?

> 10) Change "configure target="simpin-unlock" to "private-simpin-unlock" or
> remove it completely.
> 
> 11) Change "costcontrol/open" and "costcontrol/topup" to
> "private-costcontrol-open" and "private-costcontrol-topup" or remove them.

I hate the fact that we are using "private-": those things should not be Web Activities. But, again, I doubt we can do anything. If we are lucky, those activities will just disappear at some point.

Regarding other activities you haven't listed, what happened with the "open", "pick, webcontacts/email" and "save-bookmark" activities? I might have missed some given that the original document is barely readable with all those comments :(

Finally, I think we still have an outstanding issue: what should we do regarding the data type? We should allow passing different data types (like blob, data-uri and URI) depending on the activity and maybe the type. Web Intents define that in the activity, see http://webintents.org/share which means that you can pass data-uris and blobs most of the time to any activity. If we do that, we could rename all those "blobs" to "data" but I'm still concerned regarding URI passing. There is no simple rules that would help us know when we actually want to allow URI passing and when we wouldn't want that.
We could easily allow the three of them the only real issue would be filtering.

Comment 34

[Mounir Lamouri (:mounir)]

Comment on attachment 679074 [details]
link to github PR for activities cleanup

f+ in general but there are still some issues that need to be looked at and discussed.

Comment 35

[Ben Francis [:benfrancis]]

David, I don't feel comfortable reviewing this. Whilst I have expressed some opinions on certain web activities I don't feel qualified to review this patch gaia-wide.

However, I would like to suggest that if some of the issues Mounir is talking about start to drag on that you might create a separate pull request for the less controversial changes and ask for reviews from individual app developers. That way we can make some improvements while discussions are still ongoing.

Sorry I didn't get back to you sooner, last week was a little hectic :)

Comment 36

[David Flanagan [:djf]]

Mounir,

Thanks for the feedback.

For the view activity, I think the convention we should use is that view takes a url.  (Browser and Video do this) And open takes a blob.  (Gallery and Music support open).

I like the idea of vnd- prefixes on the made-up mime types.

I still have issues with 'browse' and 'capture' for the camera<->gallery communication, but I went with those anyway.

The target="device" think for the configure activity was just completely unused. section specifies which page of settings to display.  The only place this is used is when you pull down the notifications screen and click on the wifi icon there... it takes you into the settings app.

I agree that the private prefixed activities should just go away. I have not filed bugs about that yet.

The reason the share, open and pick activities aren't mentioned here is that we got blob support, and I convered them to use blobs. So we no longer have the issue of temporarily requiring data uris.

The reason that the save-bookmark activity isn't here is that no one had a better suggestion that would meet UX's requirements, so I left it alone.

On the data type issue, let's punt on that for v1. Right now each activity supports only one data type, generally either a blob or an http: URL.  Blobs have replaced data URIs everywhere they were used.  We can add more types and increase flexibility later if we need to.

Based on your feedback, I'd say that what I need to do is add vnd- prefixes, and generally modify the patch so that it is not bitrotted anymore.

Comment 37

[Dietrich Ayala (:dietrich)]

This hasn't moved in a few weeks. I posit again that it does not block the V1 release, marking blocking-. If you get a patch reviewed and ready to land, ask for approval to land it.

Comment 38

[David Flanagan [:djf]]

Comment on attachment 679074 [details]
link to github PR for activities cleanup

cancelling the review request

Comment 39

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Asking for this to be cleaned up as ( bug 796565 ) browser only shares via bluetooth at this moment in time rather than sharing to email, sms, twitter, facebook, any other social app.

Comment 40

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

It's a nice to have, only helps out devs.

Comment 41

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #40)
> It's a nice to have, only helps out devs.

Note that potential re-normalization/cleanup of our web activities may need to occur in 1.3 for the download manager stuff.  For example, 'pick' for most apps/types means "return a blob", but for the contacts app for mime-types "webcontacts/contact" and "webcontacts/email" it returns a contact id.

Comment 42

[Joshua Smith [:joshua-s]]

Any progress on this?

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think the first thing that we need here is a spec so that we know what to implement. Moving this to Travis who has been driving this. Not sure what the latest update is.

Comment 44

[Travis Choma]

I'm aiming to restart the earlier conversation around sanitizing web activities. The motivation is so that 3rd party developers understand how to use web activities in a standard way and that we have change control over the inputs and outputs that are used by a given activity, rather than a nebulous data dictionary. I've pasted the initial review at the bottom of the original etherpad discussion here: https://etherpad.mozilla.org/gaia-activities-audit . Feel free to add comments to the etherpad and I will revise accordingly.

Comment 45

[BMO Automation]

Firefox OS is not being worked on