fix(cli): validate GitHub remote hosts - #5327

Independently verified — the bypass is real and the fix closes it. I reproduced main's /github\.com/ substring test against the new isGitHubRemoteUrl, and all of these flip from accepted (bug) → rejected:

Legit forms (https / [email protected]: SCP / ssh:// / :443 / GitHub.com casing) all still pass — nice.

One suggestion (non-blocking): consider new URL(remoteUrl).hostname instead of .host. .host keeps non-default ports, so it wrongly rejects explicit-port GitHub remotes — and .hostname strips the port while still rejecting every lookalike:

So just:

return new URL(remoteUrl).hostname === 'github.com';

The current .host behavior is fail-closed (a benign UX miss on an uncommon form), so this is safe to merge as-is — but .hostname is strictly more correct. LGTM either way; flip it out of draft when you're ready. Thanks!

return new URL(remoteUrl).hostname === 'github.com';

@qwen-code /triage

✅ Local verification — PR #5327 (fix(cli): validate GitHub remote hosts)

As a maintainer, I built real tests in an isolated tmux session and verified this PR end-to-end on Linux, including a real-git integration harness (the existing unit tests mock execSync, so they never exercise actual git remote -v parsing). Evidence below for the merge decision.

Environment: Linux x86-64 · Node v22.22.2 · Vitest 3.2.4 · isolated tmux -L pr5327 · branch pull/5327/head @ cf2d7af31

TL;DR

The fix is correct and well-tested. It closes the #5326 lookalike-host bypass and an SSH-explicit-port correctness bug, with regression tests that genuinely catch the old behavior (proven by running them against main). A real-git E2E confirms it works against actual git remote -v output, including an extra phishing vector the unit tests don't cover. No regressions for legitimate remotes. ESLint / typecheck / prettier / build all clean; CI green on 3 OSes. LGTM — safe to merge. Two small, non-blocking suggestions below.

1. Scope & security context

• 2 files: gitUtils.ts (+20/−4), gitUtils.test.ts (+59/−1).
• isGitHubRepository() guards /setup-github (setupGithubCommand.ts:117), which writes .github/workflows/ files, downloads the qwen-code-action workflows, and opens the repo's Actions secrets settings URL. The old guard was a plain substring ‎/github\.com/ test over the whole git remote -v output — so lookalike hosts and github.com appearing anywhere (path, etc.) passed.

2. Tests pass on the PR branch

npx vitest run src/utils/gitUtils.test.ts
→ Test Files 1 passed (1) | Tests 31 passed (31)   (main had 24)

+7 new cases (3 reject security cases, 3 accept SSH cases, 1 SSH-port getGitHubRepoInfo case) + 1 reformatted to realistic git remote -v output.

3. Vulnerability proven — the new tests fail against main's code

I ran the PR's new test file against the OLD gitUtils.ts (checked out from main). Exactly 4 tests fail, each mapping to a real bug this PR fixes:

The other 27 pass on the old code too → legitimate GitHub remotes are unaffected (no regression). This confirms the tests are real regression guards, not vacuous.

4. Real-git E2E (no execSync mock) — 17/17 pass

I wrote a harness that creates real git repos with real remotes (git init + git remote add) and runs the actual functions against real git remote -v / git remote get-url output:

getGitHubRepoInfo() against real remotes: https / ssh://…:22 / SCP all return {owner, repo}; lookalike throws. The multi-remote case exercises the real multi-line, tab-separated parsing that the mocked unit tests only approximate.

5. Static checks (the PR's own test plan)

Note: a fresh checkout initially shows ~200 tsc errors, but they are all in unrelated files (acp-integration/, serve/, …) from stale workspace builds — main has them too. After npm run build of core + acp-bridge, tsc --noEmit for cli is fully clean. The PR's file is type-clean throughout.

6. Non-blocking suggestions

1. Lock in the userinfo-phishing case with a unit test. https://[email protected]/owner/repo.git is correctly rejected (new URL(...).hostname === 'evil.com'), but there's no unit test pinning it — it's a classic bypass worth guarding against regressions.
2. SCP host check is case-sensitive. isGitHubRemoteUrl (and the pre-existing getGitHubRepoInfo) use startsWith('[email protected]:'), so [email protected]:owner/repo.git is a false-negative. DNS hosts are case-insensitive and the URL path already lowercases via new URL(); only the SCP branch differs. Low real-world impact (people write it lowercase) and pre-existing, but a .toLowerCase() on the SCP host would make it consistent.

Verdict

LGTM / safe to merge. Correctly fixes the #5326 lookalike-host bypass plus an SSH-port correctness bug, backed by regression tests proven to catch the old behavior and a real-git E2E. No regressions for valid remotes. The §6 items are minor follow-ups, not blockers.

npx vitest run src/utils/gitUtils.test.ts
→ Test Files 1 passed (1) | Tests 31 passed (31)   (main 为 24)

_{Verified locally with real test runs (tmux): full suite, new-tests-against-old-code diff, a real-git integration harness, and the PR's full static-check plan. The harness file was temporary and removed; tracked source is clean at cf2d7af31.}