Implemented possible solution for path traversal#28
Conversation
… prevent path traversal attacks.
…g path does not exists to prevent internal error.
schenkd
left a comment
schenkd
left a comment
There was a problem hiding this comment.
Hey Chris, thanks for your PR. It looks good and you're solving a problem for the v0.3 release, thanks for that!
| config_file = os.path.join(nginx_path, name) | ||
|
|
||
| if not os.path.commonprefix(os.path.realpath(config_file), nginx_path): | ||
| return flask.make_response({'success': False}), 200 |
There was a problem hiding this comment.
I would choose a different status code. For example a 400 (Bad Request) or 403 (Forbidden).
There was a problem hiding this comment.
Also note that os.path.commonprefix(("/a/b", "/c/d")) returns "/" which is truthy, so the condition is not met. And commonprefix takes an iterable and not variadic arguments so I think this errors out. Maybe use something like os.path.realpath(config_file).startswith(os.path.realpath(nginx_path) + os.sep) (though not as this ugly oneliner maybe).
|
|
||
| if not os.path.exists(config_path): | ||
| errorMessage = 'The config folder "{}" does not exists.'.format(config_path) | ||
| return flask.render_template('domains.html', errorMessage=errorMessage, sites_available=(), sites_enabled=()), 200 |
There was a problem hiding this comment.
Classic case of Java developer. Lower CamelCase is uncommon in Python. error_message would be nice! :)
There was a problem hiding this comment.
I would also like to have the string formatting with f-string syntax. Then it will be consistent in the code.
error_message = f'The config folder "{config_path}" doesn't exists.'| <div class="ui cards" id="domain_cards"> | ||
| {% if errorMessage %} | ||
| <div> | ||
| {{errorMessage}} |
There was a problem hiding this comment.
It is functional, but you could enhance it with a message from the semantic framework. Here is a link to the component: Error Messages
|
It seems like you addressed the path traversal issue only in a single endpoint. Looking through |
|
I added a new function to generate valid paths or return |
@chris18191 First of all, thank you very much for your efforts. I see you're using lower camel-case everywhere. I did not want to mention every method in the review. Could you adapt it to the Python convention? Unfortunately, I will not be able to do an extensive review before the weekend. |
3 participants
No description provided.