Support loading the MCP Registry token from a secret manager - #626
Support loading the MCP Registry token from a secret manager#626wonderwhy-er wants to merge 2 commits into
Conversation
The registry grants publish rights based on the GitHub identity that authenticates rather than repo access, so a second maintainer cannot publish using their own account. Allow the release script to pick up a shared credential instead of requiring an interactive login. - Resolve the token from MCP_GITHUB_TOKEN, then a configured secret, then fall back to interactive login as before - Auto-refresh an expired registry session when a credential is available, instead of failing the release outright - Reuse the same login path for the existing 401 retry in the publish step so it can run unattended - Redact the credential from login failure messages; execSync embeds the full command in error.message, which would otherwise print it - Downgrade missing/expired session from error to warning, as it is now recoverable Configuration lives in .release-config.json (gitignored) or the environment, so no environment-specific identifiers are committed. See .release-config.example.json. With nothing configured the script behaves exactly as it did before.
📝 WalkthroughWalkthroughThe release tooling now supports shared MCP Registry credentials from environment variables, local configuration, or Google Secret Manager. Pre-flight checks and publish retries use automatic session refresh when credentials are available. ChangesMCP Registry release authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseScript
participant CredentialSources
participant MCPRegistry
ReleaseScript->>CredentialSources: Resolve MCP Registry credentials
CredentialSources-->>ReleaseScript: Return cached token or interactive-login option
ReleaseScript->>MCPRegistry: Refresh authentication session
MCPRegistry-->>ReleaseScript: Return session status
ReleaseScript->>MCPRegistry: Retry publish with authenticated session
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: 3
🤖 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 @.release-config.example.json:
- Line 2: Update the _comment description to state that interactive registry
login occurs only when neither file-based credentials from .release-config.json
nor environment credentials such as MCP_GITHUB_TOKEN or MCP_SECRET_NAME are
configured.
In `@scripts/publish-release.cjs`:
- Around line 148-151: Update the MCP_GITHUB_TOKEN handling in the
token-resolution flow to trim the environment value before accepting or caching
it, and only return it when the trimmed value is non-empty. For blank or
whitespace-only values, continue to secret-manager resolution instead of setting
cachedMcpToken or returning early.
- Around line 180-182: Replace shell-interpolated command strings in
getMcpToken() and mcpLogin() with execFileSync() or an equivalent argument-array
API. Pass secretName and project as separate gcloud arguments, and pass token as
a separate argument to mcp-publisher login github so controlled values are never
interpreted by a shell.
🪄 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: ce1a1dd6-1928-401e-b1ad-f72339b60f8f
📒 Files selected for processing (3)
.gitignore.release-config.example.jsonscripts/publish-release.cjs
| @@ -0,0 +1,5 @@ | |||
| { | |||
| "_comment": "Copy to .release-config.json (gitignored) and fill in. Optional: without it, registry login is interactive.", | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the credential fallback description.
Line 2 says that login is interactive without .release-config.json. This is not true when MCP_GITHUB_TOKEN or MCP_SECRET_NAME is configured in the environment. State that interactive login occurs only when no environment or file-based credential source is configured.
🤖 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 @.release-config.example.json at line 2, Update the _comment description to
state that interactive registry login occurs only when neither file-based
credentials from .release-config.json nor environment credentials such as
MCP_GITHUB_TOKEN or MCP_SECRET_NAME are configured.
| 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 blank MCP_GITHUB_TOKEN.
Line 148 accepts a whitespace-only environment value. Line 150 then caches an empty token and prevents secret-manager resolution. A CI job with MCP_GITHUB_TOKEN="" will fail instead of using the configured shared secret.
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 148 - 151, Update the
MCP_GITHUB_TOKEN handling in the token-resolution flow to trim the environment
value before accepting or caching it, and only return it when the trimmed value
is non-empty. For blank or whitespace-only values, continue to secret-manager
resolution instead of setting cachedMcpToken or returning early.
| const token = execSilent( | ||
| `gcloud secrets versions access latest --secret=${secretName} --project=${project} 2>/dev/null`, | ||
| { ignoreError: true } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the existing child-process wrapper and all credential-bearing commands.
rg -n -C 3 'execSync|execFileSync|function exec\(|gcloud secrets versions access|mcp-publisher login github' \
scripts/publish-release.cjs
# Inspect declared Node.js version sources before introducing execFileSync().
fd -HI '^(package\.json|\.nvmrc|\.node-version|\.tool-versions)$' . -x sh -c \
'echo "==> $1"; sed -n "1,160p" "$1"' sh {}Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 10171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the secret/config values and release config path used around the credential commands.
sed -n '140,225p' scripts/publish-release.cjs
printf '\n=== secret/config sources ===\n'
sed -n '225,405p' scripts/publish-release.cjsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 10507
🌐 Web query:
Node.js child_process execSync command string shell command execution execFileSync argument array
💡 Result:
In Node.js, the child_process module provides synchronous methods to execute external commands by blocking the event loop until the child process completes [1][2]. execSync(command[, options]) This method is the synchronous version of exec [1]. It spawns a shell and executes the provided command string within that shell [1]. Because it uses a shell, it is suitable for running command strings that may contain shell features like pipes, redirects, or environment variable expansions [2]. - Usage: execSync('echo hello world') - Note: Since it spawns a shell, users must be cautious when passing untrusted input to avoid command injection vulnerabilities [3]. execFileSync(file[, args][, options]) This method is the synchronous version of execFile [1]. Unlike execSync, it executes the specified file directly without spawning a shell by default [1]. It accepts an optional array of arguments (args), allowing you to separate the executable from its arguments [3]. This is generally safer and more performant than using a shell [2]. - Usage: execFileSync('node', ['--version']) - Note: It does not interpret shell-specific features (like pipes or redirects) unless you explicitly enable a shell in the options [4][3]. Key Differences - Command Execution: execSync takes a single command string intended for a shell [5]. execFileSync takes the path to an executable and an optional array of arguments [3]. - Shell Usage: execSync spawns a shell by default [1]. execFileSync does not spawn a shell by default [1]. - Safety: execFileSync is considered safer because it avoids shell interpretation, reducing the risk of shell injection attacks when handling user input [3]. - Blocking: Both methods block the Node.js event loop entirely until the child process exits [1][2]. They return the stdout of the process (or throw if the process fails) [2][6].
Citations:
- 1: https://nodejs.org/api/child_process.html
- 2: https://www.thenodebook.com/child-processes/spawn-exec-fork
- 3: http://nodejs.org/docs/latest/api/child_process.html
- 4: https://2ality.com/2022/07/nodejs-child-process.html
- 5: https://github.com/nodejs/node/blob/master/doc/api/child_process.md
- 6: https://stackoverflow.com/questions/4443597/node-js-execute-system-command-synchronously
Do not interpolate credentials or config into shell commands.
getMcpToken() and mcpLogin() build command strings with secretName, project, or token, then pass them to execSync(). A controlled value can inject shell commands. Use execFileSync() (or another argument-array API) for gcloud, and pass token through mcp-publisher login github without shell interpretation.
🤖 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 180 - 182, Replace
shell-interpolated command strings in getMcpToken() and mcpLogin() with
execFileSync() or an equivalent argument-array API. Pass secretName and project
as separate gcloud arguments, and pass token as a separate argument to
mcp-publisher login github so controlled values are never interpreted by a
shell.
Registry sessions are short-lived and a full release takes longer than one lasts, so a session created during pre-flight is always stale by the time the publish step runs. That wasted an attempt on every release and emitted expiry warnings that were never actionable. Pre-flight now only confirms a credential resolves; the session is created immediately before publishing. When no credential is configured the previous session check is retained unchanged.
2 participants
Lets the release script pick up the MCP Registry credential from a secret manager instead of requiring an interactive login, so publishing isn't tied to one machine's session.
Resolution order
MCP_GITHUB_TOKENenvironment variablegcloudmcp-publisher login githubConfiguration lives in
.release-config.json(gitignored) or the environment — see.release-config.example.json. With nothing configured, behaviour is unchanged.Changes
execSyncembeds the full command inerror.message, so a failed login printed the credential. Now redacted before the message surfaces..gitignore:.release-config.json,.release-state.json.--helpdocuments the resolution order.Testing
node --checkandnpm run buildpass. Verified against a realmcp-publisherbinary with a credential configured, with one configured but unreadable, with an invalid credential, and with nothing configured. Confirmed the credential does not appear in output on failure.Summary by CodeRabbit