refactor: clean up router validation#2836
Merged
Merged
Conversation
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:
📝 WalkthroughWalkthroughAdds org-scoped resource resolution and access helpers, applies them across project/v2-project, workspace/v2-workspace, project secrets, and task routers; converts task endpoints to protected procedures; adds a comprehensive task test suite; removes organization router tests; and lazily loads the DB in session-org resolver. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant TRPC as TRPC Router
participant Auth as OrgAuthUtils
participant DB as Database
Client->>TRPC: call create/update/delete (payload, resourceId)
TRPC->>Auth: requireActiveOrgId / requireActiveOrgMembership
Auth-->>TRPC: organizationId or throws
TRPC->>Auth: requireOrgScopedResource / requireOrgResourceAccess(resolveResource)
Auth->>DB: resolveResource query (by id)
DB-->>Auth: resource (with organizationId) or null
Auth-->>TRPC: validated resource or throws
TRPC->>DB: perform insert/update/delete using validated resource.id
DB-->>TRPC: result
TRPC-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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. Comment |
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 9 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="packages/trpc/src/router/workspace/workspace.ts">
<violation number="1" location="packages/trpc/src/router/workspace/workspace.ts:189">
P2: The delete flow checks admin access only after loading the workspace, which allows ID enumeration via `NOT_FOUND` vs `FORBIDDEN` responses.</violation>
</file>
<file name="packages/trpc/src/router/task/task.test.ts">
<violation number="1" location="packages/trpc/src/router/task/task.test.ts:199">
P2: `beforeEach` reassigns `dbState`, but the module mock keeps using the original `db` instance, so DB-call assertions can become false positives.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/trpc/src/router/task/task.ts (2)
409-412: Same observation: consider using resolvedtaskAccess.idin delete WHERE clause.Similar to the update mutation, the delete WHERE clause uses
inputdirectly rather than a resolved identifier fromgetTaskAccess. Since this is within a transaction wheregetTaskAccesswas just called, the values are the same, but using the resolved ID would be more consistent.🤖 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 409 - 412, The delete mutation uses the raw input value in the tx.update WHERE clause instead of the resolved identifier returned by getTaskAccess; change the WHERE predicate to use the resolved taskAccess.id (from getTaskAccess) and keep the isNull(tasks.deletedAt) check so you update the same authorized record consistently (refer to tx.update, tasks, getTaskAccess, and taskAccess.id).
385-389: Consider usingtaskAccess.idin the WHERE clause for consistency.The WHERE clause uses
eq(tasks.id, id)whereidis the raw input, whilegetTaskAccessreturns the resolvedtaskAccess.id. Although functionally equivalent (same value), usingtaskAccess.idwould be more consistent with the pattern established in other routers (e.g.,project.tsline 138,workspace.tsline 197).🔧 Optional: Use resolved taskAccess.id for consistency
const [task] = await tx .update(tasks) .set(updateData) - .where(and(eq(tasks.id, id), isNull(tasks.deletedAt))) + .where(and(eq(tasks.id, taskAccess.id), isNull(tasks.deletedAt))) .returning();🤖 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 385 - 389, The WHERE clause in the tx.update for tasks uses the raw input id (eq(tasks.id, id)) but should use the resolved taskAccess.id for consistency with other routers; update the condition in the transaction where call (the tx.update(tasks).set(updateData).where(...) expression) to use eq(tasks.id, taskAccess.id) while keeping the isNull(tasks.deletedAt) check intact so the update targets the resolved task record.packages/trpc/src/router/v2-workspace/v2-workspace.ts (1)
71-77: Consider varying the error message based on organization context.The
getWorkspaceAccesshelper in this file always returns "Workspace not found", while the equivalent inworkspace.tsvaries the message ("Workspace not found in this organization" vs "Workspace not found") based on whetherorganizationIdis provided. Consider aligning the behavior for consistency.🔧 Optional: Vary error message based on organizationId presence
async function getWorkspaceAccess( userId: string, workspaceId: string, options?: { access?: "admin" | "member"; organizationId?: string; }, ) { return requireOrgResourceAccess( userId, () => dbWs.query.v2Workspaces.findFirst({ columns: { id: true, organizationId: true, }, where: eq(v2Workspaces.id, workspaceId), }), { access: options?.access, - message: "Workspace not found", + message: options?.organizationId + ? "Workspace not found in this organization" + : "Workspace not found", organizationId: options?.organizationId, }, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/v2-workspace/v2-workspace.ts` around lines 71 - 77, The current getWorkspaceAccess helper always returns the message "Workspace not found"; change it to vary the message based on whether options.organizationId is present (match the pattern used in workspace.ts): use "Workspace not found in this organization" when organizationId is provided, otherwise "Workspace not found". Update the object returned/constructed in getWorkspaceAccess (the block that sets access/message/organizationId) to compute the message accordingly so behavior is consistent with workspace.ts.packages/trpc/src/router/v2-project/v2-project.ts (1)
145-156: Reuse the validated repository ID in the update payload.The repo is already looked up here, but the write still uses
input.githubRepositoryId. Writingrepo.idinstead keeps validation and persistence tied to the same object.♻️ Small cleanup
- if (input.githubRepositoryId) { - await getScopedGithubRepository( - project.organizationId, - input.githubRepositoryId, - ); - } + const repo = input.githubRepositoryId + ? await getScopedGithubRepository( + project.organizationId, + input.githubRepositoryId, + ) + : undefined; const data = { - githubRepositoryId: input.githubRepositoryId, + githubRepositoryId: repo?.id, name: input.name, slug: input.slug, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/v2-project/v2-project.ts` around lines 145 - 156, The lookup result from getScopedGithubRepository is not being used; change the call to capture the returned repository (e.g., const repo = await getScopedGithubRepository(...)) and then set data.githubRepositoryId to that repo.id instead of input.githubRepositoryId so the update payload uses the validated repository ID; update references in the surrounding block (getScopedGithubRepository and the data object) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/trpc/src/router/task/task.ts`:
- Around line 409-412: The delete mutation uses the raw input value in the
tx.update WHERE clause instead of the resolved identifier returned by
getTaskAccess; change the WHERE predicate to use the resolved taskAccess.id
(from getTaskAccess) and keep the isNull(tasks.deletedAt) check so you update
the same authorized record consistently (refer to tx.update, tasks,
getTaskAccess, and taskAccess.id).
- Around line 385-389: The WHERE clause in the tx.update for tasks uses the raw
input id (eq(tasks.id, id)) but should use the resolved taskAccess.id for
consistency with other routers; update the condition in the transaction where
call (the tx.update(tasks).set(updateData).where(...) expression) to use
eq(tasks.id, taskAccess.id) while keeping the isNull(tasks.deletedAt) check
intact so the update targets the resolved task record.
In `@packages/trpc/src/router/v2-project/v2-project.ts`:
- Around line 145-156: The lookup result from getScopedGithubRepository is not
being used; change the call to capture the returned repository (e.g., const repo
= await getScopedGithubRepository(...)) and then set data.githubRepositoryId to
that repo.id instead of input.githubRepositoryId so the update payload uses the
validated repository ID; update references in the surrounding block
(getScopedGithubRepository and the data object) accordingly.
In `@packages/trpc/src/router/v2-workspace/v2-workspace.ts`:
- Around line 71-77: The current getWorkspaceAccess helper always returns the
message "Workspace not found"; change it to vary the message based on whether
options.organizationId is present (match the pattern used in workspace.ts): use
"Workspace not found in this organization" when organizationId is provided,
otherwise "Workspace not found". Update the object returned/constructed in
getWorkspaceAccess (the block that sets access/message/organizationId) to
compute the message accordingly so behavior is consistent with workspace.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd8d7890-7be5-4b9e-bfbc-41a103d21391
📒 Files selected for processing (9)
packages/trpc/src/router/project/project.tspackages/trpc/src/router/project/secrets/secrets.tspackages/trpc/src/router/task/task.test.tspackages/trpc/src/router/task/task.tspackages/trpc/src/router/utils/active-org.tspackages/trpc/src/router/utils/org-resource-access.tspackages/trpc/src/router/v2-project/v2-project.tspackages/trpc/src/router/v2-workspace/v2-workspace.tspackages/trpc/src/router/workspace/workspace.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/auth/src/lib/resolve-session-organization-state.ts (1)
21-29: Consider clearing cached promise on import failure.The lazy loading pattern is appropriate for deferring db module initialization. However, if the dynamic import fails, the rejected promise is cached permanently, causing all subsequent
getDb()calls to fail even if the underlying issue becomes resolvable.♻️ Optional: Clear cache on rejection
async function getDb() { if (!dbPromise) { - dbPromise = import("@superset/db/client").then(({ db }) => db); + dbPromise = import("@superset/db/client").then(({ db }) => db).catch((err) => { + dbPromise = undefined; + throw err; + }); } return dbPromise; }For dynamic imports, failures are typically deterministic (module resolution errors), so this may be acceptable as-is if you prefer the simpler approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth/src/lib/resolve-session-organization-state.ts` around lines 21 - 29, The cached Promise dbPromise may remain permanently rejected if the dynamic import in getDb() fails; update getDb() to clear dbPromise when the import rejects so subsequent calls retry: when creating dbPromise via import("@superset/db/client").then(...), attach a .catch handler that sets dbPromise = undefined before rethrowing the error, so dbPromise and getDb remain lazy-retryable after failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/auth/src/lib/resolve-session-organization-state.ts`:
- Around line 21-29: The cached Promise dbPromise may remain permanently
rejected if the dynamic import in getDb() fails; update getDb() to clear
dbPromise when the import rejects so subsequent calls retry: when creating
dbPromise via import("@superset/db/client").then(...), attach a .catch handler
that sets dbPromise = undefined before rethrowing the error, so dbPromise and
getDb remain lazy-retryable after failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2681c961-859c-4a17-bcbf-1f8c31ee44c3
📒 Files selected for processing (1)
packages/auth/src/lib/resolve-session-organization-state.ts
This reverts commit d76ab15.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (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="packages/auth/src/lib/resolve-session-organization-state.ts">
<violation number="1">
P2: This top-level DB import makes DB client initialization unconditional and bypasses the lazy/DI behavior in this resolver. It can trigger unnecessary startup work (and potential env/runtime failures) even when no DB call is needed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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
Testing
Summary by cubic
Centralized org-scoped validation with shared helpers and enforced them across routers. Task reads are now protected and scoped to the active org; all write paths validate related resources belong to the same org and return clear errors.
Refactors
requireActiveOrgId,requireActiveOrgMembership,requireOrgScopedResource,requireOrgResourceAccess; replaced ad‑hoc checks inproject,v2-project,workspace,v2-workspace,secrets, andtask.all,byOrganization,byId,bySlug) to protected routes using the active org; validatestatusId,assigneeId,githubRepositoryId,projectId, anddeviceIdbefore writes; reject empty update payloads; normalize deletes to org membership + resource scoping.@superset/auth; trimmed unstable desktop workspace tests.Bug Fixes
allby the active org; prevent updates/deletes on already-deleted tasks.Written for commit b2b5efe. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
Chores