feat(permissions): custom roles with scopes, API guards, and post review workflows - #630
feat(permissions): custom roles with scopes, API guards, and post review workflows#630mezotv wants to merge 12 commits into
Conversation
…iew workflows Adds a workspace permission system and an Ordinal-style publishing review flow: - organization_roles (4 seeded system roles + custom roles) with scoped permissions, member_role_assignments, approval_workflows with ordered steps (reviewer role + required approvals), post_approval_requests with step snapshots, and post_reviews - posts gain in_review/approved statuses plus createdBy/publishedAt/ publishedBy - Effect-based scope resolver and assertOrganizationScopes guard; every mutating oRPC procedure (content, skills, brand, automation, api-keys, integrations, social accounts, roles, reviews) is scope-guarded - publish state machine: submit for review, per-step approvals, changes-requested back to draft, posts:publish requires an approved post when a workflow applies, posts:publish_override bypasses - public REST API PATCH /posts enforces approval requirements before allowing status=published - dashboard: roles + publishing settings pages, member role assignment, scope-aware content cards/editor with review controls and progress, reviews inbox at /[slug]/reviews, gated skills/content affordances Claude-Session: https://claude.ai/code/session_01XJnuvXeWGDh63rsRas81if
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Comp AI code review complete — 3 issues found. Commit |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 5 Skipped Deployments
|
|
@greptile review |
|
React Doctor found 5 new issues in 5 files · 5 warnings · score 72 / 100 (Needs work) · 2 fixed · vs 5 warnings
Reviewed by React Doctor for commit |
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryAdds workspace access groups with scoped permissions, multi-step post approval workflows, and scope guards across mutating APIs and the dashboard.
Confidence Score: 5/5This PR appears safe to merge; previously reported review/publish races and stale approval-request issues are addressed on the current HEAD. Concurrent approval locking, atomic cancel with status updates, withdraw permission ordering, and status-conditioned updates remove the earlier blocking failure paths; no remaining blocking failure is evident on HEAD. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant API as posts PATCH / content.update
participant DB as Postgres
Client->>API: status change (draft/published)
API->>API: assert publish/withdraw scopes
API->>DB: BEGIN
API->>DB: "UPDATE posts WHERE status = expected"
alt 0 rows
DB-->>API: conflict 409
else updated
API->>DB: cancel pending post_approval_requests
API->>DB: COMMIT
DB-->>Client: success
end
Reviews (11): Last reviewed commit: "fix(api): guard REST status transitions ..." | Re-trigger Greptile |
Locks the approval request row (SELECT FOR UPDATE) while recording a review so concurrent approvals cannot under-count a step, and re-checks request state under the lock. Withdrawing an in-review or approved post back to draft now requires publish/review permission or being the person who requested the review. Claude-Session: https://claude.ai/code/session_01XJnuvXeWGDh63rsRas81if
|
@greptile review |
![comp-ai-code-review[bot]](https://avatars.githubusercontent.com/in/2105518?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file.
Prompt for AI agents (all issues)
Check whether each issue below is valid; if so, find the root cause and fix it. Read the referenced code to confirm the problem before changing it, and use sub-agents to handle independent issues in parallel.
<file name="apps/dashboard/src/lib/reviews/workflow.ts">
<issue n="1" at="apps/dashboard/src/lib/reviews/workflow.ts:243-399" severity="HIGH">Concurrent approvals can permanently stall an approval workflow step (lost-update under READ COMMITTED) — In `reviewPost`, the pending approval request is read OUTSIDE the transaction (lines ~243-250) and the approve path's transaction (lines 353-401) does not lock the request row (`SELECT ... FOR UPDATE`) and does not re-verify `status = 'pending'` before advancing/finalizing. The approval count is computed inside the transaction with a plain `findMany` (lines 366-373) under the default READ COMMITTED isolation. For a step with `requiredApprovals >= 2`, two reviewers A and B can approve concurrently. Each transaction inserts its own review (distinct `reviewerId`, so the unique constraint `(requestId, stepOrder, reviewerId)` does not conflict) and then counts approvals. Under READ COMMITTED, each transaction's count query sees only committed rows plus its own insert — so each sees a count of 1, computes `1 < requiredApprovals`, and returns `approved_waiting` without advancing `currentStepOrder`. Once both commit, the required number of approvals exist for the step, but `currentStepOrder` was never advanced and the request stays `pending` with the step stuck. There is no reconciliation/catch-up logic anywhere (the `state` and `inbox` endpoints only read; nothing recomputes and advances a satisfied step), so the post is marooned in `in_review` indefinitely and can never be approved through the normal flow. An attacker who is a legitimate reviewer can reliably trigger this by issuing concurrent approve requests, denying the organization the ability to publish that content via the workflow. This is a denial-of-availability / broken-workflow bug. Fix: Lock the request row at the start of the transaction and re-check its state inside the transaction. For example, inside `db.transaction` (ideally at SERIALIZABLE isolation, or with an explicit `SELECT ... FOR UPDATE` on the `post_approval_requests` row): re-fetch the request, verify `status === 'pending'` and that `currentStepOrder` still matches the value read earlier; if not, abort/retry. Compute the approval count after the lock so all concurrent approvers serialize on the same row, ensuring exactly one of them observes the threshold met and performs the advance/finalize. Alternatively, perform the count + advance as a single conditional UPDATE guarded by `status = 'pending' AND current_step_order = :expected`.</issue>
</file>
2 other commit reviews still in progress for this PR — findings may follow.
Commit 4d966d1 · Posted by Comp AI Code Reviews.
| db.query.postApprovalRequests.findFirst({ | ||
| where: and( | ||
| eq(postApprovalRequests.postId, postId), | ||
| eq(postApprovalRequests.organizationId, organizationId), | ||
| eq(postApprovalRequests.status, "pending") | ||
| ), | ||
| }) | ||
| ); | ||
|
|
||
| if (!request) { | ||
| return yield* Effect.fail( | ||
| new ReviewStateError({ | ||
| message: "This post has no pending review request", | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| if (request.requestedBy === reviewerUserId) { | ||
| return yield* Effect.fail( | ||
| new ReviewPermissionError({ | ||
| message: "You cannot review a post you submitted for review", | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| const currentStep = findCurrentStep(request.steps, request.currentStepOrder); | ||
| if (!currentStep) { | ||
| return yield* Effect.fail( | ||
| new ReviewStateError({ | ||
| message: "The review request references an unknown step", | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| const reviewerRoleIds = yield* resolveMemberRoleIds({ | ||
| memberId: reviewerMemberId, | ||
| }).pipe( | ||
| Effect.mapError( | ||
| (cause) => | ||
| new ReviewPersistenceError({ | ||
| message: "Failed to resolve reviewer roles", | ||
| cause, | ||
| }) | ||
| ) | ||
| ); | ||
|
|
||
| const isWorkspaceAdmin = | ||
| reviewerMemberRole === "owner" || reviewerMemberRole === "admin"; | ||
| if ( | ||
| !(isWorkspaceAdmin || reviewerRoleIds.includes(currentStep.reviewerRoleId)) | ||
| ) { | ||
| return yield* Effect.fail( | ||
| new ReviewPermissionError({ | ||
| message: `This step requires a review from the ${currentStep.reviewerRoleName} role`, | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| const existingReview = yield* tryDb(() => | ||
| db.query.postReviews.findFirst({ | ||
| where: and( | ||
| eq(postReviews.requestId, request.id), | ||
| eq(postReviews.stepOrder, request.currentStepOrder), | ||
| eq(postReviews.reviewerId, reviewerUserId) | ||
| ), | ||
| columns: { | ||
| id: true, | ||
| }, | ||
| }) | ||
| ); | ||
|
|
||
| if (existingReview) { | ||
| return yield* Effect.fail( | ||
| new ReviewStateError({ | ||
| message: "You already reviewed this step", | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| const reviewId = nanoid(); | ||
|
|
||
| if (decision === "changes_requested") { | ||
| yield* tryDb(() => | ||
| db.transaction(async (tx) => { | ||
| await tx.insert(postReviews).values({ | ||
| id: reviewId, | ||
| requestId: request.id, | ||
| postId, | ||
| stepOrder: request.currentStepOrder, | ||
| reviewerId: reviewerUserId, | ||
| decision, | ||
| comment: comment ?? null, | ||
| }); | ||
|
|
||
| await tx | ||
| .update(postApprovalRequests) | ||
| .set({ status: "rejected", resolvedAt: new Date() }) | ||
| .where(eq(postApprovalRequests.id, request.id)); | ||
|
|
||
| await tx | ||
| .update(posts) | ||
| .set({ status: "draft", updatedAt: new Date() }) | ||
| .where(eq(posts.id, postId)); | ||
| }) | ||
| ); | ||
|
|
||
| return { outcome: "changes_requested" as const }; | ||
| } | ||
|
|
||
| const nextStep = findNextStep(request.steps, request.currentStepOrder); | ||
|
|
||
| const result = yield* tryDb(() => | ||
| db.transaction(async (tx) => { | ||
| await tx.insert(postReviews).values({ | ||
| id: reviewId, | ||
| requestId: request.id, | ||
| postId, | ||
| stepOrder: request.currentStepOrder, | ||
| reviewerId: reviewerUserId, | ||
| decision, | ||
| comment: comment ?? null, | ||
| }); | ||
|
|
||
| const approvalsForStep = await tx.query.postReviews.findMany({ | ||
| where: and( | ||
| eq(postReviews.requestId, request.id), | ||
| eq(postReviews.stepOrder, request.currentStepOrder), | ||
| eq(postReviews.decision, "approved") | ||
| ), | ||
| columns: { | ||
| id: true, | ||
| }, | ||
| }); | ||
|
|
||
| if (approvalsForStep.length < currentStep.requiredApprovals) { | ||
| return { outcome: "approved_waiting" as const }; | ||
| } | ||
|
|
||
| if (nextStep) { | ||
| await tx | ||
| .update(postApprovalRequests) | ||
| .set({ currentStepOrder: nextStep.stepOrder }) | ||
| .where(eq(postApprovalRequests.id, request.id)); | ||
|
|
||
| return { | ||
| outcome: "advanced" as const, | ||
| nextStepOrder: nextStep.stepOrder, | ||
| }; | ||
| } | ||
|
|
||
| await tx | ||
| .update(postApprovalRequests) | ||
| .set({ status: "approved", resolvedAt: new Date() }) | ||
| .where(eq(postApprovalRequests.id, request.id)); | ||
|
|
||
| await tx | ||
| .update(posts) |
There was a problem hiding this comment.
HIGH: Concurrent approvals can permanently stall an approval workflow step (lost-update under READ COMMITTED)
In reviewPost, the pending approval request is read OUTSIDE the transaction (lines ~243-250) and the approve path's transaction (lines 353-401) does not lock the request row (SELECT ... FOR UPDATE) and does not re-verify status = 'pending' before advancing/finalizing. The approval count is computed inside the transaction with a plain findMany (lines 366-373) under the default READ COMMITTED isolation.
For a step with requiredApprovals >= 2, two reviewers A and B can approve concurrently. Each transaction inserts its own review (distinct reviewerId, so the unique constraint (requestId, stepOrder, reviewerId) does not conflict) and then counts approvals. Under READ COMMITTED, each transaction's count query sees only committed rows plus its own insert — so each sees a count of 1, computes 1 < requiredApprovals, and returns approved_waiting without advancing currentStepOrder. Once both commit, the required number of approvals exist for the step, but currentStepOrder was never advanced and the request stays pending with the step stuck. There is no reconciliation/catch-up logic anywhere (the state and inbox endpoints only read; nothing recomputes and advances a satisfied step), so the post is marooned in in_review indefinitely and can never be approved through the normal flow. An attacker who is a legitimate reviewer can reliably trigger this by issuing concurrent approve requests, denying the organization the ability to publish that content via the workflow. This is a denial-of-availability / broken-workflow bug.
Suggestion: Lock the request row at the start of the transaction and re-check its state inside the transaction. For example, inside db.transaction (ideally at SERIALIZABLE isolation, or with an explicit SELECT ... FOR UPDATE on the post_approval_requests row): re-fetch the request, verify status === 'pending' and that currentStepOrder still matches the value read earlier; if not, abort/retry. Compute the approval count after the lock so all concurrent approvers serialize on the same row, ensuring exactly one of them observes the threshold met and performs the advance/finalize. Alternatively, perform the count + advance as a single conditional UPDATE guarded by status = 'pending' AND current_step_order = :expected.
Prompt for AI agents
Check whether this issue is valid; if so, find the root cause and fix it. Read the referenced code to confirm the problem before changing it.
<issue at="apps/dashboard/src/lib/reviews/workflow.ts:243-399" severity="HIGH">Concurrent approvals can permanently stall an approval workflow step (lost-update under READ COMMITTED) — In `reviewPost`, the pending approval request is read OUTSIDE the transaction (lines ~243-250) and the approve path's transaction (lines 353-401) does not lock the request row (`SELECT ... FOR UPDATE`) and does not re-verify `status = 'pending'` before advancing/finalizing. The approval count is computed inside the transaction with a plain `findMany` (lines 366-373) under the default READ COMMITTED isolation. For a step with `requiredApprovals >= 2`, two reviewers A and B can approve concurrently. Each transaction inserts its own review (distinct `reviewerId`, so the unique constraint `(requestId, stepOrder, reviewerId)` does not conflict) and then counts approvals. Under READ COMMITTED, each transaction's count query sees only committed rows plus its own insert — so each sees a count of 1, computes `1 < requiredApprovals`, and returns `approved_waiting` without advancing `currentStepOrder`. Once both commit, the required number of approvals exist for the step, but `currentStepOrder` was never advanced and the request stays `pending` with the step stuck. There is no reconciliation/catch-up logic anywhere (the `state` and `inbox` endpoints only read; nothing recomputes and advances a satisfied step), so the post is marooned in `in_review` indefinitely and can never be approved through the normal flow. An attacker who is a legitimate reviewer can reliably trigger this by issuing concurrent approve requests, denying the organization the ability to publish that content via the workflow. This is a denial-of-availability / broken-workflow bug. Fix: Lock the request row at the start of the transaction and re-check its state inside the transaction. For example, inside `db.transaction` (ideally at SERIALIZABLE isolation, or with an explicit `SELECT ... FOR UPDATE` on the `post_approval_requests` row): re-fetch the request, verify `status === 'pending'` and that `currentStepOrder` still matches the value read earlier; if not, abort/retry. Compute the approval count after the lock so all concurrent approvers serialize on the same row, ensuring exactly one of them observes the threshold met and performs the advance/finalize. Alternatively, perform the count + advance as a single conditional UPDATE guarded by `status = 'pending' AND current_step_order = :expected`.</issue>
Commit 4d966d1.
There was a problem hiding this comment.
Fixed in d877874 (this review ran against 4d966d1): the approve path now takes SELECT ... FOR UPDATE on the approval request row at the start of the transaction and re-verifies status = 'pending' and the unchanged currentStepOrder under the lock before inserting and counting. Concurrent approvals serialize per request, so the second transaction observes the first insert and advances the step.
There was a problem hiding this comment.
No blocking issues found across the changed files.
1 other commit review still in progress for this PR — findings may follow.
Commit d877874 · Posted by Comp AI Code Reviews.
|
@greptile review |
There was a problem hiding this comment.
1 issue found across 1 file.
Prompt for AI agents (all issues)
Check whether each issue below is valid; if so, find the root cause and fix it. Read the referenced code to confirm the problem before changing it, and use sub-agents to handle independent issues in parallel.
<file name="apps/dashboard/src/lib/orpc/routers/content.ts">
<issue n="1" at="apps/dashboard/src/lib/orpc/routers/content.ts:1187-1585" severity="MEDIUM">generate and preview endpoints lack endpoint-level rate limiting and per-org concurrency caps — ... Fix: ...</issue>
</file>
Commit 75df4d7 · Posted by Comp AI Code Reviews.
|
Re: Comp AI's last finding (generate/preview rate limiting) — these endpoints and their behavior predate this PR; this change only added scope guards to them. Endpoint-level rate limiting for content generation/preview is worth its own issue rather than expanding this PR. |
Renames the permission-bundle feature from roles to access groups end to end: access_groups and access_group_members tables, workflow columns applies_to_access_group_id and reviewer_access_group_id, the oRPC router (accessGroups), hooks, components, the settings route, and all user-facing copy. The better-auth workspace role (Owner/Admin/Member) keeps its name, resolving the confusing Role vs Roles columns on the members page; owners now show Full access. Migration 0059 was regenerated with the new names per request; databases that applied the previous 0059 need the revert applied (done for the dev database). Claude-Session: https://claude.ai/code/session_01XJnuvXeWGDh63rsRas81if
|
@greptile review |
There was a problem hiding this comment.
3 issues found across 3 files.
Prompt for AI agents (all issues)
Check whether each issue below is valid; if so, find the root cause and fix it. Read the referenced code to confirm the problem before changing it, and use sub-agents to handle independent issues in parallel.
<file name="apps/dashboard/src/app/api/organizations/[organizationId]/chat/posts/route.ts">
<issue n="1" at="apps/dashboard/src/app/api/organizations/[organizationId]/chat/posts/route.ts:21-164" severity="MEDIUM">Chat post creation triggers unmetered, unrate-limited LLM call (collection title generation) — The POST handler is authenticated and enforces the `posts:create` scope, but applies NO rate limiting before inserting the post and scheduling an `after()` callback. That callback invokes `maybeGenerateCollectionTitle({ collectionId, organizationId })` (imported from `@notra/ai/jobs/collection-title`), which calls `generateCollectionTitle` -> `generateObject({ model: gateway(COLLECTION_TITLE_MODEL_ID), ... })` — a real LLM API call. Crucially, unlike the sibling standalone-chat route (`/api/organizations/[organizationId]/chat`), this route neither calls `enforceChatGenerationRatelimit(...)` nor tracks AI credits via Autumn (`autumn.track`). The collection-title LLM job has no Autumn metering either (grep for `autumn`/`track`/`credit` in `packages/ai/src/jobs/collection-title.ts` returns nothing). The codebase convention is to rate-limit and meter expensive AI operations (see `utils/chat-ratelimit.ts`, `apps/api/src/utils/ratelimit.ts`), and this route is the exception. Trigger conditions are easy to satisfy: every new chat-sourced collection is created with `expectedPostCount: null` (so `isComplete` is always true) and a name built by `buildPostCollectionName([contentType], now)` (e.g. 'Blog post - July 30th 2026'), which matches `isLegacyPostCollectionName` -> the LLM fires. An authenticated org member (or any script with a valid session + `posts:create`) can hammer this endpoint with distinct `chatId`/`contentType` values, each spinning up a new collection and a fresh LLM call, driving unbounded, unmetered LLM spend and compute. Because the call runs in Next.js `after()`, failures are swallowed and do not break the request, so the abuse is silent and reliable. Evidence: route.ts L21 (entry, no ratelimit call), L100-130 (collection upsert with expectedPostCount null), L160-164 (`after(() => maybeGenerateCollectionTitle(...))`); contrast with chat/route.ts L73-78 (`enforceChatGenerationRatelimit`) and L97-126 (Autumn check/track). Fix: Apply the same protections used by the standalone-chat route before the DB insert: (1) call `enforceChatGenerationRatelimit(organizationId, auth.context.user.id)` and return its response when limited; (2) gate the LLM `after()` job behind an Autumn `autumn.check({ customerId: organizationId, featureId: FEATURES.AI_CREDITS })` allowance and `autumn.track(...)` the resulting cost, mirroring `generateChatFinishMetadata`/onUsage in chat/route.ts. Additionally consider deduping so the title-generation LLM only runs once per collection (e.g. set a `titleGenerationStatus` flag) rather than being re-triggerable by repeated post creations.</issue>
</file>
<file name="apps/dashboard/src/lib/permissions/resolve-scopes.ts">
<issue n="2" at="apps/dashboard/src/lib/permissions/resolve-scopes.ts:46-47" severity="MEDIUM">Legacy role fallback grants broad sensitive scopes when a member has no access-group assignments — resolveMemberScopes resolves a member's scopes from their access-group memberships, but when memberships.length === 0 it falls back to LEGACY_ROLE_SCOPES[memberRole] (lines 46-47). For the common "member" role this legacy set is broad and includes sensitive scopes such as api-keys:manage, integrations:manage, posts:publish, automation:manage, and brand:edit (see packages/db/src/constants/permissions.ts LEGACY_ROLE_SCOPES.member). This inverts the access-group permission model: the state that appears least-privileged to an admin (a member removed from / never assigned to any access group) is actually highly privileged. Concretely, an admin who unassigns a member from a restrictive group (e.g. the "Contributor" system group, scopes posts:create/edit only) expecting to reduce their access instead restores api-keys:manage, posts:publish, and integrations:manage to that member. System access groups are seeded by ensureSystemAccessGroups but are NOT auto-assigned to members, so any "member"-role user not explicitly assigned to a group hits this fallback. The access-group model is silently undermined for legacy-role users. The owner role short-circuit (line 21) is DB-sourced and not a bypass; the cross-tenant angle (line 29 query lacks an organizationId filter) is mitigated because memberId is the org-scoped membership PK validated upstream in assertOrganizationAccess and assignments verify org ownership. Fix: Make access-group assignment the authoritative source of permissions: when a member has zero access-group memberships, return an empty scope set (deny by default) rather than a broad legacy set. If backward compatibility is required, gate the legacy fallback behind an explicit migration/adopted-access-groups flag per organization, or reduce the legacy "member" scope set to a minimal safe baseline (exclude api-keys:manage, integrations:manage, posts:publish, automation:manage). Add tests covering the "unassigned member" state to prevent regression.</issue>
</file>
<file name="packages/db/migrations/meta/0059_snapshot.json">
<issue n="3" at="packages/db/migrations/meta/0059_snapshot.json" severity="MEDIUM">JWT signing private key (jwks.private_key) and other long-lived secrets stored in plaintext columns — The schema snapshot reveals an inconsistent secret-storage strategy. Integration tables store secrets in explicitly app-level-encrypted columns (e.g. github_integrations.encrypted_token, github_integrations.encrypted_webhook_secret, granola_integrations.encrypted_api_key, linear_integrations.encrypted_access_token/encrypted_webhook_secret, mcp_oauth_credentials.encrypted_tokens/encrypted_client_information/encrypted_authorization_server_information, mcp_oauth_pending_authorizations.encrypted_state/encrypted_code_verifier, mcp_server_integrations.encrypted_headers), which proves the codebase has an application-layer encryption utility. However, several high-value secrets are stored in plain `text` columns with no encryption: (1) `public.jwks.private_key` (text, notNull) — the JWT signing private key; (2) `public.oauth_clients.client_secret` (text); (3) `public.sessions.token` (text, unique); (4) `public.oauth_access_tokens.token` (text, unique); (5) `public.oauth_refresh_tokens.token` (text, unique); (6) `public.accounts.access_token/refresh_token/id_token` (text). The most impactful is jwks.private_key: it is a long-lived signing key. If the database is read-compromised via any other vector (SQL injection in another code path, a leaked database backup, a compromised DB/admin credential, an SSRF to an internal DB proxy), an attacker obtains the JWT signing private key and can forge valid JWTs for ANY user, achieving a full authentication bypass that persists until key rotation. Session/OAuth tokens stored in plaintext similarly enable mass session hijacking on DB read-compromise, whereas hashing them (and looking up by hash) would limit the blast radius. The contrast with the encrypted_* columns shows this plaintext storage is an oversight rather than an intentional decision that the DB is fully trusted. Fix: Encrypt jwks.private_key at the application layer using the same encryption utility used for the encrypted_* integration columns (store as encrypted_private_key, decrypt only at signing time), or — preferably — keep JWT signing keys in a dedicated secret manager (e.g. KMS/Vault) rather than the database. For sessions.token, oauth_access_tokens.token, and oauth_refresh_tokens.token, store a hash (e.g. SHA-256) of the token rather than the plaintext value and look up sessions by hash, so a DB read-compromise does not immediately yield usable live sessions. Ensure oauth_clients.client_secret is hashed (client-secret comparison can use the stored hash). Apply consistent encryption to accounts.access_token/refresh_token/id_token as is already done for the integration tables.</issue>
</file>
Commit 53e2917 · Posted by Comp AI Code Reviews.
| collection.createdAt | ||
| ), | ||
| updatedAt: now, | ||
| }) |
There was a problem hiding this comment.
MEDIUM: Chat post creation triggers unmetered, unrate-limited LLM call (collection title generation)
The POST handler is authenticated and enforces the posts:create scope, but applies NO rate limiting before inserting the post and scheduling an after() callback. That callback invokes maybeGenerateCollectionTitle({ collectionId, organizationId }) (imported from @notra/ai/jobs/collection-title), which calls generateCollectionTitle -> generateObject({ model: gateway(COLLECTION_TITLE_MODEL_ID), ... }) — a real LLM API call.
Crucially, unlike the sibling standalone-chat route (/api/organizations/[organizationId]/chat), this route neither calls enforceChatGenerationRatelimit(...) nor tracks AI credits via Autumn (autumn.track). The collection-title LLM job has no Autumn metering either (grep for autumn/track/credit in packages/ai/src/jobs/collection-title.ts returns nothing). The codebase convention is to rate-limit and meter expensive AI operations (see utils/chat-ratelimit.ts, apps/api/src/utils/ratelimit.ts), and this route is the exception.
Trigger conditions are easy to satisfy: every new chat-sourced collection is created with expectedPostCount: null (so isComplete is always true) and a name built by buildPostCollectionName([contentType], now) (e.g. 'Blog post - July 30th 2026'), which matches isLegacyPostCollectionName -> the LLM fires. An authenticated org member (or any script with a valid session + posts:create) can hammer this endpoint with distinct chatId/contentType values, each spinning up a new collection and a fresh LLM call, driving unbounded, unmetered LLM spend and compute. Because the call runs in Next.js after(), failures are swallowed and do not break the request, so the abuse is silent and reliable. Evidence: route.ts L21 (entry, no ratelimit call), L100-130 (collection upsert with expectedPostCount null), L160-164 (after(() => maybeGenerateCollectionTitle(...))); contrast with chat/route.ts L73-78 (enforceChatGenerationRatelimit) and L97-126 (Autumn check/track).
Suggestion: Apply the same protections used by the standalone-chat route before the DB insert: (1) call enforceChatGenerationRatelimit(organizationId, auth.context.user.id) and return its response when limited; (2) gate the LLM after() job behind an Autumn autumn.check({ customerId: organizationId, featureId: FEATURES.AI_CREDITS }) allowance and autumn.track(...) the resulting cost, mirroring generateChatFinishMetadata/onUsage in chat/route.ts. Additionally consider deduping so the title-generation LLM only runs once per collection (e.g. set a titleGenerationStatus flag) rather than being re-triggerable by repeated post creations.
Prompt for AI agents
Check whether this issue is valid; if so, find the root cause and fix it. Read the referenced code to confirm the problem before changing it.
<issue at="apps/dashboard/src/app/api/organizations/[organizationId]/chat/posts/route.ts:21-164" severity="MEDIUM">Chat post creation triggers unmetered, unrate-limited LLM call (collection title generation) — The POST handler is authenticated and enforces the `posts:create` scope, but applies NO rate limiting before inserting the post and scheduling an `after()` callback. That callback invokes `maybeGenerateCollectionTitle({ collectionId, organizationId })` (imported from `@notra/ai/jobs/collection-title`), which calls `generateCollectionTitle` -> `generateObject({ model: gateway(COLLECTION_TITLE_MODEL_ID), ... })` — a real LLM API call. Crucially, unlike the sibling standalone-chat route (`/api/organizations/[organizationId]/chat`), this route neither calls `enforceChatGenerationRatelimit(...)` nor tracks AI credits via Autumn (`autumn.track`). The collection-title LLM job has no Autumn metering either (grep for `autumn`/`track`/`credit` in `packages/ai/src/jobs/collection-title.ts` returns nothing). The codebase convention is to rate-limit and meter expensive AI operations (see `utils/chat-ratelimit.ts`, `apps/api/src/utils/ratelimit.ts`), and this route is the exception. Trigger conditions are easy to satisfy: every new chat-sourced collection is created with `expectedPostCount: null` (so `isComplete` is always true) and a name built by `buildPostCollectionName([contentType], now)` (e.g. 'Blog post - July 30th 2026'), which matches `isLegacyPostCollectionName` -> the LLM fires. An authenticated org member (or any script with a valid session + `posts:create`) can hammer this endpoint with distinct `chatId`/`contentType` values, each spinning up a new collection and a fresh LLM call, driving unbounded, unmetered LLM spend and compute. Because the call runs in Next.js `after()`, failures are swallowed and do not break the request, so the abuse is silent and reliable. Evidence: route.ts L21 (entry, no ratelimit call), L100-130 (collection upsert with expectedPostCount null), L160-164 (`after(() => maybeGenerateCollectionTitle(...))`); contrast with chat/route.ts L73-78 (`enforceChatGenerationRatelimit`) and L97-126 (Autumn check/track). Fix: Apply the same protections used by the standalone-chat route before the DB insert: (1) call `enforceChatGenerationRatelimit(organizationId, auth.context.user.id)` and return its response when limited; (2) gate the LLM `after()` job behind an Autumn `autumn.check({ customerId: organizationId, featureId: FEATURES.AI_CREDITS })` allowance and `autumn.track(...)` the resulting cost, mirroring `generateChatFinishMetadata`/onUsage in chat/route.ts. Additionally consider deduping so the title-generation LLM only runs once per collection (e.g. set a `titleGenerationStatus` flag) rather than being re-triggerable by repeated post creations.</issue>
Commit 53e2917.
There was a problem hiding this comment.
Pre-existing: the after() collection-title job and its lack of metering predate this PR, which only added the scope guard and author/publish stamping to this route. Agreed it deserves rate limiting and Autumn tracking, but that applies to the shared collection-title job across all chat-collection paths, so tracking it as its own issue.
| if (memberships.length === 0) { | ||
| return LEGACY_ROLE_SCOPES[memberRole] ?? []; |
There was a problem hiding this comment.
MEDIUM: Legacy role fallback grants broad sensitive scopes when a member has no access-group assignments
resolveMemberScopes resolves a member's scopes from their access-group memberships, but when memberships.length === 0 it falls back to LEGACY_ROLE_SCOPES[memberRole] (lines 46-47). For the common "member" role this legacy set is broad and includes sensitive scopes such as api-keys:manage, integrations:manage, posts:publish, automation:manage, and brand:edit (see packages/db/src/constants/permissions.ts LEGACY_ROLE_SCOPES.member). This inverts the access-group permission model: the state that appears least-privileged to an admin (a member removed from / never assigned to any access group) is actually highly privileged. Concretely, an admin who unassigns a member from a restrictive group (e.g. the "Contributor" system group, scopes posts:create/edit only) expecting to reduce their access instead restores api-keys:manage, posts:publish, and integrations:manage to that member. System access groups are seeded by ensureSystemAccessGroups but are NOT auto-assigned to members, so any "member"-role user not explicitly assigned to a group hits this fallback. The access-group model is silently undermined for legacy-role users. The owner role short-circuit (line 21) is DB-sourced and not a bypass; the cross-tenant angle (line 29 query lacks an organizationId filter) is mitigated because memberId is the org-scoped membership PK validated upstream in assertOrganizationAccess and assignments verify org ownership.
Suggestion: Make access-group assignment the authoritative source of permissions: when a member has zero access-group memberships, return an empty scope set (deny by default) rather than a broad legacy set. If backward compatibility is required, gate the legacy fallback behind an explicit migration/adopted-access-groups flag per organization, or reduce the legacy "member" scope set to a minimal safe baseline (exclude api-keys:manage, integrations:manage, posts:publish, automation:manage). Add tests covering the "unassigned member" state to prevent regression.
Prompt for AI agents
Check whether this issue is valid; if so, find the root cause and fix it. Read the referenced code to confirm the problem before changing it.
<issue at="apps/dashboard/src/lib/permissions/resolve-scopes.ts:46-47" severity="MEDIUM">Legacy role fallback grants broad sensitive scopes when a member has no access-group assignments — resolveMemberScopes resolves a member's scopes from their access-group memberships, but when memberships.length === 0 it falls back to LEGACY_ROLE_SCOPES[memberRole] (lines 46-47). For the common "member" role this legacy set is broad and includes sensitive scopes such as api-keys:manage, integrations:manage, posts:publish, automation:manage, and brand:edit (see packages/db/src/constants/permissions.ts LEGACY_ROLE_SCOPES.member). This inverts the access-group permission model: the state that appears least-privileged to an admin (a member removed from / never assigned to any access group) is actually highly privileged. Concretely, an admin who unassigns a member from a restrictive group (e.g. the "Contributor" system group, scopes posts:create/edit only) expecting to reduce their access instead restores api-keys:manage, posts:publish, and integrations:manage to that member. System access groups are seeded by ensureSystemAccessGroups but are NOT auto-assigned to members, so any "member"-role user not explicitly assigned to a group hits this fallback. The access-group model is silently undermined for legacy-role users. The owner role short-circuit (line 21) is DB-sourced and not a bypass; the cross-tenant angle (line 29 query lacks an organizationId filter) is mitigated because memberId is the org-scoped membership PK validated upstream in assertOrganizationAccess and assignments verify org ownership. Fix: Make access-group assignment the authoritative source of permissions: when a member has zero access-group memberships, return an empty scope set (deny by default) rather than a broad legacy set. If backward compatibility is required, gate the legacy fallback behind an explicit migration/adopted-access-groups flag per organization, or reduce the legacy "member" scope set to a minimal safe baseline (exclude api-keys:manage, integrations:manage, posts:publish, automation:manage). Add tests covering the "unassigned member" state to prevent regression.</issue>
Commit 53e2917.
There was a problem hiding this comment.
Good catch, fixed in 596bb52: the legacy fallback now applies only while a workspace has zero access-group assignments (pre-adoption compatibility). Once any group is assigned, members without one resolve to no scopes, so unassigning someone's last group strictly reduces their permissions. Workspace owners and admins always resolve to full access so administration cannot be locked out.
…spaces Once a workspace has assigned any access group, members without a group resolve to no scopes instead of the broad legacy member set, so removing someone's last group no longer increases their permissions. Workspace owners and admins always resolve to full access. Claude-Session: https://claude.ai/code/session_01XJnuvXeWGDh63rsRas81if
|
@greptile review |
|
@greptile review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| reviews: { stepOrder: number; decision: string }[], | ||
| requestStatus: string | ||
| ) { | ||
| const ordered = [...steps].sort((a, b) => a.stepOrder - b.stepOrder); |
There was a problem hiding this comment.
React Doctor · react-doctor/js-tosorted-immutable (warning)
This wastes work because [...array].sort() copies the array just to sort it, so use array.toSorted() to sort without the extra copy (ES2023)
Fix → Use array.toSorted() (ES2023) instead of [...array].sort() so you sort without copying the array first
| steps: ApprovalWorkflowStepSnapshot[], | ||
| currentStepOrder: number | ||
| ) => | ||
| [...steps] |
There was a problem hiding this comment.
React Doctor · react-doctor/js-tosorted-immutable (warning)
This wastes work because [...array].sort() copies the array just to sort it, so use array.toSorted() to sort without the extra copy (ES2023)
Fix → Use array.toSorted() (ES2023) instead of [...array].sort() so you sort without copying the array first
There was a problem hiding this comment.
No blocking issues found across the changed files.
1 other commit review still in progress for this PR — findings may follow.
Commit 596bb52 · Posted by Comp AI Code Reviews.
There was a problem hiding this comment.
3 issues found across 1 file.
Prompt for AI agents (all issues)
Check whether each issue below is valid; if so, find the root cause and fix it. Read the referenced code to confirm the problem before changing it, and use sub-agents to handle independent issues in parallel.
<file name="apps/api/src/routes/posts.ts">
<issue n="1" at="apps/api/src/routes/posts.ts:828-895" severity="MEDIUM">Internal service error details leaked to API clients via generation job status endpoint — When `POST /posts/generate` triggers `triggerContentGenerationWorkflow`, the internal workflow service call (`startDashboardWorkflow` in utils/internal-workflow.ts) can throw `new Error("Workflow start failed with status ${response.status}: ${detail}")` where `detail` is the RAW response body from the internal `WORKFLOW_BASE_URL` service (which may contain stack traces, internal paths, or service internals). This error is caught in the createPostGenerationRoute handler (lines 827-829) and stored verbatim into Redis: the job's `error` field (via `setContentGenerationJobStatus(..., { error: message })` at lines 844-845) and the event's `message` field (line 853). Although the immediate 503 HTTP response is generic ("Failed to queue content generation", line 862), the `GET /posts/generate/{jobId}` endpoint (getPostGenerationRoute, lines 870-896) returns the full `job` object and `events` array to the client — and `getPostGenerationResponseSchema` (schemas/content.ts:831) exposes the `contentGenerationJobSchema.error` field and `contentGenerationJobEventSchema.message` field. So any authenticated user who triggers a failing generation job can subsequently poll the job-status endpoint and read the internal service's raw error response body and HTTP status, leaking internal infrastructure details. The ownership check on the job (`job.organizationId !== orgId`) is correct, so this is scoped to a user's own jobs, but the leaked internal details are still sensitive. Fix: Do not persist raw internal error messages in client-readable job/event fields. Map internal errors to generic user-facing messages (e.g., 'Content generation failed') before storing in the Redis job `error` and event `message` fields, or strip/redact the internal `detail` from `startDashboardWorkflow`'s thrown error before persisting it. Log the full internal error server-side (via logError) for debugging, but return only a generic, non-revealing message to clients in the getPostGenerationRoute response.</issue>
<issue n="2" at="apps/api/src/routes/posts.ts:409-432" severity="MEDIUM">getPostRoute returns 200 with null post instead of 404 when post is not found — The getPostRoute handler (lines 392-434) performs a scoped `findFirst` and, when no post matches, returns `c.json({ post: null, organization }, 200)` with HTTP 200. The route's declared 404 response ('Post or organization not found') is never produced for a missing post — only for a missing organization. This is an API contract inconsistency: clients cannot distinguish 'post exists' from 'post does not exist' by status code, and any client expecting 404 for a missing resource will misbehave. Not a security vulnerability, but a notable logic/contract bug. Fix: Return a 404 response (e.g., `{ error: 'Post not found' }`) when the scoped lookup returns no post, consistent with the other post routes (deletePostRoute and patchPostRoute both return 404 for missing posts).</issue>
<issue n="3" at="apps/api/src/routes/posts.ts:737-841" severity="MEDIUM">Orphaned postCollections DB row if Redis job creation or workflow trigger fails after insert — In createPostGenerationRoute, a `postCollections` row is inserted into the database (lines 737-750) BEFORE the Redis content-generation job is created (line 756) and before the workflow is triggered (line 795). The catch block (lines 827-867) only deletes the collection row when `collectionCreated && !workflowTriggered`. However, there is no transaction wrapping the DB insert with the Redis job creation: if the process crashes or the Redis `createContentGenerationJob` call throws in a way that escapes the try block, or if `addActiveGeneration` fails after the job is created, the collection row is left orphaned (with expectedPostCount=1, completedPostCount=0, never completed). The cleanup also relies on the catch block running synchronously within the same request; a hard crash leaves the row permanently dangling. This is a data-integrity/resilience bug, not a direct security issue. Fix: Make the collection-row lifecycle resilient: either create the collection row only after the Redis job and workflow trigger succeed, or run a periodic cleanup job that removes orphaned collections (source='api', completedPostCount=0, older than a threshold). Consider wrapping the insert in logic that can roll back on partial failure.</issue>
</file>
Commit e5c8d4f · Posted by Comp AI Code Reviews.
1 participant
What
Workspace roles & permissions plus a multi-step post review/publishing workflow, inspired by Ordinal's approval flow (blocking approvals, review vs. approval separation, approvals inbox).
Roles & scopes
organization_rolestable with ascopesjsonb array. Four system roles are seeded per workspace (Admin, Content Manager, Reviewer, Contributor) and users can create custom roles ("Product Manager can review and publish posts") from Settings → Roles.member_role_assignmentsjoins members to roles; members can hold multiple roles (scopes union). Owners always have every scope. Members without assigned roles fall back to a non-breaking legacy mapping (owner/admin → all, member → content scopes without admin scopes or publish-override).create/edit/delete/review/publish/publish_override), skills (create/edit/delete), workspace (brand:edit,automation:manage,integrations:manage,api-keys:manage) and administration (members:manage,roles:manage,publishing:manage).API guarding
resolveMemberScopes) +assertOrganizationScopesguard. Every mutating oRPC procedure is now scope-checked: content, skills, brand, automation, api-keys, integrations, social accounts, and the new roles/reviews routers. Reads remain member-level.PATCH /posts/:idwithstatus=publishedis rejected (403) while an approval workflow applies and the post is not approved.Data.TaggedErrorclasses mapped to HTTP errors at the boundary (Effect.fn+runPromise, matching existing repo patterns).Review workflows (publishing requirements)
in_reviewandapprovedstatuses pluscreatedBy/publishedAt/publishedBy. Submitting snapshots the workflow steps onto the request so mid-flight edits don't corrupt open reviews; approval counting happens inside a transaction (race-safe), a partial unique index prevents duplicate pending requests, self-review is blocked, and one review per reviewer per step is enforced.posts:publishpublishes only approved posts (or when no workflow applies);posts:publish_overridepublishes immediately regardless and cancels the pending request; changes-requested sends the post back to draft with the reviewer's comment.Dashboard
/[slug]/reviews(nav item appears for reviewers): everything waiting on your review with step progress and quick approve/request-changes.Migration
One clean migration
0059_normal_banshee.sql(new tables + enum values + post columns). Enum additions are top-level statements, not used within the same migration.Notes for reviewers
autoPublishand generation pipelines are intentionally untouched by scopes (server-side, configured byautomation:manageholders); they now stamppublishedAt.@notra/onboarding-agentbuild failure on@aws-sdk/client-s3is pre-existing (fails on a clean tree) and unrelated.https://claude.ai/code/session_01XJnuvXeWGDh63rsRas81if
Summary by cubic
Adds workspace access groups with scoped permissions, a multi-step post review/publishing workflow, and scope guards across mutating APIs. The dashboard adds Access groups and Publishing settings, a Reviews inbox, editor review controls, and posts now show draft/in_review/approved/published with badges and command palette labels.
New Features
assertOrganizationScopesprotects mutating oRPC routes (includingaccess-groupsandreviews) and chat post creation; RESTPATCH /posts/:idto published returns 403 if approval is required; editable statuses limited to draft/published.posts:publishrequires approved,posts:publish_overridebypasses. Steps snapshotted; transactional approval counting; self-review blocked./[slug]/reviews; status badges and command palette labels.0059_common_lockjaw.sql(new tables/enums, post columns). Auto-publishing stampspublishedAt.Bug Fixes
posts:publish,posts:publish_override, orposts:reviewholders, or the submitter; reviewers can withdraw without edit scope.Written for commit e5c8d4f. Summary will update on new commits.
Summary by Comp AI
3 issues found.
Written for commit
e5c8d4f. New commits will trigger a re-review. Generated by Comp AI.