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?

• 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.

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.