Story #1971
When a job output contains a single image file, show a thumbnail image inline on Workbench pipeline_instance and job pages.
100%
Subtasks
History
#1
Updated by Peter Amstutz over 8 years ago
- Story points set to 1.0
#2
Updated by Tom Clegg over 8 years ago
- Subject changed from Visualize analysis results in Workbench (e.g., show a set of outputs as image thumbnails) to When a job output contains a single image file, show a thumbnail image inline on Workbench pipeline_instance and job pages.
#3
Updated by Tom Clegg over 8 years ago
- Target version set to 2014-03-05 Data management
#4
Updated by Tom Clegg about 8 years ago
- Status changed from New to In Progress
#5
Updated by Tom Clegg about 8 years ago
- Target version deleted (
2014-03-05 Data management)
#6
Updated by Tom Clegg about 8 years ago
- Release changed from 4 to 5
#7
Updated by Peter Amstutz about 8 years ago
- Target version set to 2014-04-16 Dev tools and data/resource management
#8
Updated by Tim Pierce about 8 years ago
Code review:
- apps/workbench/app/helpers/application_helper.rb
- This code looks like it will render thumbnails for the first three images it finds in the collection we're rendering, which seems to differ from the story here (render a thumbnail IFF the collection contains exactly one image). Can we clarify the goal?
- Comments would be helpful here too, e.g. "If thumbnails were requested, render thumbnails for the first three images found in the collection"
- apps/workbench/app/helpers/collections_helper.rb
- The role of
CollectionsHelper.file_path
isn't immediately clear. Is the idea to provide a relative file path that Workbench can use to construct a URL for viewing just a single file within a collection? Doc comments for each helper method would help. - What happens if a user manages to upload a collection with "file" names like
../../../../etc/passwd
? Do we have sufficient protections against this happening before it gets as far aslink_to_if_arvados_object
?
- The role of
#9
Updated by Peter Amstutz about 8 years ago
- Target version changed from 2014-04-16 Dev tools and data/resource management to 2014-05-07 Storing and Organizing Data
#10
Updated by Peter Amstutz about 8 years ago
- Assigned To set to Peter Amstutz
#11
Updated by Peter Amstutz about 8 years ago
- Story points changed from 1.0 to 0.5
#12
Updated by Peter Amstutz about 8 years ago
- Fixed behavior to conform to story
- Added comments and method documentation
file_path is only used to construct a relative path to append to the collection URL that is passed to the browser, so if http://qr1hi/collections/abcdef+123/../../etc/passwd is a viable exploit, we have bigger problems. Since Workbench uses arv-get to fetch files, the path is only ever processed in the context of the manifest and is never passed to open(), but this could be a problem if we were using arv-mount. Checking and removing '..' from manifests is something that arv-normalize should do (which is used by API server to validate manifests), but I can't rememeber if it does it presently. That is probably a good story/bug to add to the backlog.
#13
Updated by Tim Pierce about 8 years ago
Peter Amstutz wrote:
- Fixed behavior to conform to story
- Added comments and method documentation
file_path is only used to construct a relative path to append to the collection URL that is passed to the browser, so if http://qr1hi/collections/abcdef+123/../../etc/passwd is a viable exploit, we have bigger problems. Since Workbench uses arv-get to fetch files, the path is only ever processed in the context of the manifest and is never passed to open(), but this could be a problem if we were using arv-mount. Checking and removing '..' from manifests is something that arv-normalize should do (which is used by API server to validate manifests), but I can't rememeber if it does it presently. That is probably a good story/bug to add to the backlog.
OK, that makes sense. I've added #2661 as an umbrella story for general security auditing. Let's talk about it at the next planning meeting.
The comments are very helpful, thanks for adding these. LGTM.
#14
Updated by Peter Amstutz about 8 years ago
- Status changed from In Progress to Resolved