Skip to content

Conversation

@SamMorrowDrums
Copy link
Contributor

Adds a section in the security best practices document on potential for session hijacking, and mitigations

Motivation and Context

With a horizontally scaled http server, there is a potential for session hijacking with incorrect implementation.

How Has This Been Tested?

Documentation only.

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

@localden localden added security documentation Improvements or additions to documentation enhancement New feature or request labels Jun 3, 2025
@connor4312
Copy link
Contributor

It may be worth adding a bullet point under no.1 in session management like:

  • The client MUST treat the session ID as a credential and handle it in a secure manner.

(I'm not sure if MUST/SHOULD verbiage is compatible with the fuzzy requirement of treating it in a 'secure manner')

MCP servers **SHOULD** bind session keys to user-specific information.
When storing or transmitting session-related data (e.g., in a queue), combine the session ID with information unique to the authorized user, such as their internal user ID. Use a key format like `<user_id>:<session_id>`. This ensures that even if an attacker guesses a session ID, they cannot impersonate another user as the user ID is derived from the user token and not provided by the client.

MCP servers can optionally leverage additional unique identifiers.
Copy link
Contributor

@connor4312 connor4312 Jun 4, 2025

Choose a reason for hiding this comment

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

IP address binding may not generally best practice as the IP address from users on or tethering to mobile devices is apt to change frequently when roaming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's admittedly hard to find good alternatives if the user isn't logged in. I did hesitate for this, as it does cause false negatives. I just wanted to add that there can be other data which can work. I'm happy to drop it if it doesn't add much.

Also, according to the spec, if you try to use a session ID and the session is no longer valid and your request is rejected, you get foreced to re-initialize. That can break the state on a stateful server though, so not exactly ideal, but for most scenarios it would just continue to work.

Choose a reason for hiding this comment

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

I was about to add LGTM but tripped over the same thoughts on IP address in the concluding paragraph. Using an IP address in this way is such an unreliable anti-pattern that the mention of it could raise doubt on the credibility of the remainder of the guidance, which is otherwise solid. If it's possible to remove this example, the rest looks good.

Copy link

@vineethsai vineethsai left a comment

Choose a reason for hiding this comment

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

LGTM

SamMorrowDrums and others added 2 commits June 11, 2025 21:50
Co-authored-by: Den Delimarsky 🌺 <53200638+localden@users.noreply.github.com>
Co-authored-by: Den Delimarsky 🌺 <53200638+localden@users.noreply.github.com>
@SamMorrowDrums SamMorrowDrums requested a review from localden June 11, 2025 20:00
localden
localden previously approved these changes Jun 11, 2025
halter73
halter73 previously approved these changes Jun 12, 2025

#### 2.3.2 Session Highjack Impersonation

```mermaid
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move the diagram so that it's next to the description? I also wonder if we should have the "Session Hijack Impersonation" first since it's the simpler variation.

Co-authored-by: Stephen Halter <halter73@gmail.com>
@localden localden enabled auto-merge June 14, 2025 03:42
@localden localden merged commit 78283c8 into modelcontextprotocol:main Jun 16, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants