Add additional path validations to UploadsPipeline
What does this MR do and why?
This refactor adds some defense-in-depth. There should be no difference to end users.
I was looking at this code earlier in the week, and noticed that it called save_avatar
before performing a check to validate the file wasn't a symlink. I couldn't exploit it though - BulkImports::FileDownloadService
makes sure whatever we download is safe before UploadsPipeline
gets around to using it. The AVATAR_PATTERN
regex was pretty loose, and the checks for not-avatars also didn't seem to validate that file_path
was in a tmpdir (just that the path matches DYNAMIC_PATH_PATTERN
regex).
It seemed like an area where things might potentially go wrong in the future, so I reshuffled the existing checks and added one more (ensure the path is in a tmpdir and not attempting to traverse out). Now even a sneaky avatar or other file shouldn't be able to get past the checks.
FWIW we addressed a severity1 in this area of code at the start of 2022: Arbitrary file read via the bulk imports Uploa... (#349524 - closed)
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.