From 6a8947217c9e6349f715450f205a8cbbd7bcfe66 Mon Sep 17 00:00:00 2001 From: Rin Date: Mon, 11 May 2026 15:38:07 +0700 Subject: [PATCH] fix(server): sanitize repo name to prevent argument injection (#1305) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(server): sanitize repo name to prevent argument injection Sanitizes the extracted repository name to prevent argument injection during git clone operations and ensures compatibility with various file systems. 1. Strips leading dashes to prevent git command-line argument injection. 2. Replaces unsafe directory characters with underscores. 3. Blocks path traversal segments ('.' and '..') and Windows reserved names. 4. Fixes ReDoS vulnerability in parseRepoNameFromUrl regex. 5. Added unit tests for sanitization and path traversal edge cases. * fix(server): expand Windows reserved name check to include extensions - Updated sanitizeRepoName to block Windows reserved names (CON, NUL, etc.) even when they have extensions (e.g., CON.txt). - Corrected regex and added unit tests for these edge cases to resolve CI failures on Windows. - Ref: https://github.com/abhigyanpatwari/GitNexus/pull/1305#issuecomment-4407200914 --------- Co-authored-by: Gergő Magyar --- gitnexus/src/server/git-clone.ts | 26 ++++++------ gitnexus/src/storage/git.ts | 63 ++++++++++++++++++++++------ gitnexus/test/unit/git-clone.test.ts | 40 ++++++++++++------ gitnexus/test/unit/git.test.ts | 59 ++++++++++++++++++++++++++ 4 files changed, 149 insertions(+), 39 deletions(-) diff --git a/gitnexus/src/server/git-clone.ts b/gitnexus/src/server/git-clone.ts index 861a16648..0ced1213a 100644 --- a/gitnexus/src/server/git-clone.ts +++ b/gitnexus/src/server/git-clone.ts @@ -11,6 +11,7 @@ import os from 'os'; import fs from 'fs/promises'; import { isIP } from 'net'; import { logger } from '../core/logger.js'; +import { parseRepoNameFromUrl } from '../storage/git.js'; /** Root directory for all cloned repositories. Targets must resolve inside this. */ const CLONE_ROOT = path.resolve(path.join(os.homedir(), '.gitnexus', 'repos')); @@ -29,20 +30,17 @@ const REPO_NAME_PATTERN = /^[a-zA-Z0-9._-]+$/; * clone root via path traversal. */ export function extractRepoName(url: string): string { - // Strip trailing slashes without a regex to avoid polynomial-ReDoS on - // pathological inputs like `https://x.com/y` + '/'.repeat(1e6). CodeQL's - // js/polynomial-redos flagged `/\/+$/` here. - let end = url.length; - while (end > 0 && url.charCodeAt(end - 1) === 47 /* '/' */) end--; - const cleaned = url.slice(0, end); - - const lastSegment = cleaned.split(/[/:]/).pop() || ''; - const stripped = lastSegment.endsWith('.git') ? lastSegment.slice(0, -4) : lastSegment; - - if (!stripped || stripped === '.' || stripped === '..' || !REPO_NAME_PATTERN.test(stripped)) { + const name = parseRepoNameFromUrl(url); + if ( + !name || + name === '.' || + name === '..' || + name === 'unknown' || + !REPO_NAME_PATTERN.test(name) + ) { throw new Error('Could not extract a valid repository name from URL'); } - return stripped; + return name; } /** Get the clone target directory for a repo name. */ @@ -399,8 +397,8 @@ export async function cloneOrPull( } // Always validate the requested URL — the prior shape only ran this in - // the clone branch, leaving the pull branch as an SSRF / blocked-host - // bypass when an existing clone shared the basename of an attacker URL. + // the code path where the repo was cloned. Now it runs unconditionally, + // preventing SSRF / blocked-host bypasses even when targetDir already exists. validateGitUrl(url); const exists = await fs.access(path.join(safeTarget, '.git')).then( diff --git a/gitnexus/src/storage/git.ts b/gitnexus/src/storage/git.ts index 4cb0154fe..75e6e91d3 100644 --- a/gitnexus/src/storage/git.ts +++ b/gitnexus/src/storage/git.ts @@ -255,24 +255,63 @@ export const getRemoteOriginUrl = (repoPath: string): string | null => { }; /** - * Parse a repository name out of a git remote URL. Handles the common - * SSH (`git@host:owner/repo.git`), HTTPS (`https://host/owner/repo.git`), - * `git://`, `ssh://`, and `file://` shapes. Returns `null` for empty / - * unparseable input. + * Sanitize a repository name to prevent argument injection and ensure + * cross-platform filesystem compatibility. * - * The heuristic: strip a trailing `.git` and trailing slashes, then - * take the segment after the last `/` or `:`. + * 1. Strips leading dashes to prevent git command-line argument injection + * (e.g., --upload-pack=evil). + * 2. Replaces characters that are unsafe for directory names across + * platforms (Windows/macOS/Linux) with underscores. + * 3. Blocks path traversal segments ("." and "..") and Windows reserved + * names (e.g., CON, NUL) to prevent directory escape. + */ +export const sanitizeRepoName = (name: string): string => { + // 1. Prevent argument injection by stripping leading dashes. + // 2. Remove characters that are not alphanumerics, dots, underscores, or dashes. + const sanitized = name.replace(/^-+/, '').replace(/[^a-zA-Z0-9._-]/g, '_'); + + // 3. Block path traversal segments and Windows reserved names. + // Windows reserved names like CON, PRN, AUX, NUL, COM1-9, LPT1-9 cannot + // be used as directory names on Windows even if they have an extension. + const reserved = /^(CON|PRN|AUX|NUL|COM[1-9]|LPT[1-9])(\..*)?$/i; + if (!sanitized || sanitized === '.' || sanitized === '..' || reserved.test(sanitized)) { + return 'unknown'; + } + + return sanitized; +}; + +/** + * Parse a repository name out of a git remote URL. Handles common shapes + * including SSH (git@host:owner/repo.git) and HTTPS (https://host/owner/repo.git). + * + * Returns a sanitized, filesystem-safe name or null if no name could be inferred. + * Returning null (rather than 'unknown') allows callers to use ?? null-coalescing + * for fallbacks without risk of registry collisions on 'unknown'. */ export const parseRepoNameFromUrl = (url: string | null | undefined): string | null => { if (!url) return null; const trimmed = url.trim(); if (!trimmed) return null; - // Strip `.git` suffix (case-insensitive) and any trailing slashes. - const withoutSuffix = trimmed.replace(/\.git\/*$/i, '').replace(/\/+$/, ''); - // Last path segment, splitting on either `/` or `:` (covers SSH form). - const m = withoutSuffix.match(/[/:]([^/:]+)$/); - const candidate = m ? m[1] : withoutSuffix; - return candidate || null; + + // Strip trailing slashes without a regex to avoid polynomial-ReDoS on + // pathological inputs like `https://x.com/y` + '/'.repeat(1e6). + let end = trimmed.length; + while (end > 0 && trimmed.charCodeAt(end - 1) === 47 /* '/' */) end--; + let cleaned = trimmed.slice(0, end); + + // Strip trailing .git (case-insensitive) + if (cleaned.toLowerCase().endsWith('.git')) { + cleaned = cleaned.slice(0, -4); + } + + // Last path segment, handling colons for SSH URLs and path traversal. + // Split on both / and : to consistently extract the last part. + const candidate = cleaned.split(/[/:]/).pop() || ''; + if (!candidate) return null; + + const safe = sanitizeRepoName(candidate); + return safe === 'unknown' ? null : safe; }; /** diff --git a/gitnexus/test/unit/git-clone.test.ts b/gitnexus/test/unit/git-clone.test.ts index 18ac8e9d6..88832aa83 100644 --- a/gitnexus/test/unit/git-clone.test.ts +++ b/gitnexus/test/unit/git-clone.test.ts @@ -7,12 +7,12 @@ import { buildCloneArgs, normalizeGitUrlForCompare, assertRemoteMatchesRequestedUrl, - getRemoteOriginUrl, } from '../../src/server/git-clone.js'; import path from 'node:path'; import os from 'node:os'; import fs from 'node:fs/promises'; import { spawn } from 'node:child_process'; +import { getRemoteOriginUrl } from '../../src/storage/git.js'; describe('git-clone', () => { describe('extractRepoName', () => { @@ -50,17 +50,6 @@ describe('git-clone', () => { expect(() => extractRepoName('https://example.com/foo:.')).toThrow('valid repository name'); }); - it('rejects URLs with shell metacharacters in the last segment', () => { - // The split on /[/:]/ does not split on backslashes or other shell chars, - // so a name like `repo;rm -rf /` would slip through without the pattern. - expect(() => extractRepoName('https://example.com/foo:repo;rm')).toThrow( - 'valid repository name', - ); - expect(() => extractRepoName('https://example.com/foo:repo$x')).toThrow( - 'valid repository name', - ); - }); - it('rejects empty input', () => { expect(() => extractRepoName('')).toThrow('valid repository name'); }); @@ -77,6 +66,31 @@ describe('git-clone', () => { // multiple seconds on 10k slashes). expect(elapsedMs).toBeLessThan(500); }); + + it('strips leading dashes to prevent argument injection', () => { + expect(extractRepoName('https://github.com/user/--upload-pack=payload.git')).toBe( + 'upload-pack_payload', + ); + expect(extractRepoName('https://github.com/user/-repo')).toBe('repo'); + }); + + it('sanitizes unsafe directory characters', () => { + // sanitizeRepoName turns into _tag_ + expect(extractRepoName('https://github.com/user/repo.git')).toBe('repo_tag_'); + }); + + it('sanitizes shell metacharacters in URL segments', () => { + // The split on /[/:]/ does not split on backslashes or other shell chars, + // so a name like `repo;rm -rf /` would slip through without the pattern. + // After fix/sanitize-repo-name, these are sanitized to underscores. + expect(extractRepoName('https://example.com/foo:repo;rm')).toBe('repo_rm'); + expect(extractRepoName('https://example.com/foo:repo$x')).toBe('repo_x'); + }); + + it('sanitizes whitespace and backslashes', () => { + expect(extractRepoName('https://example.com/foo:repo name')).toBe('repo_name'); + expect(extractRepoName('https://example.com/foo:repo\\name')).toBe('repo_name'); + }); }); describe('getCloneDir', () => { @@ -209,7 +223,7 @@ describe('git-clone', () => { it('blocks NAT64 with embedded RFC1918 addresses', () => { // The startsWith('64:ff9b:') check covers any embedded IPv4. These - // explicit RFC1918 cases document SSRF coverage for the full private + // explicit RFC1918 architectures document SSRF coverage for the full private // IPv4 surface — not just loopback and cloud metadata. expect(() => validateGitUrl('http://[64:ff9b::a00:1]/repo.git')).toThrow('private/internal'); // 10.0.0.1 expect(() => validateGitUrl('http://[64:ff9b::ac10:1]/repo.git')).toThrow('private/internal'); // 172.16.0.1 diff --git a/gitnexus/test/unit/git.test.ts b/gitnexus/test/unit/git.test.ts index af339ce69..1bebf4143 100644 --- a/gitnexus/test/unit/git.test.ts +++ b/gitnexus/test/unit/git.test.ts @@ -8,6 +8,8 @@ import { getCurrentCommit, getGitRoot, findGitRootByDotGit, + parseRepoNameFromUrl, + sanitizeRepoName, } from '../../src/storage/git.js'; // Mock child_process.execSync @@ -164,4 +166,61 @@ describe('git utilities', () => { } }); }); + + describe('sanitizeRepoName', () => { + it('strips leading dashes', () => { + expect(sanitizeRepoName('--repo')).toBe('repo'); + }); + + it('replaces unsafe characters with underscores', () => { + expect(sanitizeRepoName('repo')).toBe('repo_tag_'); + expect(sanitizeRepoName('repo:name')).toBe('repo_name'); + expect(sanitizeRepoName('repo"quoted"')).toBe('repo_quoted_'); + }); + + it('blocks path traversal segments', () => { + expect(sanitizeRepoName('.')).toBe('unknown'); + expect(sanitizeRepoName('..')).toBe('unknown'); + }); + + it('blocks Windows reserved names', () => { + expect(sanitizeRepoName('CON')).toBe('unknown'); + expect(sanitizeRepoName('prn')).toBe('unknown'); + expect(sanitizeRepoName('AUX')).toBe('unknown'); + expect(sanitizeRepoName('NUL')).toBe('unknown'); + expect(sanitizeRepoName('COM1')).toBe('unknown'); + expect(sanitizeRepoName('LPT9')).toBe('unknown'); + + // Reserved names with extensions + expect(sanitizeRepoName('CON.txt')).toBe('unknown'); + expect(sanitizeRepoName('NUL.tar.gz')).toBe('unknown'); + expect(sanitizeRepoName('AUX.local')).toBe('unknown'); + }); + + it('returns unknown for empty or invalid input', () => { + expect(sanitizeRepoName('')).toBe('unknown'); + expect(sanitizeRepoName('---')).toBe('unknown'); + }); + }); + + describe('parseRepoNameFromUrl', () => { + it('extracts and sanitizes name from HTTPS URL', () => { + expect(parseRepoNameFromUrl('https://github.com/user/my-repo.git')).toBe('my-repo'); + expect(parseRepoNameFromUrl('https://github.com/user/--payload.git')).toBe('payload'); + }); + + it('extracts and sanitizes name from SSH URL', () => { + expect(parseRepoNameFromUrl('git@github.com:user/my-repo.git')).toBe('my-repo'); + expect(parseRepoNameFromUrl('git@github.com:--payload.git')).toBe('payload'); + }); + + it('returns null for all-dash inputs (prevents registry collision)', () => { + expect(parseRepoNameFromUrl('https://github.com/user/---.git')).toBeNull(); + }); + + it('returns null for empty URL', () => { + expect(parseRepoNameFromUrl('')).toBeNull(); + expect(parseRepoNameFromUrl(null)).toBeNull(); + }); + }); });