Project

General

Profile

Actions

Story #1971

closed

When a job output contains a single image file, show a thumbnail image inline on Workbench pipeline_instance and job pages.

Added by Tom Clegg about 9 years ago. Updated almost 9 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Start date:
03/05/2014
Due date:
% Done:

100%

Estimated time:
(Total: 3.00 h)
Release relationship:
Auto

Subtasks 2 (0 open2 closed)

Task #2141: Detect common image formats and show thumbnail image inline (linking to full-size image) next to normal output link (which links to collection detail page)ResolvedPeter Amstutz03/05/2014

Actions
Task #2315: Review 1971-show-image-thumbnails branch for PeterResolvedPeter Amstutz03/07/2014

Actions
Actions #1

Updated by Peter Amstutz about 9 years ago

  • Story points set to 1.0
Actions #2

Updated by Tom Clegg about 9 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.
Actions #3

Updated by Tom Clegg about 9 years ago

  • Target version set to 2014-03-05 Data management
Actions #4

Updated by Tom Clegg almost 9 years ago

  • Status changed from New to In Progress
Actions #5

Updated by Tom Clegg almost 9 years ago

  • Target version deleted (2014-03-05 Data management)
Actions #6

Updated by Tom Clegg almost 9 years ago

  • Release changed from 4 to 5
Actions #7

Updated by Peter Amstutz almost 9 years ago

  • Target version set to 2014-04-16 Dev tools and data/resource management
Actions #8

Updated by Tim Pierce almost 9 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 as link_to_if_arvados_object?
Actions #9

Updated by Peter Amstutz almost 9 years ago

  • Target version changed from 2014-04-16 Dev tools and data/resource management to 2014-05-07 Storing and Organizing Data
Actions #10

Updated by Peter Amstutz almost 9 years ago

  • Assigned To set to Peter Amstutz
Actions #11

Updated by Peter Amstutz almost 9 years ago

  • Story points changed from 1.0 to 0.5
Actions #12

Updated by Peter Amstutz almost 9 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.

Actions #13

Updated by Tim Pierce almost 9 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.

Actions #14

Updated by Peter Amstutz almost 9 years ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF