Changed Packages

Codecov Report

❌ Patch coverage is 47.16981% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.28%. Comparing base (d903ed7) to head (52739c2).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

@@           Coverage Diff           @@
##             main    #3347   +/-   ##
=======================================
  Coverage   54.28%   54.28%           
=======================================
  Files        2345     2345           
  Lines       89626    89648   +22     
  Branches    25111    25106    -5     
=======================================
+ Hits        48650    48665   +15     
- Misses      40673    40680    +7     
  Partials      303      303           

hey @maysunfaisal - so claude and I looked at two error scenarios:

1. If the user's RBAC policy denies a specific tool invocation

for this, claude's analysis was that the rejection happens inside @backstage/plugin-mcp-actions-backend, not in this code. Lightspeed streams back whatever LCS returns — so the user would see whatever error or refusal the LCS/MCP Actions layer produces in the chat stream. The Lightspeed backend is fully opaque to per-tool authorization outcomes.

Assuming this is correct, I'm curious if it makes sense to catch the LCS/MCP actions layer error and display a potentially more user friendly indication of permission issues.

WDYT?

2. Unexpected error minting a Backstage token (getPluginRequestToken throws)

Claude claims the entire chat query fails (I'll post the details in a moment). It then goes on that is a chat query results in querying multiple MCP servers, none of those servers gets called.

I suppose a user prompt could result in multiple queries, but even if so, it seems unpredictable whether calling the other mcp servers is productive i.e. how dependent would the output of the backstage mcp server be.

I'm inclined to leave this be, but thought I would surface it in the review as due diligence to get your and everyone else's take.

Here is the code analysis from claude on gitPluginRequestToken

  There are two call sites for getPluginRequestToken, and neither has a local try/catch around it:

  A) During /v1/query (chat flow) — buildMcpHeaders() calls getPluginRequestToken at ~line 133. If it throws, the exception propagates unhandled out of buildMcpHeaders and is caught by the outer catch in the /v1/query handler (~line 775):

  } catch (error) {
      const errormsg = `Error fetching completions from ${provider}: ${error}`;
      logger.error(errormsg);
      response.status(500).json({ error: errormsg });
  }

  The entire chat query fails with a 500. Even if only one of several MCP servers uses auth: dcr, a token-minting failure for that one server aborts the whole request — the error is not isolated per-server. The other MCP servers (static-token or other DCR servers) never get their headers built.

  B) During POST /mcp-servers/:name/validate (~line 393) — same pattern: no local catch, the outer handler returns 500 with "Validation failed".

  Note: there is a guard for when authService/credentials are absent (logs a warning, skips the server), but that only handles the case where the services weren't injected — not a runtime failure in getPluginRequestToken itself.

@gabemontero Thanks for flagging this. You're correct, RBAC tool-level denials happen inside @backstage/plugin-mcp-actions-backend, and Lightspeed is opaque to those outcomes; it streams back whatever LCS returns. Surfacing a user-friendly permission error would require parsing the LCS response stream for authorization-specific error patterns and rendering them differently in the chat UI and that touches the frontend streaming logic and response parsing, which is a bigger scope than this PR. I'll capture it as a follow-up enhancement (parsing MCP/RBAC errors from LCS and rendering actionable messages in the Lightspeed chat). For now, the user will see whatever LCS surfaces in the streamed response.

Addressed your second concern

@gabemontero Thanks for flagging this. You're correct, RBAC tool-level denials happen inside @backstage/plugin-mcp-actions-backend, and Lightspeed is opaque to those outcomes; it streams back whatever LCS returns. Surfacing a user-friendly permission error would require parsing the LCS response stream for authorization-specific error patterns and rendering them differently in the chat UI and that touches the frontend streaming logic and response parsing, which is a bigger scope than this PR. I'll capture it as a follow-up enhancement (parsing MCP/RBAC errors from LCS and rendering actionable messages in the Lightspeed chat). For now, the user will see whatever LCS surfaces in the streamed response.

Addressed your second concern

sounds good @maysunfaisal thanks

@gabemontero Here is the issue to address surfacing user friendly MCP permission denial msgs https://redhat.atlassian.net/browse/RHIDP-14907

Screenshots - Local

RBAC Example - catalog.example.read = deny

rbac policy file

Lightspeed (prior to rebrand) interacting with Backstage/RHDH MCP with DCR - does not return Catalog resources

MCP Settings for DCR auth server does not let you edit token from the UI

Cursor MCP Settings - backstage-actions pointing to Backstage/RHDH MCP with DCR

Redirected to Backstage/RHDH for DCR auth and successful auth

Cursor MCP backstage-actions now shows a successful connection

Query the Backstage/RHDH Catalog, denied due to rbac policy

RBAC Example - catalog.example.read = allow

rbac policy file

Lightspeed query now passes for the same prompt using MCP with DCR

Cursor query now passes for the same prompt using MCP with DCR

@redhat-developer/rhdh-ui @redhat-developer/rhdh-plugins-maintainers Could you PTAL at this PR? Thanks!

New changes are detected. LGTM label has been removed.

/fs-review

🤖 Finished Review · ✅ Success · Started 11:29 PM UTC · Completed 11:42 PM UTC
Commit: d903ed7 · View workflow run →

Review

Findings

Medium

• [api-contract] workspaces/lightspeed/plugins/lightspeed-backend/src/plugin.ts — RouterOptions.auth is now a required field (auth: AuthService), which is a breaking change for any external consumer of createRouter() that does not pass it. Within the Backstage plugin system, coreServices.auth is always injected by the framework, so internal usage is safe. However, JavaScript consumers bypassing TypeScript checks could hit a runtime TypeError. The changeset declares a minor version bump, which may be insufficient for a breaking API change.
  Remediation: Consider making auth optional (auth?: AuthService) with a runtime guard, or document as a breaking change.

• [test-inadequate] workspaces/lightspeed/plugins/lightspeed-backend/src/service/mcp-server.test.ts — The new DCR tests cover GET /mcp-servers (auth field presence) and the /v1/query flow (token minting and mixed DCR/static headers). However, two code paths remain untested: (1) POST /mcp-servers/:name/validate with a DCR-configured server, and (2) the PATCH /mcp-servers/:name endpoint's 502 error path when auth.getPluginRequestToken throws.
  Remediation: Add tests for the validate endpoint's DCR path and the PATCH 502 error path.

Low

• [edge-case] workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts — In buildMcpHeaders, when a DCR server is configured but dcrAuth is unavailable, the function logs a warning and silently skips the server. The /v1/query request proceeds without that server's authorization header, and LCORE will likely return a 401. In practice, this path only triggers if createRouter is used by an external caller without the auth service.

• [api-contract] workspaces/lightspeed/plugins/lightspeed-backend/config.d.ts — The auth field is typed as string rather than the literal type 'dcr'. While runtime validation exists (unsupported values trigger a warning and fall back to static-token mode), the schema could be tightened for better developer experience.

• [data-exposure] workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts — The auth field is returned in the GET /mcp-servers API response, but config.d.ts marks it with @visibility backend. The field value is non-sensitive (just the string 'dcr' or undefined) and is needed by the frontend to render the correct modal UI. The @visibility annotation should be updated to frontend for consistency.

• [authorization] workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts — targetPluginId is hardcoded to 'mcp-actions' for all DCR servers. This is by design — DCR mode targets Backstage-internal MCP servers that all route through the mcp-actions backend plugin, which enforces per-resource permissions via RBAC. A documentation note clarifying this design decision would be helpful.

• [input-validation] workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts — The auth config value is checked via strict equality (authValue === 'dcr'). A typo like 'DCR' or 'dcr ' falls back to static-token mode with a warning log, which is reasonable. Case-insensitive comparison would be a minor UX improvement.

Labels: PR adds a new DCR authentication feature for MCP servers with security-relevant auth changes and test additions.

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Review

Findings

Medium

• [api-contract] workspaces/lightspeed/plugins/lightspeed-backend/src/plugin.ts — RouterOptions.auth is now a required field (auth: AuthService), which is a breaking change for any external consumer of createRouter() that does not pass it. Within the Backstage plugin system, coreServices.auth is always injected by the framework, so internal usage is safe. However, JavaScript consumers bypassing TypeScript checks could hit a runtime TypeError. The changeset declares a minor version bump, which may be insufficient for a breaking API change.
  Remediation: Consider making auth optional (auth?: AuthService) with a runtime guard, or document as a breaking change.

This is an internal plugin — coreServices.auth is always injected by the Backstage plugin framework. Making it optional would add unnecessary null checks for a code path that can't occur in practice.

• [test-inadequate] workspaces/lightspeed/plugins/lightspeed-backend/src/service/mcp-server.test.ts — The new DCR tests cover GET /mcp-servers (auth field presence) and the /v1/query flow (token minting and mixed DCR/static headers). However, two code paths remain untested: (1) POST /mcp-servers/:name/validate with a DCR-configured server, and (2) the PATCH /mcp-servers/:name endpoint's 502 error path when auth.getPluginRequestToken throws.
  Remediation: Add tests for the validate endpoint's DCR path and the PATCH 502 error path.

Partially addressed. Added test for the DCR validate endpoint path. The 502 error case (auth service throws) requires overriding coreServices.auth mock internals which is non-trivial with the current test infrastructure.

Low

• [edge-case] workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts — In buildMcpHeaders, when a DCR server is configured but dcrAuth is unavailable, the function logs a warning and silently skips the server. The /v1/query request proceeds without that server's authorization header, and LCORE will likely return a 401. In practice, this path only triggers if createRouter is used by an external caller without the auth service.

Intentional design — graceful degradation. The warning log is sufficient; crashing the entire request when one server's auth is unavailable would be worse UX.

• [api-contract] workspaces/lightspeed/plugins/lightspeed-backend/config.d.ts — The auth field is typed as string rather than the literal type 'dcr'. While runtime validation exists (unsupported values trigger a warning and fall back to static-token mode), the schema could be tightened for better developer experience.

Addressed. Changed type from string to 'dcr'.

• [data-exposure] workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts — The auth field is returned in the GET /mcp-servers API response, but config.d.ts marks it with @visibility backend. The field value is non-sensitive (just the string 'dcr' or undefined) and is needed by the frontend to render the correct modal UI. The @visibility annotation should be updated to frontend for consistency.

Addressed. Changed @visibility backend to @visibility frontend.

• [authorization] workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts — targetPluginId is hardcoded to 'mcp-actions' for all DCR servers. This is by design — DCR mode targets Backstage-internal MCP servers that all route through the mcp-actions backend plugin, which enforces per-resource permissions via RBAC. A documentation note clarifying this design decision would be helpful.

By design — all DCR-enabled MCP servers in RHDH are proxied through @backstage/plugin-mcp-actions-backend, so the target is always mcp-actions.

• [input-validation] workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts — The auth config value is checked via strict equality (authValue === 'dcr'). A typo like 'DCR' or 'dcr ' falls back to static-token mode with a warning log, which is reasonable. Case-insensitive comparison would be a minor UX improvement.

Addressed. Added .toLowerCase() so DCR, Dcr, etc. all work.

Labels: PR adds a new DCR authentication feature for MCP servers with security-relevant auth changes and test additions.

@its-mitesh-kumar Addressed the fullsend reviews