Confirmed users
152
edits
Security/Reviews/Gaia/Video: Difference between revisions
Review for 1.2
(Review for 1.2) |
|||
| Line 2: | Line 2: | ||
* App: Video | * App: Video | ||
* Review Date: | * Review Date: 14 Sep 2013 | ||
* | * Latest Commit: https://github.com/mozilla-b2g/gaia/commit/73d7f74142f204241505bd6dfbb84d0ed83c323a | ||
* Branch Reviewed: Master | |||
* Reviewer: Stéphanie Ouillon | |||
Please see page history for details of previous reviews. | |||
=== Overview === | === Overview === | ||
| Line 13: | Line 15: | ||
The app presents a list view of available videos. Long tapping on a video opens a dialog asking if you want to delete the video. Short apping the video opens a player that immediately starts playing the video. The player also has a delete button and player controls (pause and a scrubber). | The app presents a list view of available videos. Long tapping on a video opens a dialog asking if you want to delete the video. Short apping the video opens a player that immediately starts playing the video. The player also has a delete button and player controls (pause and a scrubber). | ||
The app exposes a "view" activity that third party apps can use to play videos. This activity accepts links to content of type video/webm, video/mp4 and video/ | The app exposes a "view" activity that third party apps can use to play videos. This activity accepts links to content of type video/webm, video/mp4, video/3gpp and video/ogg. | ||
Playing videos from Youtube in the Video app is not longer available since https://github.com/mozilla-b2g/gaia/commit/c7133fc4b543ab0223e29ef1bf559d28f80eeaf6 | |||
in response of {{bug|887454}}. Instead, attempting to play a YouTube video will launch it on the YouTube page. | |||
===Architecture=== | ===Architecture=== | ||
| Line 23: | Line 26: | ||
====Relevant Source Code==== | ====Relevant Source Code==== | ||
Source code can be found at https://github.com/mozilla-b2g/gaia/tree/ | Source code can be found at https://github.com/mozilla-b2g/gaia/tree/master/apps/video | ||
Application code: | Application code: | ||
| Line 30: | Line 33: | ||
* view.html - The UI for the view that is shown for the 'view' Activity | * view.html - The UI for the view that is shown for the 'view' Activity | ||
* js/video.js - The code for the main application | * js/video.js - The code for the main application | ||
* js/ | * js/metadata.js - Metadata parsing for video files | ||
* js/ | * js/db.js - Handles the videos list | ||
* js/thumbnail_list.js - The code to render video data into a list | |||
* js/thumbnail_date_group.js - Grouping mechanism | |||
* js/thumbnail_item.js - Render a single video view object | |||
Shared code: | Shared code: | ||
* shared/js/l10n.js | * shared/js/l10n.js | ||
* shared/js/ | * shared/js/l10n_date.js | ||
* shared/js/template.js | |||
* shared/js/device_storage/enumerate_all.js< | |||
* shared/js/mediadb.js | * shared/js/mediadb.js | ||
* shared/js/blobview.js | * shared/js/blobview.js | ||
* shared/js/get_video_rotation | * shared/js/media/get_video_rotation.js | ||
====Permissions==== | ====Permissions==== | ||
| Line 58: | Line 58: | ||
* "audio-channel-content":{}, | * "audio-channel-content":{}, | ||
* "settings":{"access":"readonly"} - Because it needs to access the chosen locale, which is stored in the settings. (This is via shared/js/l10n.js, which accesses the language.current setting through navigator.mozSettings) | * "settings":{"access":"readonly"} - Because it needs to access the chosen locale, which is stored in the settings. (This is via shared/js/l10n.js, which accesses the language.current setting through navigator.mozSettings) | ||
* deprecated-hwvideo | * "deprecated-hwvideo":{}, | ||
* "audio-channel-content" | * "audio-channel-content":{}, | ||
* systemXHR | * "systemXHR":{} | ||
The application needs access to both pictures and videos. This is because when you take a video with the Camera app, it will also create a poster image and store that image in the pictures storage. When the Video app deletes a video, that poster image is also deleted, hence the requirement on readwrite to pictures. | The application needs access to both pictures and videos. This is because when you take a video with the Camera app, it will also create a poster image and store that image in the pictures storage. When the Video app deletes a video, that poster image is also deleted, hence the requirement on readwrite to pictures. | ||
| Line 66: | Line 66: | ||
The access to Settings is questionable and only used for keeping track of the locale. See the bugs filed for that in the Actions & Recommendations section. | The access to Settings is questionable and only used for keeping track of the locale. See the bugs filed for that in the Actions & Recommendations section. | ||
The systemXHR permission | The systemXHR permission was required to make a call to the YouTube API which is no longer used. This permission should be removed. | ||
====Web Activity Handlers ==== | ====Web Activity Handlers ==== | ||
| Line 72: | Line 72: | ||
The application makes the following activities available to other apps: | The application makes the following activities available to other apps: | ||
* view - To let third party applications play video files | * view - To let third party applications play video files in a standard UI. | ||
The code handling <code>view</code> is in <code>js/view.js</code>. It only knows how to play movies that you reference by type | The code handling <code>view</code> is in <code>js/view.js</code>. It only knows how to play movies that you reference by type. The following types are supported: video/webm video/mp4 video/ogg video/3gpp. | ||
* pick - To select a video. Supported types: video/* video/webm video/mp4 video/ogg video/3gpp | |||
* video/mp4 video/ogg video/3gpp | * open - To launch a video. Supported types: video/webm video/mp4 video/ogg video/3gpp | ||
* | |||
====Web Activity Usage ==== | ====Web Activity Usage ==== | ||
The | The app calls the following Web Activities: | ||
* configure - To open the media storage panel when the default storage is unavailable | |||
* record - To launch the Camera app | |||
* share - To send a video, eg. by SMS or Email | |||
==== Notable Event Handlers ==== | ==== Notable Event Handlers ==== | ||
| Line 94: | Line 93: | ||
No XSS or Injection vulnerabilities have been found in the code. | No XSS or Injection vulnerabilities have been found in the code. | ||
innerHTML is used in thumbnail_item.js and thumbnail_date_group.js with escaped input. | |||
====2. Secure Communications ==== | ====2. Secure Communications ==== | ||
====3. (Secure) data storage ==== | ====3. (Secure) data storage ==== | ||
| Line 119: | Line 104: | ||
* The IndexDB API is used to manage the metadata of the video files (key frame/thumbnail) | * The IndexDB API is used to manage the metadata of the video files (key frame/thumbnail) | ||
All the code to do this is contained in /shared/js/mediadb.js | All the code to do this is contained in /shared/js/mediadb.js. | ||
====4. Denial of Service ==== | ====4. Denial of Service ==== | ||
| Line 127: | Line 112: | ||
* DeviceStorage - used to access the video and image (posters) files | * DeviceStorage - used to access the video and image (posters) files | ||
* Settings - used by shared/js/l10n.js to keep track of locale changes | * Settings - used by shared/js/l10n.js to keep track of locale changes | ||
* SystemXHR | * SystemXHR - no longer used | ||
The usage of Settings API is questionable. See the bugs filed for that in the Actions & Recommendations section. | The usage of Settings API is questionable. See the bugs filed for that in the Actions & Recommendations section. | ||
====6. Interfaces with other Apps/Content==== | ====6. Interfaces with other Apps/Content==== | ||
==== 7. Oddities ==== | ==== 7. Oddities ==== | ||
| Line 140: | Line 123: | ||
=== Actions & Recommendations === | === Actions & Recommendations === | ||
* {{bug|932795}} SystemXHR permission should be removed. | |||
The application unnecessarily has access to all system settings. This is an issue with the Settings API that should be improved in a future version of Firefox OS: | The application unnecessarily has access to all system settings. This is an issue with the Settings API that should be improved in a future version of Firefox OS: | ||
| Line 145: | Line 130: | ||
* {{bug|841071}} Settings are globally shared between applications | * {{bug|841071}} Settings are globally shared between applications | ||
* {{bug|841196}} Applications should stop using settings permission to just get locale info | * {{bug|841196}} Applications should stop using settings permission to just get locale info | ||