Load MCP Registry token from GCP Secret Manager - #625
Conversation
The registry ties the io.github.wonderwhy-er/* namespace to the GitHub identity that authenticates, not to repo collaborator status, so a second maintainer cannot publish with their own account. Share a scoped PAT (read:org + read:user) through GCP Secret Manager instead. - Add getMcpToken(): MCP_GITHUB_TOKEN env -> gcloud secret -> null - Add mcpLogin(): prefer shared token, fall back to interactive device flow - Pre-flight now auto-refreshes an expired registry session when a shared token is available instead of failing the release - Step 6 retry loop reuses mcpLogin() for its 401 recovery - Redact the token from any login failure message; execSync embeds the full command in error.message, which would otherwise leak the PAT into terminal scrollback and CI logs - Downgrade missing/expired session from error to warning, since it is now a recoverable condition - Document the auth chain and the per-member IAM grant in --help Secret location overridable via MCP_SECRET_NAME / MCP_SECRET_PROJECT. Behaviour is unchanged when no token is present anywhere.
📝 WalkthroughWalkthroughThe release script adds MCP Registry credential resolution from environment variables or GCP Secret Manager. It refreshes missing or expired sessions during pre-flight checks and reuses shared authentication during publish retries. ChangesMCP Registry authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseScript as publish-release.cjs
participant SecretManager as GCP Secret Manager
participant MCPPublisher as mcp-publisher
participant Registry as MCP Registry
ReleaseScript->>SecretManager: Resolve shared token when needed
SecretManager-->>ReleaseScript: Return token
ReleaseScript->>MCPPublisher: Refresh or perform login
MCPPublisher-->>ReleaseScript: Return session status
ReleaseScript->>Registry: Retry MCP publish
Registry-->>ReleaseScript: Return publish result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/publish-release.cjs`:
- Around line 142-145: Update the MCP_GITHUB_TOKEN handling in the token-loading
function to trim the environment value before checking it, and only cache and
return the normalized token when it is non-empty. Treat whitespace-only values
as absent so the existing GCP Secret Manager fallback executes.
- Around line 154-156: The release script currently interpolates
credential-derived values into shell command strings. Update the command
execution in the token retrieval flow and the mcp-publisher invocation to use
execFileSync or spawnSync with separate argv arguments, preserving existing
commands and behavior while preventing secrets from being shell-interpreted.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28595034-9415-41b9-b74e-52e9e5c74a9a
📒 Files selected for processing (1)
scripts/publish-release.cjs
| if (process.env.MCP_GITHUB_TOKEN) { | ||
| printInfo('Using MCP Registry token from MCP_GITHUB_TOKEN'); | ||
| cachedMcpToken = process.env.MCP_GITHUB_TOKEN.trim(); | ||
| return cachedMcpToken; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ignore a whitespace-only environment token.
MCP_GITHUB_TOKEN=" " passes the current truthy check, then becomes an empty cached value. This suppresses the GCP Secret Manager fallback. Trim once before the condition and use the normalized value only when it is non-empty.
Proposed fix
- if (process.env.MCP_GITHUB_TOKEN) {
+ const envToken = process.env.MCP_GITHUB_TOKEN?.trim();
+ if (envToken) {
printInfo('Using MCP Registry token from MCP_GITHUB_TOKEN');
- cachedMcpToken = process.env.MCP_GITHUB_TOKEN.trim();
+ cachedMcpToken = envToken;
return cachedMcpToken;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (process.env.MCP_GITHUB_TOKEN) { | |
| printInfo('Using MCP Registry token from MCP_GITHUB_TOKEN'); | |
| cachedMcpToken = process.env.MCP_GITHUB_TOKEN.trim(); | |
| return cachedMcpToken; | |
| const envToken = process.env.MCP_GITHUB_TOKEN?.trim(); | |
| if (envToken) { | |
| printInfo('Using MCP Registry token from MCP_GITHUB_TOKEN'); | |
| cachedMcpToken = envToken; | |
| return cachedMcpToken; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/publish-release.cjs` around lines 142 - 145, Update the
MCP_GITHUB_TOKEN handling in the token-loading function to trim the environment
value before checking it, and only cache and return the normalized token when it
is non-empty. Treat whitespace-only values as absent so the existing GCP Secret
Manager fallback executes.
| const token = execSilent( | ||
| `gcloud secrets versions access latest --secret=${GCP_SECRET_NAME} --project=${GCP_PROJECT_ID} 2>/dev/null`, | ||
| { ignoreError: true } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the local command wrapper and all credential-derived command construction.
rg -n -C 12 'function exec\b|const exec\b|let exec\b|exec\s*=' scripts/publish-release.cjs
rg -n -C 5 'gcloud secrets versions access|mcp-publisher login github' scripts/publish-release.cjsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 7181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the imports and top-level command construction to determine the exec wrapper behavior.
sed -n '1,110p' scripts/publish-release.cjsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 2862
🌐 Web query:
Node child_process execSync string command shell option windows command argument vector array documentation
💡 Result:
In Node.js, the child_process.execSync(command, options) function is a synchronous method that executes a command by spawning a shell [1][2]. The shell option in execSync is used to specify the shell to execute the command with [1][3]. Unlike execFile, which executes a file directly, execSync is designed to run commands through a shell [1][4]. Key details regarding the shell option and command execution: 1. Behavior: When shell is enabled (default is true for exec/execSync), the provided command string is executed inside a shell [1][2]. 2. Default Shell: - On Unix, it defaults to /bin/sh [1][3]. - On Windows, it defaults to the value of process.env.ComSpec (typically cmd.exe) [1][3][5]. 3. Customization: A different shell can be specified as a string value for the shell option [1]. 4. Security Warning: Because execSync runs commands through a shell, passing unsanitized user input to the command string can lead to shell injection vulnerabilities [1][4]. 5. Argument Handling: Because execSync processes the entire command as a string, it does not support passing arguments as an array [2][6]. If you need to pass arguments securely, you should use child_process.execFileSync or child_process.spawnSync, which support passing arguments as a separate array and do not require shell invocation by default [2][4][6]. On Windows specifically, the windowsVerbatimArguments option (boolean) can be used to prevent automatic quoting or escaping of arguments when the shell is not used; however, when the shell is set to cmd.exe, this option is generally ignored or handled by the shell itself [3]. The combination of using execSync and a shell means that the command string is interpreted by the shell, making it distinct from methods that treat input as a direct argument vector [6].
Citations:
- 1: https://nodejs.org/api/child_process.html
- 2: https://2ality.com/2022/07/nodejs-child-process.html
- 3: https://github.com/nodejs/node/blob/main/doc/api/child_process.md
- 4: https://nodejs.org/learn/concurrency/comparing-nodejs-concurrency-models
- 5: nodejs/node@4be373bc4b
- 6: https://www.nazarboyko.com/articles/when-to-use-child-processes-in-nodejs
Do not pass credential-derived values through execSync command strings.
exec calls execSync(command) with a shell command string. The gcloud invocation at lines 154-156 interpolates secret overrides, and the mcp-publisher invocation at line 184 interpolates the resolved PAT. Use a non-shell process API, such as execFileSync/spawnSync, with argv so secret data is not interpreted by the shell.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/publish-release.cjs` around lines 154 - 156, The release script
currently interpolates credential-derived values into shell command strings.
Update the command execution in the token retrieval flow and the mcp-publisher
invocation to use execFileSync or spawnSync with separate argv arguments,
preserving existing commands and behavior while preventing secrets from being
shell-interpreted.
1 participant
Problem
The MCP Registry grants publish rights for
io.github.wonderwhy-er/*based on the GitHub identity that authenticates, not on repo collaborator status. Sincewonderwhy-eris a personal account rather than an org, there is nothing for a second maintainer to be a member of — @edgarsskore logging in with his own account getsio.github.edgarsskore/*and cannot publish this server.That has made registry publishing a single-person bottleneck tied to one laptop's interactive login.
Approach
Share a scoped PAT (
read:org+read:user) through GCP Secret Manager in the project we already use for telemetry (airy-runway-456110-g5), rather than passing a token around by hand.Resolution order in
publish-release.cjs:MCP_GITHUB_TOKENenv var — CI or ad-hoc overridemcp-registry-pat) viagcloud— normal team pathmcp-publisher login github— unchanged fallbackSecret location is overridable with
MCP_SECRET_NAME/MCP_SECRET_PROJECT.Changes
getMcpToken()— resolves the PAT through the chain above, cached, never throws, never prints the token.mcpLogin()— prefers the shared token, falls back to the browser device flow.mcpLogin(), so it works unattended.execSyncembeds the full command inerror.message, so a failed login printed the PAT verbatim. Caught during testing; the token is now scrubbed before the message surfaces.--help— documents the auth chain and the per-member IAM grant.Setup (not done by this PR)
Mint the PAT as fine-grained with an expiry and only
read:org+read:user. Nothing else in the release path needs it.Testing
Verified on macOS against the real
mcp-publisher(v1.6.0):node --checkclean;npm run buildpasses--helprenders the new sectionNot yet exercised end-to-end with a real PAT, since the secret does not exist yet.
Notes
wonderwhy-eras publisher regardless of who runs it. Secret Manager controls distribution and gives an access audit trail; it does not change attribution. Scoping the PAT tightly is what bounds the blast radius.desktop-commanderorg, revisit.src/server.ts:221had a stray double-space (isWe lcomePageEligibleClient) that broketsc, while line 226 still used the correct name. Reverted so the build would pass — that file is not part of this diff. Worth checking it was not mid-edit intentionally.mcp-publisheris at v1.6.0 locally; v1.8.0 is available. Unrelated to this PR, but note CVE-2026-44428 (OIDC token replay across registry deployments) was fixed registry-side in 1.7.6.Summary by CodeRabbit
New Features
Bug Fixes