security: harden sanitize_path regex against #641 traversal class + regression tests#690
Open
wtfashwin wants to merge 1 commit into
Open
Conversation
…Neo#641) The path-sanitisation regex used the shorthand \.\.+ which expands to "a dot followed by one or more dots", i.e. two or more consecutive dots. That is semantically correct, but the shorthand is the same trap that produced the original \.\.\.+ bug reported in ParisNeo#641 (three-dot minimum allowing the canonical ../ traversal sequence through). Replace it with the explicit \.{2,} quantifier in both sanitize_path and sanitize_path_from_endpoint. Same matching behaviour, but the intent is unambiguous and trivially reviewable. Add tests/unitary_tests/test_security_sanitize_path.py — an 80-case regression suite that imports the real helpers and pins: - every traversal payload from the ParisNeo#641 PoC (../, ../../, ..\\, dot runs of length 2..16) is rejected with HTTP 400 - shell command substitution and collapsed-separator tricks are rejected - safe relative paths, opt-in absolute paths, and ./ when explicitly allowed continue to round-trip unchanged - every character in the unauthorised-punctuation set is rejected If anyone ever "simplifies" the quantifier back to a 3+-dot form, the parametrised dot-run test fails at length 2 and CI blocks the merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes / addresses #641.
\.\.+with the explicit\.{2,}quantifier in bothsanitize_pathandsanitize_path_from_endpoint. Same matching behaviour, but the intent is unambiguous and trivially reviewable — exactly the form the issue reporter suggested.tests/unitary_tests/test_security_sanitize_path.py, an 80-case regression suite that imports the real helpers and locks in the correct behaviour.Why this change matters
The originally-reported regex (
\.\.\.+, three-dot minimum) let canonical../through. The current pattern (\.\.+, two-dot minimum) is correct in behaviour, but uses the same compact shorthand whose readability ambiguity is what caused the original mistake in the first place. Switching to\.{2,}makes the lower bound visible at a glance and makes any future regression caught at code-review time, not after disclosure.The test suite then pins the behaviour so the same class of bug cannot silently come back: if anyone ever changes the quantifier to require 3+ dots, the parametrised
test_dot_run_of_any_length_ge_two_is_blocked[2]case fails and CI blocks the merge.What the test suite covers
../etc/passwd,../../etc/passwd,..../etc/passwd,...../etc/passwd,..\\etc\\passwd— all rejected with HTTP 400.foo/../bar,foo/./../../bar) and Windows-style separators.$(whoami),logs/$(id)/out.txt).foo//bar,foo///bar).allow_absolute_path=True,allow_current_folder=True) still work, and traversal is still blocked even when absolute paths are explicitly allowed.Test plan
python3 -m pytest tests/unitary_tests/test_security_sanitize_path.py -v— 80 passed, 0 failed, runs in <0.5s locally.../etc/passwd,../../etc/passwd,..\\etc\\passwd, bare..) all now raiseHTTPException(400)through the realsanitize_pathandsanitize_path_from_endpointentry points.Closes #641.