fix(vnext): address security review feedback + add PR CI - #6
Open
eshwar-sundar-glean wants to merge 1 commit into
Open
fix(vnext): address security review feedback + add PR CI#6eshwar-sundar-glean wants to merge 1 commit into
eshwar-sundar-glean wants to merge 1 commit into
Conversation
Addresses Swarup's review on #3: - [High] Approval gate fails open on unknown metadata (run-tool.ts): the gate keyed on `toolMeta?.requires_approval`, so a missing/unparseable tool JSON (evicted by evictStaleSkills, called from memory without a fresh discover, or corrupt) made it falsy and the tool ran with zero approval while the native prompt was already suppressed. Now fail CLOSED — only skip the gate when requires_approval is explicitly false. - [High] Auto-approve hook matcher too broad (hooks.json, auto-approve-run-tool.mjs): `mcp__.*glean.*run_tool` / includes("glean") matched any *glean* server, so a user-connected REMOTE glean server's run_tool got auto-approved (native prompt suppressed, our elicitation never runs for it). Scope to the exact glean-local server segment. - [Med] OAuth state CSRF check was dead code (auth-provider.ts): the SDK never set a `state` param so expectedState was always undefined. Generate a state nonce on the authorize URL so the loopback callback's check is live. - [Med] Concurrent token refresh could wipe tokens (remote-client.ts): serialize connect/finishAuth behind a shared in-flight lock so parallel handlers don't race the SDK refresh and trip invalidateCredentials on refresh-token rotation. - [Med] Approval-prompt line injection via argument keys (approval-args.ts): keys were interpolated raw. Sanitize keys like values and disclose omitted args with a "(+N more)" line. - [Med] Windows SKILL.md reference dropped (skill-writer.ts): match SKILL.md under either path separator. - [Med] No CI ran the suite (.github/workflows/test.yml): run typecheck:bundle + test:bundle + build/validate on PRs and pushes to main. Note: [High] "hook shipped to cursor" is already resolved — cursor's target components exclude `hooks` (and glean-vnext), so the hook is claude-only. typecheck:bundle clean; test:bundle 194/194 (5 new); npm test 62/65/56.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the review feedback from @swarup-padhi-glean on #3 (merged) — a set of security hardening fixes to the
glean-vnextruntime server, plus the missing PR-triggered CI.Fixes (mapped to review threads)
run-tool.tstoolMeta?.requires_approval, so a missing/unparseable tool JSON (evicted byevictStaleSkills, called from memory without a fresh discover, or corrupt) made it falsy → the tool executed with zero approval while the native prompt was already suppressed viareadOnlyHint. Now fail closed: only skip the gate whenrequires_approvalis explicitlyfalse.hooks.json,auto-approve-run-tool.mjsmcp__.*glean.*run_tool/includes("glean")matched any server containing "glean" — including a user-connected remotegleanserver, whoserun_toolwould be auto-approved (native prompt suppressed) whileglean-local's elicitation never runs for it. Now matches the exactglean-local__run_toolsegment.componentsexcludehooks(andglean-vnext), so the auto-approve hook is claude-only. Noted for completeness.auth-provider.tsstateCSRF check made live. The SDK never set astateparam, soexpectedStatewas alwaysundefinedand the loopback callback's check was dead code. Generate astatenonce on the authorize URL; the server echoes it back and the callback verifies it.remote-client.tsrefresh_token; with rotation, the loser getsinvalid_grant→invalidateCredentials("tokens")wipes the just-saved tokens.connect/finishAuthnow run behind a shared in-flight lock, so the second reads fresh tokens and skips refresh.approval-args.ts(+N more)line.skill-writer.tsendsWith("/SKILL.md")dropped the SKILL.md reference on Windows (\separator); match either separator..github/workflows/test.ymlpublish.ymlonly runs on release. New workflow runstypecheck:bundle+test:bundle+npm test(build/validate) on PRs and pushes tomain.Testing
npm run typecheck:bundle— cleannpm run test:bundle— 194/194 (5 new: fail-closed accept/decline, arg-key injection, remote-glean-not-approved, prefixed-form matches)npm test— build + validate green (cursor 62 / claude 65 / codex 56)Open question
Replying separately on the
targets/codex/.mcp.jsonthread re: why codex needs a per-target override.