Reject a whitespace-only repository name from a host - #81
Merged
Merged
Conversation
A host returning a repository name of only whitespace produced an ArgumentException from GitRepositoryName's own validation rather than a GitHostingException, because LocalPath is assigned before Name in the object initializer and Path.Combine accepts the whitespace segment. That exception escapes the failure surface these methods document, so a caller catching GitHostingException misses it entirely. ToLocalRepositoryPath now rejects a whitespace-only leaf alongside an empty, "." or ".." one, throwing GitHostingRequestException before Name is assigned. Both providers fail the same way on the same input.
|
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
1 participant
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.
A host returning a repository name of only whitespace currently throws an
ArgumentExceptionout ofGetRepositoriesAsync, which is outside the failure surface that method documents. A caller writingcatch (GitHostingException)misses it.Why it happens
GitProvider.ToLocalRepositoryPathrejects a leaf that is empty,., or.., but testsstring.IsNullOrEmpty, so whitespace survives andPath.Combineaccepts it. The exception then comes from somewhere else entirely:LocalPathis assigned beforeNamein the object initializer, so the path is built successfully andGitRepositoryName's own[HasNonWhitespaceContent]validation throws a moment later.The result is two different failure shapes for one input, decided by field initialization order rather than by design.
The change
ToLocalRepositoryPathrejects a whitespace-only leaf alongside the cases it already rejected, throwingGitHostingRequestExceptionnaming the provider and the offending value, beforeNameis ever assigned. Both providers now fail the same way on the same input.One word of logic, one
<exception>doc correction, and a test in each provider's test file beside the existing rooted and traversal cases.Verification
Build 0 warnings and 0 errors. Suite 481/481 passing, 0 skipped, run under
KTSU_GIT_INTEGRATION_TESTS_REQUIRED=1 GIT_CONFIG_NOSYSTEM=1so the integration tier is exercised the way the POSIX runners see it.Mutation-checked: reverting the guard makes both new tests fail, and the failure they produce is the
ArgumentExceptiondescribed above rather than a silent pass, which is the behaviour this fixes.Context
Follow-up to #80. This was found while probing the path-containment invariant added there, not by the automated reviewer. Containment itself was never at risk, since a whitespace path stays under the current directory. This is about the error contract.