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

Raise BadRequest if static file name is invalid #1763

Merged
merged 3 commits into from Apr 2, 2016

Conversation

@chaosagent
Copy link
Contributor

commented Apr 1, 2016

Fixes issue #1761

try:
if not os.path.isfile(filename):
raise NotFound()
except TypeError:

This comment has been minimized.

Copy link
@ThiefMaster

ThiefMaster Apr 1, 2016

Member

except (TypeError, ValueError):

try:
rv = flask.send_from_directory('static', 'bad\x00')
rv.close()
assert False

This comment has been minimized.

Copy link
@ThiefMaster

ThiefMaster Apr 1, 2016

Member

why not use pytest.raises to ensure an exception is raised?

with pytest.raises(ValueError):
    flask.send_from_directory('static', 'bad\x00')
@chaosagent

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2016

Thanks for the tips; I'm kinda new to serious software development ;)

@chaosagent chaosagent force-pushed the chaosagent:issues/1761 branch from 69ecee4 to 3353c14 Apr 1, 2016

with app.test_request_context():
with pytest.raises(BadRequest):
rv = flask.send_from_directory('static', 'bad\x00')
rv.close()

This comment has been minimized.

Copy link
@ThiefMaster

ThiefMaster Apr 1, 2016

Member

This shouldn't be necessary - the previous line already raises an exception so you never get here

@untitaker

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

LGTM!

@ThiefMaster When merging this, you might want to merge into the 0.10-maintenance branch, and then merge 0.10-maintenance into master, such that we can do a bugfix-release without releasing 1.0. Just make sure you only merge this PRs changes, not all the commits from master.

@chaosagent

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2016

Should I squash these commits before merge?

@ThiefMaster

This comment has been minimized.

Copy link
Member

commented Apr 2, 2016

yes, please!

@untitaker

This comment has been minimized.

Copy link
Member

commented Apr 2, 2016

@ThiefMaster https://github.com/blog/2141-squash-your-commits

On 2 April 2016 08:36:37 CEST, Adrian notifications@github.com wrote:

yes, please!


You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#1763 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@untitaker untitaker merged commit 9f1be8e into pallets:master Apr 2, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@untitaker

This comment has been minimized.

Copy link
Member

commented Apr 2, 2016

Awesome, thanks!

untitaker added a commit that referenced this pull request Apr 2, 2016

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