-
Notifications
You must be signed in to change notification settings - Fork 3
Add encryptedSession for separate auth tokens for Onboarding API and MCPs #156
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
Conversation
22272a8 to
5b16418
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo needs to be fixed but lgtm
As discussed,
We should do a follow up to check if we can improve the API further by using a custom store or already existing methods in the fastify session library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo needs to be fixed but lgtm
As discussed,
We should do a follow up to check if we can improve the API further by using a custom store or already existing methods in the fastify session library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces the legacy request.session storage with a new request.encryptedSession plugin, providing per-user encrypted session handling for both Onboarding and MCP authentication flows.
- Swapped all session reads/writes in routes and plugins to use
encryptedSession - Removed the old
session.jsplugin and introducedencrypted-session.js - Updated configuration and dependencies to support secure-session and session encryption
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/routes/auth-onboarding.js | Switched session storage to encryptedSession for onboarding tokens |
| server/routes/auth-mcp.js | Switched session storage to encryptedSession for MCP tokens |
| server/plugins/session.js | Removed legacy Fastify session plugin |
| server/plugins/http-proxy.js | Updated token retrieval and refresh logic to use encryptedSession |
| server/plugins/auth-utils.js | Updated OAuth state and code verifier storage to use encryptedSession |
| server/encrypted-session.js | Added new encrypted session plugin for secure per-user session encryption |
| server/config/env.js | Added SESSION_SECRET environment variable requirement |
| server/app.js | Registered the encryptedSession plugin |
| package.json | Added @fastify/secure-session dependency |
| .env.template | Added placeholder for SESSION_SECRET |
Comments suppressed due to low confidence (2)
server/encrypted-session.js:157
- [nitpick] Function name
encryptSymetricis misspelled; consider renaming toencryptSymmetricfor clarity and consistency.
function encryptSymetric(plaintext, key) {
server/encrypted-session.js:20
- [nitpick] Consider adding unit tests for the
encryptedSessionplugin to cover encryption/decryption flows and session persistence.
async function encryptedSession(fastify) {
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…consistent session date
What this PR does / why we need it:
This PR switches the request.session storage to use request.encryptedSession that is introduced in the encrypted-session.js.
Doing so will use a combination of secure-session (cookie that stores a user side encryption key) and fastify-session (where the users session data is stored with an encryption per user).
Decision documented here openmcp-project/backlog#151 (comment)
Which issue(s) this PR fixes:
Fixes #
Additional notes:
all data is stored into the underlying session as one encrypted object. We might want to store it as encrypted key/values in the session maybe with expiration date to be able to invalidate items after a max-age.
The original
request.session.destroyis changed torequest.encryptedSession.clear. It will clear the map used in memory to store the data and in the onSend hook, the original session data is overwritten with a newly encrypted (in this case empty) json encryption of the map.