Add RDI native API proxy endpoint for @rdi-ui/pipeline - #6504
Merged
Merged
Conversation
@rdi-ui/pipeline ships its own SDK that speaks the native RDI API. Route those calls through RedisInsight so instance credentials and self-signed-certificate handling stay server-side and the browser never talks to RDI directly - which also takes CORS out of the picture. Deliberately scoped to a narrow JSON passthrough for that one client rather than a general-purpose proxy, since "transparent" is what makes this kind of endpoint hard to get right: - Headers are allowlisted in both directions. RedisInsight's own credentials (x-csrf-token, x-window-id) cannot leak upstream, and RDI cannot set origin-scoped policy (HSTS, CSP, report-to, nel) on RedisInsight's origin. A denylist would need extending every time either side adds a header. - Upstream redirects are refused with a 502 rather than followed or rewritten. Following one would make our backend issue the follow-up request with the RDI bearer token attached; rewriting Location back onto the proxy means reconstructing the global prefix and RI_PROXY_PATH. The RDI API has exactly one redirect (GET / -> /docs) and the SDK never requests it. - The upstream URL is resolved once, so the URL that gets validated is the URL that gets sent - no decoded-vs-encoded divergence. Containment is checked against rdi.url's path, so an RDI hosted under a subpath stays scoped and an absolute or scheme-relative path is rejected. - Non-JSON content types are refused with a 415. Every RDI endpoint is JSON, and the globally installed JSON body parser already enforces maxPayloadSize and its 413 handling. Responses are read as arraybuffer so axios never parses and re-serializes a payload, and carry a forced sandboxed CSP and nosniff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6925180165
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Contributor
Code Coverage - Backend unit tests
Test suite run success3924 tests passing in 331 suites. Report generated by 🧪jest coverage report action from 1b525e6 |
Contributor
Code Coverage - Integration Tests
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6925180. Configure here.
…containment Both from PR review on #6504. The content-type guard accepted `+json` suffixes (`application/merge-patch+json`, `application/problem+json`), but `bodyParser.json()`'s default type is the bare `application/json` and skips those - so `req.body` arrived undefined and the request was forwarded with an empty body and the caller's content type. That is the exact failure the guard was added to prevent. Narrowed the pattern to what the parser actually parses, verified case-by-case against `type-is` with body-parser's own type string, and documented that widening one without the other reintroduces the bug. RDI's API uses no `+json` type. Path containment only checked the URL as sent. `new URL()` correctly treats %2F as an opaque character rather than a separator, so `..%2fadmin` passed as a single segment - but an upstream (or a reverse proxy in front of RDI) that percent-decodes before normalizing dot segments reads the same path as `../admin` and lands outside the configured subpath, with the RDI bearer token attached. Now validates that reading as well, so containment holds whichever order the upstream uses. An encoded slash inside a real path segment still passes, since it resolves within the base either way - RDI's name path params are unconstrained strings, so banning %2F outright would have been wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc771aa474
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…encodings Review on #6504 reported `%252e%252e%252fadmin`: the previous fix only recognised direct `%2f`/`%5c`, so a nested encoding passed both containment checks and two decoding layers in front of RDI would normalize it to `/admin`. That is the fourth variant of one attack (`..`, then `%2e%2e`, then `..%2f`, now `%252e%252e%252f`), which is the real signal: adding a `%25` case would just invite `%2525`. Predicting how many layers decode, and in what order, is not a winnable game - there is always one more encoding, including overlong UTF-8 and invalid escapes. So this stops predicting. Every caller-supplied path segment must now be inert: it has to decode cleanly, and the result must not contain `/`, `\`, `?`, `#` or `%`, nor be a dot segment. `%` is the load-bearing case - encoding a `%` is the only way to nest encodings at all, so refusing it rules out every depth with one condition. This replaces the single-pass separator check rather than adding to it. Trade-off, taken deliberately: a pipeline name containing `/` or `%` can no longer be addressed through the proxy. This reverses the reasoning in my earlier review reply, which defended allowing an encoded separator inside a segment - that is precisely what keeps the decoding-layer question open. Names are identifier-shaped in practice and an encoded separator would not survive RDI's own path routing either. Spaces and non-ASCII characters are unaffected and covered by tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
VaskoAtanasovRedis
approved these changes
Sep 15, 2026
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
2 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.
What
@rdi-ui/pipelineships its own SDK that speaks the native RDI API. This routes those calls through RedisInsight's own API so instance credentials and self-signed-certificate handling stay server-side and the browser never talks to RDI directly — which also takes CORS out of the picture.Replaces #6503 (same goal, reworked from scratch off latest
main).The change in approach is the point. #6503 aimed at a transparent proxy, and that scope is what generated the review load — 23 findings across it and #6468, of which 20 fell into three buckets: redirect
Locationhandling (11), raw-body/payload limits (6), and header denylist misses (3). All three are consequences of transparency, not individual defects.This is deliberately a narrow JSON passthrough for one known client instead:
x-csrf-token,x-window-id) can't leak upstream, and RDI can't set origin-scoped policy (HSTS,CSP,report-to,nel) on RedisInsight's origin. A denylist needs extending every time either side adds a header.Locationback onto the proxy means reconstructing the global prefix andRI_PROXY_PATH. The RDI API has exactly one redirect (GET /→/docs) and the SDK never requests it. This also clears the CodeQL server-side-redirect alert from Add transparent proxy endpoint for RDI native API #6468.rdi.url's path, so an RDI hosted under a subpath stays scoped and an absolute or scheme-relative path is rejected.multipart,UploadFile,octet-stream,PlainTextResponse,media_type=orresponse_classanywhere), and the globally installed JSON body parser already enforcesmaxPayloadSizeand its 413 handling — somain.tsis untouched.Net effect: ~197 lines of source vs ~297 in #6503, and the two unbounded finding-generators are gone by construction rather than fixed case by case.
Structure follows the module:
RdiProxyController(HTTP adapter) →RdiProxyService(header/redirect policy) →ApiRdiClient.proxyRequest(transport, reusing the client's authenticated axios instance). URL handling lives inutils/rdi-proxy.util.tsas pure functions.proxyRequestis not added to the abstractRdiClient—ApiV2RdiClientextendsApiRdiClient, so both concrete clients get it while the abstraction stays clean.Testing
npx jest -w 1 src/modules/rdi— 367 tests / 29 suites passingnpm run type-check— baseline gate green, "no new errors"eslint+prettier --checkon all touched files — cleanreq.urlpreserves%2Fthrough routing (the assumption behindgetRdiUpstreamPath), and the barerdi/:id/proxyis a 404 by designresolveRdiUpstreamUrlhas 31 focused tests covering absolute-URL rejection, scheme-relative paths,..//%2e%2e/.%2eescapes, encoded-slash containment, trailing-slash normalisation, and the sibling-prefix case (/rdi-othervs base/rdi/)Manual verification against a live RDI instance has not been done yet — the checks above are automated only.
Known gaps
test/api/integration test, though the convention exists (test/api/rdi/GET-rdi-id-*.test.ts). Happy to add one if wanted.RdiAnalytics; a per-request proxy event looked like noise, but say the word.🤖 Generated with Claude Code
Note
High Risk
New server-side proxy to user-configured RDI hosts with bearer-token upstream calls; mitigations (URL containment, header allowlists, no redirects) are central to avoiding SSRF and credential/header leakage.
Overview
Adds a narrow JSON passthrough at
rdi/:id/proxy/*so the@rdi-ui/pipelineSDK can call the RDI native API through RedisInsight (credentials, TLS, and CORS stay server-side).HTTP surface:
RdiProxyControllerallows GET/POST/PUT/PATCH/DELETE, rejects non-application/jsonbodies with 415 (avoids forwarding empty bodies when the global parser did not run), and streams upstream status/headers/raw body back to the client.Policy layer:
RdiProxyServiceforwards only allowlisted request headers (content-type,accept) and response headers (content-type), injectscontent-security-policy: sandboxandx-content-type-options: nosniff, and turns upstream redirect statuses (301/302/303/307/308) into 502 instead of following or exposingLocation.Transport:
ApiRdiClient.proxyRequestresolves the target URL, reuses the authenticated axios client withvalidateStatus: null,maxRedirects: 0, andresponseType: 'arraybuffer'so non-2xx and odd JSON shapes pass through unchanged.URL safety:
rdi-proxy.utilstrips the proxy mount from the raw URL (preserving percent-encoding), builds the upstream URL once, enforces containment under the configuredrdi.url, and rejects traversal/double-encoding via segment inertness checks.Wired into
RdiModulewith newRdiProxyRequest/RdiProxyResponsetypes and unit tests across controller, service, client, and URL helpers.Reviewed by Cursor Bugbot for commit 1b525e6. Bugbot is set up for automated code reviews on this repo. Configure here.