Conversation
nucleogenic
left a comment
There was a problem hiding this comment.
Lots of good stuff in here! Added a few thoughts.
python/web/src/web.py
Outdated
| file_name = request.form.get("file_name") | ||
| file_name = Path(request.form.get("file_name")) | ||
| if file_name.is_absolute() or ".." in str(file_name): | ||
| file_name = file_name.name |
There was a problem hiding this comment.
I think there are two considerations here:
-
DRY principle, should this be extracted to a function? (6 uses in web.py)
-
What should that function actually do? Upon reflection, it seems a bit unpredictable to have the side effect of changing an invalid path (e.g.
/mac/foo.hdsbecomesfoo.hds, losing the dir) to a valid, but different path. Perhaps, if validation fails, we should return an error response instead, so the client can correct the error?
There was a problem hiding this comment.
+1 on #1. I felt dirty copy pasting the code snippet. I added a utility method is_safe_path() to web_utils.py
As for point #2, the above utility method now returns a message an error that will display the path and a notice that it is invalid. Beyond that, do you have ideas on a more sophisticated way to detect unsafe paths, and what to do with them?
| os.mkdir(tmp_dir) | ||
| tmp_full_path = tmp_dir + file_name | ||
| iso_filename = f"{server_info['image_dir']}/{file_name}.iso" | ||
| tmp_dir = Path("/tmp") / str(tmp_ts) |
There was a problem hiding this comment.
BTW, I just realised, you could use TemporaryDirectory context manager here to handle the temporary dir generation and cleanup.
from tempfile import TemporaryDirectory
with TemporaryDirectory() as tmp_dir:
do stuff
There's an example of this in unarchiver.py.
There was a problem hiding this comment.
Wow, that was so easy get up and running! I've had this on my todo list for a year. See #543
|
SonarCloud Quality Gate failed. |
Uh oh!
There was an error while loading. Please reload this page.