feat(lightspeed): [RHIDP-11659] Implementation of MCP Service Selection#2581
Merged
maysunfaisal merged 4 commits intoredhat-developer:mainfrom Mar 26, 2026
Merged
Conversation
maysunfaisal
changed the title
maysunfaisal
force-pushed
the
RHIDP-11799-2
branch
from
a37d3e6 to
2e7fb93
Compare
maysunfaisal
requested review from
a team,
karthikjeeyar,
rohitkrai03 and
yangcao77
as code owners
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
Review Summary by QodoImplement MCP Service Selection with per-user preferences and validation
WalkthroughsDescription• Implement MCP Service Selection with per-user preferences and validation • Add database-backed store for user MCP server settings and token management • Create MCP server validator supporting JSON-RPC and SSE protocols • Add REST endpoints for listing, validating, and managing MCP servers • Introduce new permissions for MCP read and manage operations Diagramflowchart LR
A["Admin Config<br/>mcpServers"] -->|name, token| B["Router<br/>MCP Endpoints"]
C["LCS<br/>GET /v1/mcp-servers"] -->|server URLs| B
B -->|GET /mcp-servers| D["User Preferences<br/>Database Store"]
B -->|PATCH /mcp-servers/:name| D
B -->|POST /mcp-servers/:name/validate| E["MCP Validator<br/>JSON-RPC + SSE"]
E -->|initialize, tools/list| F["MCP Server<br/>HTTP Endpoint"]
D -->|user tokens, enabled state| B
B -->|MCP-HEADERS| G["LCS Query<br/>Streaming"]
File Changes1. workspaces/lightspeed/plugins/lightspeed-backend/__fixtures__/lcsHandlers.ts
|
Code Review by Qodo
1. MCP API missing endpoint input
|
Contributor
Author
Lightspeed Stack Config (lightspeed-stack.yaml):Backstage App Config (app-config.yaml, two corresponding MCP Servers, one without token and to be patched via API as an example)Lightspeed Backend APIsList MCP ServersReturns the MCP Servers configured with Llama Stack, shows the cached status to avoid latency or making network calls. At this stage, the MCP Server status is unknown and tool count is 0. Validate MCP Server on demand (for MCP Server with no token)Provide token to MCP Server via PATCHMCP Server token gets updated along with status and tool count List MCP Servers againThe MCP Server that was validated, has its status and tool count updated Verify DB configured via BackstageValidate the 2nd MCP ServerList now has all connectedShow all DB columnsAfter Validation, DB has all the entries. Toggle MCP Servers Off and OnLets users enable/disable MCP Servers to be used |
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
Comment on lines
+206
to
+218
| router.post('/mcp-servers/validate', async (req, res) => { | ||
| try { | ||
| const credentials = await httpAuth.credentials(req); | ||
| await authorizer.authorizeUser(lightspeedMcpReadPermission, credentials); | ||
|
|
||
| const { url, token } = req.body; | ||
| if (!url || !token) { | ||
| res.status(400).json({ error: 'url and token are required' }); | ||
| return; | ||
| } | ||
|
|
||
| const result = await mcpValidator.validate(url, token); | ||
| res.json(result); |
There was a problem hiding this comment.
2. Ssrf validate endpoint 🐞 Bug ⛨ Security
POST /mcp-servers/validate accepts a user-provided URL and makes server-side fetch calls to it, enabling SSRF to internal network resources. The backend also forwards the user-provided token as an Authorization header to that URL.
Agent Prompt
### Issue description
`POST /mcp-servers/validate` performs server-side `fetch` calls to an arbitrary, user-supplied `url`, enabling SSRF.
### Issue Context
The endpoint is guarded by `lightspeedMcpReadPermission`, but that still permits any authorized user to make the backend reach out to internal services.
### Fix Focus Areas
- Add strict URL validation/allowlisting (e.g., only URLs returned from LCS, or only https + allowed hostnames), and reject IP-literals/private ranges.
- Consider removing this endpoint entirely or changing it to validate by `serverName` only.
- Ensure error responses don’t leak internal network details.
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts[206-227]
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/mcp-server-validator.ts[34-105]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+229
to
+273
| router.post('/mcp-servers/:name/validate', async (req, res) => { | ||
| try { | ||
| const credentials = await httpAuth.credentials(req); | ||
| await authorizer.authorizeUser(lightspeedMcpReadPermission, credentials); | ||
| const user = await userInfo.getUserInfo(credentials); | ||
|
|
||
| const { name } = req.params; | ||
| const server = staticServers.find(s => s.name === name); | ||
| if (!server) { | ||
| res.status(404).json({ | ||
| error: `MCP server '${name}' not found in configuration`, | ||
| }); | ||
| return; | ||
| } | ||
| // Resolve URL: config override → LCS cache → fresh LCS fetch | ||
| let serverUrl = resolveServerUrl(server.name); | ||
| if (!serverUrl) { | ||
| await refreshLcsUrlCache(); | ||
| serverUrl = resolveServerUrl(server.name); | ||
| } | ||
| if (!serverUrl) { | ||
| res | ||
| .status(400) | ||
| .json({ error: 'Server has no URL — not found in LCS or config' }); | ||
| return; | ||
| } | ||
|
|
||
| const setting = await settingsStore.get(name, user.userEntityRef); | ||
| const effectiveToken = setting?.token || server.token; | ||
| if (!effectiveToken) { | ||
| res | ||
| .status(400) | ||
| .json({ error: 'No token available — provide one first' }); | ||
| return; | ||
| } | ||
|
|
||
| const validation = await mcpValidator.validate(serverUrl, effectiveToken); | ||
| const status: McpServerStatus = validation.valid ? 'connected' : 'error'; | ||
|
|
||
| await settingsStore.updateStatus( | ||
| name, | ||
| user.userEntityRef, | ||
| status, | ||
| validation.toolCount, | ||
| ); |
There was a problem hiding this comment.
3. Read permission writes db 🐞 Bug ⛨ Security
POST /mcp-servers/:name/validate is authorized with lightspeedMcpReadPermission but writes validation status/tool_count into lightspeed_mcp_user_settings. This allows users with read-only permission to mutate persisted per-user state.
Agent Prompt
### Issue description
`POST /mcp-servers/:name/validate` uses `lightspeedMcpReadPermission` but persists cached validation results to the DB.
### Issue Context
Even though the data written is only status/tool_count, it is still a write side-effect and should be protected by the manage/update permission.
### Fix Focus Areas
- Change authorization for `POST /mcp-servers/:name/validate` to `lightspeedMcpManagePermission` (or stop persisting from this endpoint).
- Update tests accordingly.
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts[229-289]
- workspaces/lightspeed/plugins/lightspeed-common/src/permissions.ts[59-77]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+58
to
+87
| const existing = await this.get(serverName, userEntityRef); | ||
| const now = new Date().toISOString(); | ||
|
|
||
| if (existing) { | ||
| const fields: Partial<McpUserSettingsRow> = { updated_at: now }; | ||
| if (updates.enabled !== undefined) fields.enabled = updates.enabled; | ||
| if (updates.token !== undefined) { | ||
| fields.token = updates.token; | ||
| if (updates.token) fields.status = 'unknown'; | ||
| } | ||
|
|
||
| await this.db(TABLE) | ||
| .where({ server_name: serverName, user_entity_ref: userEntityRef }) | ||
| .update(fields); | ||
| return (await this.get(serverName, userEntityRef))!; | ||
| } | ||
|
|
||
| const row: McpUserSettingsRow = { | ||
| id: randomUUID(), | ||
| server_name: serverName, | ||
| user_entity_ref: userEntityRef, | ||
| enabled: updates.enabled ?? true, | ||
| token: updates.token ?? null, | ||
| status: 'unknown', | ||
| tool_count: 0, | ||
| created_at: now, | ||
| updated_at: now, | ||
| }; | ||
| await this.db(TABLE).insert(row); | ||
| return row; |
There was a problem hiding this comment.
4. Non-atomic upsert race 🐞 Bug ⛯ Reliability
McpUserSettingsStore uses read-then-insert logic for upsert/updateStatus, which can violate the unique(server_name,user_entity_ref) constraint under concurrent requests. This can cause intermittent 500s on PATCH/validate endpoints.
Agent Prompt
### Issue description
The store uses read-then-insert, which is not safe under concurrency due to a unique constraint.
### Issue Context
The table has `unique(['server_name','user_entity_ref'])`, so inserts must be atomic.
### Fix Focus Areas
- Implement atomic upsert using Knex `.insert(...).onConflict(['server_name','user_entity_ref']).merge(...)` (Postgres/SQLite supported) for both `upsert` and `updateStatus`.
- Ensure status/tool_count updates are part of the same atomic statement.
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/mcp-server-store.ts[53-88]
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/mcp-server-store.ts[91-116]
- workspaces/lightspeed/plugins/lightspeed-backend/migrations/20260302120000_add_mcp_servers.js[20-33]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ciiay
reviewed
Mar 24, 2026
Member
ciiay
left a comment
ciiay
left a comment
There was a problem hiding this comment.
Hi @maysunfaisal , thanks for the detailed walkthrough and curl examples. The happy path looks solid from frontend integration perspective. I just have a concern about the status mapping. We currently need to derive UI states like “Token required”, “Failed”, “Disabled”, “Ready/Connected” from:
status(connected|error|unknown)enabledhasToken
I see the mapping table in your document, but I still have questions regarding priority/order when multiple conditions are meet:
- If
enabled: falseandhasToken: false, should UI showDisabledorToken Required? - If
status: errorandenabled: false, which one wins? - Should
unknownbe shown only during active validation, or also for untouched/new rows?
Looping in @aprilma419 as well for UX alignment on edge-case state priority.
| if (updates.enabled !== undefined) fields.enabled = updates.enabled; | ||
| if (updates.token !== undefined) { | ||
| fields.token = updates.token; | ||
| if (updates.token) fields.status = 'unknown'; |
Member
There was a problem hiding this comment.
Status is only reset when updates.token is truthy. If token is cleared (null/empty), previous status and tool_count remain. In PATCH /mcp-servers/:name, validation only runs when token && serverUrl, so a cleared token can leave server state like status: connected while hasToken: false (inconsistent UI behavior).
Contributor
Author
There was a problem hiding this comment.
Priority order: Disabled > Token Required > Error > Connected > Unknown
- If enabled: false and hasToken: false -> UI shows Disabled (enabled takes precedence)
- If status: error and enabled: false -> UI shows Disabled
- unknown is the default state for untouched/new rows and also appears after a token is cleared.. Meaning no validation has been performed yet..
Contributor
Author
There was a problem hiding this comment.
This has been addressed in the latest commit where the upsert() method now resets status='unknown' and tool_count=0 whenever the token field is updated, whether setting a new token or clearing it (null). I also updated the tests for it.
Contributor
Author
There was a problem hiding this comment.
List
mfaisal-mac:lightspeed maysun$ curl -s http://localhost:7007/api/lightspeed/mcp-servers \
> -H "Authorization: Bearer $BACKSTAGE_TOKEN" | jq '.servers[] | select(.name == "test-mcp-server")'
{
"name": "test-mcp-server",
"url": "http://localhost:8888/mcp",
"enabled": true,
"status": "unknown",
"toolCount": 0,
"hasToken": false
}
Patch Token for missing Token
mfaisal-mac:lightspeed maysun$ curl -s -X PATCH http://localhost:7007/api/lightspeed/mcp-servers/test-mcp-server \
> -H "Authorization: Bearer $BACKSTAGE_TOKEN" \
> -H "Content-Type: application/json" \
> -d '{"token": "test-secret-token"}' | jq .
{
"server": {
"name": "test-mcp-server",
"url": "http://localhost:8888/mcp",
"enabled": true,
"status": "connected",
"toolCount": 4,
"hasToken": true
},
"validation": {
"valid": true,
"toolCount": 4,
"tools": [
{
"name": "echo",
"description": "Echo back a message - useful for testing connectivity."
},
{
"name": "get_current_time",
"description": "Get the current server time."
},
{
"name": "add_numbers",
"description": "Add two numbers together."
},
{
"name": "get_server_info",
"description": "Get information about this MCP server."
}
]
}
}
List again
mfaisal-mac:lightspeed maysun$ curl -s http://localhost:7007/api/lightspeed/mcp-servers \
> -H "Authorization: Bearer $BACKSTAGE_TOKEN" | jq '.servers[] | select(.name == "test-mcp-server")'
{
"name": "test-mcp-server",
"url": "http://localhost:8888/mcp",
"enabled": true,
"status": "connected",
"toolCount": 4,
"hasToken": true
}
Clear token/ update to null
mfaisal-mac:lightspeed maysun$ curl -s -X PATCH http://localhost:7007/api/lightspeed/mcp-servers/test-mcp-server \
> -H "Authorization: Bearer $BACKSTAGE_TOKEN" \
> -H "Content-Type: application/json" \
> -d '{"token": null}' | jq .
{
"server": {
"name": "test-mcp-server",
"url": "http://localhost:8888/mcp",
"enabled": true,
"status": "unknown",
"toolCount": 0,
"hasToken": false
}
}
List again
mfaisal-mac:lightspeed maysun$ curl -s http://localhost:7007/api/lightspeed/mcp-servers \
> -H "Authorization: Bearer $BACKSTAGE_TOKEN" | jq '.servers[] | select(.name == "test-mcp-server")'
{
"name": "test-mcp-server",
"url": "http://localhost:8888/mcp",
"enabled": true,
"status": "unknown",
"toolCount": 0,
"hasToken": false
}
Patch Token again and verify DB
mfaisal-mac:lightspeed maysun$ curl -s -X PATCH http://localhost:7007/api/lightspeed/mcp-servers/test-mcp-server \
> -H "Authorization: Bearer $BACKSTAGE_TOKEN" \
> -H "Content-Type: application/json" \
> -d '{"token": "test-secret-token"}' | jq .
{
"server": {
"name": "test-mcp-server",
"url": "http://localhost:8888/mcp",
"enabled": true,
"status": "connected",
"toolCount": 4,
"hasToken": true
},
"validation": {
"valid": true,
"toolCount": 4,
"tools": [
{
"name": "echo",
"description": "Echo back a message - useful for testing connectivity."
},
{
"name": "get_current_time",
"description": "Get the current server time."
},
{
"name": "add_numbers",
"description": "Add two numbers together."
},
{
"name": "get_server_info",
"description": "Get information about this MCP server."
}
]
}
}
mfaisal-mac:lightspeed maysun$ sqlite3 -header packages/backend/sqlite-data/lightspeed.sqlite \
> "SELECT server_name, token IS NOT NULL as has_token, status, tool_count FROM lightspeed_mcp_user_settings;"
server_name|has_token|status|tool_count
Contributor
Author
There was a problem hiding this comment.
Here is the case, where someone tries to patch with wrong token, GET updates the status:
mfaisal-mac:lightspeed maysun$ curl -s http://localhost:7007/api/lightspeed/mcp-servers -H "Authorization: Bearer $BACKSTAGE_TOKEN" | jq .
{
"servers": [
{
"name": "test-mcp-server",
"url": "http://localhost:8888/mcp",
"enabled": true,
"status": "connected",
"toolCount": 4,
"hasToken": true
},
{
"name": "mcp-integration-tools",
"url": "http://localhost:7008/api/mcp-actions/v1",
"enabled": true,
"status": "connected",
"toolCount": 7,
"hasToken": true
}
]
}
mfaisal-mac:lightspeed maysun$ curl -s -X PATCH http://localhost:7007/api/lightspeed/mcp-servers/test-mcp-server -H "Authorization: Bearer $BACKSTAGE_TOKEN" -H "Content-Type: application/json" -d '{"token": "test-secret-token-wrong"}' | jq .
{
"server": {
"name": "test-mcp-server",
"url": "http://localhost:8888/mcp",
"enabled": true,
"status": "error",
"toolCount": 0,
"hasToken": true
},
"validation": {
"valid": false,
"toolCount": 0,
"tools": [],
"error": "Invalid credentials — server returned 401/403"
}
}
mfaisal-mac:lightspeed maysun$ curl -s http://localhost:7007/api/lightspeed/mcp-servers -H "Authorization: Bearer $BACKSTAGE_TOKEN" | jq .
{
"servers": [
{
"name": "test-mcp-server",
"url": "http://localhost:8888/mcp",
"enabled": true,
"status": "error",
"toolCount": 0,
"hasToken": true
},
{
"name": "mcp-integration-tools",
"url": "http://localhost:7008/api/mcp-actions/v1",
"enabled": true,
"status": "connected",
"toolCount": 7,
"hasToken": true
}
]
}
gabemontero
reviewed
Mar 24, 2026
Contributor
gabemontero
left a comment
gabemontero
left a comment
There was a problem hiding this comment.
one admittedly esoteric consideration @maysunfaisal
but it is conceivable address it is simply making the error message a bit more detailed
workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts
Outdated
Show resolved
Hide resolved
yangcao77
reviewed
Mar 24, 2026
| }), | ||
| signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS), | ||
| }).catch(() => { | ||
| // notifications may not return a response — ignore errors |
Contributor
There was a problem hiding this comment.
do we want to log it at debug/trace level?
yangcao77
reviewed
Mar 24, 2026
yangcao77
requested changes
Mar 24, 2026
Contributor
yangcao77
left a comment
yangcao77
left a comment
There was a problem hiding this comment.
generally looks good! just some minor comments.
also please include a changeset within this PR
maysunfaisal
force-pushed
the
RHIDP-11799-2
branch
from
2e7fb93 to
6121cc7
Compare
Contributor
Author
@yangcao77 changeset included in latest commit |
ciiay
reviewed
Mar 25, 2026
Member
ciiay
left a comment
ciiay
left a comment
There was a problem hiding this comment.
Looks good from frontend side, and the latest fixes help a lot (especially token-clear reset and status priority).
Two quick clarifications before integration:
- Could the API return
tokenSource(orhasUserToken) in addition tohasToken?
frontend needs to distinguish user-saved token vs admin default for correct UX/actions. - Can we confirm encrypted-at-rest handling for saved tokens?
This is part of the feature requirements, and I don’t see it explicitly in this PR.
Once these are clear, the frontend wiring should be straightforward.
If these are planned for a follow-up PR, that works too 🤝
Contributor
Author
|
I will need to update the API reports for the CI to pass. I can implement @ciiay first suggestion about |
Assisted-by: Claude Opus 4.6 Generated-by: Cursor Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Assisted-by: Claude Opus 4.6 Generated-by: Cursor Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Assisted-by: Claude Opus 4.6 Generated-by: Cursor Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Assisted-by: Claude Opus 4.6 Generated-by: Cursor Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com> Made-with: Cursor
maysunfaisal
force-pushed
the
RHIDP-11799-2
branch
from
0d58c8f to
0e0ddf8
Compare
|
Contributor
Author
|
Merging the PR, CI passes after I updated the API Reports. @ciiay I have included |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hey, I just made a Pull Request!
Fixes Issue
https://redhat.atlassian.net/browse/RHIDP-11659
Description
This PR implements the MCP Service Selection for Lightspeed Backend as the Figma Diagram outlined in https://redhat.atlassian.net/browse/DTUX-2411 and introduces backend API endpoints for MCP server management in the Lightspeed plugin. MCP servers are configured by admins in app-config.yaml; users can view, toggle, and provide personal access tokens for them.
Endpoints
GET /api/lightspeed/mcp-serversReturns the list of admin-configured MCP servers merged with the calling user's preferences (enabled/disabled state, validation status, tool count). Server URLs are resolved from LCS at startup via GET /v1/mcp-servers.PATCH /api/lightspeed/mcp-servers/:nameAllows users to toggle a server on/off (enabled) and/or provide a personal access token override (token). When a token is provided, the backend immediately validates it against the MCP server and returns the result.POST /api/lightspeed/mcp-servers/:name/validateOn-demand validation of a named MCP server. Performs an MCP protocol handshake (initialize + tools/list) using the effective token (user override > admin default) and persists the updated status and tool count.POST /api/lightspeed/mcp-servers/validateStateless credential test — accepts a url and token in the request body, validates connectivity without persisting anything. Intended for "test connection" flows before saving.Key Design Decisions
✔️ Checklist