refactor(lightspeed): create permission middleware and update proxy passthrough

[Jdubrick]

PR Summary by Qodo

lightspeed: add permission middleware and replace proxy catch-all routes
✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Walkthroughs

User Description

Hey, I just made a Pull Request!

What was done

• Replaced the catch-all proxy middleware with explicit route definitions
  • Old router used a single router.use('/') catch-all that proxied any matching path through to LCORE, relying on an allowlist and a blocklist to decide what to proxy vs. skip
• Extracted a reusable createPermissionMiddleware
  • Permission checks were previously done inline via await authorizer.authorizeUser(...) with try/catch blocks duplicated across every route handler. Now a standalone Express middleware factory
• Moved proxy path-rewriting logic to utils.ts
  • The user_id and history_length query-param injection that was inline in the catch-all's pathRewrite callback is now the standalone rewriteLightspeedProxyPath function, independently testable
• Moved SKIP_USER_ID_ENDPOINTS and DEFAULT_HISTORY_LENGTH to constant.ts
  • These lived in router.ts and types.ts, now are with the other constants
• Added unit tests for the new createPermissionMiddleware and rewriteLightspeedProxyPath utility, and updated existing router tests for the new route structure

Why it was done

• Security
  • Explicit routes eliminate the risk of accidentally proxying unintended paths. Each route declares exactly which permission it requires
• Readability
  • The route table shows every endpoint, its HTTP method, and its required permission, rather than tracing through a catch-all with conditional logic
• Testability
  • Permission checking and path rewriting are now isolated units with their own tests
• Maintainability
  • Adding a new proxied endpoint is now a one liner (router.get('/path', requirePermission(...), apiProxy))

✔️ Checklist

• A changeset describing the change and affected packages. (more info)
• Added or Updated documentation
• Tests for new functionality and regression tests for bug fixes
• Screenshots attached (for UI changes)

AI Description

• Replace catch-all proxy with explicit, permission-scoped routes to Lightspeed Core
• Extract reusable permission middleware and proxy path rewrite utility
• Add unit/regression tests for auth failures, validation ordering, and path rewriting

Diagram

graph TD
  R["Lightspeed router"] --> PM["Permission middleware"] --> PS["Backstage permissions"]
  R --> PX["HTTP proxy"] --> LCS{{"Lightspeed Core"}}
  PX --> U["Path rewrite util"]
  T["Unit tests"] --> R --> PX
  T --> PM
  T --> U

  subgraph Legend
    direction LR
    _r["Router/module"] ~~~ _m["Middleware"] ~~~ _e{{"External service"}} ~~~ _t["Tests"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep catch-all proxy + stricter allowlist matcher

• ➕ Less route boilerplate for new upstream endpoints
• ➕ Single enforcement point for path filtering
• ➖ Higher risk of unintended exposure via matching mistakes or future edits
• ➖ Harder to reason about per-endpoint permission mapping and ordering

2. Declarative route table (config-driven) that generates routes

• ➕ Central list of endpoints + required permission reduces repetition
• ➕ Easy to audit and extend while keeping explicitness
• ➖ Adds an abstraction layer that may hinder debugging
• ➖ Still requires careful handling of per-route exceptions (e.g., validation-before-auth)

Recommendation: The PR’s approach (explicit route definitions + shared permission middleware) is the most secure and reviewable option because it makes the exposed surface area and required permissions auditable in code. A declarative route table could be considered later if endpoint count grows, but the current explicit implementation is a good trade-off for clarity and security.

File Changes

Enhancement (2)

createPermissionMiddleware.ts Add reusable permission-check middleware factory +45/-0

Add reusable permission-check middleware factory

• Adds createPermissionMiddleware(authorizer, permission, logger) to standardize authorization handling for routes. Converts NotAllowedError into 403 responses and treats unexpected failures as 500 with a generic message.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createPermissionMiddleware.ts

---

utils.ts Extract proxy path rewrite into testable utility +28/-0

Extract proxy path rewrite into testable utility

• Adds rewriteLightspeedProxyPath() to centralize user_id injection and optional history_length injection while respecting SKIP_USER_ID_ENDPOINTS. Keeps existing error sanitization and fetch error handling intact.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/utils.ts

---

Refactor (4)

constant.ts Centralize proxy rewrite constants +5/-22

Centralize proxy rewrite constants

• Removes proxy allowlist/passthrough constants tied to the old catch-all proxy. Adds SKIP_USER_ID_ENDPOINTS and DEFAULT_HISTORY_LENGTH to support shared rewrite logic in utils.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/constant.ts

---

router.ts Replace catch-all proxy with explicit, permission-scoped routes +315/-371

Replace catch-all proxy with explicit, permission-scoped routes

• Refactors proxying to define explicit GET/DELETE routes using a shared apiProxy and per-route permission middleware, eliminating the old allowlist-based catch-all. Extracts permission checks into requirePermission(), moves path rewrite logic into rewriteLightspeedProxyPath(), and adds a final 404 handler for any unregistered paths.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts

---

types.ts Introduce Authorizer type and remove misplaced constants +4/-3

Introduce Authorizer type and remove misplaced constants

• Defines an Authorizer type alias based on userPermissionAuthorization() return type for reuse by middleware. Removes DEFAULT_HISTORY_LENGTH from types in favor of constants.ts.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/types.ts

---

validation.ts Remove proxy allowlist validation helper +0/-7

Remove proxy allowlist validation helper

• Removes isAllowedProxyPath() and its dependency on ALLOWED_PROXY_PREFIXES, since explicit route registration now governs what can be reached. Leaves validateCompletionsRequest() as the request-body validator for /v1/query.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/validation.ts

---

Tests (3)

createPermissionMiddleware.test.ts Unit test permission middleware success/deny/error paths +128/-0

Unit test permission middleware success/deny/error paths

• Adds tests verifying next() on success, 403 on NotAllowedError, and 500 on unexpected authorizer failures. Also asserts logging behavior for denied/unexpected cases.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createPermissionMiddleware.test.ts

---

router.test.ts Update router tests for explicit proxy routes and auth ordering +165/-5

Update router tests for explicit proxy routes and auth ordering

• Adds coverage for new explicit proxy routes (e.g., /v1/shields) and unauthorized (403) behavior. Introduces regression tests ensuring /v1/query validation runs before authorization, adds a helper to simulate permission service outages (500), and updates 404 expectations for unregistered paths and prefix-adjacent paths.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.test.ts

---

utils.test.ts Add unit tests for proxy path rewriting +58/-1

Add unit tests for proxy path rewriting

• Extends utils tests to cover rewriteLightspeedProxyPath() behavior: skip endpoints, user_id injection, and conditional history_length injection. Uses DEFAULT_HISTORY_LENGTH from constants to lock in expected defaults.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/utils.test.ts

---

Other (1)

tasty-moons-judge.md Add patch changeset for Lightspeed backend refactor +5/-0

Add patch changeset for Lightspeed backend refactor

• Introduces a changeset marking a patch release for the Lightspeed backend package to capture the permission/proxy refactor.

workspaces/lightspeed/.changeset/tasty-moons-judge.md

---

[rhdh-gh-app]

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
@red-hat-developer-hub/backstage-plugin-lightspeed-backend	workspaces/lightspeed/plugins/lightspeed-backend	patch	v2.9.0

[codecov]

Codecov Report

❌ Patch coverage is 90.00000% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.01%. Comparing base (9fb8693) to head (41dfeae).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3316      +/-   ##
==========================================
- Coverage   54.01%   54.01%   -0.01%     
==========================================
  Files        2403     2403              
  Lines       87411    87389      -22     
  Branches    24182    24159      -23     
==========================================
- Hits        47219    47202      -17     
+ Misses      38709    38704       -5     
  Partials     1483     1483              

Flag	Coverage Δ		*Carryforward flag
adoption-insights	83.58% <ø> (ø)		Carriedforward from 9fb8693
ai-integrations	70.03% <ø> (ø)		Carriedforward from 9fb8693
app-defaults	69.60% <ø> (ø)		Carriedforward from 9fb8693
augment	46.39% <ø> (ø)		Carriedforward from 9fb8693
bulk-import	72.86% <ø> (ø)		Carriedforward from 9fb8693
cost-management	17.48% <ø> (ø)		Carriedforward from 9fb8693
dcm	61.23% <ø> (ø)		Carriedforward from 9fb8693
extensions	62.24% <ø> (ø)		Carriedforward from 9fb8693
global-floating-action-button	74.30% <ø> (ø)		Carriedforward from 9fb8693
global-header	61.63% <ø> (ø)		Carriedforward from 9fb8693
homepage	51.52% <ø> (ø)		Carriedforward from 9fb8693
install-dynamic-plugins	56.23% <ø> (ø)		Carriedforward from 9fb8693
konflux	91.01% <ø> (ø)		Carriedforward from 9fb8693
lightspeed	68.49% <90.00%> (-0.04%)	⬇️
mcp-integrations	85.46% <ø> (ø)		Carriedforward from 9fb8693
orchestrator	37.33% <ø> (ø)		Carriedforward from 9fb8693
quickstart	62.09% <ø> (ø)		Carriedforward from 9fb8693
sandbox	79.49% <ø> (ø)		Carriedforward from 9fb8693
scorecard	83.86% <ø> (ø)		Carriedforward from 9fb8693
theme	64.54% <ø> (ø)		Carriedforward from 9fb8693
translations	8.49% <ø> (ø)		Carriedforward from 9fb8693
x2a	78.79% <ø> (ø)		Carriedforward from 9fb8693

*This pull request uses carry forward flags. Click here to find out more.

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fb8693...41dfeae. Read the comment docs.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Jdubrick]

/hold going to wait for #3296

[Jdubrick]

/unhold

[rhdh-qodo-merge]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0)

1. History length never injected 🐞 Bug ≡ Correctness

Description

rewriteLightspeedProxyPath only appends history_length when the literal substring "conversation_id"
exists in the URL, so normal conversation-detail requests like /v2/conversations/<id> won’t get the
default history limit. This defeats DEFAULT_HISTORY_LENGTH and can change behavior/perf by returning
more history than intended.

Code

workspaces/lightspeed/plugins/lightspeed-backend/src/service/utils.ts[R99-104]

+  if (!path.includes('history_length') && path.includes('conversation_id')) {
+    const historyLengthQuery = `history_length=${DEFAULT_HISTORY_LENGTH}`;
+    newPath = newPath.includes('?')
+      ? `${newPath}&${historyLengthQuery}`
+      : `${newPath}?${historyLengthQuery}`;
+  }

Relevance

⭐⭐ Medium

Same conversation_id substring gating for history_length was previously merged without objection (PR
#2324); no contrary history found.

PR-#2324

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The rewrite logic gates history_length on a literal substring check, but both the frontend and
backend tests show conversation IDs like "conv-123" / "conversation-id-1" that will not contain
"conversation_id", so the condition will never fire for real paths proxied by `GET
/v2/conversations/:conversation_id`.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/utils.ts[83-106]
workspaces/lightspeed/plugins/lightspeed/src/api/LightspeedApiClient.ts[155-163]
workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts[490-499]
workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.test.ts[34-40]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`rewriteLightspeedProxyPath` appends `history_length` only when `path.includes('conversation_id')`, but real requests use `/v2/conversations/:conversation_id` where the actual ID value (e.g. `conv-123`) does not contain the substring `conversation_id`. As a result, `DEFAULT_HISTORY_LENGTH` is effectively never applied to conversation detail requests.

### Issue Context
- The frontend calls `/v2/conversations/${encodeURIComponent(conversation_id)}`.
- The backend proxies `GET /v2/conversations/:conversation_id` through `apiProxy`, which uses `rewriteLightspeedProxyPath`.

### Fix Focus Areas
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/utils.ts[83-107]
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/utils.test.ts[177-225]
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts[490-509]

### Implementation notes
- Parse `path` as a URL (e.g., `new URL(path, 'http://localhost')`) and decide based on `url.pathname`.
- Trigger `history_length` injection for conversation-detail routes (e.g. `url.pathname.startsWith('/v2/conversations/') && url.pathname !== '/v2/conversations'`) rather than checking for a literal substring.
- Add/adjust unit tests to cover a realistic ID like `/v2/conversations/conv-123` (and ensure list endpoint `/v2/conversations` does not get `history_length`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Permission middleware can throw ✓ Resolved 🐞 Bug ☼ Reliability

Description

createPermissionMiddleware calls getIdentity(req) before entering its try/catch, so if identity
middleware didn’t run (or was skipped/misordered), getIdentity throws and the async middleware
rejects without returning the intended 401/403/500 JSON contract. This makes the middleware brittle
to reuse and can surface as unhandled errors.

Code

workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createPermissionMiddleware.ts[R31-33]

+  return async (req, res, next) => {
+    const { credentials } = getIdentity(req);
+    try {

Relevance

⭐⭐⭐ High

Team accepted hardening identity/auth error handling around getIdentity throwing (PR #3227).

PR-#3227

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The permission middleware dereferences identity before its try/catch, and getIdentity throws when
identity fields are missing; this creates an uncaught exception path if identity isn’t present on
the request.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createPermissionMiddleware.ts[26-44]
workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/getIdentity.ts[64-78]
workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts[202-220]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`createPermissionMiddleware` calls `getIdentity(req)` outside its `try/catch`. If identity is missing (e.g., middleware ordering changes, a route is added without identity middleware, or identity is intentionally skipped), `getIdentity` throws and bypasses the middleware’s error handling.

### Issue Context
`getIdentity` explicitly throws when `req.credentials` / `req.userEntityRef` are absent.

### Fix Focus Areas
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createPermissionMiddleware.ts[26-44]
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/getIdentity.ts[64-78]

### Implementation notes
- Move `const { credentials } = getIdentity(req);` inside the `try` block.
- On identity extraction failure, either:
 - respond with a controlled `500` (or `401/403` if you can discriminate), or
 - `return next(error)` so the shared Backstage/Express error middleware handles it consistently.
- Consider logging structured context (permission name) in the unexpected-error path without leaking sensitive details to the client.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[Jdubrick]

Will deal with the history_length raised bug in https://redhat.atlassian.net/browse/RHDHBUGS-3328

[yangcao77]

changes look good to me