Repository navigation
feat(cli): add secure login and tool listing - #1024
Conversation
helpcodeai
left a comment
There was a problem hiding this comment.
Nice work @Coding-Professional, this is a careful first part: the key never goes on the command line, the config file is written atomically with 0600, the endpoint is always /mcp/<serverId>, and error messages don't leak the key. Four things to fix before merge, two of them security:
Must fix
- The key is sent along on cross-origin redirects (
src/mcp.ts).fetchfollows redirects and only stripsAuthorization, notX-API-Key. A 307 to another origin (or from https to http) sends the key there. UserequestInit: { redirect: 'error', headers: { 'X-API-Key': profile.key } }, and add a test where the server redirects to a second server that must never see the key. - Plain
http://is accepted for any host (src/mcp.ts). Allow http only forlocalhost,127.0.0.0/8and::1; anything else should be refused (or need an explicit--allow-insecure-http). - A stored key can go to a different host (
src/index.ts).tools ls --url https://other.host, or a leftoverAMCP_URL, combines the overridden URL with the profile's saved key. Use the stored key only when the URL actually used has the same origin as the profile. When the URL comes from a flag or env var, require the key fromAMCP_API_KEYas well. - Sessions are never closed on the server.
client.close()only aborts locally, so everyamcp tools lsleaves a stateful session that lives for 30 minutes and counts toward the instance's session cap. Callawait transport.terminateSession().catch(() => {})beforeclient.close().
Nice to have, here or in part 2
- Usage errors should exit with 2 as the issue says (invalid
--profile,--urlwithout a value, theERR_PARSE_ARGS_*codes). Today they exit with 4. - Don't fail env-only mode (
AMCP_URL,AMCP_SERVER_ID,AMCP_API_KEY) when the config file is corrupt. - Say why a connection failed: the HTTP status, or the
cause.code(ENOTFOUND, ECONNREFUSED, CERT_*), without the raw message. Accept a pasted/mcp/<id>URL, or show the "pass the base URL" hint instead of swallowing it. - Strip control characters from tool names and descriptions before printing; they come from bridged third-party servers.
- Run the CLI tests in CI (
npm test -w packages/cli). - Prompt on stderr, handle stdin
end/errorin the raw prompt, bump@modelcontextprotocol/clientto^2.2.0, ignore a relativeXDG_CONFIG_HOME, and keep the test file out ofdist.
Also, if you can, try it once against a local docker compose instance and say so in the PR.
|
Thank you for the review and clarification! I will work on the suggested changes and update the PR once they are completed. |
|
@helpcodeai Thank you for the detailed review. I have addressed all four must-fix points: redirects are rejected before the API key can be forwarded, HTTP is limited to loopback addresses, URL overrides require AMCP_API_KEY instead of reusing a stored key, and remote MCP sessions are terminated before closing the client. I also added regression tests and the CLI CI check. The branch is synced with current main, and all PR checks, including CLI tests and Docker build, are green. Could you please re-review when you have a chance? |
helpcodeai
left a comment
There was a problem hiding this comment.
Checked the new commits and all four must-fix points are done: redirect: 'error' with a test that a second server never sees the key, http only on loopback, URL overrides need AMCP_API_KEY, and the session gets terminated before close. Most of the nice-to-haves made it in too (exit codes, control chars stripped, CLI tests in CI, client 2.2). Approving and merging. Part 2 (tools call and --json) can go in a new PR against #1009 whenever you're ready.
Summary
Adds the first phase of #1009: an
amcpCLI withlogin,logout,whoami, andtools ls. It lists tools from/mcp/<serverId>on both self-hosted and Cloud instances.Changes
AMCP_API_KEY; never accepts it as a command argument. Saves named profiles atomically with mode0600on Unix.AMCP_API_KEYso a saved key cannot be sent to another origin. Terminates remote MCP sessions after each command.packages/cli/README.md.Validation
npm test -w packages/cli: 9 tests pass, including redirect, key isolation, session termination, and HTTP restrictions.npm run build -w packages/cliandnpm run lint -w packages/cli: pass.Local
docker build .and live Cloud/self-hosted key checks were unavailable because the Docker daemon and test credentials were not available. The in-process MCP tests cover the request and authentication paths.This is the maintainer-suggested first phase.
tools calland--jsonare planned for a follow-up PR.