Story #1971

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 almost 8 years ago. Updated over 7 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

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 Amstutz

Task #2315: Review 1971-show-image-thumbnails branch for PeterResolvedPeter Amstutz

History

#1 Updated by Peter Amstutz over 7 years ago

  • Story points set to 1.0

#2 Updated by Tom Clegg over 7 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 7 years ago

  • Target version set to 2014-03-05 Data management

#4 Updated by Tom Clegg over 7 years ago

  • Status changed from New to In Progress

#5 Updated by Tom Clegg over 7 years ago

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

#6 Updated by Tom Clegg over 7 years ago

  • Release changed from 4 to 5

#7 Updated by Peter Amstutz over 7 years ago

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

#8 Updated by Tim Pierce over 7 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?

#9 Updated by Peter Amstutz over 7 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 over 7 years ago

  • Assigned To set to Peter Amstutz

#11 Updated by Peter Amstutz over 7 years ago

  • Story points changed from 1.0 to 0.5

#12 Updated by Peter Amstutz over 7 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 over 7 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 over 7 years ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF