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

Route for downloading slide image #26

Merged
merged 2 commits into from
Apr 8, 2020

Conversation

akhil-rana
Copy link
Contributor

A new route
/getSlide/<image_name> is created which returns the image file.

@akhil-rana
Copy link
Contributor Author

This is used in caMicroscope #345

SlideServer.py Outdated Show resolved Hide resolved
Comment on lines +156 to +157
if(os.path.isfile("/images/"+image_name)):
return flask.send_from_directory(app.config["UPLOAD_FOLDER"], filename=image_name, as_attachment=True)
Copy link
Member

@birm birm Apr 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this idea is workable, since we can ask users/admins concerned about having private slides to mount them in a different directory, or otherwise not mount them to this container. For the former:
It looks like isfile can be fooled by path traversal

>>> os.path.isfile("/home/ryan/Documents/Distro/images" + "/../README.md")
True

Can you confirm that this doesn't work with send_from_directory? (e.g. put a slide file somewhere in the container outside of /images and its children, and see if you can force the server into downloading it)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As to my knowledge send_from_directory() is secure for passing file names.
send_file() should not be used in such cases so I used this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be used to get inside of the if statement but I don't think that send_from_directory() will fall into this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right.

@birm birm self-requested a review April 8, 2020 04:04
Co-Authored-By: Ryan Birmingham <birm@rbirm.us>
@birm birm merged commit a99e8a5 into camicroscope:develop Apr 8, 2020
@akhil-rana akhil-rana deleted the downloadSlide branch June 18, 2020 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants