PRO-1042: route Cursor plugin login through typed CLI auth - #9
Conversation
|
Dependency update: lua-core-services #2183 must merge and deploy before this draft can release. Direct |
richard-lua
left a comment
There was a problem hiding this comment.
Code review — MEDIUM risk
The security direction is solid: this PR removes credential/OTP collection from the model conversation, routes new logins to lua auth configure in a private terminal, denies model-run lua auth configure in before-shell-execution.mjs (with matching tests and lint guards), removes the unsupported ${env:LUA_API_KEY} MCP literal, and adds structural lints so the old secret-bearing flows can't regress. The permission-template, hook, doc, and lint changes are internally consistent and well-tested. Two things keep this at medium: a hardcoded client-version header that diverges from the value the new test derives, and the fact that the added X-Lua-Client header ships ahead of the backend per the PR's own release gate. Respect the draft/release gate before merge.
Major
mcp/lua-platform/src/api-client.mjs:38—X-Lua-Clientis hardcoded to'cursor-plugin/1.0.0', buttests/api-client.test.mjsasserts it equalscursor-plugin/${package.json.version}. This only passes while the version is exactly1.0.0; the release gate calls for a version bump, which will both break the test and mislabel traffic as1.0.0. Derive the version frompackage.json(a build-time constant) so header and test stay in lockstep.
Minor
mcp/lua-platform/src/api-client.mjs:38— The header is now sent unconditionally, but the PR body says direct api-client traffic should stay unlabeled until the backend (lua-core-services #2171) accepts thecursor-pluginfamily. Confirm the backend ignores unknownX-Lua-Clientvalues and keep this blocked behind the stated release gate; otherwise every MCP tool call could fail until #2171 deploys.mcp/lua-platform/tests/api-client.test.mjs:70— The "all direct API calls behind the wrapper" test greps forfetch(/fetchFn(. A future tool usingundici,node:http, or an aliased fetch would evade the regex and silently pass. Note the limitation in a comment or extend it to flag http/https/undici imports undersrc/tools/.
Otherwise the change looks well-scoped and safe once the release gate clears.
PR Risk Reviewer — automated senior review of 37046a4 · risk: medium · confidence: 0.78
richard-lua
left a comment
There was a problem hiding this comment.
Code review — MEDIUM risk
Solid security-hardening change: login now runs lua auth configure in a private terminal, the model is blocked from running it via both the permission template and the before-shell-execution hook, and email/OTP/raw-key collection is removed from the conversation surface. Lint and test coverage back the new invariants, and the docs are updated consistently. The findings below are advisory — the main one is a hardcoded client-version string that will drift the moment the plugin version is bumped (which the release gate requires).
Major
mcp/lua-platform/src/api-client.mjs:38—X-Lua-Clientis the literal'cursor-plugin/1.0.0', butapi-client.test.mjsasserts it equalscursor-plugin/${package.json version}. They match only because the root package is at 1.0.0; the required pre-release version bump will break the test and ship a stale client version to the backend that's being taught to accept thecursor-pluginfamily. Derive the version from package.json (runtime read or build-time constant) instead of freezing it.
Minor
scripts/lint-cli-flags.mjs:31— the newlua auth configure --email/--api-keydenylist entries are only self-excluded forlint-cli-flags.mjs. Withscriptsnow inSCAN_DIRS, any future lint/doc that must name these forbidden patterns will trip the denylist. Exclude allscripts/lint-*.mjsor encode the literals non-matchingly.mcp/lua-platform/tests/api-client.test.mjs:70— the "single choke point" guard greps src for/\b(?:fetch|fetchFn)\s*\(/. A comment, aprefetch(helper, or an aliased fetch could produce false results either way. Assert the structural invariant (every tool importsapiRequest) rather than token-scanning.mcp.json:4— dropping the MCPenvblock silently changes how the spawned server obtains credentials; it now relies on Cursor passing through the parent environment plus the credentials-file/.envfallbacks. Verify Cursor inherits the shell env (or document thatLUA_API_KEY-only users need the credentials file), and have a tester validate the env-only path before release.
Note: the PR body's own instruction to keep this in draft until the CLI/backend dependencies ship is respected here — do not merge ahead of those gates.
PR Risk Reviewer — automated senior review of 37046a4 · risk: medium · confidence: 0.80
richard-lua
left a comment
There was a problem hiding this comment.
Code review — LOW risk (re-review of 48870ed)
Previously flagged
- X-Lua-Client version hardcoded, drifts from package.json — resolved. The header is now
cursor-plugin/1.1.0matching the root version bump, and the newscripts/lint-release-version.mjsenforces thatapi-client.mjs,server.mjs(name +plugin_version×2),.cursor-plugin/plugin.json, and the MCPpackage.jsonall agree withpackage.json's version. The test derives the expected header frompackage.json, so header and test now move in lockstep and future bumps fail loudly if any file lags. - X-Lua-Client sent before backend accepts it — resolved (as a process gate). Still sent unconditionally, but the thread now names lua-core-services #2183 as a hard release-gate dependency and the PR remains draft. Confirm the backend ignores unknown
X-Lua-Clientvalues before release. - Directory-scan test is brittle — still open. The wrapper-coverage test continues to token-scan
src/forfetch(/fetchFn(; a futurenode:http/undici/aliased-fetch caller would evade it silently. Minor, non-blocking.
Assessment
The security direction remains solid and the new commits are clean: version metadata is now consistent and lint-enforced, the hook/permission/doc/lint changes are internally coherent and well-tested, and the release gate is explicit. Nothing new raises the risk. The remaining items are advisory test-robustness and env-passthrough notes.
Minor
mcp/lua-platform/tests/api-client.test.mjs:70— the "every direct Lua API call behind the identified wrapper" guard token-scans forfetch(/fetchFn(. A future tool usingnode:http,undici, or an aliased fetch evades the regex. Assert the structural invariant (everysrc/tools/*.mjsimportsapiRequest; nohttp/https/undiciimport undersrc/) or document the limitation.scripts/lint-cli-flags.mjs:30—scriptsis now scanned and the auth-flow deny patterns are literals, but onlylint-cli-flags.mjsis self-excluded. A future guard script that must namelua auth configure --email/--api-keywould trip the denylist. Exclude allscripts/lint-*.mjsor encode the literals non-matchingly.mcp.json:4— dropping theenvblock means the spawned MCP server relies on Cursor passing through the parent environment plus the credentials-file/.envfallbacks. Verify Cursor inherits the shell env for spawned servers (or document thatLUA_API_KEY-only users need the credentials file/.env), and have a tester validate the env-only path.
Respect the PR's own release gate (lua-core-services #2183 + published lua-cli 3.28.0) — do not merge ahead of those dependencies.
PR Risk Reviewer — automated senior review of 48870ed · risk: low · confidence: 0.82
PR Risk Reviewer — automated senior review of 48870ed · risk: low · confidence: 0.82 · re-review
Summary
LUA_API_KEY,~/.lua-cli/credentials, and.envunchanged. This includes non-dotted legacy keys.lua auth configurein a private terminal. lua-cli 3.28.0 and later issue a typed personal credential after the user selects an organization, one or more agents, and a role. Builder remains the default role.lua auth configure, andlua auth key*remains denied.${env:LUA_API_KEY}from both Cursor MCP registration paths. The MCP process inherits the real environment and can fall back to the CLI credentials file or.env.cursor-plugin/1.1.0. Calls that spawn lua-cli use the CLI's own versioned identity.Source-derived caller inventory
The inventory came from a repository-wide
rgbefore editing.skills/lua-auth/SKILL.md,skills/lua-doctor/SKILL.md, and the auth preflight inskills/lua-init/SKILL.mdown setup.lib/credentials.mjsandmcp/lua-platform/src/auth.mjsresolveLUA_API_KEY, the CLI credentials file, and.env. This PR does not change their order or formats.scripts/install.mjsandmcp.jsonboth register the bundled MCP server. Both now omit the unsupported credential placeholder.mcp/lua-platform/src/tools/list-agents.mjsandget-agent.mjsspawnlua agents --json. Those requests usecli/<version>.mcp/lua-platform/src/api-client.mjsowns direct Lua API requests for deployment status, primitive versions, and logs. Those requests now usecursor-plugin/1.1.0, which lua-core-services #2183 recognizes as a bounded family.Merge gate
Merge only after
npm view lua-cli versionreports 3.28.0 or later. The current PR head must also have green CI and current-head approval.This PR does not rotate, revoke, rewrite, or invalidate an existing credential. It does not use work from the closed #2008 prototype.
Verification
npm run lintnpm test -- --coverage(249 tests)node scripts/check-coverage.mjsnpm test(83 tests)npm run buildnode scripts/check-bundle-size.mjsLinear: PRO-1042