Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
github-actions
Bot
added
scope/shell
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional endpoint-level upstreamStaticBody (schema, types, persistence, seeding), a ClickHouse Query connector and docs, and changes gateway header injection to prevent forwarding consumer Authorization when connector auth is basic (with a test). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant SecretsStore
participant Upstream
Client->>Gateway: HTTP request (may include consumer Authorization + body)
Gateway->>SecretsStore: Resolve connector secrets (username/password)
SecretsStore-->>Gateway: Return credentials
Gateway->>Gateway: buildUpstreamHeaders (remove consumer Authorization when authType = basic)
Gateway->>Upstream: Forward request with injected Basic Authorization and final body
Upstream-->>Gateway: Response
Gateway-->>Client: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
plugins/service-gateway/connectors/connector-template.schema.json (1)
188-192: Add conditional validation for static body transforms.Recommend requiring
upstreamStaticBodywhenbodyTransformisstaticto prevent invalid templates at authoring time.Schema hardening example
"items": { "type": "object", "required": ["name", "method", "path", "upstreamPath"], "additionalProperties": false, "properties": { ... "upstreamStaticBody": { "type": "string", "maxLength": 65536, "description": "Static body to send to upstream (used with bodyTransform: static)" } - } + }, + "allOf": [ + { + "if": { "properties": { "bodyTransform": { "const": "static" } } }, + "then": { "required": ["upstreamStaticBody"] } + } + ] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/connectors/connector-template.schema.json` around lines 188 - 192, Add a conditional JSON Schema rule so that when the property bodyTransform is set to "static" the upstreamStaticBody property is required and validated (e.g., non-empty string / maxLength preserved). Implement an if/then block in the connector-template.schema.json using "if": {"properties":{"bodyTransform":{"const":"static"}}} and "then": {"required":["upstreamStaticBody"], "properties":{"upstreamStaticBody":{"type":"string","minLength":1,"maxLength":65536}}} to enforce the dependency between bodyTransform and upstreamStaticBody.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web-next/src/app/api/v1/gw/admin/templates/route.ts`:
- Line 121: The line setting upstreamStaticBody uses the || null coalescing
which will convert empty strings to null and mismatches the seeding behavior;
change the expression that assigns upstreamStaticBody (the property in the
object returned/constructed in route.ts) from using || null to the nullish
coalescing operator ?? null so empty strings are preserved and behavior matches
bin/seed-public-connectors.ts.
In `@plugins/service-gateway/connectors/clickhouse-query.json`:
- Around line 18-20: The manifest currently lists authConfig and secretRefs with
two Basic-auth refs ("username","password") but uses a single envKey mapping
which writes the same value to both refs; update the connector schema and seed
logic to support per-secret environment mappings (e.g. add an envKeys object
like envKeys: { "username": "<ENV_VAR>", "password": "<ENV_VAR>" }) and change
the seed/loader that consumes envKey to prefer envKeys when present (fall back
to previous behavior only when secretRefs length == 1); ensure the code paths
handling "authConfig", "secretRefs" and the envKey-based seeding are updated to
map each secretRef to its corresponding envKeys entry instead of duplicating one
envKey for both secrets.
- Around line 13-15: Remove the hardcoded "allowedHosts" entry from the
connector JSON so host validation derives from the chosen "upstreamBaseUrl" at
runtime; specifically, delete the "allowedHosts":
["s7kh1yt2go.us-east-2.aws.clickhouse.cloud"] line in the clickhouse-query.json
so template overrides of "upstreamBaseUrl" won't be blocked and host
allowlisting is determined dynamically.
---
Nitpick comments:
In `@plugins/service-gateway/connectors/connector-template.schema.json`:
- Around line 188-192: Add a conditional JSON Schema rule so that when the
property bodyTransform is set to "static" the upstreamStaticBody property is
required and validated (e.g., non-empty string / maxLength preserved). Implement
an if/then block in the connector-template.schema.json using "if":
{"properties":{"bodyTransform":{"const":"static"}}} and "then":
{"required":["upstreamStaticBody"],
"properties":{"upstreamStaticBody":{"type":"string","minLength":1,"maxLength":65536}}}
to enforce the dependency between bodyTransform and upstreamStaticBody.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78c3ae3a-33c3-479b-a1bc-634b2f8e0bf6
📒 Files selected for processing (7)
apps/web-next/src/app/api/v1/gw/admin/templates/route.tsbin/seed-public-connectors.tsplugins/service-gateway/connectors/clickhouse-query.jsonplugins/service-gateway/connectors/connector-template.schema.jsonplugins/service-gateway/connectors/loader.tsplugins/service-gateway/docs/clickhouse-query-connector.mdplugins/service-gateway/docs/connector-catalog.md
Comment on lines
+18
to
+20
| "authConfig": { "usernameRef": "username", "passwordRef": "password" }, | ||
| "secretRefs": ["username", "password"], | ||
| "streamingEnabled": false, |
There was a problem hiding this comment.
envKey mapping is incompatible with two Basic-auth secret refs.
secretRefs requires username and password, but envKey provides only one value. In current seed flow, that single value is written to both refs, which can break auth setup.
Suggested direction
- "envKey": "CLICKHOUSE_QUERY_API_KEY"
+ "envKey": nullOr introduce per-secret env mapping in schema/seed logic (e.g. envKeys: { "username": "...", "password": "..." }).
Also applies to: 65-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/service-gateway/connectors/clickhouse-query.json` around lines 18 -
20, The manifest currently lists authConfig and secretRefs with two Basic-auth
refs ("username","password") but uses a single envKey mapping which writes the
same value to both refs; update the connector schema and seed logic to support
per-secret environment mappings (e.g. add an envKeys object like envKeys: {
"username": "<ENV_VAR>", "password": "<ENV_VAR>" }) and change the seed/loader
that consumes envKey to prefer envKeys when present (fall back to previous
behavior only when secretRefs length == 1); ensure the code paths handling
"authConfig", "secretRefs" and the envKey-based seeding are updated to map each
secretRef to its corresponding envKeys entry instead of duplicating one envKey
for both secrets.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web-next/src/lib/gateway/__tests__/transform.test.ts (2)
199-200: Minor: Empty request init object is unnecessary.The empty object can be removed for cleaner code:
🧹 Suggested cleanup
- const request = new Request('https://example.com', { - }); + const request = new Request('https://example.com');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/lib/gateway/__tests__/transform.test.ts` around lines 199 - 200, The test creates a Request with an unnecessary empty init object; update the Request instantiation in transform.test.ts (the const request variable) to call new Request('https://example.com') without the second argument to clean up the code.
205-205: Remove stale assertion checking for non-existent value.The string
'gw_not_forwarded'doesn't appear anywhere in this test's setup. This assertion always passes trivially and adds confusion about what's being tested. Consider removing it or replacing it with an assertion that tests actual values from the test setup.🧹 Suggested cleanup
const expected = `Basic ${Buffer.from('admin:secret').toString('base64')}`; expect(result.headers.get('Authorization')).toBe(expected); - expect(result.headers.get('Authorization')).not.toContain('gw_not_forwarded'); expect(result.headers.get('Authorization')).not.toContain('consumer-style-token');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/lib/gateway/__tests__/transform.test.ts` at line 205, Remove the stale assertion that checks for the non-existent string 'gw_not_forwarded' on the Authorization header; find the line with expect(result.headers.get('Authorization')).not.toContain('gw_not_forwarded') and either delete it or replace it with a meaningful assertion that verifies the actual expected Authorization value from the test setup (e.g., assert equality or that it contains the real token/header fragment), keeping the check focused on result.headers.get('Authorization').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web-next/src/lib/gateway/__tests__/transform.test.ts`:
- Around line 199-200: The test creates a Request with an unnecessary empty init
object; update the Request instantiation in transform.test.ts (the const request
variable) to call new Request('https://example.com') without the second argument
to clean up the code.
- Line 205: Remove the stale assertion that checks for the non-existent string
'gw_not_forwarded' on the Authorization header; find the line with
expect(result.headers.get('Authorization')).not.toContain('gw_not_forwarded')
and either delete it or replace it with a meaningful assertion that verifies the
actual expected Authorization value from the test setup (e.g., assert equality
or that it contains the real token/header fragment), keeping the check focused
on result.headers.get('Authorization').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6fd47ca3-ccf8-45d4-ae01-10712b9dc328
📒 Files selected for processing (2)
apps/web-next/src/lib/gateway/__tests__/transform.test.tsapps/web-next/src/lib/gateway/transform.ts
seanhanca
force-pushed
the
feat/support-clickhouse-query-connector
branch
from
31679df to
7d54ec8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/service-gateway/connectors/connector-template.schema.json`:
- Around line 194-199: The JSON Schema's conditional uses "if": { "properties":
{ "bodyTransform": { "const": "static" } } } which passes when bodyTransform is
missing; update the "if" to require the presence of bodyTransform by adding
"required": ["bodyTransform"] inside that "if" (so the conditional only matches
when bodyTransform exists and equals "static"), leaving the "then" unchanged
(which requires "upstreamStaticBody"); target the allOf conditional block
referencing bodyTransform and upstreamStaticBody when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 802b4a5a-f40f-4b0e-968e-c478a6978423
📒 Files selected for processing (2)
apps/web-next/src/lib/gateway/__tests__/transform.test.tsplugins/service-gateway/connectors/connector-template.schema.json
…support Wire `upstreamStaticBody` through the connector template pipeline (loader interface, JSON schema, admin template route, seed script) so endpoints can send a pre-configured request body to the upstream service. Add the `clickhouse-query` connector template with four endpoints: - /network_prices — static SQL for orchestrator pricing data - /query — dynamic SELECT-only queries with regex + blacklist - /ping — ClickHouse health check - /tables — list tables via SHOW TABLES Include a how-to guide with dashboard visualization example and update the connector catalog. Made-with: Cursor
…ded with basic auth Updated the buildUpstreamRequest and buildUpstreamHeaders functions to ensure that when the connector's authType is set to 'basic', the consumer's Authorization header is not forwarded to the upstream service. This change enhances security by preventing potential credential leaks. Added corresponding tests to verify this behavior.
- Use ?? null instead of || null for upstreamStaticBody to preserve empty strings and match the seed script's nullish coalescing - Remove hardcoded allowedHosts from clickhouse-query.json so host validation derives from upstreamBaseUrl at runtime, avoiding conflicts when users override the base URL - Remove envKey from clickhouse-query.json — a single env var cannot map to two separate Basic-auth secrets (username + password); users configure credentials via the Settings tab UI instead - Add JSDoc docstrings to all exported interfaces and functions in the connector template loader and route handlers Made-with: Cursor
- Add conditional JSON Schema validation requiring upstreamStaticBody when bodyTransform is "static" - Remove empty Request init object in basic-auth test - Remove stale gw_not_forwarded assertion that always passed trivially Made-with: Cursor
Without "required": ["bodyTransform"] in the if-block, endpoints that omit bodyTransform (defaulting to passthrough) would incorrectly pass the condition and be forced to provide upstreamStaticBody. Made-with: Cursor
seanhanca
force-pushed
the
feat/support-clickhouse-query-connector
branch
from
16610ca to
32deb62
Compare
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.
Summary
upstreamStaticBodythrough the connector template pipeline — adds the field to the TypeScript loader interface, JSON schema, admin template creation route, and seed script so endpoints can send a pre-configured static request body to the upstream service.clickhouse-queryconnector template — a new config-only connector that proxies SQL queries to any ClickHouse instance via its HTTP query interface with Basic auth, SELECT-only enforcement, and caching./network_pricesdata to populate dashboard widgets (pricing table + distribution chart).Endpoints
/ping/tables/query/network_pricesFiles changed
plugins/service-gateway/connectors/loader.tsupstreamStaticBodyto TypeScript interfaceplugins/service-gateway/connectors/connector-template.schema.jsonupstreamStaticBodyto JSON schemaapps/web-next/src/app/api/v1/gw/admin/templates/route.tsupstreamStaticBodyduring template instantiationbin/seed-public-connectors.tsupstreamStaticBodyduring seedplugins/service-gateway/connectors/clickhouse-query.jsonplugins/service-gateway/docs/clickhouse-query-connector.mdplugins/service-gateway/docs/connector-catalog.mdTest plan
GET /ping→ returnsOk.GET /tables→ returns table list in JSON formatPOST /querywithSELECT 1 FORMAT JSON→ returns{ "test_value": 1 }POST /network_prices→ returns 31 rows of orchestrator pricing dataMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes / Security
Documentation
Tests