Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move PlantTribes datatypes to a separate file with enhancements and fixes #4137

Merged
merged 4 commits into from Jun 8, 2017

Conversation

Projects
None yet
5 participants
@gregvonkuster
Copy link
Contributor

commented Jun 1, 2017

Replaces #3999

@galaxybot galaxybot added the triage label Jun 1, 2017

@galaxybot galaxybot added this to the 17.09 milestone Jun 1, 2017

line_no = 0
with open(filename, "r") as fh:
line_no += 1
if line_no > 10000:

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Jun 1, 2017

Member

It seems you're never incrementing line_no to more than 1. Was this perhaps supposed to be a for loop?

This comment has been minimized.

Copy link
@gregvonkuster

gregvonkuster Jun 1, 2017

Author Contributor

Yes, I believe you're right (yikes!). I have not idea how this happened, but I think it is now fixed. Thanks!!

MetadataElement(name="num_files", default=0, desc="Number of files in files_path directory", param=MetadataParameter, readonly=True, visible=False, no_value=0)

def display_data(self, trans, data, preview=False, filename=None, to_ext=None, **kwd):
file_path = trans.app.object_store.get_filename(data.dataset, extra_dir='dataset_%s_files' % data.dataset.id, alt_name=filename)

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Jun 2, 2017

Member

I tend to think this (generating the HTML files) is best done in the tool(s) that generates PlantTribes datasets, so that you don't need to define your own custom display_data method.
That would ensure the display of your datatype depends on the tool version that the user has used to make the file and that it does not change when the admin upgrades galaxy, and it would reduce the amount of duplicated logic in galaxy. Finally, how is this method handling the preview=False case ? Most datatypes would delegate this to self._serve_raw(trans, dataset, to_ext, **kwd).

This comment has been minimized.

Copy link
@gregvonkuster

gregvonkuster Jun 2, 2017

Author Contributor

Generating the HTML files is done within the tools as you describe within this function https://github.com/gregvonkuster/galaxy_tools/blob/master/tools/plant_tribes/utils.py#L57. Overriding the display_data() function here is necessary for datasets whose associated files_path consists of a hierarchy of directories more than 1 level deep. The base display_data() function does not allow for clicking on a directory link to display its contents: https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/datatypes/data.py#L342... This function overrides the base display_data() only if the link clicked is a directory:

if os.path.isdir(file_path):

If it is a file, the function passes processing to the base display_data() function which handles the preview=False case:

else:
    return super(PlantTribes, self).display_data(trans, data, preview=preview, filename=filename, to_ext=to_ext, **kwd)

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Jun 2, 2017

Member

Overriding the display_data() function here is necessary for datasets whose associated files_path consists of a hierarchy of directories more than 1 level deep. The base display_data() function does not allow for clicking on a directory link to display its contents:

OK, so this was disabled because of security concerns (which may not be entirely valid), so we should either modify this in the Html class or not allow it at all. Users can force their datasets to have the PlantTribes datatype, so I don't think we should introduce this backdoor in just a single datatype.

This comment has been minimized.

Copy link
@gregvonkuster

gregvonkuster Jun 2, 2017

Author Contributor

I was not aware of the reasons for not allowing directory link clicks, so figured I'd take this approach. What are the security concerns if they are, in fact, valid? I'd be happy to move this code into the base display_data() function - I think it is important to be able to click directory links in these Html datatypes. Restricting users from doing this does not allow for a proper view of the dataset. If the security concerns are valid, can we come up with an approach that will alleviate them?

This comment has been minimized.

Copy link
@erasche

erasche Jun 2, 2017

Member

Thought process:

  • exposing a directory listing would expose all of the contents in the folder via the web
    • exposing secret files? No: the accessible files could just be downloaded and the contents examined.
    • exposing files which should not be served? No: we're already bleaching / whitelisting tools, serving other files could already happen via knowing the file + constructing the URL to it.

I personally cannot immediately think of any security concerns by producing directory listings (but this does not preclude them from existing).

Some webapp scanners will alert / complain about it. E.g. http://www.tenable.com/plugins/index.php?view=single&id=40984

Looks like it was added originally in 6d7d849 @blankenberg any rationale when that commit was made 6 years ago? ;)

This comment has been minimized.

Copy link
@gregvonkuster

gregvonkuster Jun 5, 2017

Author Contributor

Please let me know if / when the consensus is for me to move this logic into the base display_data() function. Thanks!

This comment has been minimized.

Copy link
@dannon

dannon Jun 5, 2017

Member

As long as it's constrained, I think it'd be fine to allow. We just really don't want anyone doing a dirlist like:

http://192.168.0.101:8080/datasets/e6967dd012a3aca3/display?filename=../../../cool_stuff_that_is_not_mine/

This comment has been minimized.

Copy link
@gregvonkuster

gregvonkuster Jun 5, 2017

Author Contributor

Ok, thanks, I've moved the logic into the base display_data() function.

@gregvonkuster

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2017

Are these test failures https://jenkins.galaxyproject.org/job/docker-api/7501/ known issues or does something need to be fixed in this PR? If the former, can this be merged?

@dannon

This comment has been minimized.

Copy link
Member

commented Jun 6, 2017

I reran the tests and they passed this time, seems to be unrelated. I want to test a couple things with the directory listing, and then I'll be happy to merge this.

@gregvonkuster

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2017

Cool - thanks! ;)

@dannon

This comment has been minimized.

Copy link
Member

commented Jun 6, 2017

Ok, tried a bunch of things to see if I could get this filename resolution to misbehave and allow access it shouldn't, and I have been unable to do anything malicious. @mvdbeek were you comfortable with the change?

@gregvonkuster

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2017

Ping @mvdbeek

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Jun 7, 2017

I don't have a screen around till Friday, feel free to merge if you think it's fine, I don't have any objections.

@gregvonkuster

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2017

Awesome! I would really appreciate a merge on this. ;)

@dannon dannon merged commit 848bcd8 into galaxyproject:dev Jun 8, 2017

5 checks passed

api test Build finished. 279 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 150 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 34 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@gregvonkuster

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2017

Thanks a bunch! ;)

@gregvonkuster gregvonkuster deleted the gregvonkuster:ptformats4 branch Jun 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.