Skip to content

fix: bind client to localhost to match server #529

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 26, 2025

Conversation

felixweinberger
Copy link
Contributor

@felixweinberger felixweinberger commented Jun 18, 2025

Bind client to localhost instead of all interfaces to match server

Motivation and Context

Complete the security hardening started in e8e9909 by also binding the client to localhost only.
Previously only the server was protected while the client remained exposed to the network,
allowing attackers to access the server through the client as a proxy.

Changes:

  • Add HOST environment variable support to client (prod mode)
  • Configure Vite dev server to bind to localhost by default
  • Update browser auto-open URLs to use actual host instead of hardcoded 127.0.0.1
  • Fix missing cancelled parameter in startProdClient function
Before After
CleanShot 2025-06-18 at 17 56 03@2x CleanShot 2025-06-18 at 17 59 25@2x

How Has This Been Tested?

prod: npm run build && npm run start - works
dev: npm run dev - works
test: npm test - works

Breaking Changes

There may be instances where clients were inadvertently relying on clients binding to *:6274 in their setup - e.g. if accessing the inspector UI via a remotely hosted machine or similar.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@felixweinberger felixweinberger changed the base branch from main to fweinberger/auto-open June 18, 2025 17:02
@felixweinberger felixweinberger force-pushed the fweinberger/client-localhost branch from 0345463 to bdfc9e7 Compare June 18, 2025 17:08
@felixweinberger felixweinberger marked this pull request as ready for review June 18, 2025 17:11
@felixweinberger felixweinberger force-pushed the fweinberger/client-localhost branch from bdfc9e7 to 58661be Compare June 18, 2025 17:21
Base automatically changed from fweinberger/auto-open to main June 18, 2025 22:13
Copy link
Contributor

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

Suggested a simple refactor of some duplicated lines.

Also... can we align the way we refer to the loopback, either make the link with the token be 127.0.0.1 instead of localhost (or make the host in the client and start script be localhost instead of 127.0.0.1).

Screenshot 2025-06-18 at 6 21 37 PM

Previously we were using 127.0.0.1 since it refers directly to the loopback and doesn't require DNS lookup, thus being slightly safer. However 127.0.0.1 is an IPv4 specific address and in IPv6 environments the loopback is ::1 and so could possibly lead to ECONNREFUSED. So localhost is arguably better for that purpose. An attacker would have to edit your hosts file to redirect 127.0.0.1 to an evil address, so you'd already have to have some compromise.

@felixweinberger felixweinberger force-pushed the fweinberger/client-localhost branch 3 times, most recently from 4a0225e to f1525aa Compare June 19, 2025 18:26
Copy link
Contributor

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

Added suggestions to fix a couple of PORT remnants from before we codified SERVER_PORT and CLIENT_PORT env vars. Below is how we want to control ports:

Screenshot 2025-06-19 at 4 25 19 PM

NOTE: Another (prior) issue exists where the client looks only for DEFAULT_MCP_PROXY_LISTEN_PORT unless MCP_PROXY_FULL_ADDRESS is set in config. A bit of a chicken/egg problem anyway since the config is fetched from the proxy's /config address. Thus if the proxy server is started on a port other than default, the client won't be able to find it.

Clearly that part needs to be revisited, since we now have a MCP_PROXY_TOKEN on the querystring, but that's for another PR, unless you feel like tackling it in this one. If SERVER_PORT was set in the environment (i.e., is not the default), we need to add it to the querystring and used in place of DEFAULT_MCP_PROXY_LISTEN_PORT in the client.

ochafik
ochafik previously approved these changes Jun 20, 2025
Copy link

@ochafik ochafik left a comment

Choose a reason for hiding this comment

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

lgtm (w/ PORT rename suggestion)

@cliffhall cliffhall added the waiting on submitter Waiting for the submitter to provide more info label Jun 20, 2025
@felixweinberger felixweinberger force-pushed the fweinberger/client-localhost branch from 585978c to af206ed Compare June 20, 2025 23:14
@felixweinberger
Copy link
Contributor Author

felixweinberger commented Jun 20, 2025

Added suggestions to fix a couple of PORT remnants from before we codified SERVER_PORT and CLIENT_PORT env vars. Below is how we want to control ports:

Amended in the latest commit, great catch!

CleanShot 2025-06-21 at 00 19 26@2x

NOTE: Another (prior) issue exists where the client looks only for DEFAULT_MCP_PROXY_LISTEN_PORT unless MCP_PROXY_FULL_ADDRESS is set in config. A bit of a chicken/egg problem anyway since the config is fetched from the proxy's /config address. Thus if the proxy server is started on a port other than default, the client won't be able to find it.

I'll pick this up in a follow-up PR.

@felixweinberger felixweinberger force-pushed the fweinberger/client-localhost branch from af206ed to ab5d053 Compare June 20, 2025 23:21
@felixweinberger felixweinberger requested review from cliffhall and removed request for jenn-newton, jerome3o-anthropic and petery-ant June 20, 2025 23:23
felixweinberger and others added 3 commits June 24, 2025 10:43
…acks

Complete the security hardening started in e8e9909 by also binding the client to localhost only.
Previously only the server was protected while the client remained exposed to the network,
allowing attackers to access the server through the client as a proxy.

Changes:
- Add HOST environment variable support to client (prod mode)
- Configure Vite dev server to bind to localhost by default
- Update browser auto-open URLs to use actual host instead of hardcoded 127.0.0.1
- Fix missing cancelled parameter in startProdClient function

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Extract duplicated URL generation code into getClientUrl() helper function in start.js
- Replace all 127.0.0.1 references with localhost for consistency across codebase
- Update server to respect HOST environment variable for URL generation
- Remove 127.0.0.1 from default allowed origins in CORS configuration
- Update documentation to use localhost instead of 127.0.0.1

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Per PR feedback from cliffhall:
- Changed PORT to SERVER_PORT in server/src/index.ts
- Changed PORT to CLIENT_PORT in client/bin/client.js
- Added parseInt() for CLIENT_PORT consistency
- Updated start.js to pass correct env var names

This improves clarity by using specific environment variables for
server and client ports instead of the generic PORT variable.
@felixweinberger felixweinberger force-pushed the fweinberger/client-localhost branch from ab5d053 to d0673ba Compare June 24, 2025 09:45
Copy link

github-actions bot commented Jun 24, 2025

🎭 Playwright E2E Test Results

✅  12 passed

Details

12 tests across 1 suite
 22 seconds
 b28a10e
ℹ️  Test Environment: Ubuntu Latest, Node.js 18
Browsers: Chromium, Firefox

📊 View Detailed HTML Report (download artifacts)

@felixweinberger
Copy link
Contributor Author

NOTE: Another (prior) issue exists where the client looks only for DEFAULT_MCP_PROXY_LISTEN_PORT unless MCP_PROXY_FULL_ADDRESS is set in config. A bit of a chicken/egg problem anyway since the config is fetched from the proxy's /config address. Thus if the proxy server is started on a port other than default, the client won't be able to find it.

I'll pick this up in a follow-up PR.

Addressed in #555 (cc @cliffhall)

@felixweinberger felixweinberger requested a review from ochafik June 24, 2025 19:15
@cliffhall cliffhall removed the waiting on submitter Waiting for the submitter to provide more info label Jun 25, 2025
@cliffhall cliffhall dismissed their stale review June 25, 2025 21:57

Fixed in #555

Copy link

@ochafik ochafik left a comment

Choose a reason for hiding this comment

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

Thanks Felix!

@felixweinberger felixweinberger merged commit eb5be0d into main Jun 26, 2025
7 checks passed
@felixweinberger felixweinberger deleted the fweinberger/client-localhost branch June 26, 2025 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants