Test Windows <reserved>.middle.extension filenames
#1844
Merged
Conversation
This file contains 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
In `gix-validate` tests that Windows-style paths are validated correctly, this changes byte strings that contain `\` to be raw byte string literals (`rb`) instead of non-raw byte string literals (`b`). This is to improve readability given that some literals contain consecutive backslashes: this makes it easier to tell if, and how many, consecutive backslashes there are.
When `<reserved>` is a reserved device name on Windows like `NUL`, in addition to filenames where it is followed by an extension being treated specially in the same way (`<reserved>.extension`), filenames where the operation of repeatedly removing extensions would produce `<reserved>` (`<reserved>.middle.extension`, `<reserved>.middle1.middle2.extension`, and so forth) are also treated by Windows like the reserved name. The existing code in `gix-validate`, and thus the validation of paths and refs that checks for reserved Windows device names, already does the right thing here. While the existing tests are presented as being for extensions, the implementation treats a name as reserved when it is parsed as a reserved name followed by a `.` (among other cases), regardless of whether what comes after the `.` is actually an extension in the strict sense. Therefore this is just adding regression tests to express the rule more clearly and, more importantly, to protect against accidental breakage in future changes. (Because of the way gitoxide uses this validation, suhc breakage would likely introduce a vulnerability similar to CVE-2024-35197, because this is part of how we safeguard against malicious names when cloning untrusted remote repos.) The idea to test this is inspired by a new check in `dunce`: https://gitlab.com/kornelski/dunce/-/commit/0ceb0ae141bf78c6d9d68488a55d22cd94658339
The test cases added in this commit are not very high value, since any `\` in what is supposed to be a component should be detected as invalid on Windows, which entails that these paths are detected as such. Adding these is mainly to clarify that the `\\?\` is not special here, comparedd to other kinds of `\` and `\\` paths.
|
That's wonderful, thank you! (And I am glad it already handled these cases) |
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.
I believe the implementation of
gix-validatecurrently covers all modified cases of Windows reserved device names likeNUL, for each name it recognizes as reserved when unmodified. But there is one common variation we didn't have test cases for: paths likeNUL.tar.xzwhere the reserved device name is followed by something that is not technically an extension. The Windows filename extension inNUL.tar.xzisxzrather thantar.xz:As detailed in f5ef10d, the implementation covers this already by looking to see if the reserved name has a
.(among other patterns) after it, regardless of what comes after the., but this seems valuable to test because it is a case that is sometimes inadvertently not covered in implementations of the Windows path rules, so a regression in this area is possible if the implementation ingix-validateis ever substantially altered.We did have tests already of related cases, such as paths where the reserved name is followed by multiple dots separated by spaces. But it seems to me that those preexisting cases--while important in their own right--neither express nor guard against the simpler case of a path like
NUL.tar.xz.Besides that, this PR includes two other changes: existing tests in the same file of paths containing
\characters are changed to use raw literals so it is clearer if and how many times the backslashes are repeated (6daaba3) and, least importantly, more UNC-style paths are tested to avoid conveying the impression that\\would only be rejected in\\?\(eaa59a3).