Strengthen session hijacking prevention types and tests#4067
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b99f3c96f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR refactors the session hijacking prevention wrapper to enforce the sessiontypes.MultiSession contract at compile time and updates tests to validate the behavior through the decorator’s public methods rather than its internal helper.
Changes:
- Change
PreventSessionHijackingto acceptsessiontypes.MultiSessiondirectly (removing the runtime type assertion). - Update edge-case tests to exercise
CallTool,ReadResource, andGetPromptend-to-end through the decorator. - Remove the concurrent validation unit test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/vmcp/session/internal/security/security.go | Tightens the PreventSessionHijacking API to take MultiSession directly and removes the runtime type assertion logic. |
| pkg/vmcp/session/internal/security/hijack_prevention_test.go | Adjusts mocks and rewrites validation edge-case tests to call decorated methods instead of validateCaller; removes concurrency test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4067 +/- ##
==========================================
- Coverage 68.56% 68.53% -0.03%
==========================================
Files 446 446
Lines 45574 45573 -1
==========================================
- Hits 31246 31235 -11
- Misses 11914 11926 +12
+ Partials 2414 2412 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@yrobla Approved. Could you please check failing e2e tests. I re-ran one of those. |
- Change PreventSessionHijacking to accept sessiontypes.MultiSession
directly instead of interface{}, enforcing the contract at compile
time and removing the runtime type assertion
- Update tests to call the decorated methods (CallTool, ReadResource,
GetPrompt) rather than the internal validateCaller, verifying that
validation is correctly integrated end-to-end
- Remove TestConcurrentValidation as the decorator holds no mutable
state, making a concurrency test unnecessary
Summary
Fixes #3867
Type of change
Test plan
task test)task test-e2e)task lint-fix)Changes
Affected: pkg/vmcp/session/internal/security
Does this introduce a user-facing change?
Special notes for reviewers