Implemented possible solution for path traversal #28
Conversation
… prevent path traversal attacks.
…g path does not exists to prevent internal error.
app/api/endpoints.py
Outdated
| 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).
app/api/endpoints.py
Outdated
| @@ -57,6 +62,9 @@ def get_domains(): | |||
| sites_available = [] | |||
| sites_enabled = [] | |||
|
|
|||
| 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.'| @@ -1,6 +1,11 @@ | |||
| <div class="column"> | |||
|
|
|||
| <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. |
No description provided.