fix(clone): close four security findings on the #94 clone endpoint - #99
Merged
Merged
Conversation
- refuse an option-shaped authority (ssh://-oProxyCommand=..., git@-oX:a/b), CVE-2017-1000117's shape, which the leading-- check on the whole URL misses - realpath the parent and re-check isPathAllowed: a symlink inside an allowed root is a legal string pointing anywhere on disk - lstat the target instead of existsSync: a dangling symlink does not exist, and git would follow it - kill the process GROUP on timeout and cap concurrent clones at 2
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.
Follow-up to #98 (issue #94). An independent review of the merged endpoint turned up four holes; each is closed here with a test.
ssh://-oProxyCommand=id/xandgit@-oX:a/bare URLs whose HOST is an ssh option — CVE-2017-1000117's shape. The leading--check on the whole string does not see them, because the option sits behind the scheme or the@.authorityIsSafe()drops a trailing:<port>and refuses any@-part opening with-. Modern git passes--to ssh itself, so this is a belt on a brace — but the module's own rule is that nothing handed to git may look like an option, and a rule that holds only on a patched binary is not that rule.isPathAllowed()never touches the filesystem, so a symlink inside an allowed root is a legal path pointing anywhere on disk. The parent is nowrealpathed and re-checked — the same layer-2 reasoning aspwd -Pin the remote file browser.existsSync()is false for a broken link, so the 409 guard missed it and git would have followed it.lstatsees the link itself.git cloneparentsgit-remote-https/ssh/index-pack; killing onlygitleft those writing into the tree the handler thenrmSynces. The child isdetachedon POSIX (its own process group) and the timeout kills the group. Windows keeps the plain kill —detachedthere opens a console window.MAX_CONCURRENT_CLONES = 2. Each clone holds its request open for as long as it runs, so an unbounded one is a disk-filling primitive behind one button.Not changed: the HOST is still unfiltered, deliberately — cloning from an internal GitLab is the feature and the endpoint sits below
auth.authMiddleware, so it is a logged-in user's own fetch rather than an open proxy. Stated in CLAUDE.md instead of half-closed.Test
test/git-clone.test.jsgrows to 60 assertions: the three authority shapes, a symlinked parent (403), a dangling-symlink target (409), and the concurrency cap driven through a fakegitthat sleeps.npm test— 78 files, all green locally.Note: the endpoint now answers with the PHYSICAL parent path, so on macOS a workdir under
/var/...is registered as/private/var/.... The suite asserts againstrealpathSyncfor that reason.