Skip to content

feat(cli): add secure login and tool listing - #1024

Merged
helpcodeai merged 9 commits into
HelpCode-ai:mainfrom
Coding-Professional:feat/1009-cli-login-list
Oct 14, 2026
Merged

helpcodeai merged 9 commits into
HelpCode-ai:mainfrom
Coding-Professional:feat/1009-cli-login-list

Conversation

@Coding-Professional

@Coding-Professional Coding-Professional commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds the first phase of #1009: an amcp CLI with login, logout, whoami, and tools ls. It lists tools from /mcp/<serverId> on both self-hosted and Cloud instances.

Changes

  • Reads the API key from a hidden prompt, stdin, or AMCP_API_KEY; never accepts it as a command argument. Saves named profiles atomically with mode 0600 on Unix.
  • Rejects redirects and non-loopback HTTP. A URL override requires AMCP_API_KEY so a saved key cannot be sent to another origin. Terminates remote MCP sessions after each command.
  • Handles usage errors, connection diagnostics, and untrusted tool text safely. Adds CLI tests to CI and keeps test files out of the production build.
  • Wires the CLI workspace into the Docker build and documents setup in 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/cli and npm run lint -w packages/cli: pass.
  • Adapter validation: 375 passed, 0 failed; quoted adapter count check passes.
  • GitHub CI includes CLI, backend, frontend, e2e, Docker build, and CodeQL checks.

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 call and --json are planned for a follow-up PR.

Comment thread packages/cli/src/config.ts Fixed
@Coding-Professional
Coding-Professional marked this pull request as ready for review October 11, 2026 05:09
Copilot AI balanced review requested due to automatic review settings October 11, 2026 05:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@helpcodeai helpcodeai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. The key is sent along on cross-origin redirects (src/mcp.ts). fetch follows redirects and only strips Authorization, not X-API-Key. A 307 to another origin (or from https to http) sends the key there. Use requestInit: { 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.
  2. Plain http:// is accepted for any host (src/mcp.ts). Allow http only for localhost, 127.0.0.0/8 and ::1; anything else should be refused (or need an explicit --allow-insecure-http).
  3. A stored key can go to a different host (src/index.ts). tools ls --url https://other.host, or a leftover AMCP_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 from AMCP_API_KEY as well.
  4. Sessions are never closed on the server. client.close() only aborts locally, so every amcp tools ls leaves a stateful session that lives for 30 minutes and counts toward the instance's session cap. Call await transport.terminateSession().catch(() => {}) before client.close().

Nice to have, here or in part 2

  • Usage errors should exit with 2 as the issue says (invalid --profile, --url without a value, the ERR_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/error in the raw prompt, bump @modelcontextprotocol/client to ^2.2.0, ignore a relative XDG_CONFIG_HOME, and keep the test file out of dist.

Also, if you can, try it once against a local docker compose instance and say so in the PR.

@Coding-Professional

Copy link
Copy Markdown
Contributor Author

@helpcodeai

Thank you for the review and clarification! I will work on the suggested changes and update the PR once they are completed.

@Coding-Professional

Copy link
Copy Markdown
Contributor Author

@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 helpcodeai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@helpcodeai
helpcodeai merged commit ddf4c46 into HelpCode-ai:main Oct 14, 2026
14 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 14, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants