feat(api): backend prereqs for CLI v1#3889
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralizes task creation on the backend, removes client-side collection insert hooks, adds list/query endpoints for tasks, hosts, and workspaces, tightens OAuth redirect/validation flows, and modifies JWT resolution to prefer JWT payload with session-based fallback. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Greptile SummaryThis PR lands backend prerequisites for CLI v1: a unified
Confidence Score: 4/5Safe to merge after addressing the jwtProcedure error-swallowing; P2s can follow in a cleanup PR. One P1 in trpc.ts changes error-propagation semantics for all jwtProcedure endpoints: revoked/hard-rejected JWTs are now silently reclassified as session auth, which masks failures and could allow unintended access. The two P2s (ILIKE wildcards, stale cookie) are low-blast-radius within-org issues. Everything else looks correct. packages/trpc/src/trpc.ts (jwtProcedure catch block), packages/trpc/src/router/task/task.ts (ilike search), apps/web/src/app/utils/pendingAuthRedirect/pendingAuthRedirect.ts (path-mismatch cookie cleanup)
|
| Filename | Overview |
|---|---|
| packages/trpc/src/trpc.ts | jwtProcedure now falls through to session auth on any JWT error, silently swallowing TRPCErrors that the old code re-threw |
| packages/trpc/src/router/task/task.ts | Consolidates createFromUi/create into a shared createTask helper; adds list (with filters/pagination) and byIdOrSlug endpoints; ilike search passes unescaped wildcards |
| packages/trpc/src/router/automation/automation.ts | verifyWorkspaceInOrg now returns projectId so it can be derived when only v2WorkspaceId is supplied |
| packages/trpc/src/router/host/host.ts | New list endpoint scoped to the caller's registered hosts via v2UsersHosts join |
| packages/trpc/src/router/v2-workspace/v2-workspace.ts | New list endpoint scoped to the caller's registered hosts; optional hostId filter |
| apps/web/src/app/utils/pendingAuthRedirect/pendingAuthRedirect.ts | New utility reads/clears a cookie stashing pre-auth query params; cookie is not cleared when path mismatches |
| apps/web/src/proxy.ts | Middleware now stashes path+params in an httpOnly cookie before redirecting unauthenticated requests to sign-in |
Sequence Diagram
sequenceDiagram
participant CLI
participant Browser
participant Middleware as proxy.ts
participant SignIn as /sign-in
participant AuthPage as /cli/authorize or /oauth/consent
participant Cookie as pendingAuthRedirect cookie
participant tRPC as tRPC (jwtProcedure)
participant DB
CLI->>AuthPage: GET /cli/authorize?state=X&redirect_uri=Y (unauthenticated)
Middleware->>Cookie: set superset_pending_auth_redirect {path, params}
Middleware->>Browser: 302 to /sign-in?redirect=/cli/authorize
Browser->>SignIn: Sign in
SignIn->>AuthPage: redirect back (no query params)
AuthPage->>Cookie: consumePendingAuthParams("/cli/authorize")
Cookie-->>AuthPage: {state, redirect_uri} restored
AuthPage-->>Browser: render CliAuthorizeForm
CLI->>tRPC: Bearer JWT to host.list / v2workspace.list
Note over tRPC: Try verifyJWT(token)
alt JWT valid and has sub
tRPC->>DB: org membership from JWT payload
else JWT invalid/error or no sub
Note over tRPC: catch {} fall through
tRPC->>DB: org membership from session
end
tRPC-->>CLI: response
Comments Outside Diff (2)
-
packages/trpc/src/router/task/task.ts, line 640-641 (link)Unescaped LIKE wildcards in search filter
input.searchis interpolated directly into anilikepattern without escaping%or_. A caller that passes%as the search term will match every task in the organization, and a term likea_cwill matchabc,axc, etc. While the result set is still scoped to the caller's organization, it defeats the intent of the search filter.Consider escaping special characters before building the pattern:
const escaped = input.search.replace(/[%_\\]/g, "\\$&"); filters.push(ilike(tasks.title, `%${escaped}%`));
Prompt To Fix With AI
This is a comment left during a code review. Path: packages/trpc/src/router/task/task.ts Line: 640-641 Comment: **Unescaped LIKE wildcards in search filter** `input.search` is interpolated directly into an `ilike` pattern without escaping `%` or `_`. A caller that passes `%` as the search term will match every task in the organization, and a term like `a_c` will match `abc`, `axc`, etc. While the result set is still scoped to the caller's organization, it defeats the intent of the search filter. Consider escaping special characters before building the pattern: ```ts const escaped = input.search.replace(/[%_\\]/g, "\\$&"); filters.push(ilike(tasks.title, `%${escaped}%`)); ``` How can I resolve this? If you propose a fix, please make it concise.
-
apps/web/src/app/utils/pendingAuthRedirect/pendingAuthRedirect.ts, line 205-208 (link)When
parsed.path !== expectedPath, the function returnsnullwithout deleting the cookie. The cookie then persists for the fullPENDING_COOKIE_TTL_SECONDS(10 minutes). If the sign-in flow routes the user through an intermediate protected page before reaching the correct destination, the cookie is never consumed and the original params are silently lost after expiry. Clearing the cookie unconditionally on mismatch would be safer:if (parsed.path !== expectedPath) { cookieStore.delete(COOKIE_NAME); // clear stale entry return null; }
Prompt To Fix With AI
This is a comment left during a code review. Path: apps/web/src/app/utils/pendingAuthRedirect/pendingAuthRedirect.ts Line: 205-208 Comment: **Cookie not cleared on path mismatch** When `parsed.path !== expectedPath`, the function returns `null` without deleting the cookie. The cookie then persists for the full `PENDING_COOKIE_TTL_SECONDS` (10 minutes). If the sign-in flow routes the user through an intermediate protected page before reaching the correct destination, the cookie is never consumed and the original params are silently lost after expiry. Clearing the cookie unconditionally on mismatch would be safer: ```ts if (parsed.path !== expectedPath) { cookieStore.delete(COOKIE_NAME); // clear stale entry return null; } ``` How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/trpc/src/trpc.ts:97-99
**Silent JWT error swallowing may mask revocation/network failures**
The `catch` block now swallows _all_ errors from `verifyJWT` — including transient network errors, library panics, or explicit token-revocation rejections that the auth library might surface as thrown exceptions. When any of these fire, execution silently falls through to session auth, so a caller supplying an explicitly revoked or deliberately invalid JWT plus a live browser session is still authenticated. The old code re-threw `TRPCError` instances, preserving that signal.
If the intent is to support both JWT and session on the same procedure, consider checking for specific "invalid token" error types before falling through, and returning 401 for any hard failure that isn't a simple parse error:
```ts
} catch (err) {
// Only fall through on token-format/signature errors.
// Re-throw anything that looks like a definitive rejection.
if (err instanceof TRPCError) throw err;
// fall through to session resolution
}
```
### Issue 2 of 3
packages/trpc/src/router/task/task.ts:640-641
**Unescaped LIKE wildcards in search filter**
`input.search` is interpolated directly into an `ilike` pattern without escaping `%` or `_`. A caller that passes `%` as the search term will match every task in the organization, and a term like `a_c` will match `abc`, `axc`, etc. While the result set is still scoped to the caller's organization, it defeats the intent of the search filter.
Consider escaping special characters before building the pattern:
```ts
const escaped = input.search.replace(/[%_\\]/g, "\\$&");
filters.push(ilike(tasks.title, `%${escaped}%`));
```
### Issue 3 of 3
apps/web/src/app/utils/pendingAuthRedirect/pendingAuthRedirect.ts:205-208
**Cookie not cleared on path mismatch**
When `parsed.path !== expectedPath`, the function returns `null` without deleting the cookie. The cookie then persists for the full `PENDING_COOKIE_TTL_SECONDS` (10 minutes). If the sign-in flow routes the user through an intermediate protected page before reaching the correct destination, the cookie is never consumed and the original params are silently lost after expiry. Clearing the cookie unconditionally on mismatch would be safer:
```ts
if (parsed.path !== expectedPath) {
cookieStore.delete(COOKIE_NAME); // clear stale entry
return null;
}
```
Reviews (1): Last reviewed commit: "feat(api): backend prereqs for CLI v1" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch { | ||
| // Fall through to session-token resolution. | ||
| } |
There was a problem hiding this comment.
The catch block now swallows all errors from verifyJWT — including transient network errors, library panics, or explicit token-revocation rejections that the auth library might surface as thrown exceptions. When any of these fire, execution silently falls through to session auth, so a caller supplying an explicitly revoked or deliberately invalid JWT plus a live browser session is still authenticated. The old code re-threw TRPCError instances, preserving that signal.
If the intent is to support both JWT and session on the same procedure, consider checking for specific "invalid token" error types before falling through, and returning 401 for any hard failure that isn't a simple parse error:
} catch (err) {
// Only fall through on token-format/signature errors.
// Re-throw anything that looks like a definitive rejection.
if (err instanceof TRPCError) throw err;
// fall through to session resolution
}Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/trpc/src/trpc.ts
Line: 97-99
Comment:
**Silent JWT error swallowing may mask revocation/network failures**
The `catch` block now swallows _all_ errors from `verifyJWT` — including transient network errors, library panics, or explicit token-revocation rejections that the auth library might surface as thrown exceptions. When any of these fire, execution silently falls through to session auth, so a caller supplying an explicitly revoked or deliberately invalid JWT plus a live browser session is still authenticated. The old code re-threw `TRPCError` instances, preserving that signal.
If the intent is to support both JWT and session on the same procedure, consider checking for specific "invalid token" error types before falling through, and returning 401 for any hard failure that isn't a simple parse error:
```ts
} catch (err) {
// Only fall through on token-format/signature errors.
// Re-throw anything that looks like a definitive rejection.
if (err instanceof TRPCError) throw err;
// fall through to session resolution
}
```
How can I resolve this? If you propose a fix, please make it concise.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/cli/authorize/page.tsx`:
- Around line 46-49: The current prefix check on redirectUri is unsafe; replace
the startsWith logic by parsing redirectUri with new URL(redirectUri) inside the
authorize/page.tsx code path that currently checks redirectUri, then validate
url.protocol === "http:", url.hostname is either "127.0.0.1" or "localhost", and
ensure url.username and url.password are empty (no userinfo); only accept the
callback if those conditions hold (and optionally validate url.port if you
require a specific range), otherwise reject the redirectUri as invalid.
In `@apps/web/src/app/utils/pendingAuthRedirect/pendingAuthRedirect.ts`:
- Around line 29-31: The code currently does JSON.parse(cookie.value) as
StoredPendingRedirect which trusts client input; add a runtime validation for
the parsed object (e.g., validate presence/types of path:string and
params:object and that params.redirect_uri is a string) before returning it from
the pendingAuthRedirect logic. Replace the unchecked cast to
StoredPendingRedirect with safe parsing + a guard function (e.g.,
isStoredPendingRedirect(parsed)) that returns false for malformed payloads (so
you discard and return undefined/null), and ensure callers like CliAuthorizePage
that expect to call startsWith on path only receive validated strings. Use the
StoredPendingRedirect symbol and the parsed variable in the guard to locate
where to add the validation.
- Around line 22-40: The helper consumePendingAuthParams currently calls
cookieStore.delete(COOKIE_NAME) (via cookies().delete()) which is not permitted
during Server Component rendering; remove those delete calls from
consumePendingAuthParams (both the delete in the JSON parse catch and the
successful-path delete) and instead have the function only read/parse and return
the parsed.params (or null) so callers in CliAuthorizePage/ConsentPage can
invoke a separate Server Action or Route Handler (e.g., export a
clearPendingAuthCookie action or route that calls cookies().delete(COOKIE_NAME))
to perform the actual deletion; keep the COOKIE_NAME and StoredPendingRedirect
parsing logic in consumePendingAuthParams, but move any deletion to the new
Server Action/Route Handler invoked from the pages.
In `@apps/web/src/proxy.ts`:
- Around line 34-54: The code unconditionally stashes all query params into the
PENDING_COOKIE_NAME cookie for any protected route (inside the branch checking
!session && !isPublicRoute(pathname)), causing unrelated params to be stored;
constrain this so only the auth flows that consume the cookie get params saved:
update the redirect branch around NextResponse.redirect to only set the cookie
when pathname is one of the auth endpoints (e.g. "/cli/authorize" or
"/oauth/consent") or when the query contains only whitelisted keys used by those
pages, preserving existing cookie options (PENDING_COOKIE_TTL_SECONDS,
httpOnly/secure/sameSite/path) and leaving other protected routes to redirect
without setting the cookie; reference isPublicRoute, pathname,
PENDING_COOKIE_NAME and PENDING_COOKIE_TTL_SECONDS when locating the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72883a28-365c-434c-87a3-84ef3e777775
📒 Files selected for processing (16)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/CreateTaskDialog.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/mobile/lib/collections/collections.tsapps/web/src/app/cli/authorize/page.tsxapps/web/src/app/oauth/consent/page.tsxapps/web/src/app/utils/pendingAuthRedirect/index.tsapps/web/src/app/utils/pendingAuthRedirect/pendingAuthRedirect.tsapps/web/src/proxy.tspackages/host-service/src/trpc/router/project/project.tspackages/trpc/src/router/automation/automation.tspackages/trpc/src/router/automation/schema.tspackages/trpc/src/router/host/host.tspackages/trpc/src/router/task/schema.tspackages/trpc/src/router/task/task.tspackages/trpc/src/router/v2-workspace/v2-workspace.tspackages/trpc/src/trpc.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts
- apps/mobile/lib/collections/collections.ts
| export async function consumePendingAuthParams( | ||
| expectedPath: string, | ||
| ): Promise<Record<string, string> | null> { | ||
| const cookieStore = await cookies(); | ||
| const cookie = cookieStore.get(COOKIE_NAME); | ||
| if (!cookie) return null; | ||
|
|
||
| let parsed: StoredPendingRedirect; | ||
| try { | ||
| parsed = JSON.parse(cookie.value) as StoredPendingRedirect; | ||
| } catch { | ||
| cookieStore.delete(COOKIE_NAME); | ||
| return null; | ||
| } | ||
|
|
||
| if (parsed.path !== expectedPath) return null; | ||
|
|
||
| cookieStore.delete(COOKIE_NAME); | ||
| return parsed.params; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all usages of consumePendingAuthParams
rg "consumePendingAuthParams" --type ts --type tsx -B 2 -A 2Repository: superset-sh/superset
Length of output: 92
🏁 Script executed:
# Check the structure of the calling files to understand if they're server components or server functions
fd "page.tsx" | grep -E "(authorize|consent)" | head -5Repository: superset-sh/superset
Length of output: 144
🏁 Script executed:
# Examine the authorize page
cat -n apps/web/src/app/cli/authorize/page.tsx | head -50Repository: superset-sh/superset
Length of output: 1924
🏁 Script executed:
# Examine the consent page
cat -n apps/web/src/app/oauth/consent/page.tsx | head -50Repository: superset-sh/superset
Length of output: 2003
🌐 Web query:
Next.js cookies() API server components vs server functions mutation
💡 Result:
In Next.js App Router, the cookies API behaves differently depending on where you call it: 1) Server Components (default) - cookies is for reading cookies from the incoming HTTP request. - Modifying cookies (calling .set / .delete) during Server Component rendering is not supported; it will throw an error (because Server Components don’t have access to the Response headers needed to emit Set-Cookie). 2) Server Functions (Server Actions) / Route Handlers - cookies can be used to both read incoming request cookies and to mutate cookies by emitting outgoing cookies (via .set / .delete). - This is allowed because Server Actions / Route Handlers run in a request/response context where Next.js can attach the required Set-Cookie headers. - After you set/delete cookies in a Server Action, Next.js re-renders the current page/layout tree so the UI reflects the new cookie value. What to do in practice - Read in Server Components: cookies.get(...), cookies.getAll(...), cookies.has(...). - Mutate via Server Action or Route Handler: 'use server' functions (Server Actions) and app/api route handlers; inside those, do const cookieStore = await cookies; cookieStore.set(...)/cookieStore.delete(...). Key rule of thumb - Server Components: side-effect free; use cookies for reading only. - Server Actions / Route Handlers: allowed for cookie mutations (set/delete) because headers can be updated.
Citations:
- 1: https://nextjs.org/docs/15/app/api-reference/functions/cookies
- 2: https://nextjs.org/docs/app/getting-started/mutating-data
- 3: Can't set cookies in server components:
cookies().set()throws error outside Server Actions or Route Handlers vercel/next.js#81570 - 4: https://nextjs.org/docs/app/api-reference/functions/cookies
Move cookie deletion out of this helper.
consumePendingAuthParams() is called from page.tsx Server Components (CliAuthorizePage and ConsentPage), but Next.js does not support cookies().delete() in Server Component rendering—it only works in Server Actions or Route Handlers. The delete calls on lines 33 and 39 will silently fail, allowing the stale auth params cookie to persist and replay on subsequent visits. Extract deletion into a separate Server Action or Route Handler that the pages can invoke.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/utils/pendingAuthRedirect/pendingAuthRedirect.ts` around
lines 22 - 40, The helper consumePendingAuthParams currently calls
cookieStore.delete(COOKIE_NAME) (via cookies().delete()) which is not permitted
during Server Component rendering; remove those delete calls from
consumePendingAuthParams (both the delete in the JSON parse catch and the
successful-path delete) and instead have the function only read/parse and return
the parsed.params (or null) so callers in CliAuthorizePage/ConsentPage can
invoke a separate Server Action or Route Handler (e.g., export a
clearPendingAuthCookie action or route that calls cookies().delete(COOKIE_NAME))
to perform the actual deletion; keep the COOKIE_NAME and StoredPendingRedirect
parsing logic in consumePendingAuthParams, but move any deletion to the new
Server Action/Route Handler invoked from the pages.
| let parsed: StoredPendingRedirect; | ||
| try { | ||
| parsed = JSON.parse(cookie.value) as StoredPendingRedirect; |
There was a problem hiding this comment.
Validate the cookie payload before returning it.
JSON.parse(cookie.value) as StoredPendingRedirect trusts unvalidated client input. A malformed cookie like {"path":"/cli/authorize","params":{"redirect_uri":42}} will flow through as typed data and can crash the later startsWith(...) check in CliAuthorizePage. Add a runtime guard/schema and discard invalid payloads instead of asserting the type.
Proposed guard
interface StoredPendingRedirect {
path: string;
params: Record<string, string>;
}
+
+function isStoredPendingRedirect(value: unknown): value is StoredPendingRedirect {
+ if (typeof value !== "object" || value === null) return false;
+ if (!("path" in value) || typeof value.path !== "string") return false;
+ if (!("params" in value) || typeof value.params !== "object" || value.params === null) {
+ return false;
+ }
+
+ return Object.values(value.params).every(
+ (param): param is string => typeof param === "string",
+ );
+}
export async function consumePendingAuthParams(
expectedPath: string,
): Promise<Record<string, string> | null> {
const cookieStore = await cookies();
@@
- let parsed: StoredPendingRedirect;
+ let parsed: StoredPendingRedirect;
try {
- parsed = JSON.parse(cookie.value) as StoredPendingRedirect;
+ const parsedValue: unknown = JSON.parse(cookie.value);
+ if (!isStoredPendingRedirect(parsedValue)) return null;
+ parsed = parsedValue;
} catch {
cookieStore.delete(COOKIE_NAME);
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.
| let parsed: StoredPendingRedirect; | |
| try { | |
| parsed = JSON.parse(cookie.value) as StoredPendingRedirect; | |
| function isStoredPendingRedirect(value: unknown): value is StoredPendingRedirect { | |
| if (typeof value !== "object" || value === null) return false; | |
| if (!("path" in value) || typeof value.path !== "string") return false; | |
| if (!("params" in value) || typeof value.params !== "object" || value.params === null) { | |
| return false; | |
| } | |
| return Object.values(value.params).every( | |
| (param): param is string => typeof param === "string", | |
| ); | |
| } | |
| export async function consumePendingAuthParams( | |
| expectedPath: string, | |
| ): Promise<Record<string, string> | null> { | |
| const cookieStore = await cookies(); | |
| const cookie = cookieStore.get(COOKIE_NAME); | |
| if (!cookie) return null; | |
| let parsed: StoredPendingRedirect; | |
| try { | |
| const parsedValue: unknown = JSON.parse(cookie.value); | |
| if (!isStoredPendingRedirect(parsedValue)) return null; | |
| parsed = parsedValue; | |
| } catch { | |
| cookieStore.delete(COOKIE_NAME); | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/utils/pendingAuthRedirect/pendingAuthRedirect.ts` around
lines 29 - 31, The code currently does JSON.parse(cookie.value) as
StoredPendingRedirect which trusts client input; add a runtime validation for
the parsed object (e.g., validate presence/types of path:string and
params:object and that params.redirect_uri is a string) before returning it from
the pendingAuthRedirect logic. Replace the unchecked cast to
StoredPendingRedirect with safe parsing + a guard function (e.g.,
isStoredPendingRedirect(parsed)) that returns false for malformed payloads (so
you discard and return undefined/null), and ensure callers like CliAuthorizePage
that expect to call startsWith on path only receive validated strings. Use the
StoredPendingRedirect symbol and the parsed variable in the guard to locate
where to add the validation.
Lands the cloud + desktop changes the new CLI depends on, ahead of the CLI itself, so the next CLI PR can stack cleanly: - task: list filters/pagination, byIdOrSlug, list+create kept under prior `task.all` / `task.createFromUi` paths so the shipped CLI on main keeps compiling against this backend during the rollout - task.update: accept-but-ignore deprecated `branch` field for the same reason - automation: list/create/update/delete + run-now coverage CLI uses - host: list/checkAccess/setOnline shape CLI talks to - v2-workspace: workspace.list + create entry points - web: cli/authorize and oauth/consent flows; proxy preserves original search params on the sign-in redirect (path + search round-trips through `?redirect=` directly — no cookie stash) - desktop/mobile: drop unused sessionHosts collection registrations - host-service: project router shape adjustments CLI sources land in a follow-up PR on top of this one.
saddlepaddle
force-pushed
the
cli-v1-backend
branch
from
dd4e55b to
35733f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/src/app/cli/authorize/page.tsx (1)
10-27:⚠️ Potential issue | 🟠 MajorFix type declaration and normalize query parameters before string operations.
Line 10 declares
searchParams: Promise<Record<string, string>>, but Next.js App Router actually provides{ [key: string]: string | string[] | undefined }. When a query parameter appears multiple times (e.g.,?redirect_uri=http://127.0.0.1:8000&redirect_uri=malicious), the value is an array. Lines 26–27 extract values without normalization, and line 40–41 calls.startsWith()on potentially non-string values, which will throw at runtime. The truthiness check on line 29 does not guard against arrays (which are truthy).Update the type and normalize values before use:
💡 Proposed fix
interface CliAuthorizePageProps { - searchParams: Promise<Record<string, string>>; + searchParams: Promise<Record<string, string | string[] | undefined>>; } export default async function CliAuthorizePage({ searchParams, }: CliAuthorizePageProps) { const session = await auth.api.getSession({ headers: await headers(), }); if (!session) { // Defensive — middleware should have caught this. return null; } const params = await searchParams; + const first = (value: string | string[] | undefined) => + Array.isArray(value) ? value[0] : value; - const state = params.state; - const redirectUri = params.redirect_uri; + const state = first(params.state); + const redirectUri = first(params.redirect_uri); if (!state || !redirectUri) { return ( <div className="flex min-h-screen items-center justify-center"> <p className="text-muted-foreground"> Missing required parameters. Use <code>superset auth login</code>. </p> </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/cli/authorize/page.tsx` around lines 10 - 27, Update the CliAuthorizePage props type and normalize incoming query values before any string operations: change the searchParams type from Promise<Record<string, string>> to Promise<Record<string, string | string[] | undefined>> (or the equivalent Next.js query type) and, inside CliAuthorizePage, resolve searchParams then coerce params.state and params.redirect_uri into plain strings by checking if the value is an array (take the first element), undefined, or a string; use the normalized string values for subsequent truthiness checks and for calling .startsWith() to avoid runtime errors when a param is an array.apps/web/src/app/oauth/consent/page.tsx (1)
11-12:⚠️ Potential issue | 🔴 CriticalHarden
client_idandscopeextraction against repeated query parameters.Next.js App Router
searchParamscan bestring,string[], orundefinedfor repeated query keys (e.g.,?scope=openid&scope=profilebecomes['openid', 'profile']). The current type at line 11 only declaresstring, and downstream usage at lines 58 and 64 assumes scalar strings without guards:
- Line 58:
scope?.split(" ")will throwTypeErrorif scope is an array- Line 64: Passing an array to the database query may cause unexpected behavior
Normalize query values to scalars before use, and update the type to reflect Next.js behavior.
💡 Proposed fix
interface ConsentPageProps { - searchParams: Promise<Record<string, string>>; + searchParams: Promise<Record<string, string | string[] | undefined>>; } @@ const params = await searchParams; - const client_id = params.client_id; - const scope = params.scope; + const first = (value: string | string[] | undefined) => + Array.isArray(value) ? value[0] : value; + const client_id = first(params.client_id); + const scope = first(params.scope);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/oauth/consent/page.tsx` around lines 11 - 12, Update the searchParams type to allow string | string[] | undefined (reflecting Next.js) and normalize values before use: extract client_id as a scalar (if Array.isArray(client_id) use the first element), normalize scope by if Array.isArray(scope) join with ' ' (or if undefined, set to ''), and ensure you only call scope.split(" ") when typeof scope === 'string'; finally pass the scalar client_id (not an array) to the database query (the variable names to change are the page prop searchParams, and the local variables client_id and scope used before the DB lookup).packages/trpc/src/router/task/task.ts (1)
398-432:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrip deprecated
branchout of the update payload.The schema comment says
branchis accepted-but-ignored, but this handler copies every parsed field intoupdateData. Older CLI callers still sendingbranchtherefore won't get the compatibility behavior this PR promises. Drop it during destructuring so the no-op is explicit here.Suggested fix
- const { id, ...data } = input; + const { id, branch: _deprecatedBranch, ...data } = input;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/task/task.ts` around lines 398 - 432, The handler copies every parsed field into updateData so a deprecated "branch" field sent by older CLIs is persisted rather than ignored; update the destructuring of input in the mutation (the block that starts with .mutation(async ({ ctx, input }) => { const { id, ...data } = input; }) to explicitly remove branch (e.g., const { id, branch, ...data } = input) so "branch" is not included in updateData, keeping the no-op/compatibility behavior promised by updateTaskSchema and the updateData handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/trpc/src/router/automation/automation.ts`:
- Around line 198-214: When persisting automation, ensure project/workspace
consistency: if both input.v2ProjectId and input.v2WorkspaceId are provided,
call verifyWorkspaceInOrg(organizationId, input.v2WorkspaceId) and compare
workspace.projectId to input.v2ProjectId, throwing a TRPCError BAD_REQUEST if
they differ; if only input.v2ProjectId is provided, validate ownership by
calling the corresponding project-owner check (e.g., verifyProjectInOrg or
similar) before continuing so you don't persist an org-mismatched v2ProjectId
(also apply the same consistency check where v2ProjectId/v2WorkspaceId are
assigned prior to storage around the code that writes the automation, referenced
by v2ProjectId and verifyWorkspaceInOrg).
In `@packages/trpc/src/router/task/task.ts`:
- Around line 240-242: The create mutation currently triggers
syncTask(result.task.id) without awaiting or handling errors; update the create
path in task.ts so the syncTask call is either awaited (e.g., await
syncTask(result.task.id)) to surface failures and cause the mutation to fail, or
at minimum chain .catch(...) to record/log the error and increment metrics so
rejected promises are not unobserved; modify the code around
syncTask(result.task.id) accordingly and ensure any logging/metrics use the
existing logger/metrics utilities used elsewhere in the router.
In `@packages/trpc/src/trpc.ts`:
- Around line 97-119: The code currently falls back to session auth after a
failed bearer token verification; modify the logic so that if an Authorization:
Bearer header was present and bearer verification failed you do NOT continue to
session resolution but instead return an unauthorized error (or throw) to
preserve bearer-only semantics; locate the try/catch around bearer verification
in trpc.ts (the block that currently "falls through to session-token
resolution"), detect the presence of the Authorization header (or set a flag
when attempting bearer verification) and if verification failed and the header
existed, call the appropriate unauthorized response instead of executing the
ctx.session branch that calls next({ ctx: { userId, email, organizationIds,
activeOrganizationId } }); if you need both modes, implement a separate
jwtOrSessionProcedure rather than silently falling back.
---
Outside diff comments:
In `@apps/web/src/app/cli/authorize/page.tsx`:
- Around line 10-27: Update the CliAuthorizePage props type and normalize
incoming query values before any string operations: change the searchParams type
from Promise<Record<string, string>> to Promise<Record<string, string | string[]
| undefined>> (or the equivalent Next.js query type) and, inside
CliAuthorizePage, resolve searchParams then coerce params.state and
params.redirect_uri into plain strings by checking if the value is an array
(take the first element), undefined, or a string; use the normalized string
values for subsequent truthiness checks and for calling .startsWith() to avoid
runtime errors when a param is an array.
In `@apps/web/src/app/oauth/consent/page.tsx`:
- Around line 11-12: Update the searchParams type to allow string | string[] |
undefined (reflecting Next.js) and normalize values before use: extract
client_id as a scalar (if Array.isArray(client_id) use the first element),
normalize scope by if Array.isArray(scope) join with ' ' (or if undefined, set
to ''), and ensure you only call scope.split(" ") when typeof scope ===
'string'; finally pass the scalar client_id (not an array) to the database query
(the variable names to change are the page prop searchParams, and the local
variables client_id and scope used before the DB lookup).
In `@packages/trpc/src/router/task/task.ts`:
- Around line 398-432: The handler copies every parsed field into updateData so
a deprecated "branch" field sent by older CLIs is persisted rather than ignored;
update the destructuring of input in the mutation (the block that starts with
.mutation(async ({ ctx, input }) => { const { id, ...data } = input; }) to
explicitly remove branch (e.g., const { id, branch, ...data } = input) so
"branch" is not included in updateData, keeping the no-op/compatibility behavior
promised by updateTaskSchema and the updateData handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aac320ea-cf49-4b27-badd-36c5cb59237d
📒 Files selected for processing (14)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/CreateTaskDialog.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/mobile/lib/collections/collections.tsapps/web/src/app/cli/authorize/page.tsxapps/web/src/app/oauth/consent/page.tsxapps/web/src/proxy.tspackages/host-service/src/trpc/router/project/project.tspackages/trpc/src/router/automation/automation.tspackages/trpc/src/router/automation/schema.tspackages/trpc/src/router/host/host.tspackages/trpc/src/router/task/schema.tspackages/trpc/src/router/task/task.tspackages/trpc/src/router/v2-workspace/v2-workspace.tspackages/trpc/src/trpc.ts
💤 Files with no reviewable changes (2)
- apps/mobile/lib/collections/collections.ts
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/src/proxy.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/CreateTaskDialog.tsx
- packages/host-service/src/trpc/router/project/project.ts
- packages/trpc/src/router/host/host.ts
| if (result.task) { | ||
| syncTask(result.task.id); | ||
| } |
There was a problem hiding this comment.
Handle syncTask() failures explicitly on create.
This is now the only create-time sync trigger, but the promise is neither awaited nor caught. If enqueueing the external sync fails, the mutation still returns success and the rejection is left unobserved. Either await it and fail the request, or at least attach a .catch(...) with logging/metrics so create-side sync failures are visible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/trpc/src/router/task/task.ts` around lines 240 - 242, The create
mutation currently triggers syncTask(result.task.id) without awaiting or
handling errors; update the create path in task.ts so the syncTask call is
either awaited (e.g., await syncTask(result.task.id)) to surface failures and
cause the mutation to fail, or at minimum chain .catch(...) to record/log the
error and increment metrics so rejected promises are not unobserved; modify the
code around syncTask(result.task.id) accordingly and ensure any logging/metrics
use the existing logger/metrics utilities used elsewhere in the router.
Address bot review on PR #3889: - cli/authorize: parse redirect_uri with `new URL()` and check the parsed `hostname` + empty userinfo. The earlier prefix-match accepted `http://127.0.0.1:80@evil.example` as a loopback callback. - trpc.jwtProcedure: re-throw TRPCError instances from verifyJWT so an explicitly-rejected token (revoked/forged) doesn't silently fall through to the session cookie path. Non-TRPC parse errors still fall through (covers expired/missing tokens for desktop's session caller). - task.list: escape `%` and `_` in the search input before interpolating into the ilike pattern.
PR #3889 follow-up: - automation.create: when both `v2ProjectId` and `v2WorkspaceId` are supplied, require them to agree. Always derive the stored `v2ProjectId` from the workspace, since the workspace is the ground truth. - automation.create: when only `v2ProjectId` is supplied (no workspace), verify the project belongs to the active organization. Previously a caller-supplied id was inserted as-is.
1 participant
Summary
Lands the cloud + desktop changes the new CLI depends on, ahead of the CLI itself, so the next CLI PR can stack cleanly.
task.list(filters + pagination),task.byIdOrSlug,task.create. Keepstask.allandtask.createFromUias deprecated aliases so the shipped CLI onmainkeeps compiling against this backend during the rollout.task.updateaccepts-but-ignores the deprecatedbranchfield for the same reason.workspace.listand create entry points the CLI uses.cli/authorizeandoauth/consentflows +pendingAuthRedirectutil.sessionHostscollection registrations.CLI sources land in a follow-up PR stacked on top of this one.
Test plan
bun run typecheckcleanbun run lint:fixcleanmaincontinues to compile/run against this backend (deprecated aliases verified)task.listfilters (priority/status/assignee/search) return expected rowsautomationcreate + dispatch works end-to-endcli/authorize+oauth/consentround-trip on webSummary by cubic
Preps the backend for CLI v1 with new task/host/workspace/automation endpoints and improved sign-in redirects. Also tightens CLI OAuth callback validation, adds stricter automation input checks, and improves JWT auth fallback.
New Features
task.list(filters + pagination incl.assigneeMe,creatorMe,search),task.byIdOrSlug, and unifiedtask.create; kepttask.allandtask.createFromUias temporary aliases;task.updateaccepts-but-ignores deprecatedbranch.createacceptsv2ProjectIdorv2WorkspaceIdand derives the project when a workspace is given; one of the two is required.host.listscoped to the user’s orgs and membership.v2-workspace.listwith org and optional host filter; includes project info./cli/authorizeand/oauth/consentnow rely onproxyto preserve and restore the full path + query via?redirect=....Bug Fixes
createnow validatesv2ProjectIdandv2WorkspaceIdconsistency (must match when both provided) and verifiesv2ProjectIdbelongs to the org when no workspace is given.redirect_urivalidation by parsing withURLand requiring http loopback with no userinfo.jwtProcedurenow re-throwsTRPCErrorfromverifyJWTand falls back to session only for unverifiable tokens; clearer bearer token errors.task.listsearch escapes%and_before building theilikepattern.Written for commit 86ad258. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Refactor