Skip to content

Conversation

@keigohtr
Copy link
Member

What is this PR for?

The current process allows to save files to the system file location when user specify the relative path via API. This PR checks the filename includes filepath or not.

This PR includes

  • Add filename checker
  • Add unittest

What type of PR is it?

Bugfix

What is the issue?

#16

How should this be tested?

client = DruckerDashClient(logger=logger, host=host)
response = client.run_upload("../../../../../../../tmp/NEW_FILE", bytes('uploaded!!!!\n', 'utf-8'))
print(response)

Copy link
Member

@yuki-mt yuki-mt left a comment

Choose a reason for hiding this comment

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

LGTM.

(As for readability, method name is_valid_upload_filename or something might be better than check_... because it explicitly describes return True is the file name is valid . very trivial, though)

@keigohtr
Copy link
Member Author

@yuki-mt Thank you for the review.
I agree with you. I have updated, could you check it again?

@codecov-io
Copy link

codecov-io commented Nov 22, 2018

Codecov Report

Merging #17 into master will increase coverage by 0.69%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
+ Coverage   85.87%   86.57%   +0.69%     
==========================================
  Files          16       16              
  Lines         885      931      +46     
==========================================
+ Hits          760      806      +46     
  Misses        125      125
Impacted Files Coverage Δ
drucker/drucker_dashboard_servicer.py 98.92% <100%> (+0.14%) ⬆️
drucker/test/test_dashboard_servicer.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ada62e0...51703e6. Read the comment docs.

Copy link
Member

@yuki-mt yuki-mt left a comment

Choose a reason for hiding this comment

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

LGTM

@keigohtr keigohtr merged commit 14703f8 into master Nov 22, 2018
@keigohtr keigohtr deleted the fix/check-invalid-filepath branch November 22, 2018 06:55
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.

4 participants