1b-agent/dockhand-upstream
1
0 Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: add branch selection to Git Stack modal with branches API endpoint (#1343)

Browse Source 
* feat: add branch selection to Git Stack modal with branches API endpoint

- Add branch dropdown in create mode (fetches branches from new API endpoint)
- Add branch field in edit mode for existing Git stacks
- Remove redundant branch badge next to repository URL in edit mode
- Add POST /api/git/branches endpoint to list available branches
- Export buildGitEnv, buildRepoUrl, execGit from git.ts for reuse

* cla: recheck with correct author email

* fix: harden git branches endpoint and fix credential resolution

* feat(git-stacks): per-stack branch override

Add a per-stack 'branch' column to git_stacks so each stack can deploy
from a branch different from its repository's default.

- Migration 0012 (drizzle + drizzle-pg): add nullable 'branch' column
- resolveStackBranch() in git.ts: effective branch = per-stack override,
  else repository default; wired into sync/deploy/test paths
- db.ts: persist/read branch; GitStackData.branch is string | null
- API routes (preview-env, stacks create/update): honor explicit branch
  override with partial-update semantics (null/empty clears the override)
- UI: GitStackModal shows a 'Repository default' sentinel + per-stack
  selector; GitSourceBadge displays effective branch with a '(per-stack)'
  marker; stacks page passes through the per-stack branch
- tests/git-stack-branch.test.ts: unit coverage for resolveStackBranch
  and effective-branch display semantics

* feat(git-branches): security hardening per PR #1343 maintainer review

Adopts the three security recommendations from the maintainer review on
the git-branch-stack feature:

1. SSRF guard (assertSafeBranchTarget)
   New src/lib/server/git-branch-lookup.ts rejects repository URLs that
   point at private/loopback/link-local addresses before any git
   subprocess is spawned. Covers IPv4 (127/8, 169.254/16, 10/8,
   172.16/12, 192.168/16, 100.64/10, 0/8), IPv6 (::1, fe80::/10,
   fc00::/7, ::), IPv4-mapped IPv6 (::ffff:a.b.c.d), and a 25-entry
   hostname denylist (localhost, metadata.google.internal,
   host.docker.internal, etc.). Raw-URL parse only — no DNS resolution,
   avoiding TOCTOU/rebinding. Runs on BOTH the url and repositoryId
   paths (a stored repo's URL could also point internal).

2. Credential-exfiltration guard (assertCredentialUrlPair)
   A raw url paired with a stored credentialId is only allowed when the
   credential is plausibly for that host. SSH and none credentials are
   always allowed (no secret to leak). Password credentials: allowed
   when the username equals the URL host, or when the credential name
   (or its hyphen/underscore-separated segments) equals the host or its
   first label (e.g. "github-pat" → github.com). Inference-based — no
   new DB column. The repositoryId path is not subject to this guard
   (the pairing is the user's own stored config).

3. Timeout on git subprocesses
   execGit now accepts a timeoutMs parameter (default GIT_TIMEOUT_MS =
   20s, env-overridable). On timeout the process is killed with SIGKILL
   and the caller receives { timedOut: true, code: 124 }. Applied to all
   execGit call sites (clone, pull, fetch, ls-remote, etc.) so a dead
   or black-holed remote cannot stall a request indefinitely.

OpenAPI: @openapi JSDoc block added to the branches endpoint; spec
regenerated via the upstream generator (359/359 coverage, all 6 gates
pass). /api/git/branches POST entry added to openapi.generated.json.

Svelte5: fix-svelte-optional-params.sh applied to 7 .svelte files
(x?: T → x: T = undefined) to work around esbuild's rejection of
TS-only optional-parameter syntax in lang="ts" script tags.

Tests: 59 tests in tests/git-branches.test.ts covering SSRF (blocked
IPs, boundary cases), credential affinity (allowed/rejected cases),
endpoint pipeline ordering, and parseRepoHost edge cases.

Rebased onto upstream/main (a485c85) — zero conflicts; GitStackModal.svelte
had not drifted since the branch was created.

* fix(git): close SSRF/RCE/exfil gaps across all git URL+credential endpoints (PR #1343 re-review)

1. SSRF: replace hand-rolled assertSafeBranchTarget with shared URL()-based
   isSafeNotificationUrl — blocks decimal/octal/hex/short-form IPv4 and
   v4-mapped IPv6 encodings (all six maintainer bypass payloads now blocked).
   ALL schemes (http/https/ssh/git/scp-like) now judge the CANONICALIZED
   host through that single check: non-http(s) schemes can't be passed to
   URL() as-is, so the parsed host (port-stripped, IPv6 bracketed only
   when it contains a colon) is run through isSafeNotificationUrl under
   an https:// scheme. Closes the ssh/git encoded-IP bypass (decimal
   2130706433, hex 0x7f000001, short-form 127.1 over ssh://, git:// and
   scp-like all resolve to 127.0.0.1 and are now blocked).
2. Credential affinity: drop the dead second rule — with rule (i) being
   username === host, the single-label variant was identical and unreachable.
3. listRemoteBranches + testRepositoryConfig call assertSafeRepoUrl explicitly
   so the ext::/file:: transport denylist holds on every path into a git
   subprocess, not only via buildRepoUrl.
4. preview-env: assertSafeRepoTarget (SSRF+RCE) + repoFilePath containment for
   composePath/envFilePath (path traversal) + assertCredentialHostMatch
   (exfil).
5. repositories/test: assertSafeRepoTarget + assertCredentialHostMatch.

Tests: 134 across 3 files (git-branches, preview-env-security,
repositories-test-security) — lock the six http(s) encoding bypasses, the
four ssh/git encoded-IP payloads at unit and endpoint level, the subdomain
attack, single-label username, path traversal, and the endpoint pipelines.

Verified safe (no change): backup/destinations/test, registries/test,
environments/test, git/repositories CRUD, git/stacks, git/repositories/[id]/test.

* fix(git): address branch stack review feedback

- scope remote branch lookup timeout to ls-remote only
- require git:edit permission for preview-env
- clarify SSRF policy while preserving private LAN Git hosts
- support free-text branch selection for new repositories
- share production branch resolution logic with tests and UI
- add testable preview-env request pipeline
This commit is contained in:
Bartosz S. authored and GitHub committed 2026-08-22 10:20:43 +02:00
1 parent 0885e8245f
commit 28bb291c32
29 files changed
+38984 -106
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

src/lib/server/git.ts
+200 -21
View File
@@ -19,6 +19,8 @@ import { deployStack, getStackDir } from './stacks';
import { sendEventNotification } from './notifications';
import { buildBasicAuthHeader } from './git-auth';
import { assertSafeRepoUrl, assertSafeGitRef, repoFilePath } from './git-url-safety';
import { assertSafeRepoTarget } from './git-branch-lookup';
import { resolveStackBranch } from '../git-stack-branch';
import {
parseManifest,
serializeManifest,
@@ -325,20 +327,89 @@ function buildRepoUrl(url: string, credential: GitCredential | null): string {
return url;
}
async function execGit(args: string[], cwd: string, env: GitEnv): Promise<{ stdout: string; stderr: string; code: number }> {
try {
/**
* Hard timeout (ms) for the remote-branch lookup (git ls-remote) ONLY.
* The lookup is the single git call whose target URL can be driven by an
* attacker-chosen URL (POST /api/git/branches, the new-repository branch
* picker), so an unbounded ls-remote is a resource-exhaustion vector.
* NOTE: this constant is deliberately NOT applied to ordinary deploy/sync
* operations (clone / pull / fetch / rev-parse) — those target the
* operator's own repository over a controlled URL, and a legitimate
* large-repo clone or slow WAN fetch can exceed this limit, so they stay
* UNBOUNDED (execGit without an explicit timeout). */
export const GIT_TIMEOUT_MS = Number(process.env.GIT_TIMEOUT_MS) || 20000;
/**
* Run a git command.
*
* Timeouts are OPT-IN: the default is UNBOUNDED. Ordinary deploy/sync
* operations (clone, pull, fetch, rev-parse) are unbounded because their
* target is the operator's own repository and a slow network or large repo
* is a legitimate outcome — they must not be killed mid-flight. Callers
* that need a bounded operation (the remote-branch lookup, whose target
* URL is attacker-influenced) pass `timeoutMs` explicitly (see
* listRemoteBranches).
*/
function execGit(
args: string[],
cwd: string,
env: GitEnv,
timeoutMs?: number
): Promise<{ stdout: string; stderr: string; code: number; timedOut?: boolean }> {
return new Promise((resolve) => {
let settled = false;
const stdoutChunks: Buffer[] = [];
const stderrChunks: Buffer[] = [];
const proc = nodeSpawn('git', args, {
cwd,
env,
stdio: ['pipe', 'pipe', 'pipe']
});
const result = await collectProcess(proc);
const finish = (
partial: { stdout: string; stderr: string; code: number; timedOut?: boolean }
) => {
if (settled) return;
settled = true;
// Only clear a timer if one was actually created — the default
// (no timeout) never creates one.
if (timer) clearTimeout(timer);
if (partial.timedOut) {
// The process is still running — kill it so the git subprocess
// (and any child ssh) cannot keep running after we give up.
proc.kill('SIGKILL');
}
resolve(partial);
};
return { stdout: result.stdout.trim(), stderr: result.stderr.trim(), code: result.exitCode };
} catch (err: any) {
return { stdout: '', stderr: err.message, code: 1 };
}
// Only arm a timer when a valid timeout was explicitly provided —
// undefined (or non-finite / non-positive) means "run unbounded".
const timer: ReturnType<typeof setTimeout> | undefined =
timeoutMs !== undefined && Number.isFinite(timeoutMs) && timeoutMs > 0
? setTimeout(() => {
finish({
stdout: Buffer.concat(stdoutChunks).toString(),
stderr: Buffer.concat(stderrChunks).toString(),
code: 124,
timedOut: true
});
}, timeoutMs)
: undefined;
proc.stdout?.on('data', (chunk: Buffer) => stdoutChunks.push(chunk));
proc.stderr?.on('data', (chunk: Buffer) => stderrChunks.push(chunk));
proc.on('error', (err: any) => {
finish({ stdout: '', stderr: err.message, code: 1 });
});
proc.on('close', (code) => {
finish({
stdout: Buffer.concat(stdoutChunks).toString(),
stderr: Buffer.concat(stderrChunks).toString(),
code: code ?? 1
});
});
});
}
/**
@@ -641,6 +712,11 @@ export async function testRepositoryConfig(options: {
return { success: false, error: 'Repository URL is required' };
}
// Transport denylist (ext::/fd::/file::/local paths) — applied here so the
// denylist holds on every path into a git subprocess, not only via
// buildRepoUrl (same pattern as listRemoteBranches).
assertSafeRepoUrl(url);
// Fetch credential from database if credentialId is provided
const credential = credentialId ? await getGitCredential(credentialId) : null;
if (credentialId && !credential) {
@@ -654,6 +730,74 @@ export async function testRepositoryConfig(options: {
});
}
/**
* List remote branches for a repository URL using git ls-remote.
* Resolves credentials from the database if a credentialId is provided.
* Bounded by a hard timeout (default GIT_TIMEOUT_MS, 20s) so a dead or
* black-holed host cannot stall the request — see execGit. This is the
* only git operation with a timeout: its target URL is the one that can be
* attacker-influenced, so an unbounded ls-remote would be a resource-
* exhaustion vector.
*
* The transport denylist (ext::/fd::/file::/local paths) is applied HERE,
* explicitly, rather than only via buildRepoUrl — a future parser change in
* buildRepoUrl must not be able to reopen command execution on this path.
*/
export async function listRemoteBranches(options: {
url: string;
credentialId?: number | null;
timeoutMs?: number;
}): Promise<{ branches: string[]; error?: string }> {
const { url, credentialId, timeoutMs } = options;
// The effective branch-lookup timeout: an explicitly provided (valid)
// override wins; otherwise the default GIT_TIMEOUT_MS. This is the ONLY
// bounded git operation — see execGit for why deploy/sync operations are
// left unbounded.
const effectiveTimeoutMs = timeoutMs ?? GIT_TIMEOUT_MS;
// Transport denylist — the same check buildRepoUrl applies, applied
// explicitly here so the denylist holds even if buildRepoUrl changes.
assertSafeRepoUrl(url);
// Fetch credential from database if credentialId is provided
const credential = credentialId ? await getGitCredential(credentialId) : null;
const env = await buildGitEnv(credential);
const repoUrl = buildRepoUrl(url, credential);
try {
const result = await execGit(
['ls-remote', '--heads', '--refs', repoUrl],
process.cwd(),
env,
effectiveTimeoutMs
);
if (result.timedOut) {
return { branches: [], error: `git ls-remote timed out after ${Math.round(effectiveTimeoutMs / 1000)}s` };
}
if (result.code !== 0) {
return { branches: [], error: cleanGitError(result.stderr) };
}
const branches = result.stdout.split('\n')
.filter(l => l.trim())
.map(l => {
const m = l.match(/refs\/heads\/(.+)$/);
return m ? m[1] : null;
})
.filter(Boolean) as string[];
return { branches };
} catch (error: any) {
return { branches: [], error: error.message };
} finally {
cleanupSshKey(credential);
}
}
export async function syncRepository(repoId: number): Promise<SyncResult> {
const repo = await getGitRepository(repoId);
if (!repo) {
@@ -872,6 +1016,13 @@ async function getPreviousCommit(repoPath: string, env: GitEnv): Promise<string
}
}
/**
* The effective branch a git stack deploys from is resolved by
* resolveStackBranch() (src/lib/git-stack-branch.ts) — a per-stack branch
* override (git_stacks.branch) wins; when it is unset, empty, or
* whitespace-only, the repository's default branch is used.
*/
export async function syncGitStack(stackId: number): Promise<SyncResult> {
const gitStack = await getGitStack(stackId);
if (!gitStack) {
@@ -901,8 +1052,14 @@ export async function syncGitStack(stackId: number): Promise<SyncResult> {
return { success: false, error: 'Repository not found' };
}
// Per-stack branch override wins; unset means the repository default
const effectiveBranch = resolveStackBranch(gitStack, repo);
console.log(`${logPrefix} Repository URL:`, repo.url);
console.log(`${logPrefix} Repository branch:`, repo.branch);
if (effectiveBranch !== repo.branch) {
console.log(`${logPrefix} Stack branch override:`, effectiveBranch);
}
const credential = repo.credentialId ? await getGitCredential(repo.credentialId) : null;
const repoPath = await getStackRepoPath(stackId, gitStack.stackName, gitStack.environmentId);
@@ -928,11 +1085,11 @@ export async function syncGitStack(stackId: number): Promise<SyncResult> {
}
console.log(`${logPrefix} Cloning repository...`);
assertSafeGitRef(repo.branch);
assertSafeGitRef(effectiveBranch);
const repoUrl = buildRepoUrl(repo.url, credential);
const result = await execGit(
['clone', '--filter=blob:none', '--branch', repo.branch, repoUrl, repoPath],
['clone', '--filter=blob:none', '--branch', effectiveBranch, repoUrl, repoPath],
process.cwd(),
env
);
@@ -1283,15 +1440,18 @@ export async function testGitStack(stackId: number): Promise<TestResult> {
return { success: false, error: 'Repository not found' };
}
// Per-stack branch override wins; unset means the repository default
const effectiveBranch = resolveStackBranch(gitStack, repo);
const credential = repo.credentialId ? await getGitCredential(repo.credentialId) : null;
const env = await buildGitEnv(credential);
assertSafeGitRef(repo.branch);
assertSafeGitRef(effectiveBranch);
const repoUrl = buildRepoUrl(repo.url, credential);
try {
// Use git ls-remote to test connection and get branch info
const result = await execGit(
['ls-remote', '--heads', '--refs', repoUrl, repo.branch],
['ls-remote', '--heads', '--refs', repoUrl, effectiveBranch],
process.cwd(),
env
);
@@ -1305,12 +1465,12 @@ export async function testGitStack(stackId: number): Promise<TestResult> {
// Parse the output to get commit hash
const lines = result.stdout.split('\n').filter(l => l.trim());
if (lines.length === 0) {
return { success: false, error: `Branch '${repo.branch}' not found in repository` };
return { success: false, error: `Branch '${effectiveBranch}' not found in repository` };
}
const match = lines[0].match(/^([a-f0-9]+)\s+refs\/heads\/(.+)$/);
const lastCommit = match ? match[1].substring(0, 7) : undefined;
const branch = match ? match[2] : repo.branch;
const branch = match ? match[2] : effectiveBranch;
cleanupSshKey(credential, env);
@@ -1368,6 +1528,9 @@ export async function deployGitStackWithProgress(
return { success: false, error: 'Repository not found' };
}
// Per-stack branch override wins; unset means the repository default
const effectiveBranch = resolveStackBranch(gitStack, repo);
const credential = repo.credentialId ? await getGitCredential(repo.credentialId) : null;
const repoPath = await getStackRepoPath(stackId, gitStack.stackName, gitStack.environmentId);
const env = await buildGitEnv(credential);
@@ -1394,13 +1557,13 @@ export async function deployGitStackWithProgress(
rmSync(repoPath, { recursive: true, force: true });
}
assertSafeGitRef(repo.branch);
assertSafeGitRef(effectiveBranch);
const repoUrl = buildRepoUrl(repo.url, credential);
// Step 3: Fetching (blobless clone - fetches all commits but blobs on-demand)
onProgress({ status: 'fetching', message: `Fetching branch ${repo.branch}...`, step: 3, totalSteps });
onProgress({ status: 'fetching', message: `Fetching branch ${effectiveBranch}...`, step: 3, totalSteps });
const cloneResult = await execGit(
['clone', '--filter=blob:none', '--branch', repo.branch, repoUrl, repoPath],
['clone', '--filter=blob:none', '--branch', effectiveBranch, repoUrl, repoPath],
process.cwd(),
env
);
@@ -1801,6 +1964,20 @@ export async function previewRepoEnvFiles(options: PreviewEnvOptions): Promise<P
// Build git environment with credentials
// Cast credential to GitCredential type (only uses id, authType, sshPrivateKey)
env = await buildGitEnv(credential as GitCredential | null);
// Security (PR #1343 maintainer review): the preview endpoint clones a
// USER-SUPPLIED URL and reads env files from it — the same SSRF / RCE /
// path-traversal surface as the branches endpoint. Apply the shared guards
// BEFORE any git subprocess or file read:
// 1. assertSafeRepoTarget — blocks loopback/link-local/metadata/reserved
// hosts (SSRF) and the ext::/file:: transport denylist (RCE). Ordinary
// private-LAN addresses are intentionally allowed (self-hosted Git).
// 2. repoFilePath — keeps composePath/envFilePath INSIDE the temp dir
// (path traversal). Without this, `envFilePath: '../../secrets/x'`
// reads a file outside the clone.
assertSafeRepoTarget(repoUrl);
const safeComposePath = repoFilePath(tempDir, composePath, 'Compose path');
const safeEnvFilePath = envFilePath ? repoFilePath(tempDir, envFilePath, 'Env file path') : null;
assertSafeGitRef(branch);
const authenticatedUrl = buildRepoUrl(repoUrl, credential as GitCredential | null);
@@ -1825,8 +2002,9 @@ export async function previewRepoEnvFiles(options: PreviewEnvOptions): Promise<P
console.log(`${logPrefix} Clone successful`);
// Determine the compose directory (where .env file should be)
const composeDir = dirname(composePath);
// Determine the compose directory (where .env file should be) — uses the
// VALIDATED path (already checked to be inside the temp dir).
const composeDir = dirname(safeComposePath);
const baseEnvPath = join(tempDir, composeDir, '.env');
const vars: Record<string, string> = {};
@@ -1846,9 +2024,10 @@ export async function previewRepoEnvFiles(options: PreviewEnvOptions): Promise<P
console.log(`${logPrefix} No .env file at ${baseEnvPath}`);
}
// Read additional env file if specified
if (envFilePath) {
const additionalEnvPath = join(tempDir, envFilePath);
// Read additional env file if specified — uses the VALIDATED path
// (already checked to be inside the temp dir).
if (safeEnvFilePath) {
const additionalEnvPath = safeEnvFilePath;
if (existsSync(additionalEnvPath)) {
console.log(`${logPrefix} Reading additional env file: ${additionalEnvPath}`);
const content = readFileSync(additionalEnvPath, 'utf-8');