Skip to content

Re-enable inspector auto-open & reloads post security fixes #513

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 18, 2025
Merged

Re-enable inspector auto-open & reloads post security fixes #513

merged 3 commits into from
Jun 18, 2025
+213 −63

Conversation

felixweinberger
Copy link
Contributor

@felixweinberger felixweinberger commented Jun 16, 2025 •
edited
Loading

Motivation and Context

We recently landed several security related changes to the inspector, including introduction of a proxy token to prevent potential remote code execution attacks.

However this introduced some friction in Dev UX, where:

  1. Inspector no longer auto-opens when running npm run start or npm run dev
  2. Code changes triggering reloads of the server in dev mode can break the client as the token is regenerated on every regen.

This PR proposes changes to restore the previous UX while retaining the security fixes. We achieve this by:

  • Generate the auth token within start.js instead of within the server (unless the server is started on its own, in which case we still generate the token directly)
  • Pass the token as an env variable to both server and client subprocesses
  • Set up client and server to use this env variable throughout any subsequent reloads.
  • Auto-start the server with the configured proxy token

How Has This Been Tested?

  • Build & run prod configuration with npm run build and npm run start and test connections, reloads, auto-start.
  • Run dev configuration with npm run dev and test connections and test connections, reloads, auto-start.
  • All existing tests pass

Dev:

CleanShot.2025-06-16.at.14.36.18-converted.2.mp4

Prod:

CleanShot.2025-06-16.at.14.47.03.mp4

Breaking Changes

None.

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 force-pushed the fweinberger/auto-open branch from 93593a4 to fb710a7 Compare June 16, 2025 13:51
@felixweinberger felixweinberger marked this pull request as ready for review June 16, 2025 13:54
cliffhall
cliffhall requested changes Jun 16, 2025
View reviewed changes
Copy link
Contributor

@cliffhall cliffhall left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The order of operations seems to have changed slightly. If the inspector is already running on the destination port, it should fail and stop.

Instead, it reports it, exists, but then auto opens anyway. In this case I was running an older version to compare functionality and it gives me that in the browser.

Screenshot 2025-06-16 at 2 20 47 PM Screenshot 2025-06-16 at 2 23 10 PM

felixweinberger and others added 3 commits June 18, 2025 10:23
@felixweinberger @claude
feat: Enable auto-open with authentication and unify start scripts
d06f3b5 
- Unified start.js to handle both dev and production modes with --dev flag
- Generate session token in parent process, pass via MCP_PROXY_TOKEN env var
- Enable browser auto-open even when authentication is enabled by including token in URL
- Auto-reloads now work seamlessly with persistent tokens across hot reloads
- Simplified npm scripts - dev and dev:windows now use the same unified script
- Better developer experience with consistent token handling

The token is generated once per session and remains stable through server/client
reloads, making development smoother while maintaining security.

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

Co-Authored-By: Claude <[email protected]>
@felixweinberger @claude
refactor: Extract server and client launch functions for better code …
9fec0aa 
…organization

- Extracted 4 functions: startDevServer, startProdServer, startDevClient, startProdClient
- Eliminated deep nesting in main() function
- Each function has a single responsibility
- Main function now clearly shows the flow: parse args → start server → start client
- No functional changes, purely organizational refactoring

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

Co-Authored-By: Claude <[email protected]>
@felixweinberger @claude
docs: Update README to reflect auto-open now works with authentication
698d245 
- Removed outdated statement about auto-open being disabled with authentication
- Clarified that browser automatically opens with token pre-filled in URL
- Updated MCP_AUTO_OPEN_ENABLED description to note it works with authentication

The README now accurately reflects the improved developer experience where
authentication no longer prevents the browser from auto-opening.

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

Co-Authored-By: Claude <[email protected]>
@felixweinberger felixweinberger force-pushed the fweinberger/auto-open branch 2 times, most recently from 32066eb to d5028bc Compare June 18, 2025 14:06
@felixweinberger felixweinberger force-pushed the fweinberger/auto-open branch from d5028bc to 837ee74 Compare June 18, 2025 14:18
@felixweinberger felixweinberger removed the request for review from cliffhall June 18, 2025 14:20
@felixweinberger felixweinberger force-pushed the fweinberger/auto-open branch 2 times, most recently from 08c2f49 to 698d245 Compare June 18, 2025 16:39
@felixweinberger
Copy link
Contributor Author

felixweinberger commented Jun 18, 2025 •
edited
Loading

@cliffhall thank you for drawing attention to this!

I spent a good chunk of time digging into this and I believe I now have a good understanding of what you're seeing here. I believe this is because the older version you were running in your terminal was still binding to all interfaces (i.e. *:6277) while the new one is on localhost:6277 (though do let me know which hash you were testing for feature comparison).

Counterintuitively, these might not actually conflict - at least they don't on my machine (macOS running Node 22.16). The reason seems to be that the wildcard *:6277 actually binds to IPv6 by default in Node, while localhost:6277 binds to IPv4. IPv4/6 are treated as different interfaces, so there's no actual conflict when running these servers.

If however you try to run 2 instances of this checkout side by side, one will fail. This is also true for the old one - but the new and the old don't conflict (in either direction!).

This change (binding to localhost) Checkout of c8016cd (before changing wildcard)
CleanShot 2025-06-18 at 17 27 40@2x CleanShot 2025-06-18 at 17 31 09@2x

So I actually think this is expected - the old version was listening on the same port but a different protocol version (IPv6 vs IPv4) so the conflicts weren't appearing as they normally should.

However while investigating this I also realized client was still listening on all interfaces by default *:6274 - so I made a separate PR to address that here: #529

cliffhall
cliffhall approved these changes Jun 18, 2025
View reviewed changes
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.

Ok verified this behavior. This version running a second time stops and does not auto-open.

@cliffhall cliffhall merged commit a7336cd into main Jun 18, 2025
6 checks passed
@cliffhall cliffhall deleted the fweinberger/auto-open branch June 18, 2025 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cliffhall cliffhall cliffhall approved these changes

@jerome3o-anthropic jerome3o-anthropic Awaiting requested review from jerome3o-anthropic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@felixweinberger @cliffhall