Add RestoreHijackPrevention and RestoreSession interface stub#4405
Add RestoreHijackPrevention and RestoreSession interface stub#4405
Conversation
…tion
Add MetadataKeyBackendSessionPrefix ("vmcp.backend.session.") constant and
extend populateBackendMetadata to write vmcp.backend.session.{workloadID} →
backend_session_id for each successfully connected backend alongside the
existing MetadataKeyBackendIDs entry.
This closes the gap identified in RC-8 (#4211): per-backend
session IDs were collected at makeSession time but never written to the
serializable transport-session metadata, so they were lost after pod restart.
With this change, the IDs flow through to Redis and are available for future
cross-pod session reconstruction.
Closes #4211
yrobla
requested review from
JAORMX,
amirejaz,
jerm-dro and
jhrozek
as code owners
There was a problem hiding this comment.
Pull request overview
Adds scaffolding to restore vMCP session hijack-prevention state from Redis-persisted metadata, enabling cross-pod token validation as part of the horizontal scaling effort (Fixes #4216).
Changes:
- Added
RestoreHijackPrevention()to reconstruct the hijack-prevention decorator from persisted token hash/salt + HMAC secret. - Extended
MultiSessionFactorywithRestoreSession()(stubbed in the default factory) and forwarded it through the decorating factory; updated mocks/test factories accordingly. - Added unit tests validating restore behavior, including cross-replica secret mismatch.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/session/internal/security/security.go | Adds RestoreHijackPrevention() helper for reconstructing token-binding decorator from persisted metadata. |
| pkg/vmcp/session/internal/security/restore_test.go | Adds unit coverage for restore behavior (nil session, malformed salt, anonymous/auth round-trip, secret mismatch). |
| pkg/vmcp/session/factory.go | Adds RestoreSession() to MultiSessionFactory, documents replica HMAC requirements, and stubs default implementation. |
| pkg/vmcp/session/decorating_factory.go | Adds RestoreSession() forwarding on the decorating factory. |
| pkg/vmcp/session/mocks/mock_factory.go | Regenerates GoMock for the interface addition. |
| pkg/vmcp/server/testfactory_test.go | Updates minimal test factory to satisfy the new interface. |
| pkg/vmcp/server/telemetry_integration_test.go | Updates backend-aware test factory to satisfy the new interface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4405 +/- ##
==========================================
+ Coverage 69.46% 69.54% +0.08%
==========================================
Files 485 485
Lines 49821 49866 +45
==========================================
+ Hits 34608 34681 +73
+ Misses 12535 12506 -29
- Partials 2678 2679 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
size/S
…4216) Add the infrastructure needed to reconstruct hijack-prevention state from Redis-persisted metadata, enabling cross-pod token validation in horizontal scaling scenarios. - security.go: Add RestoreHijackPrevention(), the restore counterpart to PreventSessionHijacking(). Rebuilds a hijackPreventionDecorator from persisted tokenHash + tokenSaltHex + hmacSecret without re-hashing a live token. Returns errors for nil session, missing salt on authenticated sessions, and invalid hex salt. - factory.go: Add RestoreSession() to the MultiSessionFactory interface with full doc comment (backend ID parsing, session hint lookup, routing-table rebuild, hijack-prevention re-application). Add a stub implementation on defaultMultiSessionFactory (returns "not yet implemented"); full reconnection logic is deferred. Document the cross-replica HMAC secret consistency requirement on defaultHMACSecret. - decorating_factory.go: Forward RestoreSession() to the base factory; decorators are not re-applied during restore. - mocks/mock_factory.go: Regenerate mock to include RestoreSession(). - restore_test.go: Unit tests covering nil session, missing salt, invalid hex, anonymous session round-trip, authenticated store→restore→validate round-trip, and cross-replica secret mismatch. Closes #4216.
github-actions
bot
added
size/M
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // We use a nil-connector session as the inner session (no real backend needed | ||
| // to test auth path). | ||
| innerSess, err := factory.MakeSessionWithID(context.Background(), uuid.New().String(), identity, false, nil) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { _ = innerSess.Close() }) | ||
|
|
||
| restored, err := security.RestoreHijackPrevention(innerSess, persistedHash, persistedSalt, hmacSecret) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
In this integration test, innerSess is created via factory.MakeSessionWithID(...), which already applies security.PreventSessionHijacking inside the factory. That means auth enforcement may be coming from the inner session even if RestoreHijackPrevention were a no-op, so the test can pass without actually validating the restore logic. Consider using an undecorated base session (e.g., construct a minimal MultiSession directly) so RestoreHijackPrevention is the only layer performing token validation.
| // RestoreSession restores a session from persisted metadata and applies the | ||
| // same decorator chain as MakeSessionWithID, ensuring consistent behavior | ||
| // between newly created and restored sessions. |
There was a problem hiding this comment.
The PR description says decorators are not re-applied during restore, but decoratingMultiSessionFactory.RestoreSession explicitly reapplies the decorator chain. Please reconcile this mismatch by either updating the PR description (if this behavior is intended) or adjusting the implementation to match the documented plan.
| sess, err := f.base.RestoreSession(ctx, id, metadata, backends) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| for _, dec := range f.decorators { | ||
| var decorated MultiSession | ||
| decorated, err = dec(ctx, sess) | ||
| if err != nil { | ||
| if closeErr := sess.Close(); closeErr != nil { | ||
| slog.Warn("failed to close session after decorator error during restore", "error", closeErr) | ||
| } | ||
| return nil, err | ||
| } | ||
| if decorated == nil { | ||
| if closeErr := sess.Close(); closeErr != nil { | ||
| slog.Warn("failed to close session after decorator returned nil during restore", "error", closeErr) | ||
| } | ||
| return nil, fmt.Errorf("decorator returned nil session without error") | ||
| } | ||
| sess = decorated | ||
| } | ||
| return sess, nil |
There was a problem hiding this comment.
RestoreSession duplicates the decorator-application loop from MakeSessionWithID with only minor differences in log messages. This duplication increases the risk that future changes (e.g., error handling tweaks) diverge between create vs restore paths; consider extracting a shared helper to apply decorators with a contextual log prefix.
jerm-dro
left a comment
jerm-dro
left a comment
There was a problem hiding this comment.
The RestoreHijackPrevention function and the RestoreSession interface definition both look good — solid tests, clean implementation, and the contract aligns with where we need to go.
Holding off on approval for now because the RestoreSession stub will need to become a real implementation as part of the #4402 rework (see feedback there on SessionDataStorage + reconstruction-on-demand). The direction here is right, but approving before the underlying storage architecture settles would be premature. Happy to re-review once #4402 and #4404 land.
github-actions
bot
added
size/L
Summary
Add the infrastructure needed to reconstruct hijack-prevention state from Redis-persisted metadata, enabling cross-pod token validation in horizontal scaling scenarios.
security.go: Add RestoreHijackPrevention(), the restore counterpart to PreventSessionHijacking(). Rebuilds a hijackPreventionDecorator from persisted tokenHash + tokenSaltHex + hmacSecret without re-hashing a live token. Returns errors for nil session, missing salt on authenticated sessions, and invalid hex salt.
factory.go: Add RestoreSession() to the MultiSessionFactory interface with full doc comment (backend ID parsing, session hint lookup, routing-table rebuild, hijack-prevention re-application). Add a stub implementation on defaultMultiSessionFactory (returns "not yet implemented"); full reconnection logic is deferred. Document the cross-replica HMAC secret consistency requirement on defaultHMACSecret.
decorating_factory.go: Forward RestoreSession() to the base factory; decorators are not re-applied during restore.
mocks/mock_factory.go: Regenerate mock to include RestoreSession().
restore_test.go: Unit tests covering nil session, missing salt, invalid hex, anonymous session round-trip, authenticated store→restore→validate round-trip, and cross-replica secret mismatch.
Fixes #4216
Type of change
Test plan
task test)task test-e2e)task lint-fix)Changes
Does this introduce a user-facing change?
Special notes for reviewers