Skip to content

fix(clone): close four security findings on the #94 clone endpoint - #99

Merged
Lexus2016 merged 1 commit into
mainfrom
fix/94-clone-review
Sep 11, 2026
Merged

Lexus2016 merged 1 commit into
mainfrom
fix/94-clone-review

Conversation

@Lexus2016

Copy link
Copy Markdown
Owner

Follow-up to #98 (issue #94). An independent review of the merged endpoint turned up four holes; each is closed here with a test.

  • An option-shaped AUTHORITY reached git. ssh://-oProxyCommand=id/x and git@-oX:a/b are 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.
  • The parent was allowlisted as a STRING. isPathAllowed() never touches the filesystem, so a symlink inside an allowed root is a legal path pointing anywhere on disk. The parent is now realpathed and re-checked — the same layer-2 reasoning as pwd -P in the remote file browser.
  • A dangling symlink target was cloned into. existsSync() is false for a broken link, so the 409 guard missed it and git would have followed it. lstat sees the link itself.
  • The timeout killed one pid out of four. git clone parents git-remote-https / ssh / index-pack; killing only git left those writing into the tree the handler then rmSynces. The child is detached on POSIX (its own process group) and the timeout kills the group. Windows keeps the plain kill — detached there opens a console window.
  • Plus a bound: 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.js grows to 60 assertions: the three authority shapes, a symlinked parent (403), a dangling-symlink target (409), and the concurrency cap driven through a fake git that 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 against realpathSync for that reason.

- 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
@Lexus2016
Lexus2016 merged commit 29c7c02 into main Sep 11, 2026
2 checks passed
@Lexus2016
Lexus2016 deleted the fix/94-clone-review branch September 11, 2026 16:31
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.

1 participant