Port our electric sql proxy to cloudflare#1867
Merged
Merged
Conversation
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
Contributor
|
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:
📝 WalkthroughWalkthroughRemoved Vercel cache-control rule for /api/electric; added an Electric Proxy Cloudflare Worker (JWT auth with JWKS caching, upstream URL & cache-key builders, per-table where-clause logic, Cloudflare Cache usage), local Wrangler/dev wiring and ports, desktop JWT plumbing with refresh, CI deploy job, and renderer defaults pointed to the proxy. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy as Electric Proxy (Worker)
participant Auth as Auth Service (JWKS)
participant Cache as Cloudflare Cache
participant Upstream as Electric Cloud
Client->>Proxy: GET /?table=...&organizationId=... (Authorization: Bearer <jwt>)
Proxy->>Auth: verify JWT (uses cached JWKS)
Auth-->>Proxy: JWT claims (sub, organizationIds) / error
Proxy->>Proxy: buildWhereClause(table, organizationId, organizationIds)
Proxy->>Proxy: buildUpstreamUrl(clientUrl, table, whereClause, env)
Proxy->>Cache: lookup(cacheKey)
alt cache hit
Cache-->>Proxy: cached Response
else cache miss
Proxy->>Upstream: fetch upstream URL (forward protocol params & credentials)
Upstream-->>Proxy: upstream Response (may include Cache-Control)
Proxy->>Cache: put(cacheKey, response) if cachable
end
Proxy-->>Client: Response + CORS headers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…WT auth Replace the API server's Electric proxy with a standalone Cloudflare Worker that authenticates via JWT instead of session tokens. This decouples Electric from the API server and enables edge caching via Cloudflare's Cache API. The worker supports both local Docker Electric (ELECTRIC_URL + ELECTRIC_SECRET) and Electric Cloud (ELECTRIC_SOURCE_ID + ELECTRIC_SOURCE_SECRET), matching the dual-mode pattern from the old API proxy. Desktop changes: - Rename electricJwt → jwt, add jwtClient() plugin to auth client - Fetch JWT from authClient.token() response body during hydration - Periodic JWT refresh every 50 minutes - Collections use JWT-only auth headers for the worker
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx (1)
51-69:⚠️ Potential issue | 🟠 MajorExplicitly fetch and update JWT after token change, especially when session refetch fails.
In the
data?.token && data?.expiresAtpath (line 53), the JWT is not refreshed. IfrefetchSession()fails, the JWT remains stale until the periodic refresh (50 minutes), causing Electric calls to fail silently.Add
setJwt(null)at the start and explicitly callauthClient.token()afterrefetchSession()(similar to the hydration path at lines 34-36) to ensure JWT is always fresh:Suggested fix
onData: async (data) => { if (data?.token && data?.expiresAt) { + setJwt(null); setAuthToken(null); await authClient.signOut({ fetchOptions: { throw: false } }); setAuthToken(data.token); try { await refetchSession(); } catch (err) { console.warn( "[AuthProvider] session refetch failed after token change", err, ); } + try { + const res = await authClient.token(); + setJwt(res.data?.token ?? null); + } catch (err) { + console.warn("[AuthProvider] JWT fetch failed after token change", err); + setJwt(null); + } setIsHydrated(true); } else if (data === null) {Also applies to: lines 81-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx` around lines 51 - 69, In the electronTrpc.auth.onTokenChanged.useSubscription handler (the branch when data?.token && data?.expiresAt), clear the stored JWT immediately by calling setJwt(null) before changing tokens, then after refetchSession() (or if refetchSession throws) explicitly fetch the fresh JWT via authClient.token() and update state (setJwt(...))—mirroring the hydration logic used earlier; apply the same changes to the other token-change branch mentioned (lines ~81-99) so JWT is always refreshed after setAuthToken and refetchSession attempts, and ensure setIsHydrated remains set afterward.
🧹 Nitpick comments (1)
apps/electric-proxy/src/auth.ts (1)
4-11: Cache JWKS byAUTH_URL, not globally.Current singleton caching can reuse keys from the wrong issuer if
AUTH_URLchanges in-process (tests, multi-env workers).♻️ Suggested refactor
-let jwks: ReturnType<typeof createRemoteJWKSet> | null = null; +const jwksByAuthUrl = new Map<string, ReturnType<typeof createRemoteJWKSet>>(); function getJWKS(authUrl: string): ReturnType<typeof createRemoteJWKSet> { - if (!jwks) { - jwks = createRemoteJWKSet(new URL("/api/auth/jwks", authUrl)); - } - return jwks; + let jwks = jwksByAuthUrl.get(authUrl); + if (!jwks) { + jwks = createRemoteJWKSet(new URL("/api/auth/jwks", authUrl)); + jwksByAuthUrl.set(authUrl, jwks); + } + return jwks; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electric-proxy/src/auth.ts` around lines 4 - 11, The current getJWKS function uses a single module-level jwks singleton which can return keys for the wrong issuer if AUTH_URL changes; replace the single jwks variable with a Map keyed by the authUrl string (e.g., jwksByUrl: Map<string, ReturnType<typeof createRemoteJWKSet>>), update getJWKS(authUrl) to check jwksByUrl.get(authUrl) and only create and store a new createRemoteJWKSet(new URL("/api/auth/jwks", authUrl)) when missing, and return the map entry; ensure types remain ReturnType<typeof createRemoteJWKSet> so other callers of getJWKS continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.superset/lib/setup/steps.sh:
- Around line 522-529: The heredoc that writes apps/electric-proxy/.dev.vars
currently only emits cloud-related variables (ELECTRIC_CLOUD_URL,
ELECTRIC_SOURCE_ID, ELECTRIC_SOURCE_SECRET) and drops local-mode settings;
update the .dev.vars generation (the heredoc producing AUTH_URL=...,
ELECTRIC_CLOUD_URL=..., etc.) to always include the local-mode variables (e.g.,
AUTH_URL and a local ELECTRIC_MODE or ELECTRIC_LOCAL_* defaults) and only
append/export ELECTRIC_SOURCE_ID and ELECTRIC_SOURCE_SECRET (and override
ELECTRIC_CLOUD_URL) when those cloud env vars are present/non-empty; implement
this by conditionally adding the cloud lines to the heredoc or by writing the
base local vars first and then appending cloud vars when [ -n
"$ELECTRIC_SOURCE_ID" ] or similar, keeping the existing AUTH_URL line
unchanged.
In `@apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx`:
- Around line 34-37: The call to authClient.token() inside the AuthProvider
hydration flow can throw and currently is unhandled, which may prevent
setIsHydrated(true) from running; wrap the await authClient.token() call (in the
same function that calls setJwt and setIsHydrated) in a try/catch (or use
.catch()) so any rejection is swallowed/handled and still allow
setIsHydrated(true) to run (or move setIsHydrated(true) into a finally block);
specifically update the code around authClient.token(), setJwt(...) and
setIsHydrated(...) in AuthProvider.tsx to catch errors and optionally log them,
then ensure setIsHydrated(true) executes regardless of token request outcome.
In `@apps/electric-proxy/src/auth.ts`:
- Around line 23-31: The JWT claim parsing in apps/electric-proxy/src/auth.ts
currently uses a type assertion for organizationIds and only checks truthiness;
update the parse/validate logic (where payload is read and the function returns
{ sub, email: ..., organizationIds }) to explicitly verify that
payload.organizationIds is an array of strings (e.g.,
Array.isArray(payload.organizationIds) and every element is typeof "string") and
return null if that check fails; also keep the existing sub presence check and
the email fallback (email ?? "") but ensure organizationIds is sanitized/typed
before returning to prevent downstream type errors in org-membership
enforcement.
In `@apps/electric-proxy/src/electric.ts`:
- Around line 39-43: The code currently sets useCloud based on
ELECTRIC_SOURCE_ID and ELECTRIC_SOURCE_SECRET and then constructs upstream,
which can later throw or accidentally fall back to localhost when cloud config
is incomplete; update the logic around useCloud/upstream to validate the trio of
env vars (ELECTRIC_SOURCE_ID, ELECTRIC_SOURCE_SECRET, ELECTRIC_CLOUD_URL) before
creating the URL: if any cloud var is present but the required companion vars
are missing, throw an error immediately (fail fast) instead of proceeding,
otherwise construct upstream from ELECTRIC_CLOUD_URL when all cloud vars are
present, or from ELECTRIC_URL (with fallback) when none are set; refer to
symbols useCloud and upstream to locate and change the conditional flow.
In `@apps/electric-proxy/src/index.ts`:
- Around line 76-101: Wrap the cache.match/cache.put and fetch calls (around
cacheRequest/cacheKey and upstreamUrl usage) in a try/catch so runtime failures
don't bubble; specifically, surround the sequence that calls
cache.match(cacheRequest), fetch(upstreamUrl.toString()), and
cache.put(cacheRequest, response.clone()) with a single try block and in the
catch return a controlled Response with status 502 and the same CORS headers
your worker uses elsewhere (include Access-Control-Allow-Origin,
Access-Control-Allow-Methods, and Access-Control-Allow-Headers as appropriate)
so errors are returned safely instead of crashing the worker.
- Around line 97-101: The current caching logic indiscriminately stores any
successful response with a Cache-Control header; update the block around
response.ok / response.headers.has("cache-control") to first read and parse
response.headers.get("cache-control") and skip cache.put(cacheRequest,
response.clone()) when the directives include "no-store" or "private" (and also
treat "no-cache" as non-cacheable unless validated). Only call await
cache.put(cacheRequest, response.clone()) when the parsed directives allow
shared caching (e.g., s-maxage or public present or no restrictive directives).
Keep using response.clone() and the same cacheRequest variable and preserve the
existing await behavior.
---
Outside diff comments:
In `@apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx`:
- Around line 51-69: In the electronTrpc.auth.onTokenChanged.useSubscription
handler (the branch when data?.token && data?.expiresAt), clear the stored JWT
immediately by calling setJwt(null) before changing tokens, then after
refetchSession() (or if refetchSession throws) explicitly fetch the fresh JWT
via authClient.token() and update state (setJwt(...))—mirroring the hydration
logic used earlier; apply the same changes to the other token-change branch
mentioned (lines ~81-99) so JWT is always refreshed after setAuthToken and
refetchSession attempts, and ensure setIsHydrated remains set afterward.
---
Nitpick comments:
In `@apps/electric-proxy/src/auth.ts`:
- Around line 4-11: The current getJWKS function uses a single module-level jwks
singleton which can return keys for the wrong issuer if AUTH_URL changes;
replace the single jwks variable with a Map keyed by the authUrl string (e.g.,
jwksByUrl: Map<string, ReturnType<typeof createRemoteJWKSet>>), update
getJWKS(authUrl) to check jwksByUrl.get(authUrl) and only create and store a new
createRemoteJWKSet(new URL("/api/auth/jwks", authUrl)) when missing, and return
the map entry; ensure types remain ReturnType<typeof createRemoteJWKSet> so
other callers of getJWKS continue to work.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.superset/lib/setup/steps.shapps/desktop/src/renderer/lib/auth-client.tsapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/electric-proxy/.dev.vars.exampleapps/electric-proxy/package.jsonapps/electric-proxy/src/auth.tsapps/electric-proxy/src/electric.tsapps/electric-proxy/src/index.tsapps/electric-proxy/src/types.tsapps/electric-proxy/src/where.tsapps/electric-proxy/tsconfig.jsonapps/electric-proxy/wrangler.jsoncpackage.json
✅ Files skipped from review due to trivial changes (3)
- apps/electric-proxy/wrangler.jsonc
- apps/electric-proxy/.dev.vars.example
- apps/electric-proxy/package.json
Comment on lines
+522
to
+529
| # Generate apps/electric-proxy/.dev.vars for local wrangler dev | ||
| cat > apps/electric-proxy/.dev.vars <<DEVVARS | ||
| AUTH_URL=http://localhost:$API_PORT | ||
| ELECTRIC_CLOUD_URL=${ELECTRIC_CLOUD_URL:-https://api.electric-sql.cloud} | ||
| ELECTRIC_SOURCE_ID=${ELECTRIC_SOURCE_ID:-} | ||
| ELECTRIC_SOURCE_SECRET=${ELECTRIC_SOURCE_SECRET:-} | ||
| DEVVARS | ||
| success "Electric proxy .dev.vars written" |
Contributor
There was a problem hiding this comment.
apps/electric-proxy/.dev.vars drops local Electric mode variables.
This file now only writes cloud-source variables. That conflicts with the stated dual-mode behavior and can break local Docker Electric proxying when cloud creds are unset.
Suggested fix
cat > apps/electric-proxy/.dev.vars <<DEVVARS
AUTH_URL=http://localhost:$API_PORT
+ELECTRIC_URL=${ELECTRIC_URL:-http://localhost:$ELECTRIC_PORT/v1/shape}
+ELECTRIC_SECRET=${ELECTRIC_SECRET:-local_electric_dev_secret}
ELECTRIC_CLOUD_URL=${ELECTRIC_CLOUD_URL:-https://api.electric-sql.cloud}
ELECTRIC_SOURCE_ID=${ELECTRIC_SOURCE_ID:-}
ELECTRIC_SOURCE_SECRET=${ELECTRIC_SOURCE_SECRET:-}
DEVVARS📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Generate apps/electric-proxy/.dev.vars for local wrangler dev | |
| cat > apps/electric-proxy/.dev.vars <<DEVVARS | |
| AUTH_URL=http://localhost:$API_PORT | |
| ELECTRIC_CLOUD_URL=${ELECTRIC_CLOUD_URL:-https://api.electric-sql.cloud} | |
| ELECTRIC_SOURCE_ID=${ELECTRIC_SOURCE_ID:-} | |
| ELECTRIC_SOURCE_SECRET=${ELECTRIC_SOURCE_SECRET:-} | |
| DEVVARS | |
| success "Electric proxy .dev.vars written" | |
| # Generate apps/electric-proxy/.dev.vars for local wrangler dev | |
| cat > apps/electric-proxy/.dev.vars <<DEVVARS | |
| AUTH_URL=http://localhost:$API_PORT | |
| ELECTRIC_URL=${ELECTRIC_URL:-http://localhost:$ELECTRIC_PORT/v1/shape} | |
| ELECTRIC_SECRET=${ELECTRIC_SECRET:-local_electric_dev_secret} | |
| ELECTRIC_CLOUD_URL=${ELECTRIC_CLOUD_URL:-https://api.electric-sql.cloud} | |
| ELECTRIC_SOURCE_ID=${ELECTRIC_SOURCE_ID:-} | |
| ELECTRIC_SOURCE_SECRET=${ELECTRIC_SOURCE_SECRET:-} | |
| DEVVARS | |
| success "Electric proxy .dev.vars written" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.superset/lib/setup/steps.sh around lines 522 - 529, The heredoc that writes
apps/electric-proxy/.dev.vars currently only emits cloud-related variables
(ELECTRIC_CLOUD_URL, ELECTRIC_SOURCE_ID, ELECTRIC_SOURCE_SECRET) and drops
local-mode settings; update the .dev.vars generation (the heredoc producing
AUTH_URL=..., ELECTRIC_CLOUD_URL=..., etc.) to always include the local-mode
variables (e.g., AUTH_URL and a local ELECTRIC_MODE or ELECTRIC_LOCAL_*
defaults) and only append/export ELECTRIC_SOURCE_ID and ELECTRIC_SOURCE_SECRET
(and override ELECTRIC_CLOUD_URL) when those cloud env vars are
present/non-empty; implement this by conditionally adding the cloud lines to the
heredoc or by writing the base local vars first and then appending cloud vars
when [ -n "$ELECTRIC_SOURCE_ID" ] or similar, keeping the existing AUTH_URL line
unchanged.
Comment on lines
+34
to
+37
| const res = await authClient.token(); | ||
| if (res.data?.token) { | ||
| setJwt(res.data.token); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx | head -100Repository: superset-sh/superset
Length of output: 3306
Wrap JWT fetch in error handler to prevent hydration from hanging.
At line 34, an unguarded await authClient.token() will cause the hydrate function to exit with an unhandled rejection before reaching setIsHydrated(true) at line 40, leaving the app in a permanent loading state.
This is particularly problematic because the conditional block (lines 22–37) only executes if a stored token exists and is not expired—if authClient.token() rejects, the error propagates and the function exits before setIsHydrated(true) is called, leaving the splash screen indefinitely.
The periodic refresh effect (lines 84–94) demonstrates the correct approach with .catch() handling.
💡 Suggested fix
- const res = await authClient.token();
- if (res.data?.token) {
- setJwt(res.data.token);
- }
+ try {
+ const res = await authClient.token();
+ if (res.data?.token) {
+ setJwt(res.data.token);
+ }
+ } catch (err) {
+ console.warn(
+ "[AuthProvider] JWT fetch failed during hydration",
+ err,
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const res = await authClient.token(); | |
| if (res.data?.token) { | |
| setJwt(res.data.token); | |
| } | |
| const res = await authClient.token(); | |
| if (res.data?.token) { | |
| setJwt(res.data.token); | |
| } |
Suggested change
| const res = await authClient.token(); | |
| if (res.data?.token) { | |
| setJwt(res.data.token); | |
| } | |
| try { | |
| const res = await authClient.token(); | |
| if (res.data?.token) { | |
| setJwt(res.data.token); | |
| } | |
| } catch (err) { | |
| console.warn( | |
| "[AuthProvider] JWT fetch failed during hydration", | |
| err, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx` around
lines 34 - 37, The call to authClient.token() inside the AuthProvider hydration
flow can throw and currently is unhandled, which may prevent setIsHydrated(true)
from running; wrap the await authClient.token() call (in the same function that
calls setJwt and setIsHydrated) in a try/catch (or use .catch()) so any
rejection is swallowed/handled and still allow setIsHydrated(true) to run (or
move setIsHydrated(true) into a finally block); specifically update the code
around authClient.token(), setJwt(...) and setIsHydrated(...) in
AuthProvider.tsx to catch errors and optionally log them, then ensure
setIsHydrated(true) executes regardless of token request outcome.
Comment on lines
+23
to
+31
| const sub = payload.sub; | ||
| const email = payload.email as string | undefined; | ||
| const organizationIds = payload.organizationIds as string[] | undefined; | ||
|
|
||
| if (!sub || !organizationIds) { | ||
| return null; | ||
| } | ||
|
|
||
| return { sub, email: email ?? "", organizationIds }; |
Contributor
There was a problem hiding this comment.
Validate JWT claim shapes before authorization checks.
Line 25 trusts organizationIds via type assertion only. A malformed non-array claim can pass truthiness checks and break org-membership enforcement downstream.
🔧 Suggested fix
- const sub = payload.sub;
- const email = payload.email as string | undefined;
- const organizationIds = payload.organizationIds as string[] | undefined;
-
- if (!sub || !organizationIds) {
+ const sub = typeof payload.sub === "string" ? payload.sub : null;
+ const email = typeof payload.email === "string" ? payload.email : "";
+ const organizationIds =
+ Array.isArray(payload.organizationIds) &&
+ payload.organizationIds.every((id): id is string => typeof id === "string")
+ ? payload.organizationIds
+ : null;
+
+ if (!sub || !organizationIds) {
return null;
}
- return { sub, email: email ?? "", organizationIds };
+ return { sub, email, organizationIds };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const sub = payload.sub; | |
| const email = payload.email as string | undefined; | |
| const organizationIds = payload.organizationIds as string[] | undefined; | |
| if (!sub || !organizationIds) { | |
| return null; | |
| } | |
| return { sub, email: email ?? "", organizationIds }; | |
| const sub = typeof payload.sub === "string" ? payload.sub : null; | |
| const email = typeof payload.email === "string" ? payload.email : ""; | |
| const organizationIds = | |
| Array.isArray(payload.organizationIds) && | |
| payload.organizationIds.every((id): id is string => typeof id === "string") | |
| ? payload.organizationIds | |
| : null; | |
| if (!sub || !organizationIds) { | |
| return null; | |
| } | |
| return { sub, email, organizationIds }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electric-proxy/src/auth.ts` around lines 23 - 31, The JWT claim parsing
in apps/electric-proxy/src/auth.ts currently uses a type assertion for
organizationIds and only checks truthiness; update the parse/validate logic
(where payload is read and the function returns { sub, email: ...,
organizationIds }) to explicitly verify that payload.organizationIds is an array
of strings (e.g., Array.isArray(payload.organizationIds) and every element is
typeof "string") and return null if that check fails; also keep the existing sub
presence check and the email fallback (email ?? "") but ensure organizationIds
is sanitized/typed before returning to prevent downstream type errors in
org-membership enforcement.
Comment on lines
+39
to
+43
| const useCloud = env.ELECTRIC_SOURCE_ID && env.ELECTRIC_SOURCE_SECRET; | ||
|
|
||
| const upstream = useCloud | ||
| ? new URL("/v1/shape", env.ELECTRIC_CLOUD_URL) | ||
| : new URL(env.ELECTRIC_URL ?? "http://localhost:3149/v1/shape"); |
Contributor
There was a problem hiding this comment.
Fail fast on incomplete cloud configuration.
If cloud vars are partially set, the current logic can either throw later or unintentionally route to localhost fallback. Validate config explicitly before URL construction.
🛠️ Suggested fix
- const useCloud = env.ELECTRIC_SOURCE_ID && env.ELECTRIC_SOURCE_SECRET;
+ const hasAnyCloudConfig = Boolean(
+ env.ELECTRIC_CLOUD_URL || env.ELECTRIC_SOURCE_ID || env.ELECTRIC_SOURCE_SECRET,
+ );
+ const useCloud = Boolean(
+ env.ELECTRIC_CLOUD_URL && env.ELECTRIC_SOURCE_ID && env.ELECTRIC_SOURCE_SECRET,
+ );
+ if (hasAnyCloudConfig && !useCloud) {
+ throw new Error(
+ "Incomplete Electric Cloud config: ELECTRIC_CLOUD_URL, ELECTRIC_SOURCE_ID, and ELECTRIC_SOURCE_SECRET are all required.",
+ );
+ }
const upstream = useCloud
? new URL("/v1/shape", env.ELECTRIC_CLOUD_URL)
: new URL(env.ELECTRIC_URL ?? "http://localhost:3149/v1/shape");Also applies to: 45-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electric-proxy/src/electric.ts` around lines 39 - 43, The code currently
sets useCloud based on ELECTRIC_SOURCE_ID and ELECTRIC_SOURCE_SECRET and then
constructs upstream, which can later throw or accidentally fall back to
localhost when cloud config is incomplete; update the logic around
useCloud/upstream to validate the trio of env vars (ELECTRIC_SOURCE_ID,
ELECTRIC_SOURCE_SECRET, ELECTRIC_CLOUD_URL) before creating the URL: if any
cloud var is present but the required companion vars are missing, throw an error
immediately (fail fast) instead of proceeding, otherwise construct upstream from
ELECTRIC_CLOUD_URL when all cloud vars are present, or from ELECTRIC_URL (with
fallback) when none are set; refer to symbols useCloud and upstream to locate
and change the conditional flow.
Comment on lines
+76
to
+101
| const cache = caches.default; | ||
| const cacheRequest = new Request(cacheKey); | ||
| let response = await cache.match(cacheRequest); | ||
|
|
||
| if (!response) { | ||
| // Cache miss — fetch from upstream Electric | ||
| response = await fetch(upstreamUrl.toString()); | ||
|
|
||
| // Strip content-encoding to avoid decompression issues | ||
| const headers = new Headers(response.headers); | ||
| if (headers.get("content-encoding")) { | ||
| headers.delete("content-encoding"); | ||
| headers.delete("content-length"); | ||
| } | ||
|
|
||
| response = new Response(response.body, { | ||
| status: response.status, | ||
| statusText: response.statusText, | ||
| headers, | ||
| }); | ||
|
|
||
| // Cache if Electric sent Cache-Control headers and the response is successful | ||
| if (response.ok && response.headers.has("cache-control")) { | ||
| // Use waitUntil-free put — Cloudflare handles it in the background | ||
| await cache.put(cacheRequest, response.clone()); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "index.ts" -path "*/electric-proxy/src/*" | head -5Repository: superset-sh/superset
Length of output: 98
🏁 Script executed:
wc -l apps/electric-proxy/src/index.ts 2>/dev/null || find . -path "*/electric-proxy/src/index.ts" -exec wc -l {} \;Repository: superset-sh/superset
Length of output: 100
🏁 Script executed:
cat -n apps/electric-proxy/src/index.ts 2>/dev/null || find . -path "*/electric-proxy/src/index.ts" -exec cat -n {} \;Repository: superset-sh/superset
Length of output: 4501
Guard cache/upstream calls with error handling.
cache.match, fetch, or cache.put failures currently bubble as uncaught runtime errors, causing unhandled worker crashes without proper CORS headers. Wrap these calls in a try/catch block and return a controlled 502 response with CORS headers.
Suggested fix
const cache = caches.default;
const cacheRequest = new Request(cacheKey);
- let response = await cache.match(cacheRequest);
+ let response: Response | undefined;
+ try {
+ response = await cache.match(cacheRequest);
if (!response) {
// Cache miss — fetch from upstream Electric
response = await fetch(upstreamUrl.toString());
// Strip content-encoding to avoid decompression issues
const headers = new Headers(response.headers);
if (headers.get("content-encoding")) {
headers.delete("content-encoding");
headers.delete("content-length");
}
response = new Response(response.body, {
status: response.status,
statusText: response.statusText,
headers,
});
// Cache if Electric sent Cache-Control headers and the response is successful
if (response.ok && response.headers.has("cache-control")) {
// Use waitUntil-free put — Cloudflare handles it in the background
await cache.put(cacheRequest, response.clone());
}
}
+ } catch {
+ return corsResponse(502, "Upstream request failed");
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electric-proxy/src/index.ts` around lines 76 - 101, Wrap the
cache.match/cache.put and fetch calls (around cacheRequest/cacheKey and
upstreamUrl usage) in a try/catch so runtime failures don't bubble;
specifically, surround the sequence that calls cache.match(cacheRequest),
fetch(upstreamUrl.toString()), and cache.put(cacheRequest, response.clone())
with a single try block and in the catch return a controlled Response with
status 502 and the same CORS headers your worker uses elsewhere (include
Access-Control-Allow-Origin, Access-Control-Allow-Methods, and
Access-Control-Allow-Headers as appropriate) so errors are returned safely
instead of crashing the worker.
Comment on lines
+97
to
+101
| // Cache if Electric sent Cache-Control headers and the response is successful | ||
| if (response.ok && response.headers.has("cache-control")) { | ||
| // Use waitUntil-free put — Cloudflare handles it in the background | ||
| await cache.put(cacheRequest, response.clone()); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/electric-proxy/src/index.ts | head -120 | tail -40Repository: superset-sh/superset
Length of output: 1569
🏁 Script executed:
# Let me get more context around the caching logic
sed -n '80,110p' apps/electric-proxy/src/index.tsRepository: superset-sh/superset
Length of output: 1058
🏁 Script executed:
# Check if there are any other cache-related functions or utilities in the file
rg -A 3 -B 3 "cache\." apps/electric-proxy/src/index.ts | head -80Repository: superset-sh/superset
Length of output: 574
🏁 Script executed:
# Search for how cache-control directives might be handled elsewhere
rg "cache-control|Cache-Control|no-store|private" apps/electric-proxy/src/index.tsRepository: superset-sh/superset
Length of output: 208
🏁 Script executed:
# Check if the file passes Biome currently and what the structure looks like
rg "no-store|private|max-age" apps/electric-proxy/src/index.tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Verify the full context of the function containing this caching logic
sed -n '50,120p' apps/electric-proxy/src/index.tsRepository: superset-sh/superset
Length of output: 2120
Respect cache directives before cache.put().
Line 98 caches any successful response that merely has a Cache-Control header, ignoring its actual directives. Responses with Cache-Control: no-store or Cache-Control: private are explicitly non-cacheable but will be stored by this implementation, violating HTTP caching semantics and risking exposure of sensitive or non-cacheable data.
🧩 Suggested fix
- // Cache if Electric sent Cache-Control headers and the response is successful
- if (response.ok && response.headers.has("cache-control")) {
+ // Cache only when response is explicitly cacheable
+ const cacheControl = response.headers.get("cache-control")?.toLowerCase() ?? "";
+ const isNonCacheable = /\b(no-store|private)\b/.test(cacheControl);
+ if (response.ok && cacheControl && !isNonCacheable) {
// Use waitUntil-free put — Cloudflare handles it in the background
await cache.put(cacheRequest, response.clone());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Cache if Electric sent Cache-Control headers and the response is successful | |
| if (response.ok && response.headers.has("cache-control")) { | |
| // Use waitUntil-free put — Cloudflare handles it in the background | |
| await cache.put(cacheRequest, response.clone()); | |
| } | |
| // Cache only when response is explicitly cacheable | |
| const cacheControl = response.headers.get("cache-control")?.toLowerCase() ?? ""; | |
| const isNonCacheable = /\b(no-store|private)\b/.test(cacheControl); | |
| if (response.ok && cacheControl && !isNonCacheable) { | |
| // Use waitUntil-free put — Cloudflare handles it in the background | |
| await cache.put(cacheRequest, response.clone()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electric-proxy/src/index.ts` around lines 97 - 101, The current caching
logic indiscriminately stores any successful response with a Cache-Control
header; update the block around response.ok /
response.headers.has("cache-control") to first read and parse
response.headers.get("cache-control") and skip cache.put(cacheRequest,
response.clone()) when the directives include "no-store" or "private" (and also
treat "no-cache" as non-cacheable unless validated). Only call await
cache.put(cacheRequest, response.clone()) when the parsed directives allow
shared caching (e.g., s-maxage or public present or no restrictive directives).
Keep using response.clone() and the same cacheRequest variable and preserve the
existing await behavior.
…mplify types.ts to Env only
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/electric-proxy/src/electric.ts (1)
85-87: Consider removing unused_authparameter or documenting future use.The
_authparameter is currently unused. If it's reserved for future cache isolation needs, a brief comment would help. Otherwise, it could be removed to simplify the API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electric-proxy/src/electric.ts` around lines 85 - 87, The buildCacheKey function currently accepts an unused parameter _auth; either remove the parameter from buildCacheKey(upstreamUrl, _auth) and update all callers and the exported signature to simplify the API, or keep it but add a short in-code comment above buildCacheKey explaining that _auth is intentionally reserved for future cache isolation (and is intentionally unused today) to make intent clear to maintainers; if removing, also update any type definitions/imports that reference the old signature.apps/electric-proxy/src/where.ts (1)
21-31: Regex-based SQL fragment extraction is fragile.The approach of building a full SELECT query and stripping everything before WHERE via regex is unconventional and could break if drizzle-orm changes its SQL output format (e.g., adds CTEs, changes casing, or modifies whitespace).
Consider using drizzle-orm's lower-level APIs if available, or at minimum add a comment explaining this workaround for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electric-proxy/src/where.ts` around lines 21 - 31, The current build function strips a full SELECT SQL string with a regex to extract the WHERE fragment (fragile), so change it to obtain the WHERE SQL from the expression builder instead of regex — e.g., use the where expression's/toSQL/compile method (whereExpr.toSQL() or the drizzle API that compiles expressions) or, if drizzle exposes a QueryBuilder/SQLCompiler option to produce only the WHERE fragment, call that (replace the regex usage in build and stop using query.replace(...)). If the library does not provide such an API, replace the regex with a short TODO comment inside build referencing QueryBuilder and whereExpr explaining the workaround and add a unit test exercising CTE/whitespace/casing cases so future changes are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/deploy-production.yml:
- Around line 451-479: The deploy step in the deploy-electric-proxy job
currently does not provide required Worker env vars; update the Deploy Worker
step (or wrangler.jsonc) so the Cloudflare Worker receives AUTH_URL (required)
and optional ELECTRIC_URL and ELECTRIC_SECRET at deploy/runtime; either add an
[env.production] section with those bindings to wrangler.jsonc or add AUTH_URL,
ELECTRIC_URL and ELECTRIC_SECRET to the Deploy Worker step's env block alongside
CLOUDFLARE_API_TOKEN and CLOUDFLARE_ACCOUNT_ID so the Worker’s Env interface
variables are available at runtime when bunx wrangler deploy is run.
In `@apps/electric-proxy/src/auth.ts`:
- Around line 14-21: The module-level jwks cache in getJWKS ignores the authUrl
argument, meaning different authUrl values would wrongly share one
createRemoteJWKSet instance; update getJWKS to use a Map keyed by authUrl (e.g.,
const jwksCache = new Map<string, ReturnType<typeof createRemoteJWKSet>>()) and
return or populate jwksCache.get(authUrl) / jwksCache.set(authUrl,
createRemoteJWKSet(...)), referencing createRemoteJWKSet and getJWKS so each
distinct authUrl gets its own JWKS instance (alternatively, if only a single
env.AUTH_URL is ever used, remove the authUrl parameter from getJWKS and the
module-scoped jwks can remain).
In `@apps/electric-proxy/src/where.ts`:
- Around line 87-90: The LIKE-based fragment in the auth.apikeys case is
vulnerable to LIKE metacharacters and brittle for JSON; update the auth.apikeys
branch so it does not build a quoted-JSON string via fragment and params using
organizationId, and instead use a JSON-aware predicate (e.g., JSONB containment
or jsonb_path_exists) against the metadata column; replace the current fragment
variable and params usage with a JSONB-safe expression that checks metadata
contains organizationId (or uses a JSON path) to avoid LIKE escaping issues and
SQL injection while still referencing the same auth.apikeys case, fragment
identifier, params array, and organizationId parameter.
---
Nitpick comments:
In `@apps/electric-proxy/src/electric.ts`:
- Around line 85-87: The buildCacheKey function currently accepts an unused
parameter _auth; either remove the parameter from buildCacheKey(upstreamUrl,
_auth) and update all callers and the exported signature to simplify the API, or
keep it but add a short in-code comment above buildCacheKey explaining that
_auth is intentionally reserved for future cache isolation (and is intentionally
unused today) to make intent clear to maintainers; if removing, also update any
type definitions/imports that reference the old signature.
In `@apps/electric-proxy/src/where.ts`:
- Around line 21-31: The current build function strips a full SELECT SQL string
with a regex to extract the WHERE fragment (fragile), so change it to obtain the
WHERE SQL from the expression builder instead of regex — e.g., use the where
expression's/toSQL/compile method (whereExpr.toSQL() or the drizzle API that
compiles expressions) or, if drizzle exposes a QueryBuilder/SQLCompiler option
to produce only the WHERE fragment, call that (replace the regex usage in build
and stop using query.replace(...)). If the library does not provide such an API,
replace the regex with a short TODO comment inside build referencing
QueryBuilder and whereExpr explaining the workaround and add a unit test
exercising CTE/whitespace/casing cases so future changes are caught.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/deploy-production.ymlapps/desktop/electron.vite.config.tsapps/desktop/src/main/env.main.tsapps/desktop/src/renderer/env.renderer.tsapps/desktop/vite/helpers.tsapps/electric-proxy/src/auth.tsapps/electric-proxy/src/electric.tsapps/electric-proxy/src/index.tsapps/electric-proxy/src/types.tsapps/electric-proxy/src/where.tsapps/electric-proxy/wrangler.jsonc
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/electric-proxy/wrangler.jsonc
- apps/electric-proxy/src/index.ts
Comment on lines
+451
to
+479
| deploy-electric-proxy: | ||
| name: Deploy Electric Proxy to Cloudflare | ||
| runs-on: ubuntu-latest | ||
| environment: production | ||
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Bun | ||
| uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: 1.3.3 | ||
|
|
||
| - name: Cache dependencies | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: ~/.bun/install/cache | ||
| key: ${{ runner.os }}-bun-${{ hashFiles('bun.lock') }} | ||
|
|
||
| - name: Install dependencies | ||
| run: bun install --frozen | ||
|
|
||
| - name: Deploy Worker | ||
| working-directory: apps/electric-proxy | ||
| env: | ||
| CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN }} | ||
| CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }} | ||
| run: bunx wrangler deploy |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the electric-proxy app directory and wrangler configuration exist
echo "=== Checking apps/electric-proxy directory ==="
if [ -d "apps/electric-proxy" ]; then
echo "Directory exists"
ls -la apps/electric-proxy/
else
echo "ERROR: apps/electric-proxy directory not found"
exit 1
fi
echo ""
echo "=== Checking for wrangler.toml ==="
if [ -f "apps/electric-proxy/wrangler.toml" ]; then
echo "wrangler.toml found:"
cat apps/electric-proxy/wrangler.toml
else
echo "ERROR: wrangler.toml not found in apps/electric-proxy"
exit 1
fiRepository: superset-sh/superset
Length of output: 638
🏁 Script executed:
# Search for any documentation or setup instructions related to secrets
echo "=== Searching for secret setup documentation ==="
find . -type f \( -name "*.md" -o -name "*.txt" \) -exec grep -l "CLOUDFLARE\|electric-proxy\|secret" {} \; | head -20Repository: superset-sh/superset
Length of output: 170
🏁 Script executed:
# Check if there are any GitHub workflow files that reference secret setup
echo "=== Checking for secret setup references in workflow files ==="
rg -i "cloudflare.*secret|electric.*url|electric.*secret" .github/workflows/ -A 2 -B 2Repository: superset-sh/superset
Length of output: 4899
🏁 Script executed:
# Look for any .github/environments or secrets documentation
echo "=== Checking for GitHub environment configuration ==="
if [ -d ".github/environments" ]; then
echo "Found .github/environments:"
find .github/environments -type f | xargs cat
fi
# Check for any documentation about setting up secrets
find . -type f -name "*.md" | xargs grep -l "production environment" 2>/dev/null | head -5Repository: superset-sh/superset
Length of output: 117
🏁 Script executed:
cat apps/electric-proxy/wrangler.jsoncRepository: superset-sh/superset
Length of output: 155
🏁 Script executed:
cat apps/electric-proxy/.dev.vars.exampleRepository: superset-sh/superset
Length of output: 458
🏁 Script executed:
# Check the src directory structure to understand what the Worker does
ls -la apps/electric-proxy/src/Repository: superset-sh/superset
Length of output: 467
🏁 Script executed:
# Look for any production environment configuration in wrangler files
find apps/electric-proxy -name "wrangler*" -exec cat {} \;Repository: superset-sh/superset
Length of output: 155
🏁 Script executed:
# Check if there are comments or setup docs in the electric-proxy package.json or README
cat apps/electric-proxy/package.jsonRepository: superset-sh/superset
Length of output: 612
🏁 Script executed:
# Search for any README or docs in the electric-proxy directory
find apps/electric-proxy -type f \( -name "README*" -o -name "*.md" \) -exec cat {} \;Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the index.ts to see how environment variables are being used
head -50 apps/electric-proxy/src/index.tsRepository: superset-sh/superset
Length of output: 1642
🏁 Script executed:
cat apps/electric-proxy/src/types.tsRepository: superset-sh/superset
Length of output: 252
🏁 Script executed:
# Check if there's any environment binding configuration in wrangler.jsonc
# that might define secrets at deployment time
git log --oneline -n 5 -- apps/electric-proxy/wrangler.jsonc 2>/dev/null || echo "No git history available"Repository: superset-sh/superset
Length of output: 148
🏁 Script executed:
# Search for how other Cloudflare Worker deployments handle secrets in this repo
rg "wrangler deploy" .github/workflows/ -B 5 -A 5Repository: superset-sh/superset
Length of output: 866
🏁 Script executed:
# Check if there's environment binding configuration later in wrangler.jsonc or if it's complete
wc -l apps/electric-proxy/wrangler.jsoncRepository: superset-sh/superset
Length of output: 100
🏁 Script executed:
# Look for any secret setup commands in the deploy-production workflow for electric-proxy
rg -A 20 "deploy-electric-proxy:" .github/workflows/deploy-production.yml | head -40Repository: superset-sh/superset
Length of output: 589
🏁 Script executed:
# Check if there's a separate wrangler.toml (in addition to jsonc)
find apps/electric-proxy -name "wrangler.*"Repository: superset-sh/superset
Length of output: 98
🏁 Script executed:
# Verify what the deploy command does - check if wrangler reads from environment automatically
rg "AUTH_URL|ELECTRIC_" apps/electric-proxy/ --type tsRepository: superset-sh/superset
Length of output: 1181
🏁 Script executed:
# Check how AUTH_URL is currently being handled - is it in wrangler.jsonc with bindings?
cat -n apps/electric-proxy/wrangler.jsoncRepository: superset-sh/superset
Length of output: 190
🏁 Script executed:
# Check if there's any environment variable binding or secret definition we missed
grep -r "env\|vars\|secret" apps/electric-proxy/ --include="*.jsonc" --include="*.json" --include="*.toml" --include="*.ts" 2>/dev/null | grep -v node_modulesRepository: superset-sh/superset
Length of output: 1188
🏁 Script executed:
# Look at the full deploy-production.yml to see if there's any environment setup elsewhere for this job
sed -n '451,485p' .github/workflows/deploy-production.ymlRepository: superset-sh/superset
Length of output: 1014
Add required Worker environment variables to the deployment command.
The wrangler.jsonc file (not wrangler.toml) exists and is valid, but it lacks environment bindings. The deploy step must pass Worker environment variables during deployment since they are required at runtime. The Worker code requires AUTH_URL (mandatory) and expects optional variables like ELECTRIC_URL and ELECTRIC_SECRET defined in the Env interface.
Update the deploy step to either:
- Add environment variable bindings to
wrangler.jsoncwith a[env.production]section, or - Pass the variables as environment to the deploy command:
env:
CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN }}
CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }}
AUTH_URL: ${{ secrets.AUTH_URL }}
ELECTRIC_URL: ${{ secrets.ELECTRIC_URL }}
ELECTRIC_SECRET: ${{ secrets.ELECTRIC_SECRET }}
run: bunx wrangler deployEnsure the corresponding GitHub secrets (AUTH_URL, ELECTRIC_URL, ELECTRIC_SECRET) are configured in the production environment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/deploy-production.yml around lines 451 - 479, The deploy
step in the deploy-electric-proxy job currently does not provide required Worker
env vars; update the Deploy Worker step (or wrangler.jsonc) so the Cloudflare
Worker receives AUTH_URL (required) and optional ELECTRIC_URL and
ELECTRIC_SECRET at deploy/runtime; either add an [env.production] section with
those bindings to wrangler.jsonc or add AUTH_URL, ELECTRIC_URL and
ELECTRIC_SECRET to the Deploy Worker step's env block alongside
CLOUDFLARE_API_TOKEN and CLOUDFLARE_ACCOUNT_ID so the Worker’s Env interface
variables are available at runtime when bunx wrangler deploy is run.
Comment on lines
+14
to
+21
| let jwks: ReturnType<typeof createRemoteJWKSet> | null = null; | ||
|
|
||
| function getJWKS(authUrl: string): ReturnType<typeof createRemoteJWKSet> { | ||
| if (!jwks) { | ||
| jwks = createRemoteJWKSet(new URL("/api/auth/jwks", authUrl)); | ||
| } | ||
| return jwks; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd apps/electric-proxy && wc -l src/auth.tsRepository: superset-sh/superset
Length of output: 78
🏁 Script executed:
cat -n apps/electric-proxy/src/auth.ts | head -50Repository: superset-sh/superset
Length of output: 1354
🏁 Script executed:
rg -n "AUTH_URL|authUrl" --type=ts -C 2 apps/electric-proxy/Repository: superset-sh/superset
Length of output: 1699
🏁 Script executed:
rg -n "verifyJWT|getJWKS" --type=ts -C 3 apps/electric-proxy/Repository: superset-sh/superset
Length of output: 1934
JWKS cache doesn't account for authUrl parameter variation.
While getJWKS accepts authUrl as a parameter, the module-scoped jwks cache ignores it—if called with different authUrl values, stale JWKS data would be reused. In practice, verifyJWT is only called with env.AUTH_URL (a static Worker environment variable), so this isn't a functional issue. However, the function signature misleads callers into thinking different URLs are supported.
Consider using a Map-keyed cache if multiple auth sources might be needed in the future, or simplify the API by removing the authUrl parameter if only one value is ever used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electric-proxy/src/auth.ts` around lines 14 - 21, The module-level jwks
cache in getJWKS ignores the authUrl argument, meaning different authUrl values
would wrongly share one createRemoteJWKSet instance; update getJWKS to use a Map
keyed by authUrl (e.g., const jwksCache = new Map<string, ReturnType<typeof
createRemoteJWKSet>>()) and return or populate jwksCache.get(authUrl) /
jwksCache.set(authUrl, createRemoteJWKSet(...)), referencing createRemoteJWKSet
and getJWKS so each distinct authUrl gets its own JWKS instance (alternatively,
if only a single env.AUTH_URL is ever used, remove the authUrl parameter from
getJWKS and the module-scoped jwks can remain).
Comment on lines
+87
to
+90
| case "auth.apikeys": { | ||
| const fragment = `"metadata" LIKE '%"organizationId":"' || $1 || '"%'`; | ||
| return { fragment, params: [organizationId] }; | ||
| } |
Contributor
There was a problem hiding this comment.
Potential SQL injection in auth.apikeys LIKE pattern.
The fragment uses string concatenation (|| $1 ||) which relies on the upstream system to properly parameterize. However, if organizationId contains special LIKE characters (%, _, \), it could match unintended rows. Additionally, the JSON pattern matching is fragile and may fail with escaped quotes in the metadata.
Consider using a JSON-aware query approach if the database supports it (e.g., jsonb_path_exists for PostgreSQL).
🔧 Suggested safer approach using JSONB operators
case "auth.apikeys": {
- const fragment = `"metadata" LIKE '%"organizationId":"' || $1 || '"%'`;
- return { fragment, params: [organizationId] };
+ // Using JSONB containment for type-safe matching
+ const fragment = `"metadata"::jsonb @> ('{"organizationId":"' || $1 || '"}')::jsonb`;
+ return { fragment, params: [organizationId] };
}Or if the column is already jsonb:
const fragment = `"metadata" @> jsonb_build_object('organizationId', $1)`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electric-proxy/src/where.ts` around lines 87 - 90, The LIKE-based
fragment in the auth.apikeys case is vulnerable to LIKE metacharacters and
brittle for JSON; update the auth.apikeys branch so it does not build a
quoted-JSON string via fragment and params using organizationId, and instead use
a JSON-aware predicate (e.g., JSONB containment or jsonb_path_exists) against
the metadata column; replace the current fragment variable and params usage with
a JSONB-safe expression that checks metadata contains organizationId (or uses a
JSON path) to avoid LIKE escaping issues and SQL injection while still
referencing the same auth.apikeys case, fragment identifier, params array, and
organizationId parameter.
…t JWTs in old proxy
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found across 22 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/electric-proxy/src/index.ts">
<violation number="1" location="apps/electric-proxy/src/index.ts:70">
P1: Cache key uses the upstream Electric URL hostname instead of the Worker's own hostname. Cloudflare docs warn that using a different hostname in cache keys leads to cache inefficiencies and may cause `caches.default` operations to silently fail, since the zone cache is scoped to the Worker's domain. Use the Worker's own request URL as the base for the cache key (preserving the query parameters that make each shape request unique).</violation>
<violation number="2" location="apps/electric-proxy/src/index.ts:74">
P2: The upstream `fetch` call has no error handling. If the Electric backend is unreachable, the unhandled exception produces a generic 500 without CORS headers, which the browser may block entirely. Wrap the fetch in a try/catch and return a `corsResponse(502, ...)` on failure.</violation>
</file>
<file name="apps/api/src/app/api/electric/[...path]/route.ts">
<violation number="1" location="apps/api/src/app/api/electric/[...path]/route.ts:25">
P2: Empty `catch {}` silently swallows JWT verification errors, making auth failures invisible in production. Log at least a warning with context so you can trace issues like expired tokens or misconfigured secrets.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/electric-proxy/src/where.ts">
<violation number="1" location="apps/electric-proxy/src/where.ts:88">
P1: LIKE pattern does not escape wildcard characters (`_`, `%`) in the parameterized organization ID. If the ID contains underscores (common in many ID formats), this will match unintended rows, potentially leaking API keys from other organizations. Consider using PostgreSQL JSON operators for correct and safe JSON field matching.</violation>
</file>
<file name="apps/electric-proxy/src/auth.ts">
<violation number="1" location="apps/electric-proxy/src/auth.ts:42">
P2: Empty `catch` block silently swallows all JWT verification errors (expired tokens, invalid signatures, JWKS fetch failures, etc.), making auth issues very hard to diagnose. Log the error with context so failures are visible in Cloudflare Worker logs.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/electric-proxy/src/electric.ts">
<violation number="1" location="apps/electric-proxy/src/electric.ts:72">
P2: Cache key includes server-side secrets (`secret`, `source_id`) from the upstream URL. These are constant across all requests and don't contribute to cache differentiation. Strip credentials from the cache key to reduce exposure surface.
Consider creating a sanitized copy of the URL for the cache key:</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const cacheKey = buildCacheKey(upstreamUrl, auth); | ||
|
|
||
| const cache = caches.default; | ||
| const cacheRequest = new Request(cacheKey); |
Contributor
There was a problem hiding this comment.
P1: Cache key uses the upstream Electric URL hostname instead of the Worker's own hostname. Cloudflare docs warn that using a different hostname in cache keys leads to cache inefficiencies and may cause caches.default operations to silently fail, since the zone cache is scoped to the Worker's domain. Use the Worker's own request URL as the base for the cache key (preserving the query parameters that make each shape request unique).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/electric-proxy/src/index.ts, line 70:
<comment>Cache key uses the upstream Electric URL hostname instead of the Worker's own hostname. Cloudflare docs warn that using a different hostname in cache keys leads to cache inefficiencies and may cause `caches.default` operations to silently fail, since the zone cache is scoped to the Worker's domain. Use the Worker's own request URL as the base for the cache key (preserving the query parameters that make each shape request unique).</comment>
<file context>
@@ -0,0 +1,104 @@
+ const cacheKey = buildCacheKey(upstreamUrl, auth);
+
+ const cache = caches.default;
+ const cacheRequest = new Request(cacheKey);
+ let response = await cache.match(cacheRequest);
+
</file context>
| return build(agentCommands, agentCommands.organizationId, organizationId); | ||
|
|
||
| case "auth.apikeys": { | ||
| const fragment = `"metadata" LIKE '%"organizationId":"' || $1 || '"%'`; |
Contributor
There was a problem hiding this comment.
P1: LIKE pattern does not escape wildcard characters (_, %) in the parameterized organization ID. If the ID contains underscores (common in many ID formats), this will match unintended rows, potentially leaking API keys from other organizations. Consider using PostgreSQL JSON operators for correct and safe JSON field matching.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/electric-proxy/src/where.ts, line 88:
<comment>LIKE pattern does not escape wildcard characters (`_`, `%`) in the parameterized organization ID. If the ID contains underscores (common in many ID formats), this will match unintended rows, potentially leaking API keys from other organizations. Consider using PostgreSQL JSON operators for correct and safe JSON field matching.</comment>
<file context>
@@ -0,0 +1,114 @@
+ return build(agentCommands, agentCommands.organizationId, organizationId);
+
+ case "auth.apikeys": {
+ const fragment = `"metadata" LIKE '%"organizationId":"' || $1 || '"%'`;
+ return { fragment, params: [organizationId] };
+ }
</file context>
| let response = await cache.match(cacheRequest); | ||
|
|
||
| if (!response) { | ||
| response = await fetch(upstreamUrl.toString()); |
Contributor
There was a problem hiding this comment.
P2: The upstream fetch call has no error handling. If the Electric backend is unreachable, the unhandled exception produces a generic 500 without CORS headers, which the browser may block entirely. Wrap the fetch in a try/catch and return a corsResponse(502, ...) on failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/electric-proxy/src/index.ts, line 74:
<comment>The upstream `fetch` call has no error handling. If the Electric backend is unreachable, the unhandled exception produces a generic 500 without CORS headers, which the browser may block entirely. Wrap the fetch in a try/catch and return a `corsResponse(502, ...)` on failure.</comment>
<file context>
@@ -0,0 +1,104 @@
+ let response = await cache.match(cacheRequest);
+
+ if (!response) {
+ response = await fetch(upstreamUrl.toString());
+
+ const headers = new Headers(response.headers);
</file context>
| organizationIds: payload.organizationIds as string[], | ||
| }; | ||
| } | ||
| } catch {} |
Contributor
There was a problem hiding this comment.
P2: Empty catch {} silently swallows JWT verification errors, making auth failures invisible in production. Log at least a warning with context so you can trace issues like expired tokens or misconfigured secrets.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/app/api/electric/[...path]/route.ts, line 25:
<comment>Empty `catch {}` silently swallows JWT verification errors, making auth failures invisible in production. Log at least a warning with context so you can trace issues like expired tokens or misconfigured secrets.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) </comment>
<file context>
@@ -3,32 +3,56 @@ import { auth } from "@superset/auth/server";
+ organizationIds: payload.organizationIds as string[],
+ };
+ }
+ } catch {}
+ }
+
</file context>
Suggested change
| } catch {} | |
| } catch (err) { | |
| console.warn("[electric] JWT verification failed", err); | |
| } |
| } | ||
|
|
||
| return { sub, email: email ?? "", organizationIds }; | ||
| } catch { |
Contributor
There was a problem hiding this comment.
P2: Empty catch block silently swallows all JWT verification errors (expired tokens, invalid signatures, JWKS fetch failures, etc.), making auth issues very hard to diagnose. Log the error with context so failures are visible in Cloudflare Worker logs.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/electric-proxy/src/auth.ts, line 42:
<comment>Empty `catch` block silently swallows all JWT verification errors (expired tokens, invalid signatures, JWKS fetch failures, etc.), making auth issues very hard to diagnose. Log the error with context so failures are visible in Cloudflare Worker logs.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) </comment>
<file context>
@@ -0,0 +1,45 @@
+ }
+
+ return { sub, email: email ?? "", organizationIds };
+ } catch {
+ return null;
+ }
</file context>
| return upstream; | ||
| } | ||
|
|
||
| export function buildCacheKey(upstreamUrl: URL, _auth: AuthContext): string { |
Contributor
There was a problem hiding this comment.
P2: Cache key includes server-side secrets (secret, source_id) from the upstream URL. These are constant across all requests and don't contribute to cache differentiation. Strip credentials from the cache key to reduce exposure surface.
Consider creating a sanitized copy of the URL for the cache key:
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/electric-proxy/src/electric.ts, line 72:
<comment>Cache key includes server-side secrets (`secret`, `source_id`) from the upstream URL. These are constant across all requests and don't contribute to cache differentiation. Strip credentials from the cache key to reduce exposure surface.
Consider creating a sanitized copy of the URL for the cache key:</comment>
<file context>
@@ -0,0 +1,74 @@
+ return upstream;
+}
+
+export function buildCacheKey(upstreamUrl: URL, _auth: AuthContext): string {
+ return upstreamUrl.toString();
+}
</file context>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx (1)
60-73:⚠️ Potential issue | 🟠 MajorRefresh/clear JWT immediately on token switch to avoid stale auth.
When a new stored token arrives (Line 60), the in-memory JWT is not reset or refreshed in this branch. If
refetchSession()fails (Line 65), the old JWT can continue to be sent until the periodic refresh runs (Line 104), which risks cross-session authorization leakage.💡 Suggested fix
onData: async (data) => { if (data?.token && data?.expiresAt) { + // Prevent stale JWT from previous session being reused + setJwt(null); setAuthToken(null); await authClient.signOut({ fetchOptions: { throw: false } }); setAuthToken(data.token); try { await refetchSession(); + const jwtRes = await authClient.token(); + setJwt(jwtRes.data?.token ?? null); } catch (err) { console.warn( "[AuthProvider] session refetch failed after token change", err, ); } setIsHydrated(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx` around lines 60 - 73, The branch handling a new stored token currently sets the in-memory token immediately (setAuthToken(data.token)) before confirming the session refetch; change the flow so you clear the in-memory token first (setAuthToken(null)), call authClient.signOut(...), then attempt refetchSession(), and only call setAuthToken(data.token) after refetchSession() succeeds; if refetchSession() throws, keep the in-memory token cleared (setAuthToken(null)) and log the error, and still setIsHydrated(true) appropriately—update the logic around setAuthToken, authClient.signOut, refetchSession, and setIsHydrated to implement this ordering and error-path token clearing.
♻️ Duplicate comments (1)
apps/electric-proxy/src/electric.ts (1)
34-38:⚠️ Potential issue | 🟠 MajorFail fast on partial cloud config before URL construction.
useCloudon Line [34] only checks source credentials, while Line [37] assumesELECTRIC_CLOUD_URLis present. This can throw an opaque URL error or route to localhost fallback when cloud vars are partially set. Validate all cloud vars together and throw a clear config error early.🛠️ Proposed fix
- const useCloud = env.ELECTRIC_SOURCE_ID && env.ELECTRIC_SOURCE_SECRET; + const hasAnyCloudConfig = Boolean( + env.ELECTRIC_CLOUD_URL || env.ELECTRIC_SOURCE_ID || env.ELECTRIC_SOURCE_SECRET, + ); + const useCloud = Boolean( + env.ELECTRIC_CLOUD_URL && env.ELECTRIC_SOURCE_ID && env.ELECTRIC_SOURCE_SECRET, + ); + if (hasAnyCloudConfig && !useCloud) { + throw new Error( + "Incomplete Electric Cloud config: ELECTRIC_CLOUD_URL, ELECTRIC_SOURCE_ID, and ELECTRIC_SOURCE_SECRET are all required.", + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electric-proxy/src/electric.ts` around lines 34 - 38, The code computes useCloud then constructs upstream using env.ELECTRIC_CLOUD_URL without validating that URL exists; update the logic around the useCloud constant and upstream initialization to validate all three cloud vars (ELECTRIC_SOURCE_ID, ELECTRIC_SOURCE_SECRET, and ELECTRIC_CLOUD_URL) together, and if any are missing throw a clear configuration error before calling new URL; specifically, replace the current useCloud/upstream branch with a check that ensures ELECTRIC_CLOUD_URL is defined when ELECTRIC_SOURCE_ID/SECRET are present (or require all three simultaneously), and produce a descriptive error instead of falling back to localhost or allowing new URL to throw.
🧹 Nitpick comments (1)
apps/electric-proxy/src/electric.ts (1)
72-73: Clarify auth-insensitive cache key intent.
_authis unused; consider either removing it from the signature or documenting that the upstream URL fully captures auth scoping to prevent future misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electric-proxy/src/electric.ts` around lines 72 - 73, The buildCacheKey function currently accepts an unused _auth parameter which is confusing; either remove the _auth parameter from buildCacheKey(upstreamUrl: URL, _auth: AuthContext): string and update all callers to stop passing auth, or if the parameter must remain for API compatibility, add a clear JSDoc above buildCacheKey stating that auth is intentionally ignored because the upstreamUrl fully scopes the cache key; reference the buildCacheKey symbol and the _auth parameter when making the change.
🤖 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/api/src/app/api/electric/`[...path]/route.ts:
- Around line 17-23: The returned AuthInfo is constructed from unchecked payload
fields; add runtime type guards after auth.api.verifyJWT to ensure payload.sub
is a string before assigning to userId, ensure payload.email is a string
(otherwise set email to empty string), and ensure payload.organizationIds is an
array and map/filter it to a string[] by keeping only elements with typeof ===
"string"; update the object returned (the block that currently returns userId:
payload.sub, email: (payload.email as string) ?? "", organizationIds:
payload.organizationIds as string[]) to use these validated values so downstream
code sees correct types.
- Around line 13-29: If an Authorization header containing a Bearer token is
present, do not fall back to session auth when JWT verification fails; detect
bearer (bearer?.startsWith("Bearer ")), extract token (token = bearer.slice(7)),
call auth.api.verifyJWT({ body: { token } }) and if verification throws or
payload is missing required fields, immediately return null instead of
proceeding to auth.api.getSession; only call auth.api.getSession when no
Authorization Bearer header exists. Ensure references to verifyJWT, getSession,
bearer, token, and sessionData are updated accordingly.
---
Outside diff comments:
In `@apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx`:
- Around line 60-73: The branch handling a new stored token currently sets the
in-memory token immediately (setAuthToken(data.token)) before confirming the
session refetch; change the flow so you clear the in-memory token first
(setAuthToken(null)), call authClient.signOut(...), then attempt
refetchSession(), and only call setAuthToken(data.token) after refetchSession()
succeeds; if refetchSession() throws, keep the in-memory token cleared
(setAuthToken(null)) and log the error, and still setIsHydrated(true)
appropriately—update the logic around setAuthToken, authClient.signOut,
refetchSession, and setIsHydrated to implement this ordering and error-path
token clearing.
---
Duplicate comments:
In `@apps/electric-proxy/src/electric.ts`:
- Around line 34-38: The code computes useCloud then constructs upstream using
env.ELECTRIC_CLOUD_URL without validating that URL exists; update the logic
around the useCloud constant and upstream initialization to validate all three
cloud vars (ELECTRIC_SOURCE_ID, ELECTRIC_SOURCE_SECRET, and ELECTRIC_CLOUD_URL)
together, and if any are missing throw a clear configuration error before
calling new URL; specifically, replace the current useCloud/upstream branch with
a check that ensures ELECTRIC_CLOUD_URL is defined when
ELECTRIC_SOURCE_ID/SECRET are present (or require all three simultaneously), and
produce a descriptive error instead of falling back to localhost or allowing new
URL to throw.
---
Nitpick comments:
In `@apps/electric-proxy/src/electric.ts`:
- Around line 72-73: The buildCacheKey function currently accepts an unused
_auth parameter which is confusing; either remove the _auth parameter from
buildCacheKey(upstreamUrl: URL, _auth: AuthContext): string and update all
callers to stop passing auth, or if the parameter must remain for API
compatibility, add a clear JSDoc above buildCacheKey stating that auth is
intentionally ignored because the upstreamUrl fully scopes the cache key;
reference the buildCacheKey symbol and the _auth parameter when making the
change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.superset/lib/setup/steps.shapps/api/src/app/api/electric/[...path]/route.tsapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/electric-proxy/.dev.vars.exampleapps/electric-proxy/src/electric.tsapps/electric-proxy/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .superset/lib/setup/steps.sh
- apps/electric-proxy/src/index.ts
- apps/electric-proxy/.dev.vars.example
Comment on lines
+13
to
+29
| const bearer = request.headers.get("Authorization"); | ||
| if (bearer?.startsWith("Bearer ")) { | ||
| const token = bearer.slice(7); | ||
| try { | ||
| const { payload } = await auth.api.verifyJWT({ body: { token } }); | ||
| if (payload?.sub && Array.isArray(payload.organizationIds)) { | ||
| return { | ||
| userId: payload.sub, | ||
| email: (payload.email as string) ?? "", | ||
| organizationIds: payload.organizationIds as string[], | ||
| }; | ||
| } | ||
| } catch {} | ||
| } | ||
|
|
||
| const sessionData = await auth.api.getSession({ headers: request.headers }); | ||
| if (!sessionData?.user) return null; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "route.ts" | grep -E "electric.*route\.ts"Repository: superset-sh/superset
Length of output: 114
🏁 Script executed:
git ls-files | grep "electric.*route\.ts"Repository: superset-sh/superset
Length of output: 112
🏁 Script executed:
fd "route.ts" apps/apiRepository: superset-sh/superset
Length of output: 1650
🏁 Script executed:
cat -n apps/api/src/app/api/electric/[...path]/route.tsRepository: superset-sh/superset
Length of output: 4632
🏁 Script executed:
# Check for Biome configuration and linting
cat biome.json 2>/dev/null || cat .biomeignore 2>/dev/null || echo "No Biome config found"Repository: superset-sh/superset
Length of output: 85
🏁 Script executed:
# Check if there are any comments or tests explaining the auth fallback behavior
rg -A 5 -B 5 "authenticate.*request" apps/api/src/app/api/electric/[...path]/route.tsRepository: superset-sh/superset
Length of output: 701
🏁 Script executed:
# Search for similar patterns of empty catch blocks in TypeScript files
rg "catch\s*\{\s*\}" --type ts -C 2 apps/api/src/app/api/electric/Repository: superset-sh/superset
Length of output: 337
Return null when Bearer token verification fails—do not fall back to session auth.
If an Authorization header with a Bearer token is provided, that signals explicit token-based authentication. When JWT verification fails, silently catching the error and falling back to session auth creates credential-precedence ambiguity. An invalid or malformed token should reject authentication entirely, not degrade to session validation.
Proposed fix
if (bearer?.startsWith("Bearer ")) {
const token = bearer.slice(7);
try {
const { payload } = await auth.api.verifyJWT({ body: { token } });
if (payload?.sub && Array.isArray(payload.organizationIds)) {
return {
userId: payload.sub,
email: (payload.email as string) ?? "",
organizationIds: payload.organizationIds as string[],
};
}
- } catch {}
+ } catch {
+ return null;
+ }
+ return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const bearer = request.headers.get("Authorization"); | |
| if (bearer?.startsWith("Bearer ")) { | |
| const token = bearer.slice(7); | |
| try { | |
| const { payload } = await auth.api.verifyJWT({ body: { token } }); | |
| if (payload?.sub && Array.isArray(payload.organizationIds)) { | |
| return { | |
| userId: payload.sub, | |
| email: (payload.email as string) ?? "", | |
| organizationIds: payload.organizationIds as string[], | |
| }; | |
| } | |
| } catch {} | |
| } | |
| const sessionData = await auth.api.getSession({ headers: request.headers }); | |
| if (!sessionData?.user) return null; | |
| const bearer = request.headers.get("Authorization"); | |
| if (bearer?.startsWith("Bearer ")) { | |
| const token = bearer.slice(7); | |
| try { | |
| const { payload } = await auth.api.verifyJWT({ body: { token } }); | |
| if (payload?.sub && Array.isArray(payload.organizationIds)) { | |
| return { | |
| userId: payload.sub, | |
| email: (payload.email as string) ?? "", | |
| organizationIds: payload.organizationIds as string[], | |
| }; | |
| } | |
| } catch { | |
| return null; | |
| } | |
| return null; | |
| } | |
| const sessionData = await auth.api.getSession({ headers: request.headers }); | |
| if (!sessionData?.user) return null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/app/api/electric/`[...path]/route.ts around lines 13 - 29, If an
Authorization header containing a Bearer token is present, do not fall back to
session auth when JWT verification fails; detect bearer
(bearer?.startsWith("Bearer ")), extract token (token = bearer.slice(7)), call
auth.api.verifyJWT({ body: { token } }) and if verification throws or payload is
missing required fields, immediately return null instead of proceeding to
auth.api.getSession; only call auth.api.getSession when no Authorization Bearer
header exists. Ensure references to verifyJWT, getSession, bearer, token, and
sessionData are updated accordingly.
Comment on lines
+17
to
+23
| const { payload } = await auth.api.verifyJWT({ body: { token } }); | ||
| if (payload?.sub && Array.isArray(payload.organizationIds)) { | ||
| return { | ||
| userId: payload.sub, | ||
| email: (payload.email as string) ?? "", | ||
| organizationIds: payload.organizationIds as string[], | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "route.ts" | grep -E "electric.*path" | head -5Repository: superset-sh/superset
Length of output: 114
🏁 Script executed:
git ls-files | grep -E "apps/api/src/app/api/electric.*route\.ts"Repository: superset-sh/superset
Length of output: 112
🏁 Script executed:
find . -path "*/apps/api/src/app/api/electric/*" -name "*.ts" -type fRepository: superset-sh/superset
Length of output: 165
🏁 Script executed:
cat -n ./apps/api/src/app/api/electric/[...path]/route.tsRepository: superset-sh/superset
Length of output: 4632
Validate JWT claim types before constructing AuthInfo.
Line 20–22 use unchecked casts without runtime narrowing. Although payload?.sub is checked for truthiness on line 18, it could be any truthy non-string value. Similarly, payload.email and payload.organizationIds array elements lack type validation. This violates the type safety guideline and could lead to incorrect behavior or failures downstream.
Add runtime type checks for sub (string), email (string), and filter organizationIds entries to ensure all are strings:
Proposed fix
try {
const { payload } = await auth.api.verifyJWT({ body: { token } });
- if (payload?.sub && Array.isArray(payload.organizationIds)) {
+ const userId = typeof payload?.sub === "string" ? payload.sub : null;
+ const email = typeof payload?.email === "string" ? payload.email : "";
+ const organizationIds = Array.isArray(payload?.organizationIds)
+ ? payload.organizationIds.filter(
+ (id): id is string => typeof id === "string",
+ )
+ : [];
+ if (userId) {
return {
- userId: payload.sub,
- email: (payload.email as string) ?? "",
- organizationIds: payload.organizationIds as string[],
+ userId,
+ email,
+ organizationIds,
};
}
} catch {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { payload } = await auth.api.verifyJWT({ body: { token } }); | |
| if (payload?.sub && Array.isArray(payload.organizationIds)) { | |
| return { | |
| userId: payload.sub, | |
| email: (payload.email as string) ?? "", | |
| organizationIds: payload.organizationIds as string[], | |
| }; | |
| const { payload } = await auth.api.verifyJWT({ body: { token } }); | |
| const userId = typeof payload?.sub === "string" ? payload.sub : null; | |
| const email = typeof payload?.email === "string" ? payload.email : ""; | |
| const organizationIds = Array.isArray(payload?.organizationIds) | |
| ? payload.organizationIds.filter( | |
| (id): id is string => typeof id === "string", | |
| ) | |
| : []; | |
| if (userId) { | |
| return { | |
| userId, | |
| email, | |
| organizationIds, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/app/api/electric/`[...path]/route.ts around lines 17 - 23, The
returned AuthInfo is constructed from unchecked payload fields; add runtime type
guards after auth.api.verifyJWT to ensure payload.sub is a string before
assigning to userId, ensure payload.email is a string (otherwise set email to
empty string), and ensure payload.organizationIds is an array and map/filter it
to a string[] by keeping only elements with typeof === "string"; update the
object returned (the block that currently returns userId: payload.sub, email:
(payload.email as string) ?? "", organizationIds: payload.organizationIds as
string[]) to use these validated values so downstream code sees correct types.
- Old Vercel proxy always routes to Fly.io (removed cloud/email check) - Cloudflare Worker always routes to Electric Cloud (removed dual-mode) - Desktop uses PostHog `electric-cloud` flag to pick proxy - Default: old proxy (Fly.io), flag on: Cloudflare Worker (Electric Cloud)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/shared/src/constants.ts (1)
62-62: Add a JSDoc comment for consistency with other feature flags.All other feature flags in this object have descriptive comments explaining their purpose. Adding one for
ELECTRIC_CLOUDwould maintain consistency and help other developers understand when this flag is used.CLOUD_ACCESS: "cloud-access", + /** Gates access to Electric Cloud proxy (Cloudflare Worker-based caching). */ ELECTRIC_CLOUD: "electric-cloud",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/constants.ts` at line 62, Add a JSDoc comment immediately above the ELECTRIC_CLOUD entry in the feature flags object describing its purpose and when to enable it (matching the style and tone of the other comments in the same file); locate the ELECTRIC_CLOUD constant in packages/shared/src/constants.ts and add a short descriptive JSDoc block that explains this flag's intent and typical usage so it is consistent with the other feature-flag comments.apps/electric-proxy/src/electric.ts (1)
61-63: Cache key includes credentials in the URL string.The
buildCacheKeyreturns the full upstream URL which includessource_idandsecretquery parameters. While this works for cache uniqueness, it means credentials are part of the cache key. Consider removing credentials from the cache key since they're constant per deployment:Suggested approach
export function buildCacheKey(upstreamUrl: URL, _auth: AuthContext): string { - return upstreamUrl.toString(); + const cacheUrl = new URL(upstreamUrl.toString()); + cacheUrl.searchParams.delete("source_id"); + cacheUrl.searchParams.delete("secret"); + return cacheUrl.toString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electric-proxy/src/electric.ts` around lines 61 - 63, buildCacheKey currently returns the full upstreamUrl.toString() which embeds credential query params (e.g., source_id, secret); change buildCacheKey(upstreamUrl: URL, _auth: AuthContext) to construct the key from a sanitized URL that removes those credential params (delete specific params like "source_id" and "secret" from upstreamUrl.searchParams or build a new URL without them), and produce a deterministic string (e.g., origin + pathname + sorted remaining query string + hash) so cache keys are stable and credentials are not included.
🤖 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/api/src/app/api/electric/`[...path]/route.ts:
- Around line 42-46: The handler currently skips validation when organizationId
is missing and later passes an empty string into buildWhereClause; change the
logic to require an explicit organizationId before proceeding: immediately
return a 400 (Bad Request) if url.searchParams.get("organizationId") is
null/empty, then verify membership using
authInfo.organizationIds.includes(organizationId) and return 403 if not a
member; only after both checks pass call buildWhereClause with the validated
organizationId. Reference symbols: the organizationId variable from
url.searchParams, authInfo.organizationIds.includes(...), and buildWhereClause.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx`:
- Around line 34-38: The Electric shapes are being created in the collections
useMemo with the current electricUrl before useEffect can call setElectricUrl,
causing a race where shapes (and preloadCollections/getCollections
subscriptions) use the wrong endpoint; fix by ensuring electricUrl is set before
shapes are created — either move the setElectricUrl call to run synchronously
during initialization (so it executes before the useMemo that builds shapes), or
add electricUrl to the dependency array of the collections useMemo so shapes are
recreated when electricUrl changes; update references to useEffect,
setElectricUrl, electricUrl, useMemo that builds
getCollections/preloadCollections, and activeOrganizationId/useElectricCloud
accordingly to avoid stale captured values.
---
Nitpick comments:
In `@apps/electric-proxy/src/electric.ts`:
- Around line 61-63: buildCacheKey currently returns the full
upstreamUrl.toString() which embeds credential query params (e.g., source_id,
secret); change buildCacheKey(upstreamUrl: URL, _auth: AuthContext) to construct
the key from a sanitized URL that removes those credential params (delete
specific params like "source_id" and "secret" from upstreamUrl.searchParams or
build a new URL without them), and produce a deterministic string (e.g., origin
+ pathname + sorted remaining query string + hash) so cache keys are stable and
credentials are not included.
In `@packages/shared/src/constants.ts`:
- Line 62: Add a JSDoc comment immediately above the ELECTRIC_CLOUD entry in the
feature flags object describing its purpose and when to enable it (matching the
style and tone of the other comments in the same file); locate the
ELECTRIC_CLOUD constant in packages/shared/src/constants.ts and add a short
descriptive JSDoc block that explains this flag's intent and typical usage so it
is consistent with the other feature-flag comments.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.superset/lib/setup/steps.shapps/api/src/app/api/electric/[...path]/route.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/electric-proxy/.dev.vars.exampleapps/electric-proxy/src/electric.tsapps/electric-proxy/src/types.tspackages/shared/src/constants.ts
✅ Files skipped from review due to trivial changes (1)
- apps/electric-proxy/.dev.vars.example
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts
- .superset/lib/setup/steps.sh
Comment on lines
+42
to
46
| const organizationId = url.searchParams.get("organizationId"); | ||
|
|
||
| if (organizationId && !allowedOrgIds.includes(organizationId)) { | ||
| if (organizationId && !authInfo.organizationIds.includes(organizationId)) { | ||
| return new Response("Not a member of this organization", { status: 403 }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the structure and find the relevant files
find apps/api/src/app/api/electric -type f -name "*.ts" -o -name "*.tsx" | head -20Repository: superset-sh/superset
Length of output: 161
🏁 Script executed:
#!/bin/bash
# Check if utils.ts exists and examine buildWhereClause
if [ -f "apps/api/src/app/api/electric/[...path]/utils.ts" ]; then
echo "=== utils.ts found ==="
cat "apps/api/src/app/api/electric/[...path]/utils.ts"
else
echo "=== Searching for utils.ts with fd ==="
fd "utils" apps/api/src/app/api/electric/
fiRepository: superset-sh/superset
Length of output: 3743
🏁 Script executed:
#!/bin/bash
# Search for buildWhereClause function definition
rg -n "buildWhereClause" apps/api/src/app/api/electric/ -A 15 -B 2Repository: superset-sh/superset
Length of output: 4364
🏁 Script executed:
#!/bin/bash
# Also check the route.ts file to see the full context
cat -n "apps/api/src/app/api/electric/[...path]/route.ts"Repository: superset-sh/superset
Length of output: 3734
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or documentation about organizationId requirement
rg -i "organizationId.*required\|optional" apps/api/src/app/api/electric/ -B 2 -A 2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check if there's any validation requiring organizationId elsewhere
rg -n "organizationId" apps/api/src/app/api/electric/\[...path\]/route.ts -B 3 -A 3Repository: superset-sh/superset
Length of output: 1168
Require organizationId validation for all endpoints.
When organizationId is missing from query params, the validation check (line 44) is entirely skipped. The code then passes an empty string to buildWhereClause (line 64). This is problematic because:
- For
auth.organizationstable: The endpoint ignores the emptyorganizationIdparameter and returns all organizations the authenticated user belongs to, bypassing organization scoping. - For other tables: Behavior is inconsistent—some return empty results with
organizationId = "", but the lack of explicit validation means scoping is not enforced at the handler level.
All endpoints should require explicit organizationId validation before calling buildWhereClause, ensuring proper organization scoping regardless of query parameters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/app/api/electric/`[...path]/route.ts around lines 42 - 46, The
handler currently skips validation when organizationId is missing and later
passes an empty string into buildWhereClause; change the logic to require an
explicit organizationId before proceeding: immediately return a 400 (Bad
Request) if url.searchParams.get("organizationId") is null/empty, then verify
membership using authInfo.organizationIds.includes(organizationId) and return
403 if not a member; only after both checks pass call buildWhereClause with the
validated organizationId. Reference symbols: the organizationId variable from
url.searchParams, authInfo.organizationIds.includes(...), and buildWhereClause.
Comment on lines
+34
to
+38
| useEffect(() => { | ||
| if (useElectricCloud) { | ||
| setElectricUrl(env.NEXT_PUBLIC_ELECTRIC_URL); | ||
| } | ||
| }, [useElectricCloud]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find and examine the collections.ts file
fd "collections.ts" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/
# Check the setElectricUrl function
rg -n "setElectricUrl" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/ -A 5 -B 2
# Check how electricUrl is used
rg -n "electricUrl" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/ -A 2 -B 2Repository: superset-sh/superset
Length of output: 13404
🏁 Script executed:
#!/bin/bash
# Find where getCollections is called in the CollectionsProvider
rg -n "getCollections" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx -B 2 -A 2
# Check the getCollections function implementation and how it uses electricUrl
rg -n "export function getCollections" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts -A 30
# Check if shapes are created lazily or at module load time
rg -n "const shapes" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts -B 2 -A 20Repository: superset-sh/superset
Length of output: 997
🏁 Script executed:
#!/bin/bash
# Get full context around line 74 to see what hook/function contains it
sed -n '60,90p' apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx
# Find and examine createOrgCollections
rg -n "function createOrgCollections" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts -A 40Repository: superset-sh/superset
Length of output: 2244
Potential race condition: Electric shapes may be created before URL is updated.
When activeOrganizationId is available, the useMemo immediately creates Electric shapes with the current electricUrl. If this happens before the useEffect calls setElectricUrl(), the shapes will use the default API URL instead of the Electric Cloud URL, and preloadCollections will start subscriptions against the wrong endpoint. Since electricUrl is captured when shape objects are created, updating it later won't affect already-initialized shapes.
Ensure setElectricUrl() is called during initialization before getCollections() is invoked, or add electricUrl to the dependencies of the collections useMemo to recreate shapes when the URL updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx`
around lines 34 - 38, The Electric shapes are being created in the collections
useMemo with the current electricUrl before useEffect can call setElectricUrl,
causing a race where shapes (and preloadCollections/getCollections
subscriptions) use the wrong endpoint; fix by ensuring electricUrl is set before
shapes are created — either move the setElectricUrl call to run synchronously
during initialization (so it executes before the useMemo that builds shapes), or
add electricUrl to the dependency array of the collections useMemo so shapes are
recreated when electricUrl changes; update references to useEffect,
setElectricUrl, electricUrl, useMemo that builds
getCollections/preloadCollections, and activeOrganizationId/useElectricCloud
accordingly to avoid stale captured values.
Contributor
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx">
<violation number="1">
P1: Setting the Electric URL in `useEffect` is too late for collections that are created during render/module initialization, so the feature flag may not actually switch all collections to the cloud endpoint.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -1,3 +1,5 @@ | |||
| import { FEATURE_FLAGS } from "@superset/shared/constants"; | |||
Contributor
There was a problem hiding this comment.
P1: Setting the Electric URL in useEffect is too late for collections that are created during render/module initialization, so the feature flag may not actually switch all collections to the cloud endpoint.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx, line 36:
<comment>Setting the Electric URL in `useEffect` is too late for collections that are created during render/module initialization, so the feature flag may not actually switch all collections to the cloud endpoint.</comment>
<file context>
@@ -20,9 +26,16 @@ const CollectionsContext = createContext<CollectionsContextType | null>(null);
+
+ useEffect(() => {
+ if (useElectricCloud) {
+ setElectricUrl(env.NEXT_PUBLIC_ELECTRIC_URL);
+ }
+ }, [useElectricCloud]);
</file context>
Merged
5 tasks
AviPeltz
added a commit
that referenced
this pull request
Apr 18, 2026
Observability was enabled in #1464 but dropped when the proxy was re-created from scratch in #1867. Without it, every wrangler deploy reconciles Cloudflare back to logs/traces off, which is why the dashboard toggles kept reverting after each production deploy. Pin invocation_logs explicitly so future config drift can't silently disable it again. Audit logs are an account-level setting and still need to be re-enabled in the Cloudflare dashboard separately.
4 tasks
AviPeltz
added a commit
that referenced
this pull request
Apr 18, 2026
…3565) Observability was enabled in #1464 but dropped when the proxy was re-created from scratch in #1867. Without it, every wrangler deploy reconciles Cloudflare back to logs/traces off, which is why the dashboard toggles kept reverting after each production deploy. Pin invocation_logs explicitly so future config drift can't silently disable it again. Audit logs are an account-level setting and still need to be re-enabled in the Cloudflare dashboard separately.
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
1 participant
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
vercel.jsonwithCDN-Cache-Control: no-storeon/api/electric/*has no effect since Vercel already doesn't cache authenticated requestsTest plan
Summary by CodeRabbit
New Features
Chores