feat(extensions): support authenticated HTTPS Git installs - #9458
Conversation
E2E Test ReportStatus: automated protocol/Core coverage passed; live private-repository E2E not run. Automated verification
Live E2E statusNot run because this environment has no disposable private repository credential. Tests use dummy credentials and mocked Git, so they do not prove GitHub service-side acceptance of every username/token combination. The recorded manual plan covers default one-time install and restart, stable snapshot identity, update rejection, stored install and restart, real update, uninstall cleanup, and scanning artifacts/logs/operations/telemetry/Git argv for the raw URL, username, token, and Basic header. The token must be restricted to the disposable repository and revoked or rotated immediately afterward. Known threat boundaryCredentials never enter Git argv, remote URLs, install metadata, operation history, or installed artifacts. While Git is running, a compromised process under the same OS user may still inspect the short-lived child environment; stored mode also intentionally retains the secret in the selected keychain/encrypted-file backend until uninstall or manual cleanup. |
|
Thanks for the PR — this is an unusually careful design for a genuinely sensitive area.
Moving on to code review. 🔍 中文说明感谢贡献!这是一份对敏感领域来说非常细致的设计。
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe implementation matches the design doc closely, and the design matches what I would have proposed independently — strip at the boundary, out-of-band scoped transport, reuse of existing secret storage, and an honest split between updatable-stored and source-free one-time. No critical blockers found. Details: What holds up
Questions, non-blocking
sequenceDiagram
participant P1 as Client
participant P2 as Daemon route
participant P3 as ExtensionManager
participant P4 as Hybrid secret storage
participant P5 as Git child process
P1->>P2: install request with URL userinfo
P2->>P2: validate, decode, strip userinfo
P2->>P3: clean URL plus in-memory credential
P3->>P4: stored mode - write secret under random key
P3->>P3: stage 0600 selector into the artifact
P3->>P5: clone clean URL, Basic header via GIT_CONFIG env
P5-->>P3: clone done, root .git excluded
P3->>P4: commit keeps the secret, discard deletes it
Files changed (17)
TestingEvidence below is the PR's own CI on the reviewed commit, fetched via API — PR code was not built or executed in this review. The author also reports focused suite results (245/245 Core, 35/35 V2 routes, 20/20 legacy) in the PR body; that is the author's claim, not independent evidence. At fetch time the primary Node suite is still running, with everything completed so far green — including TruffleHog and the dependency audit, which matter for a credential-handling change. The finalize job updates the table below once CI settles. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Bot orchestration checks (triage, review-pr, precheck) are excluded — they belong to Sandboxed verification would settle the one thing this suite cannot: 中文说明代码审查实现与设计文档高度一致,设计本身也与我独立构想的方案吻合——边界处剥离凭据、带外按精确仓库作用域传递、复用现有机密存储、可更新的存储式与无 source 的一次性两条路径划分清晰。未发现阻塞性问题。 站得住的部分
非阻塞的问题
测试以下证据来自 API 拉取的该 commit 自身 CI——本审查未构建或运行 PR 代码。作者在 PR 正文中报告的聚焦套件结果(Core 245/245、V2 路由 35/35、legacy 20/20)是作者声明,不是独立证据。拉取时主 Node 套件仍在运行,已完成的部分全部为绿——包括对这个凭据处理变更很重要的 TruffleHog 与依赖审计。CI 落定后 finalize 任务会更新下方表格。 沙箱验证可以解决本套件无法覆盖的一点: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean static review with no blocking findings, but this is a 761-production-line change across core/daemon/SDK on the credential-handling surface, and both policy and prudence put the final call with a maintainer. Stepping back: this is better than what I would have written. My independent proposal was the same shape — strip at the boundary, scoped out-of-band transport, reuse the existing secret storage, split stored-vs-one-time — and the PR goes further in ways that earn their keep: selector symlink defenses, repo-supplied selector removal, validated install identities, and a design doc that states the threat model honestly (including what it does not protect against). Every file in the diff serves the stated goal; I found no drive-by changes. If I had to maintain this in six months, the lifecycle symmetry (prepare/commit/discard, uninstall artifact-first then best-effort secret cleanup) is exactly the structure I'd want. Why defer rather than approve:
No request-changes, because nothing I found is blocking — the questions above are for discussion, and the implementation quality is high. ⏸️ Deferring to @yiliang114 (most recent human reviewer; no maintainer handle is configured for this run). Needs a human call on: (1) security/protocol sign-off for the credential surface per the escalation above, (2) whether to run 中文说明置信度:3/5 —— 静态审查干净、无阻塞性发现,但这是一次横跨 core/daemon/SDK、约 761 行生产代码的凭据处理面变更,按策略和审慎原则,最终决定都应交给维护者。 退一步看:这份实现比我自己会写出的更好。我的独立方案形状相同——边界剥离、作用域化带外传递、复用现有机密存储、存储式/一次性分流——而 PR 在值得的地方走得更远:selector 符号链接防御、仓库自带 selector 的移除、经过校验的安装身份,以及一份如实陈述威胁模型(包括它不保护什么)的设计文档。diff 中每个文件都服务于既定目标,没有顺手夹带的改动。如果六个月后由我维护,prepare/commit/discard 的对称性、卸载时先提交 artifact 删除再尽力清理 secret 的结构,正是我会想要的样子。 为什么是转交而不是批准:
不请求修改,因为我没有发现阻塞性问题——上面的问题是讨论项,实现质量本身是高的。 ⏸️ 转交 @yiliang114(最近的人类审查者;本次运行未配置维护者句柄)。需要人工决定:(1) 按上述升级策略对凭据面做安全/协议把关;(2) 合并前是否运行 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
features[] |
— | "extension_git_credentials" |
— Qwen Code · serve A/B
CI follow-upThe failed Ubuntu job was isolated to the required no-AK integration gate. Production tests, lint, formatting, build checks, secret scan, Serve A/B, and daemon E2E had passed. Root cause: the capabilities integration fixture still expected the pre-PR baseline, while the daemon correctly advertised the new Fix: commit Verification after the fix:
|
doudouOUC
left a comment
There was a problem hiding this comment.
Approving from a correctness/security review standpoint — no blocking issues found. Reviewed at fc284f5; the maintainer security/protocol sign-off this PR requests is still the gating call (and branch protection keeps merge blocked until then).
What holds up
- Leak surface is closed end-to-end: userinfo stripped at the route before the op is queued; one-time ops omit
source; git only ever sees the clean URL with the credential injected out-of-band ashttp.<exact-url>.extraHeaderviaGIT_CONFIG_*child-env (never in argv or.git/config); root.gitexcluded from artifacts; one-time telemetry uses a generic category. Tests confirm the token is absent from argv, telemetry, bridge events, and metadata. - Stored-secret lifecycle is symmetric: secret→selector write with rollback on selector failure; idempotent commit/discard; uninstall removes the artifact first then best-effort deletes the secret with
extension_credential_cleanup_failed; update resolves the selector before any network access (extension_credential_unavailablewith no git call when missing). - Hostile-input defenses are real:
lstat-based selector read (symlink-rejected, size-capped), repo-provided selectors removed before the managed one is written, regex-validatedinstallIdhonored only for snapshot/stored metadata, and CRLF/control-char rejection both raw and percent-decoded. snapshottype is threaded through SDK + acp-bridge status types; the only UI consumer rendersinstallTypeas free text, so nothing breaks.
Non-blocking notes (for discussion, already raised in triage):
- Unifying
GITHUB_TOKENonto the extraHeader mechanism meansrestrictGitEnvironmentnow builds the isolated env (noHOME,GIT_CONFIG_NOSYSTEM,GIT_CONFIG_GLOBAL=/dev/null) even on the non-publicCLI path whenever a credential is present. Safer, but CLI users relying on global gitconfig (e.g. proxy) withGITHUB_TOKENset would see it ignored — worth a line in the docs/PR body. GIT_CONFIG_COUNT-style config needs Git ≥ 2.31. Daemon installs enforce Git ≥ 2.37 via the public-path assertion, so they're covered; the non-public CLIGITHUB_TOKENpath has no version check, so on Git ≤ 2.30 the counted config is silently ignored and a private clone fails with an opaque auth error instead of a clear message.
Heads up: the branch has just gone into conflict with main (mergeable: CONFLICTING) — the head commit is unchanged, so this review still applies as-is, but it needs a rebase before it can land.
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the security-critical paths (route validation in workspace-extensions.ts, extension-git-credentials.ts, and the git auth in github.ts).
Approve. Credential handling is sound and matches the design doc.
What I verified:
- Userinfo is validated then stripped.
parseExtensionInstallSourcerejects control chars/NUL/CR-LF, caps username/password at 256/4096 UTF-8 bytes, and rejects malformed percent-encoding beforedecodeURIComponent. After validationparsed.username/passwordare cleared, so only the clean URL + an in-memory credential cross into Core — no credential persists in the URL. - Git auth avoids URL and argv.
restrictGitEnvironmentpasses the credential viaGIT_CONFIG_KEY_0 = http.<clean-url>.extraHeader/GIT_CONFIG_VALUE_0 = Authorization: Basic <base64>(counted config), scoped to the exact clean repository URL.GITHUB_TOKENis migrated to the same header mechanism instead of being inserted into a clone URL — a genuine improvement over the oldaddGitHubToken. - Selector/secret storage is fail-closed.
readSelectorrejects symlinks, non-files, >4KB, wrong version/backend/key-prefix;writeGitCredentialSelectorusesatomicWriteJSONwithmode: 0600+forceMode+noFollow. The secret value ({username,password}) never enters the artifact — only the 0600 selector (version/backend/secretKey) does. - One-time snapshots retain nothing durable. Snapshot metadata carries no source/ref/commit/credential, reports
not updatable, and the catalog/status projection omits source fortype === 'snapshot'— so a one-time install cannot leak the repository locator.
One non-blocking note: the design doc's own residual risk is accurate — the child Git process necessarily sees the short-lived header in its environment, so this protects durable product state and argv, not an already-compromised same-user process. That's the right, honestly-stated boundary.
0 unresolved threads.
ytahdn
left a comment
There was a problem hiding this comment.
LGTM — approved.
中文
LGTM — 批准。
doudouOUC
left a comment
There was a problem hiding this comment.
Approving from a correctness/security review standpoint at 54a6b73 — no blocking issues found. The maintainer security/protocol sign-off this PR requests is still the gating call.
What holds up
- Leak surface is closed end-to-end. Userinfo is stripped at the route (
parseExtensionInstallSource) before the operation is queued, andbody.sourceis rewritten so operation history stays clean. One-time operations omitsourceentirely; git only ever sees the clean URL, with the credential arriving ashttp.<exact-repo-url>.extraHeaderviaGIT_CONFIG_COUNT/KEY_0/VALUE_0in the isolated child environment — never in argv, never persisted to.git/config. Root.gitis excluded from installed artifacts on any remote git install. Redacted telemetry for one-time reports the genericcredentialed HTTPS Git source/snapshotcategories. Tests assertfine-grained-tokenis absent fromgit argv, telemetry, bridge events, and staged install metadata. - Stored-secret lifecycle is symmetric.
prepareStoredGitCredentialwrites the secret first, then the selector, and rolls the secret back on selector failure;commit()/discard()are idempotent;cleanupPreparedExtensiononly discards when!gitCredentialActivated, so an activated secret can't be blown away by a later cleanup. Update reusesgitCredential.selector(no rewrite, nopreparedGitCredential) and only callsresolveStoredGitCredentialbefore any git access — the test assertsmockGit.listRemoteis never called when the selector is missing. Uninstall commits artifact removal first and then best-effort deletes the secret with anextension_credential_cleanup_failedwarning rather than a rollback, matching the design doc. - Hostile-input defenses are real.
readSelectoruseslstat(regular file only, 4 KiB cap, symlinks rejected), staging removes any repo-provided selector before writing the managed one,installIdis/^[a-f0-9]{64}$/-validated and only honored forsnapshot/storedmetadata, and the route rejects control characters (C0/C1 +0x7f) both raw and post-decode plus enforces 256 / 4096 byte caps on username/password. .git-exclusion regression I chased is a non-issue.copyExtension({ excludeRootGitDirectory: remoteGitInstall })skips.gitfor every remote git install, not just credentialed ones. Verified this is safe becausecloneFromGitalways populatesinstallMetadata.gitCommit, socheckForExtensionUpdatetakes thegitCommitfast path; thegetRemotes()+revparse('HEAD')fallback is only reached on legacy on-disk data that pre-dates this change (whose.gitdirectory is not touched retroactively).getExtensionIdthrow on malformedinstallIdwon't wedge enumeration. The recursive-load path callsloadExtensionwith defaultthrowOnError: false, which catches the error and logsWarning: Skipping extension. Only the install/update self-check paths passthrowOnError: true, and there the failure is intentional.- Conventions are clean. kebab-case module, collocated tests, typed error subclasses with stable wire codes (
extension_credential_unavailable,extension_not_updatable,extension_credential_cleanup_failed), ESM.jsimports, noany.
Non-blocking note (already surfaced in triage — leaving here for maintainer eyes)
- Unifying
GITHUB_TOKENonto the extraHeader mechanism meansrestrictGitEnvironmentnow builds the isolated environment (noHOME, no proxy env,GIT_CONFIG_NOSYSTEM=1,GIT_CONFIG_GLOBAL=/dev/null) whenever a credential is present — even on the non-publicCLI path. Daemon installs were alreadypublic, so the only affected population is non-daemon CLI installs withGITHUB_TOKENset; users whose proxy / CA /insteadOf/ credential-helper config lives in global gitconfig would see those ignored during clone. Probably the right trade for the leak this closes (the token no longer ends up in.git/config), and the env-var config path already requires Git ≥ 2.31 (public path requires 2.37 anyway). Worth a line in the PR body or a follow-up doc note so CLI-plus-proxy users aren't surprised.
中文说明
在 54a6b73 上从正确性/安全角度批准 —— 未发现阻塞性问题。 PR 明确请求的 maintainer 安全与协议审签仍是最终关口。
站得住的点
- 凭据泄漏面端到端封闭。 userinfo 在路由
parseExtensionInstallSource中即被剥离,body.source被改写为 clean URL,operation history 保持干净。one_time 的 operation 完全不返回source;git 只看到 clean URL,凭据通过隔离子进程环境的GIT_CONFIG_COUNT/KEY_0/VALUE_0落在http.<clean-url>.extraHeader,不进 argv、不进.git/config;所有远端 git 安装的 artifact 都不再包含根.git。one_time 的遥测使用credentialed HTTPS Git source/snapshot占位。测试断言 token 未出现在 git argv、telemetry、bridge event 与 staging metadata。 - stored 凭据生命周期对称。
prepareStoredGitCredential先写 secret 再写 selector,selector 失败时回滚 secret;commit()/discard()幂等;cleanupPreparedExtension仅在!gitCredentialActivated时清理,激活后的 secret 不会被误删。update 复用gitCredential.selector时不会重写 secret,且resolveStoredGitCredential严格前置于任何 git 访问 —— 测试断言 selector 缺失时mockGit.listRemote完全不会被调用。卸载先落地 artifact 删除,secret 删除失败仅以extension_credential_cleanup_failedwarning 返回,符合设计文档。 - 敌意输入防御是真的。
readSelector使用lstat检查(仅普通文件、4 KiB 上限、拒绝符号链接),staging 阶段先删除仓库自带 selector 再写入 managed selector;installId用/^[a-f0-9]{64}$/校验且只在snapshot/stored分支采信;路由层对 control character(C0/C1 +0x7f)原样与百分号解码后都做拦截,并对 username/password 分别限制 256/4096 字节。 - 我担心的"非凭据 git 安装丢
.git"经核实不成立。copyExtension({ excludeRootGitDirectory: remoteGitInstall })对所有远端 git 安装都跳过根.git,而cloneFromGit一定写gitCommit,checkForExtensionUpdate走gitCommit快路径,getRemotes()fallback 只对存量、缺gitCommit的老安装触发,本次改动不会追溯性删除其.git。 getExtensionId对非法installId抛错不会拖崩枚举。 加载/枚举走loadExtension默认throwOnError: false,异常被捕获后打Warning: Skipping extension并跳过该扩展;只有 install/update 的自检路径显式throwOnError: true,这里失败应当整体失败,符合预期。- 风格与规范干净。 kebab-case 模块、测试与源码同目录、有稳定 wire code 的错误子类(
extension_credential_unavailable/extension_not_updatable/extension_credential_cleanup_failed)、ESM.js导入、无any。
非阻塞提示(triage 已提,留给 maintainer 参考)
- 将
GITHUB_TOKEN也归拢到 extraHeader 机制后,只要存在凭据,restrictGitEnvironment就会构建隔离子进程环境(丢HOME、丢代理相关变量、GIT_CONFIG_NOSYSTEM=1、GIT_CONFIG_GLOBAL=/dev/null),即使networkPolicy不是public。daemon 一直是public,因此实际受影响群体只有设置了GITHUB_TOKEN的非 daemon CLI 用户 —— 他们放在全局 gitconfig 的代理 / CA /insteadOf/ credential helper 在 clone 时将不再生效。这大概率是"换掉 token 落进.git/config的泄漏"的合理代价(env-based git config 要求 Git ≥ 2.31,public 路径本来就要求 2.37),但建议在 PR body 或后续文档里加一行说明,避免 CLI + 代理用户意外撞坑。
|
Released in v0.21.15. |
What this PR does
Adds authenticated generic HTTPS Git extension installs to both daemon install surfaces, advertised through the new
extension_git_credentialscapability. Requests may selectcredentialPersistence: "stored" | "one_time"; credentialed requests that omit the field default toone_time, while non-credentialed requests keep existing behavior and reject the field.The daemon removes URL userinfo before queueing the operation. Git clone, fetch, and remote checks receive only the clean URL, with an exact-repository Basic header passed through an isolated child-process environment. The existing
GITHUB_TOKENpath now uses the same header mechanism, and newly cloned remote artifacts exclude the root.gitdirectory.Stored installs keep the clean source and a
0600selector while the secret is held by the existing hybrid storage (system keychain first, encrypted-file fallback). One-time installs persist a source-free snapshot, retain a random stable install identity across restart, and returnextension_not_updatableon update. Stored secret failures returnextension_credential_unavailablebefore network access; uninstall cleanup failures are surfaced as warnings after the artifact is removed.This PR intentionally does not add WebShell UI. A follow-up can gate a three-way stored / one-time / cancel prompt on the new capability. Because the change spans Core, daemon protocol, and SDK types, maintainer security and protocol review is explicitly requested.
Why it's needed
Fine-grained personal access tokens can be restricted to one repository and are often more flexible than machine-wide SSH key setup, but URL credentials were previously rejected outright. Persisting the raw URL would leak the token into Git remotes, process arguments, metadata, logs, or operation history; dropping the source entirely would make every install non-updatable. This change provides both safe one-time installation and an explicit managed-storage path for updates without retaining credentials in repository URLs.
Reviewer Test Plan
How to verify
credentialPersistence; expect an accepted one-time install whose terminal operation omitssource, whose catalog/status entry omitssource, and whose update fails withextension_not_updatable.credentialPersistence: "stored"; expect the terminal result to expose only the clean source, persistence mode, and selected storage backend. Restart, check for an update, and confirm clone/fetch/list-remote still use the clean URL and the stable install id..git/config, and the installed artifact. Confirm a repository-provided selector and root.gitare not retained.extension_credential_unavailablebefore Git access. Uninstall a stored install and confirm the secret is deleted; simulate cleanup failure and expect a warning without rollback.Automated evidence on macOS: focused Core suites passed 245/245, the V2 daemon route suite passed 35/35, and the focused legacy daemon/capability/status suite passed 20/20.
npm run lint,npm run typecheck, andnpm run buildall passed after rebasing onto the latestorigin/main.Evidence (Before & After)
N/A — protocol, storage, and Core behavior only; WebShell UI is out of scope.
Tested on
Environment (optional)
Node.js 22 workspace dependencies; focused Vitest suites used the package-specific configs. Tests use dummy credentials and mocked Git. A real private-repository E2E was not run because no disposable credential was available.
Risk & Scope
extension_credential_cleanup_failed.Linked Issues
N/A
中文说明
本 PR 做了什么
为两个 daemon 安装入口增加通用 HTTPS Git 凭据安装能力,并通过新的
extension_git_credentialscapability 对外声明。请求可选择credentialPersistence: "stored" | "one_time";含凭据但未传该字段时默认one_time,不含凭据的请求维持现有行为并拒绝该字段。daemon 会在操作入队前移除 URL userinfo。Git clone、fetch 和远端检查只接收清理后的 URL,Basic header 通过隔离后的子进程环境、按精确仓库 URL 作用域传入。现有
GITHUB_TOKEN路径也改用同一 header 机制,新克隆的远端安装 artifact 不再包含根.git目录。stored 安装保留 clean source 和权限为
0600的 selector,secret 使用现有混合存储(优先系统 Keychain,不可用时回退加密文件)。one-time 安装持久化为无真实 source 的 snapshot,通过随机且稳定的安装 ID 保证重启后身份不变,并在更新时返回extension_not_updatable。stored secret 不可用时会在网络访问前返回extension_credential_unavailable;卸载后 secret 清理失败只返回 warning,不回滚 artifact 删除。本 PR 明确不修改 WebShell UI。后续 PR 可根据新 capability 提供“安全存储 / 一次性安装 / 取消”三选一交互。由于变更跨越 Core、daemon 协议和 SDK 类型,明确请求 maintainer 进行安全与协议审查。
为什么需要
Fine-grained personal access token 可以只授权单个仓库,通常比在机器上配置全局 SSH key 更灵活,但此前 URL 中只要含凭据就会被完全拒绝。直接持久化原始 URL 会让 token 泄露到 Git remote、进程参数、元数据、日志或 operation history;完全丢弃 source 又会导致所有安装都无法更新。本变更同时提供安全的一次性安装,以及显式选择的受管凭据存储更新路径,并且不会把凭据保留在仓库 URL 中。
Reviewer 测试计划
如何验证
credentialPersistence;预期接受为 one-time 安装,终态 operation 不含source,catalog/status 不含source,更新返回extension_not_updatable。credentialPersistence: "stored"提交同类 URL;预期终态结果只暴露 clean source、持久化模式和存储后端。重启后检查更新,确认 clone/fetch/list-remote 仍使用 clean URL,安装 ID 保持不变。.git/config和已安装 artifact 中均不含凭据;确认仓库伪造的 selector 与根.git不会被保留。extension_credential_unavailable。卸载 stored 安装后确认 secret 被删除;模拟清理失败时应返回 warning 且不回滚卸载。macOS 自动化证据:Core 定向测试 245/245 通过,V2 daemon 路由测试 35/35 通过,legacy daemon/capability/status 定向测试 20/20 通过。在变基到最新
origin/main后,npm run lint、npm run typecheck和npm run build均通过。Before & After 证据
N/A——本 PR 只涉及协议、存储和 Core 行为,WebShell UI 不在范围内。
测试平台
环境(可选)
Node.js 22 workspace 依赖;定向 Vitest 使用各 package 的专用配置。测试使用 dummy credential 和 mocked Git。由于没有可抛弃的测试凭据,未执行真实私有仓库 E2E。
风险与范围
extension_credential_cleanup_failed。关联 Issue
N/A