Skip to content

[RHIDP-13061] Add Rate Limiting for Lightspeed Plugin#3531

Open
Jdubrick wants to merge 8 commits into
redhat-developer:mainfrom
Jdubrick:add-rate-limiting-lightspeed
Open

[RHIDP-13061] Add Rate Limiting for Lightspeed Plugin#3531
Jdubrick wants to merge 8 commits into
redhat-developer:mainfrom
Jdubrick:add-rate-limiting-lightspeed

Conversation

@Jdubrick

Copy link
Copy Markdown
Contributor

Hey, I just made a Pull Request!

  • Adds rate limiting for the Lightspeed plugin (including Notebooks)
  • Extends the customizable config by admins so they can tweak the limits if desired

Fixes https://redhat.atlassian.net/browse/RHIDP-13061

✔️ 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)

@rhdh-gh-app

rhdh-gh-app Bot commented Jun 22, 2026

Copy link
Copy Markdown

Important

This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior.

Changed Packages

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

@Jdubrick Jdubrick force-pushed the add-rate-limiting-lightspeed branch from 599e82e to b137cdb Compare June 22, 2026 15:21
@codecov

codecov Bot commented Jun 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.13559% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.24%. Comparing base (71cfae4) to head (f2cf76d).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3531      +/-   ##
==========================================
+ Coverage   54.21%   54.24%   +0.02%     
==========================================
  Files        2312     2313       +1     
  Lines       88532    88572      +40     
  Branches    24661    24675      +14     
==========================================
+ Hits        48000    48045      +45     
+ Misses      39040    39035       -5     
  Partials     1492     1492              
Flag Coverage Δ *Carryforward flag
adoption-insights 83.70% <ø> (ø) Carriedforward from 71cfae4
ai-integrations 67.95% <ø> (ø) Carriedforward from 71cfae4
app-defaults 69.79% <ø> (ø) Carriedforward from 71cfae4
augment 46.39% <ø> (ø) Carriedforward from 71cfae4
boost 74.35% <ø> (ø) Carriedforward from 71cfae4
bulk-import 72.46% <ø> (ø) Carriedforward from 71cfae4
cost-management 14.10% <ø> (ø) Carriedforward from 71cfae4
dcm 61.79% <ø> (ø) Carriedforward from 71cfae4
extensions 61.53% <ø> (ø) Carriedforward from 71cfae4
global-floating-action-button 71.18% <ø> (ø) Carriedforward from 71cfae4
global-header 59.71% <ø> (ø) Carriedforward from 71cfae4
homepage 49.84% <ø> (ø) Carriedforward from 71cfae4
install-dynamic-plugins 56.77% <ø> (ø) Carriedforward from 71cfae4
konflux 91.49% <ø> (ø) Carriedforward from 71cfae4
lightspeed 68.86% <88.13%> (+0.31%) ⬆️
mcp-integrations 85.46% <ø> (ø) Carriedforward from 71cfae4
orchestrator 38.30% <ø> (ø) Carriedforward from 71cfae4
quickstart 63.76% <ø> (ø) Carriedforward from 71cfae4
sandbox 79.56% <ø> (ø) Carriedforward from 71cfae4
scorecard 82.67% <ø> (ø) Carriedforward from 71cfae4
theme 61.26% <ø> (ø) Carriedforward from 71cfae4
translations 7.25% <ø> (ø) Carriedforward from 71cfae4
x2a 78.68% <ø> (ø) Carriedforward from 71cfae4

*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 71cfae4...f2cf76d. 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 Jdubrick marked this pull request as ready for review June 22, 2026 16:18
@rhdh-qodo-merge

Copy link
Copy Markdown

PR Summary by Qodo

Add per-user rate limiting to Lightspeed backend (incl. Notebooks)
✨ Enhancement ⚙️ Configuration changes 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Grey Divider

Description

• Enforce per-user request rate limits on Lightspeed and Notebooks authenticated endpoints.
• Add admin-configurable limits (general vs expensive tiers) with safe defaults.
• Document behavior and add tests for 429/Retry-After and health endpoint exclusions.
Diagram

graph TD
A["Authenticated client"] --> B["Lightspeed backend"] --> C["Identity middleware"] --> D["Rate limiter (general/expensive)"] --> E["Lightspeed routes"] --> F["Lightspeed Core / LLM"]
B --> C --> G["Notebooks routes"] --> D --> F
H["Backstage config"] --> D
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use a distributed rate-limit store (e.g., Redis)
  • ➕ Works correctly across multiple backend replicas behind a load balancer
  • ➕ Survives process restarts and provides more consistent enforcement
  • ➖ Requires additional infrastructure and operational configuration
  • ➖ More complexity than the current in-memory default store
2. Enforce rate limits at the ingress/API gateway
  • ➕ Centralized enforcement for all services and routes
  • ➕ No per-process state; scales naturally
  • ➖ Harder to key by Backstage userEntityRef unless propagated consistently
  • ➖ Less flexible for per-route tiering within the plugin
3. Adopt token-bucket/sliding-window limiting for smoother behavior
  • ➕ Avoids fixed-window bursts at minute boundaries
  • ➕ Often perceived as fairer under sustained load
  • ➖ More implementation complexity than express-rate-limit defaults
  • ➖ May still need a shared store for multi-replica deployments

Recommendation: The PR’s approach (tiered per-user limits with clear 429/Retry-After semantics and admin overrides) is a good baseline and is appropriately covered by tests and documentation. The main strategic follow-up to consider is swapping the default in-memory store for a shared store (e.g., Redis) if this backend is commonly deployed with multiple replicas; otherwise limits will be enforced per-instance rather than globally.

Files changed (12) +577 / -25

Enhancement (5) +149 / -25
constant.tsIntroduce default rate limit constants +7/-0

Introduce default rate limit constants

• Defines a fixed 1-minute rate limit window and default max request counts for expensive and general tiers.

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

express.d.tsAugment Express Request with rateLimit metadata +3/-0

Augment Express Request with rateLimit metadata

• Extends the Express request typing to include the rateLimit info populated by express-rate-limit, enabling handler logic to compute Retry-After from resetTime.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/express.d.ts

createRateLimitMiddleware.tsAdd tiered per-user rate limiting middleware +76/-0

Add tiered per-user rate limiting middleware

• Implements getRateLimitMax and createRateLimitMiddleware using express-rate-limit, keyed by userEntityRef, with a consistent 429 error response and Retry-After header. Supports disabling a tier by setting max to 0.

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

notebooksRouters.tsApply rate limiting to Notebooks endpoints by tier +14/-0

Apply rate limiting to Notebooks endpoints by tier

• Introduces general and expensive rate limiter middleware and wires them into notebooks session CRUD, document upload, and query routes, aligning expensive operations with the tighter tier.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouters.ts

router.tsWire rate limiting into Lightspeed backend routes +49/-25

Wire rate limiting into Lightspeed backend routes

• Creates tiered rate limiters and applies them across authenticated Lightspeed endpoints (MCP management, proxy routes, feedback, query). Ensures notebook conversation ID route is also throttled under the general tier.

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

Tests (3) +353 / -0
createRateLimitMiddleware.test.tsAdd unit tests for rate limit middleware and config defaults +163/-0

Add unit tests for rate limit middleware and config defaults

• Covers default max values, configured overrides, disabling with max=0, per-user separation, and the 429 JSON error + Retry-After header behavior.

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

notebooksRouter.test.tsAdd Notebooks rate limiting integration tests +92/-0

Add Notebooks rate limiting integration tests

• Adds tests validating expensive vs general tier behavior on notebooks endpoints, confirms 429/Retry-After on limit exceeded, and verifies health endpoints are not rate limited.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouter.test.ts

router.test.tsAdd Lightspeed router rate limiting integration tests +98/-0

Add Lightspeed router rate limiting integration tests

• Validates rate limiting on /v1/query (expensive) and a general endpoint, ensures health endpoints remain unthrottled, and confirms expensive/general counters are independent.

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

Documentation (1) +37 / -0
README.mdDocument rate limiting config and behavior +37/-0

Document rate limiting config and behavior

• Documents the new lightspeed.rateLimit configuration, default tier limits, excluded health endpoints, and the 429/Retry-After response contract with an example configuration.

workspaces/lightspeed/plugins/lightspeed-backend/README.md

Other (3) +38 / -0
five-pots-change.mdAdd changeset for Lightspeed backend rate limiting +5/-0

Add changeset for Lightspeed backend rate limiting

• Introduces a changeset marking a minor version bump for the Lightspeed backend package due to the new rate limiting behavior.

workspaces/lightspeed/.changeset/five-pots-change.md

config.d.tsExtend backend config schema with rateLimit tiers +32/-0

Extend backend config schema with rateLimit tiers

• Adds typed configuration for lightspeed.rateLimit with expensive/general tiers and default semantics (including disabling a tier with max: 0).

workspaces/lightspeed/plugins/lightspeed-backend/config.d.ts

package.jsonAdd express-rate-limit dependency +1/-0

Add express-rate-limit dependency

• Adds express-rate-limit to implement per-user request throttling in the backend.

workspaces/lightspeed/plugins/lightspeed-backend/package.json

@rhdh-qodo-merge

rhdh-qodo-merge Bot commented Jun 22, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used
✅ Tickets: RHIDP-13061

Grey Divider


Remediation recommended

1. Rate limit max unbounded ✓ Resolved 🐞 Bug ☼ Reliability
Description
createRateLimitMiddleware passes lightspeed.rateLimit.*.max through without validating it is a
non-negative integer, and only treats exactly 0 as disabled. A misconfigured negative/decimal
value can therefore produce incorrect throttling behavior instead of cleanly defaulting or
disabling.
Code

workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts[R31-56]

+export function getRateLimitMax(config: Config, tier: RateLimitTier): number {
+  const configKey =
+    tier === 'expensive'
+      ? 'lightspeed.rateLimit.expensive.max'
+      : 'lightspeed.rateLimit.general.max';
+  const defaultMax =
+    tier === 'expensive'
+      ? DEFAULT_EXPENSIVE_RATE_LIMIT_MAX
+      : DEFAULT_GENERAL_RATE_LIMIT_MAX;
+
+  return config.getOptionalNumber(configKey) ?? defaultMax;
+}
+
+export function createRateLimitMiddleware(
+  config: Config,
+  tier: RateLimitTier,
+): RequestHandler {
+  const max = getRateLimitMax(config, tier);
+
+  if (max === 0) {
+    return (_req, _res, next) => next();
+  }
+
+  return rateLimit({
+    windowMs: RATE_LIMIT_WINDOW_MS,
+    max,
Relevance

⭐⭐⭐ High

Team frequently accepts config/input hardening; e.g., added strict request-size validation and tests
in PR #3359.

PR-#3359

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The configured max is returned verbatim and then passed to rateLimit({ max }) with no bounds
checking beyond max === 0.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts[31-59]

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

### Issue description
Rate limit `max` values are accepted from config and used directly, but there is no guard that the value is an integer >= 0.

### Issue Context
Admins can customize these values; a simple typo (e.g. `-1` or `1.5`) would not be normalized/handled explicitly.

### Fix Focus Areas
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts[31-56]

### Suggested fix
- Validate `configuredMax` before returning it:
 - If `configuredMax` is `undefined`, use the default.
 - If `configuredMax <= 0`, treat it as disabled (0) or fallback to default (pick one consistent behavior; README currently documents 0=disable).
 - Otherwise, coerce to an integer (e.g. `Math.floor`) and use it.
- Optionally log a warning when the configured value is invalid and you fallback/normalize.

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


2. Rate limit not shared 🐞 Bug ☼ Reliability
Description
The rate limiter uses express-rate-limit’s default in-memory store (no store configured), so
counters are per backend process. In multi-replica deployments, users can exceed the intended
per-user limit by distributing requests across instances.
Code

workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts[R54-59]

+  return rateLimit({
+    windowMs: RATE_LIMIT_WINDOW_MS,
+    max,
+    standardHeaders: false,
+    legacyHeaders: false,
+    keyGenerator: req => getIdentity(req).userEntityRef,
Relevance

⭐⭐ Medium

No prior evidence requiring distributed rate-limit stores; repo often keeps simple in-process
approaches and rejects extra infra complexity.

PR-#2671
PR-#2861

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The rateLimit options do not include a store, so the library’s default in-memory store is used,
which keeps counters local to the running process.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts[54-59]

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

### Issue description
Rate limiting currently uses the default per-process in-memory store, which does not enforce a global per-user limit across multiple running instances.

### Issue Context
This backend can be deployed with multiple replicas; per-process counters weaken abuse prevention.

### Fix Focus Areas
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts[54-75]
- workspaces/lightspeed/plugins/lightspeed-backend/README.md[46-66]

### Suggested fix
- Add an optional configuration-driven shared store (e.g. Redis-backed store) and wire it into the `rateLimit({ store: ... })` option.
- If you do not want to add a store dependency now, document explicitly in the README that the limiter is per-instance and that global limits require a shared store or a single replica.

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


3. Notebooks limiter after auth ✓ Resolved 🐞 Bug ➹ Performance
Description
In createNotebooksRouter, requireNotebooksPermission is mounted for all /v1 routes before the
per-route rate limiters, so even requests that exceed limits still perform permission authorization
before returning 429. This leaves the permissions backend effectively unthrottled and adds avoidable
load on blocked requests.
Code

workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouters.ts[R275-278]

  notebooksRouter.post(
    '/v1/sessions',
+    generalRateLimiter,
    withAuth(async (req, res, userId) => {
Relevance

⭐⭐ Medium

No clear historical pattern on middleware ordering for rate limiting vs permission/auth; evidence
insufficient to predict acceptance.

PR-#3227

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The Notebooks router registers requireNotebooksPermission for /v1 before defining routes that
add generalRateLimiter, so Express will always execute the permission middleware before the
limiter for those endpoints.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouters.ts[268-278]

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

### Issue description
Notebooks requests hit `requireNotebooksPermission` before `generalRateLimiter`/`expensiveRateLimiter`, meaning the permission authorization work still happens even when the request should be rejected with 429.

### Issue Context
`requireNotebooksPermission` is registered via `notebooksRouter.use('/v1', ...)`, which runs before any route-level middleware for matching routes.

### Fix Focus Areas
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouters.ts[268-310]

### Suggested fix
1. Remove the global `notebooksRouter.use('/v1', requireNotebooksPermission)`.
2. Add `requireNotebooksPermission` to each `/v1/...` route after the appropriate limiter, e.g.:
  - General endpoints: `generalRateLimiter, requireNotebooksPermission, withAuth(...)`
  - Expensive endpoints: `expensiveRateLimiter, requireNotebooksPermission, ...`
This preserves tier-specific limits while ensuring permission checks are also protected by the limiter.

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


Grey Divider

Qodo Logo

@rhdh-qodo-merge rhdh-qodo-merge Bot added documentation Improvements or additions to documentation enhancement New feature or request Tests labels Jun 22, 2026
@Jdubrick

Copy link
Copy Markdown
Contributor Author

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used
✅ Tickets: RHIDP-13061
Grey Divider

Remediation recommended
  1. Rate limit max unbounded 🐞 Bug ☼ Reliability

Description

createRateLimitMiddleware passes lightspeed.rateLimit.*.max through without validating it is a
non-negative integer, and only treats exactly 0 as disabled. A misconfigured negative/decimal
value can therefore produce incorrect throttling behavior instead of cleanly defaulting or
disabling.

Code

[workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts[R31-56]](https://github.com/redhat-developer/rhdh-plugins/pull/3531/files#diff-643e1db0715770367846633559fa54e78fb569fe9c06e25e37ba7df4eabd514eR31-R56)

+export function getRateLimitMax(config: Config, tier: RateLimitTier): number {
+  const configKey =
+    tier === 'expensive'
+      ? 'lightspeed.rateLimit.expensive.max'
+      : 'lightspeed.rateLimit.general.max';
+  const defaultMax =
+    tier === 'expensive'
+      ? DEFAULT_EXPENSIVE_RATE_LIMIT_MAX
+      : DEFAULT_GENERAL_RATE_LIMIT_MAX;
+
+  return config.getOptionalNumber(configKey) ?? defaultMax;
+}
+
+export function createRateLimitMiddleware(
+  config: Config,
+  tier: RateLimitTier,
+): RequestHandler {
+  const max = getRateLimitMax(config, tier);
+
+  if (max === 0) {
+    return (_req, _res, next) => next();
+  }
+
+  return rateLimit({
+    windowMs: RATE_LIMIT_WINDOW_MS,
+    max,

Relevance

⭐⭐⭐ High
Team frequently accepts config/input hardening; e.g., added strict request-size validation and tests
in PR #3359.

PR-#3359
ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The configured max is returned verbatim and then passed to rateLimit({ max }) with no bounds
checking beyond max === 0.

[workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts[31-59]](https://github.com/redhat-developer/rhdh-plugins/blob/0688717981085b41d4910993973ade542bcecb92/workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts/#L31-L59)

Agent prompt

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

### Issue description
Rate limit `max` values are accepted from config and used directly, but there is no guard that the value is an integer >= 0.

### Issue Context
Admins can customize these values; a simple typo (e.g. `-1` or `1.5`) would not be normalized/handled explicitly.

### Fix Focus Areas
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts[31-56]

### Suggested fix
- Validate `configuredMax` before returning it:
 - If `configuredMax` is `undefined`, use the default.
 - If `configuredMax <= 0`, treat it as disabled (0) or fallback to default (pick one consistent behavior; README currently documents 0=disable).
 - Otherwise, coerce to an integer (e.g. `Math.floor`) and use it.
- Optionally log a warning when the configured value is invalid and you fallback/normalize.

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

  1. Notebooks limiter after auth 🐞 Bug ➹ Performance

Description

In createNotebooksRouter, requireNotebooksPermission is mounted for all /v1 routes before the
per-route rate limiters, so even requests that exceed limits still perform permission authorization
before returning 429. This leaves the permissions backend effectively unthrottled and adds avoidable
load on blocked requests.

Code

[workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouters.ts[R275-278]](https://github.com/redhat-developer/rhdh-plugins/pull/3531/files#diff-420fde566d04ee75dee2a5f148a9b59c5089cf085c831c85c6567fe3337ea3bdR275-R278)

  notebooksRouter.post(
    '/v1/sessions',
+    generalRateLimiter,
    withAuth(async (req, res, userId) => {

Relevance

⭐⭐ Medium
No clear historical pattern on middleware ordering for rate limiting vs permission/auth; evidence
insufficient to predict acceptance.

PR-#3227
ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The Notebooks router registers requireNotebooksPermission for /v1 before defining routes that
add generalRateLimiter, so Express will always execute the permission middleware before the
limiter for those endpoints.

[workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouters.ts[268-278]](https://github.com/redhat-developer/rhdh-plugins/blob/0688717981085b41d4910993973ade542bcecb92/workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouters.ts/#L268-L278)

Agent prompt

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

### Issue description
Notebooks requests hit `requireNotebooksPermission` before `generalRateLimiter`/`expensiveRateLimiter`, meaning the permission authorization work still happens even when the request should be rejected with 429.

### Issue Context
`requireNotebooksPermission` is registered via `notebooksRouter.use('/v1', ...)`, which runs before any route-level middleware for matching routes.

### Fix Focus Areas
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouters.ts[268-310]

### Suggested fix
1. Remove the global `notebooksRouter.use('/v1', requireNotebooksPermission)`.
2. Add `requireNotebooksPermission` to each `/v1/...` route after the appropriate limiter, e.g.:
  - General endpoints: `generalRateLimiter, requireNotebooksPermission, withAuth(...)`
  - Expensive endpoints: `expensiveRateLimiter, requireNotebooksPermission, ...`
This preserves tier-specific limits while ensuring permission checks are also protected by the limiter.

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

  1. Rate limit not shared 🐞 Bug ☼ Reliability

Description

The rate limiter uses express-rate-limit’s default in-memory store (no store configured), so
counters are per backend process. In multi-replica deployments, users can exceed the intended
per-user limit by distributing requests across instances.

Code

[workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts[R54-59]](https://github.com/redhat-developer/rhdh-plugins/pull/3531/files#diff-643e1db0715770367846633559fa54e78fb569fe9c06e25e37ba7df4eabd514eR54-R59)

+  return rateLimit({
+    windowMs: RATE_LIMIT_WINDOW_MS,
+    max,
+    standardHeaders: false,
+    legacyHeaders: false,
+    keyGenerator: req => getIdentity(req).userEntityRef,

Relevance

⭐⭐ Medium
No prior evidence requiring distributed rate-limit stores; repo often keeps simple in-process
approaches and rejects extra infra complexity.

PR-#2671
PR-#2861
ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The rateLimit options do not include a store, so the library’s default in-memory store is used,
which keeps counters local to the running process.

[workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts[54-59]](https://github.com/redhat-developer/rhdh-plugins/blob/0688717981085b41d4910993973ade542bcecb92/workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts/#L54-L59)

Agent prompt

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

### Issue description
Rate limiting currently uses the default per-process in-memory store, which does not enforce a global per-user limit across multiple running instances.

### Issue Context
This backend can be deployed with multiple replicas; per-process counters weaken abuse prevention.

### Fix Focus Areas
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts[54-75]
- workspaces/lightspeed/plugins/lightspeed-backend/README.md[46-66]

### Suggested fix
- Add an optional configuration-driven shared store (e.g. Redis-backed store) and wire it into the `rateLimit({ store: ... })` option.
- If you do not want to add a store dependency now, document explicitly in the README that the limiter is per-instance and that global limits require a shared store or a single replica.

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

Grey Divider

Qodo Logo

The architecture of Lightspeed should be revisited in the future to look into implementing non-memory only solutions for rate limiting. Currently it is assumed RHDH/Lightspeed is running in a single instance, but extending it to allow for a configurable redis store would be nice to have.

@Jdubrick Jdubrick force-pushed the add-rate-limiting-lightspeed branch from 4154c15 to b039138 Compare June 23, 2026 17:22
@Jdubrick

Copy link
Copy Markdown
Contributor Author

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 25, 2026

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure · Started 1:24 PM UTC · Completed 1:39 PM UTC
Commit: 381f36a · View workflow run →

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Critical

  • [api-contract] workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts:39 — Config namespace mismatch: getRateLimitMax reads from lightspeed.rateLimit.expensive.max and lightspeed.rateLimit.general.max, but the project has migrated to the intelligent-assistant config namespace. All other config reads in router.ts and notebooksRouters.ts use intelligent-assistant.*. The plugin's plugin.ts explicitly warns that the lightspeed key is deprecated and "no longer read." In production, users who configure rate limits under the correct intelligent-assistant.rateLimit path will have their settings silently ignored and the defaults will always apply. The config.d.ts schema in this same PR correctly defines rateLimit under intelligent-assistant, further confirming the inconsistency.
    Remediation: Change the config keys from lightspeed.rateLimit.{tier}.max to intelligent-assistant.rateLimit.{tier}.max.

High

  • [test-integrity] workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouter.test.ts:460 — The rate-limiting beforeEach block creates config with the lightspeed key, but createNotebooksRouter reads required config via config.getString('intelligent-assistant.notebooks.queryDefaults.model'). Since getString throws when the key is absent, createNotebooksRouter will throw during test setup, making this test block non-functional. The existing (non-rate-limit) tests correctly use intelligent-assistant.
    Remediation: Change the rate-limit test config from lightspeed: { ... } to 'intelligent-assistant': { ... }.

Medium

  • [test-integrity] workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.test.ts:1143RATE_LIMIT_CONFIG spreads BASE_CONFIG.lightspeed (which is undefined since BASE_CONFIG uses intelligent-assistant). The rate limit config is placed under the wrong namespace, which matches the buggy middleware code but not the documented config schema.
    Remediation: Change RATE_LIMIT_CONFIG to place rateLimit under intelligent-assistant.

  • [test-integrity] workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.test.ts:189 — All unit tests use lightspeed as the config key, matching the buggy code but not the intended intelligent-assistant namespace. Tests will pass but are testing the wrong contract.
    Remediation: Update test config data to use intelligent-assistant after fixing the middleware.

  • [stale-doc] workspaces/lightspeed/plugins/lightspeed-backend/README.md:39 — The new rate limit YAML examples use lightspeed: as the top-level key, which contradicts the migration guide in the same README that states lightspeed: was renamed to intelligent-assistant: and is no longer recognized.
    Remediation: Change the YAML examples to use intelligent-assistant: as the top-level config key.

  • [design-smell] workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouters.ts:269 — The blanket notebooksRouter.use("/v1", requireNotebooksPermission) was removed and replaced with per-route application. While all 10 existing /v1 routes received the per-route permission middleware (no current permission gap), any future route added under /v1 will now silently lack permission enforcement unless the developer remembers to add requireNotebooksPermission. This is a defense-in-depth regression.
    Remediation: Consider keeping the blanket requireNotebooksPermission middleware and applying rate limiters per-route instead.

Low

  • [fail-open] workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts:48 — Setting max to 0 (or any negative number) completely disables rate limiting for that tier, returning a passthrough middleware. While documented as intentional, an operator misconfiguring the value will silently disable rate limiting. Consider adding a logger.warn() when rate limiting is disabled.

Labels: PR adds rate limiting security feature to the lightspeed backend plugin

@Jdubrick Jdubrick force-pushed the add-rate-limiting-lightspeed branch from b039138 to 3dbe16b Compare June 25, 2026 13:56
@Jdubrick

Copy link
Copy Markdown
Contributor Author

@fullsend

Review

Findings

Critical

  • [api-contract] workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts:39 — Config namespace mismatch: getRateLimitMax reads from lightspeed.rateLimit.expensive.max and lightspeed.rateLimit.general.max, but the project has migrated to the intelligent-assistant config namespace. All other config reads in router.ts and notebooksRouters.ts use intelligent-assistant.*. The plugin's plugin.ts explicitly warns that the lightspeed key is deprecated and "no longer read." In production, users who configure rate limits under the correct intelligent-assistant.rateLimit path will have their settings silently ignored and the defaults will always apply. The config.d.ts schema in this same PR correctly defines rateLimit under intelligent-assistant, further confirming the inconsistency.
    Remediation: Change the config keys from lightspeed.rateLimit.{tier}.max to intelligent-assistant.rateLimit.{tier}.max.

High

  • [test-integrity] workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouter.test.ts:460 — The rate-limiting beforeEach block creates config with the lightspeed key, but createNotebooksRouter reads required config via config.getString('intelligent-assistant.notebooks.queryDefaults.model'). Since getString throws when the key is absent, createNotebooksRouter will throw during test setup, making this test block non-functional. The existing (non-rate-limit) tests correctly use intelligent-assistant.
    Remediation: Change the rate-limit test config from lightspeed: { ... } to 'intelligent-assistant': { ... }.

Medium

  • [test-integrity] workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.test.ts:1143RATE_LIMIT_CONFIG spreads BASE_CONFIG.lightspeed (which is undefined since BASE_CONFIG uses intelligent-assistant). The rate limit config is placed under the wrong namespace, which matches the buggy middleware code but not the documented config schema.
    Remediation: Change RATE_LIMIT_CONFIG to place rateLimit under intelligent-assistant.
  • [test-integrity] workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.test.ts:189 — All unit tests use lightspeed as the config key, matching the buggy code but not the intended intelligent-assistant namespace. Tests will pass but are testing the wrong contract.
    Remediation: Update test config data to use intelligent-assistant after fixing the middleware.
  • [stale-doc] workspaces/lightspeed/plugins/lightspeed-backend/README.md:39 — The new rate limit YAML examples use lightspeed: as the top-level key, which contradicts the migration guide in the same README that states lightspeed: was renamed to intelligent-assistant: and is no longer recognized.
    Remediation: Change the YAML examples to use intelligent-assistant: as the top-level config key.
  • [design-smell] workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouters.ts:269 — The blanket notebooksRouter.use("/v1", requireNotebooksPermission) was removed and replaced with per-route application. While all 10 existing /v1 routes received the per-route permission middleware (no current permission gap), any future route added under /v1 will now silently lack permission enforcement unless the developer remembers to add requireNotebooksPermission. This is a defense-in-depth regression.
    Remediation: Consider keeping the blanket requireNotebooksPermission middleware and applying rate limiters per-route instead.

Low

  • [fail-open] workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/createRateLimitMiddleware.ts:48 — Setting max to 0 (or any negative number) completely disables rate limiting for that tier, returning a passthrough middleware. While documented as intentional, an operator misconfiguring the value will silently disable rate limiting. Consider adding a logger.warn() when rate limiting is disabled.

Labels: PR adds rate limiting security feature to the lightspeed backend plugin

The design-smell is not appropriate. Moving to per-route application lets us maintain the ordering, and makes each route's design intentional. While there is a risk that a new route is missed, that is a review and testing problem.

* Per-user rate limiting for Lightspeed API endpoints.
* @visibility backend
*/
rateLimit?: {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add some commented out configs in the app-config file so we have an easy reference directly..

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But i see you updated the README

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I updated the readme but missed this section, I updated it with an example in f2cf76d

Jdubrick added 4 commits June 26, 2026 09:18
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Jdubrick added 4 commits June 26, 2026 09:18
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
… rate limit -> permission check ordering to reduce redundant permission checks

Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
@Jdubrick Jdubrick force-pushed the add-rate-limiting-lightspeed branch from 3dbe16b to f2cf76d Compare June 26, 2026 13:21
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request Tests workspace/lightspeed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants