-
Couldn't load subscription status.
- Fork 274
Implemented possible solution for path traversal #28
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
Conversation
... prevent path traversal attacks.
...g path does not exists to prevent internal error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Chris, thanks for your PR. It looks good and you're solving a problem for the v0.3 release, thanks for that!
app/api/endpoints.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would choose a different status code. For example a 400 (Bad Request) or 403 (Forbidden).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Classic case of Java developer. Lower CamelCase is uncommon in Python. error_message would be nice! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is functional, but you could enhance it with a message from the semantic framework. Here is a link to the component: Error Messages
erikgeiser
commented
Jul 1, 2020
It seems like you addressed the path traversal issue only in a single endpoint. Looking through endpoints.py, it seems that most API endpoints work rather similar an possibly share the same issue.
I added a new function to generate valid paths or return None if the path is not valid, e.g. it is no subdirectory of the given folder.
Errors can now also easily be visualized by calling rendeError, which makes use Semantics error message.
I added a new function to generate valid paths or return
Noneif the path is not valid, e.g. it is no subdirectory of the given folder.
Errors can now also easily be visualized by callingrendeError, which makes use Semantics error message.
@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.