Skip to content
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

Path validation in upload #372

Merged
merged 8 commits into from
Feb 10, 2025
Merged

Path validation in upload #372

merged 8 commits into from
Feb 10, 2025

Conversation

harminius
Copy link
Contributor

@harminius harminius commented Feb 7, 2025

Resolves https://github.com/MerginMaps/server-private/issues/2724

Problem:
Filepath can be manipulated during the upload which breaks the project manipulation and is also a potential vulnerability 🪄

Solution:
Validate file path at the beginning of the upload - block absolute paths and those including ./, .\

🛡️

These paths are included in the test:

filepaths = [
    ("/home/user/mm/project/survey.gpkg", False),
    ("C:\Documents\Summer2018.pdf", False),
    ("\Desktop\Summer2019.pdf", False),
    ("../image.png", False),
    ("./image.png", False),
    ("assets/photos/im?age.png", False),
    ("assets/photos/CON.png", False),
    ("assets/photos/CONfile.png", True),
    ("image.png", True),
    ("images/image.png", True),
    ("media/photos/image.png", True),
    ("med..ia/pho.tos/ima...ge.png", True),
]

- filepath does not contain invalid characters
- filepath is not absolute (both Windows & Unix)
- filepath is not traversed
- filename does not contain invalid characters
- filename is not reserved name
@harminius harminius requested a review from MarcelGeo February 7, 2025 12:03
@coveralls
Copy link

coveralls commented Feb 7, 2025

Pull Request Test Coverage Report for Build 13240086351

Details

  • 23 of 23 (100.0%) changed or added relevant lines in 4 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 93.233%

Files with Coverage Reduction New Missed Lines %
server/mergin/sync/public_api_controller.py 2 91.71%
Totals Coverage Status
Change from base Build 13114400777: 0.02%
Covered Lines: 6710
Relevant Lines: 7197

💛 - Coveralls

@MarcelGeo MarcelGeo requested a review from varmar05 February 10, 2025 11:17
@MarcelGeo MarcelGeo merged commit fd70caa into develop Feb 10, 2025
4 checks passed
@MarcelGeo MarcelGeo deleted the path_validation_in_upload branch February 10, 2025 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants