feat(auth): add login/logout/whoami and require auth for push/pull#28
feat(auth): add login/logout/whoami and require auth for push/pull#28
Conversation
…h/pull Implement CLI authentication with LEAPERone's auth system: - Add credentials module (~/.envx/credentials.json) for token storage - Add `envx login` with browser and device flow support - Add `envx logout` to clear credentials - Add `envx whoami` to show current user - Require auth token for push/pull, with clear error on 401 Closes #3, closes #4
leaperone-bot
left a comment
There was a problem hiding this comment.
🤖 Code Review 🟡
认证功能 PR 审查
整体评价
功能实现完整,涵盖了 login(浏览器流程 + device 流程)、logout、whoami 以及对 push/pull 的认证要求。代码结构清晰,凭据优先级逻辑(devConfig > env > stored token)合理。以下是需要关注的问题:
🔴 安全问题
-
openBrowser中的命令注入风险(login.ts:21)exec(`${cmd} "${url}"`); `url` 来自 `baseUrl` 拼接,虽然默认值是可控的,但 `--base-url` 允许用户自定义输入。如果 `url` 包含 `"` 或 `$()` 等字符,可能导致命令注入。建议改用 `execFile` 或 `spawn` 并将 URL 作为参数数组传入,而非字符串拼接: ```ts import { execFile } from 'node:child_process'; execFile(cmd, [url]);
-
本地回调服务器缺少来源验证(
login.ts:28-72)
本地 HTTP 服务器监听在随机端口上接收 authorization code,但没有验证请求来源。同一机器上的恶意进程可以向127.0.0.1:<port>/callback?code=<stolen_code>发送请求来尝试抢先交换 token。虽然这是常见的 CLI OAuth 模式,但建议至少加入一个state参数来做 CSRF 防护。
🟡 Bug / 健壮性问题
-
device flow 与 browser flow API 路径不一致
- browser flow 使用
/api/v1/cli/auth/exchange - device flow 使用
/api/auth/device/code和/api/auth/device/token(缺少v1/cli前缀) whoami和 login 验证使用/api/v1/cli/me
请确认 device flow 的 API 路径是否正确,还是遗漏了版本前缀?
- browser flow 使用
-
clearCredentials只删除 token 但保留 baseUrl(credentials.ts:39-42)export function clearCredentials(): void { const credentials = loadCredentials(); delete credentials.token; saveCredentials(credentials); }
logout 后
baseUrl仍然残留在文件中。whoami中调用getAuthBaseUrl()会读取这个残留的 baseUrl,但用户可能期望 logout 是完全清除状态。建议要么一并清除,要么在注释中说明保留 baseUrl 的意图。 -
本地服务器超时后未正确清理(
login.ts:85-88)
setTimeout中调用server.close()+reject(),但如果此时 Promise 已经 resolve/reject 过了(比如 exchange 正在进行中),会导致双重 reject。建议用一个settled标志来防止。 -
whoami中 source 判断逻辑不完全准确(whoami.ts:55)const source = process.env.ENVX_API_KEY ? 'api-key (env)' : 'session token';
实际使用的凭据可能来自
getCredential(),但如果 devConfig 中有apiKey,这里的 source 显示就不准确了。不过 whoami 不经过 devConfig 路径,所以影响较小。
💡 建议改进
-
凭据文件权限仅在创建时设置
saveCredentials用mode: 0o600写入文件,ensureDir用mode: 0o700创建目录——这很好。但如果文件已存在且权限被意外更改,不会被修正。可以考虑在loadCredentials时检查权限并警告用户。 -
缺少测试覆盖
项目当前没有测试基础设施(testscript 是空的),但credentials.ts这类纯逻辑模块非常适合单测。建议至少为loadCredentials、saveCredentials、getCredential优先级逻辑添加测试。 -
push/pull 中的重复代码
push.ts和pull.ts中认证相关的代码(获取凭据、401 处理)几乎完全相同,建议抽取为共享的认证中间件/工具函数。 -
exec打开浏览器没有错误处理(login.ts:21)
exec()的返回值和回调都被忽略了。如果浏览器打开失败(比如无头服务器),用户不会看到任何提示。建议捕获错误并提示用户手动打开 URL。
leaperone-bot
left a comment
There was a problem hiding this comment.
🤖 Code Review 🟡
PR Review: feat(auth): add login/logout/whoami and require auth for push/pull
整体实现思路清晰,browser flow + device flow + credential 持久化的设计合理。以下是需要关注的问题:
🔴 安全问题
-
openBrowser存在命令注入风险(login.ts:21)exec(`${cmd} "${url}"`); `url` 来源于拼接的 `baseUrl`(可通过 `--base-url` 传入),如果包含 `"` 或 `$()` 等 shell 特殊字符,可导致命令注入。建议使用 `execFile` 或对 URL 进行严格校验: ```ts import { execFile } from 'node:child_process'; execFile(cmd, [url]);
-
本地回调服务器绑定在
127.0.0.1但缺少 CSRF 防护(login.ts:83-91)
任何本地进程或恶意网页脚本都可以向http://127.0.0.1:{port}/callback?code=xxx发送请求。虽然攻击者不知道合法 code,但建议在授权 URL 中加入随机state参数并在 callback 中校验,这是 OAuth 标准实践。
🟡 Bug / 一致性问题
-
API 路径不一致 — browser flow 使用
/api/v1/cli/auth/exchange,device flow 使用/api/auth/device/code和/api/auth/device/token(没有v1/cli前缀)。确认这是否是服务端实际设计,还是拼写遗漏?如果是不同版本的 API,混用可能带来维护风险。 -
browserLogin中timer的unref()不会清除(login.ts:96-99)
成功获取 token 后server.close()被调用,但timer从未被clearTimeout。虽然unref()使其不阻止进程退出,但如果 promise resolve 后进程继续运行(如测试环境),timer 仍会触发 reject。建议在 resolve/reject 时都clearTimeout(timer)。 -
whoami中 source 判断不准确(whoami.ts:55)const source = process.env.ENVX_API_KEY ? 'api-key (env)' : 'session token';
但
getCredential()的优先级是ENVX_API_KEY > stored token,如果ENVX_API_KEY存在但为空字符串,getCredential()会返回空字符串(falsy),实际使用 stored token,但whoami中process.env.ENVX_API_KEY为 truthy(空字符串是 defined),导致 source 显示错误。 -
whoami使用getAuthBaseUrl()而非 credential 中存储的baseUrl— 如果用户 login 时指定了--base-url,token 和 baseUrl 一起存储在 credentials.json 中,getAuthBaseUrl()能正确读取。但如果用户设置了ENVX_BASEURL环境变量指向另一个服务器,whoami会向错误的服务器验证 token。这个行为可能令人困惑,至少应在文档中说明优先级。
🟡 代码质量
-
clearCredentials只删除 token 不删除 baseUrl(credentials.ts:39-42)export function clearCredentials(): void { const credentials = loadCredentials(); delete credentials.token; saveCredentials(credentials); }
logout 后 credentials.json 仍保留
baseUrl。这是有意为之(方便下次 login)还是遗漏?如果是故意的,建议加注释说明。 -
缺少超时处理 —
browserLogin中 exchange API 的fetch调用和deviceLogin中的 pollingfetch都没有设置AbortController超时。如果服务端无响应,会长时间卡住。 -
缺少测试 — 这是一个 416 行新增的功能变更,涉及认证、凭据存储、网络请求等关键路径,但没有任何单元测试。虽然项目目前
testscript 是占位符,但 credential 工具函数(loadCredentials、saveCredentials、getCredential、getAuthBaseUrl)完全可以单元测试,建议至少补充这部分。
💡 建议
CREDENTIALS_FILE在credentials.ts中既作为内部常量使用,又被login.ts导入用于提示信息显示——这没问题,但建议CREDENTIALS_DIR和CREDENTIALS_FILE导出时加上as const或 JSDoc 注明只读语义。ensureDir中mode: 0o700是好的安全实践 👍,saveCredentials中mode: 0o600也很好。
Summary
envx logincommand with browser-based and device flow authentication (reusing LEAPERone's auth system)envx logoutandenvx whoamicommands~/.envx/credentials.jsonpush/pullcommands (priority: devConfig apiKey >ENVX_API_KEYenv > stored credential)Test plan
envx login— browser opens, authorize, token saved to~/.envx/credentials.jsonenvx login --device— device code shown, authorize in browser, token savedenvx whoami— shows authenticated user infoenvx push/pullwithout auth — error message prompts loginenvx push/pullafter login — requests succeed with tokenenvx logout— credentials cleared, subsequent push/pull failCloses #3, closes #4