fix(lightspeed): [RHIDP-11659] Encrypt MCP user tokens at rest, fix Bearer prefix for MCP Server validation#2671
Conversation
maysunfaisal
requested review from
a team,
karthikjeeyar,
rohitkrai03 and
yangcao77
as code owners
Review Summary by QodoEncrypt MCP user tokens at rest and fix Bearer prefix for direct server validation
WalkthroughsDescription• Encrypt MCP user tokens at rest using AES-256-GCM when backend.auth.keys is configured • Fix Bearer prefix in Authorization header for direct MCP server validation • Implement transparent token encryption/decryption in McpUserSettingsStore • Add comprehensive encryption tests and integration tests for encrypted token workflows Diagramflowchart LR
A["MCP User Token"] -->|encrypt| B["TokenEncryptor"]
B -->|AES-256-GCM| C["Encrypted Token<br/>enc:..."]
C -->|store| D["Database"]
D -->|retrieve| E["Encrypted Token"]
E -->|decrypt| B
B -->|plaintext| F["MCP Validator"]
F -->|Bearer prefix| G["Direct MCP Server"]
File Changes1. workspaces/lightspeed/plugins/lightspeed-backend/src/service/token-encryption.ts
|
Code Review by Qodo
1. Legacy tokens become null
|
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
| decrypt(stored: string): string | null { | ||
| if (!stored.startsWith(ENCRYPTED_PREFIX)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
1. Legacy tokens become null 🐞 Bug ≡ Correctness
When backend.auth.keys is configured, AesGcmEncryptor.decrypt returns null for any stored token without the "enc:" prefix, so existing plaintext DB tokens are treated as missing and won’t be used for MCP-HEADERS or direct validation.
Agent Prompt
### Issue description
With encryption enabled, previously stored plaintext tokens (no `enc:` prefix) decrypt to `null`, so user overrides stop working after enabling `backend.auth.keys`.
### Issue Context
Tokens are read via `McpUserSettingsStore.decryptRow()` and later used in `buildMcpHeaders()` / direct validation selection (`setting?.token || server.token`). Returning `null` for legacy plaintext causes silent behavior changes.
### Fix Focus Areas
- Decide on explicit behavior for legacy plaintext tokens when encryption is enabled:
- Option A (recommended for backward compatibility): treat non-prefixed stored values as plaintext and return them (and optionally log + re-encrypt/migrate on next write).
- Option B: if plaintext must be rejected, surface an explicit error/state to the user instead of silently dropping, and document migration.
- file paths/lines:
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/token-encryption.ts[64-83]
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/mcp-server-store.ts[60-65]
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts[81-88]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| private decryptRow(row: McpUserSettingsRow): McpUserSettingsRow { | ||
| if (row.token) { | ||
| return { ...row, token: this.encryptor.decrypt(row.token) }; | ||
| } |
There was a problem hiding this comment.
2. Decrypt error breaks apis 🐞 Bug ☼ Reliability
Decrypt can throw (e.g., tampered/invalid ciphertext) and McpUserSettingsStore calls decrypt without try/catch, so a single bad DB token can cause /mcp-servers listing and other MCP endpoints to return 500.
Agent Prompt
### Issue description
If `decrypt()` throws due to corrupted/tampered ciphertext, MCP endpoints can fail with 500 because decryption is performed inline while loading user settings.
### Issue Context
`McpUserSettingsStore.listByUser()` calls `decryptRow()` for every row, and `decryptRow()` directly calls `encryptor.decrypt()` with no exception handling.
### Fix Focus Areas
- Catch decryption errors and degrade gracefully (e.g., log warn/error + treat token as null for that row), so one bad row doesn’t break the whole endpoint.
- Alternatively, change `TokenEncryptor.decrypt()` contract to never throw (return `null` on any failure) and update tests accordingly.
- file paths/lines:
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/mcp-server-store.ts[60-65]
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/token-encryption.ts[64-83]
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/token-encryption.test.ts[93-98]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
maysunfaisal
force-pushed
the
RHIDP-11659-2
branch
from
9066798 to
9790231
Compare
Changed Packages
|
maysunfaisal
changed the title
There was a problem hiding this comment.
Looks great overall, thanks for the PR 🙌
Just have two small backend concerns before we rely on this in UI flows:
-
Migration/backward compatibility: if a user already has a saved plaintext token and encryption is later enabled, it looks like that token may become unreadable and the user appears disconnected. Could we keep a compatibility path (or migration) so existing users don’t have to re-enter credentials unexpectedly?
-
Key rotation resilience: we currently seem to use only the first
backend.auth.keysentry for decrypt. Can we support decrypting with older keys too, so rotating key order doesn’t break previously saved tokens?
Both would really help preserve the “set it and forget it” UX from the user side.
…or direct MCP server validation Assisted-by: Claude Opus 4.6 Generated-by: Cursor Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
maysunfaisal
force-pushed
the
RHIDP-11659-2
branch
from
9790231 to
8355262
Compare
@ciiay added a new commit that handles:
|
maysunfaisal
force-pushed
the
RHIDP-11659-2
branch
from
8355262 to
3699a4f
Compare
…dle key rotations Assisted-by: Claude Opus 4.6 Generated-by: Cursor Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
maysunfaisal
force-pushed
the
RHIDP-11659-2
branch
from
3699a4f to
7d2690c
Compare
|
2 participants
Hey, I just made a Pull Request!
Address all outstanding mplementation for https://redhat.atlassian.net/browse/RHIDP-11659
✔️ Checklist
Test Changes
Enable backend.auth and test out the Lightspeed backend MCP APIs, tokens stored in the DB will be encrypted.
app-config:
Update a MCP Server token:
Verify DB (local sqlite3):
If backend.auth.keys is absent, token will be stored as plaintext:
Verify DB (local postgres):
Confirming Decryption works as well when the validate API is called against a MCP Server: