Conversation
- Implemented `recordActivity` function to log user activities in an immutable audit log. - Created `requirePermission` function to enforce access control on API routes. - Defined `activityLog` schema for validating activity log queries. - Added schemas for comment creation, updating, and querying. - Established centralized access control permissions for ATS resources, including roles for owner, admin, and member.
|
🚅 Deployed to the reqcore-pr-65 environment in applirank
|
|
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 tenant-scoped collaboration: centralized RBAC and shared permissions, Better Auth organization plugin config, a server-side requirePermission guard, activity logging utility and schema, polymorphic comments with APIs and DB migrations, client permission composable, settings UI/pages, many API routes converted to permission checks, and two large docs (design + testing/security). Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client (Vue)
participant API as Server (Nitro)
participant Auth as BetterAuth
participant DB as PostgreSQL
UI->>API: Request protected endpoint (e.g., GET /api/jobs/:id)
API->>Auth: auth.api.getSession(event)
Auth-->>API: session (user + activeOrganizationId)
API->>Auth: auth.api.hasPermission(session, permissions)
Auth-->>API: allowed / denied
alt allowed
API->>DB: query resource scoped to organization
DB-->>API: resource
API-->>UI: 200 + resource
else denied
API-->>UI: 403 Forbidden
end
sequenceDiagram
participant User as User (UI)
participant Client as Client (Vue)
participant API as Server (Nitro)
participant Auth as BetterAuth
participant DB as PostgreSQL
User->>Client: Submit new comment
Client->>API: POST /api/comments
API->>Auth: getSession + hasPermission(comment:create)
Auth-->>API: authorized session
API->>DB: verify target exists in org
alt target exists
API->>DB: insert comment
DB-->>API: created comment
API->>DB: insert activity_log (fire-and-forget)
API-->>Client: 201 + comment
else not found
API-->>Client: 404 Not Found
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api/applications/index.post.ts (1)
53-59:⚠️ Potential issue | 🔴 CriticalAdd guard to check
createdbefore accessingcreated.idon line 75.The destructuring pattern
const [created]can result inundefinedif the insert returns an empty array. Currently,created.idis accessed on line 75 without a null/undefined check, causing a TypeScript error (TS18048). Add a guard immediately after the insert to narrow the type, then usecreated.idsafely.Suggested fix
const [created] = await db.insert(application).values({ organizationId: orgId, candidateId: body.candidateId, jobId: body.jobId, notes: body.notes, status: 'new', }).returning({ id: application.id, candidateId: application.candidateId, jobId: application.jobId, status: application.status, score: application.score, notes: application.notes, createdAt: application.createdAt, updatedAt: application.updatedAt, }) + if (!created) { + throw createError({ statusCode: 500, statusMessage: 'Failed to create application' }) + } + - recordActivity({ + await recordActivity({ organizationId: orgId, actorId: session.user.id, action: 'created', resourceType: 'application', resourceId: created.id, metadata: { candidateId: body.candidateId, jobId: body.jobId }, }) setResponseStatus(event, 201) return created🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/applications/index.post.ts` around lines 53 - 59, The insert call using the destructuring const [created] from db.insert(...).returning may yield undefined; add an immediate guard after that insert to check if created is truthy (e.g., if (!created) { return or throw a controlled error/response }) before any usage of created.id, then proceed to use created.id safely so the type is narrowed and TS18048 is resolved; update the code path that follows the insert (the block referencing created.id) to rely on that guard.
🧹 Nitpick comments (4)
app/pages/dashboard/settings.vue (1)
9-14: Consider responsive fallback for the two-column settings layout.Current layout is always horizontal with sticky sidebar; on narrow screens this can compress content significantly. A breakpoint-based column switch would make settings usable on mobile.
📱 Suggested responsive tweak
- <div class="flex min-w-0 flex-1 -mx-6 -my-8"> - <div class="sticky -top-8 h-screen shrink-0"> + <div class="flex min-w-0 flex-1 -mx-6 -my-8 flex-col md:flex-row"> + <div class="shrink-0 md:sticky md:-top-8 md:h-screen"> <SettingsSidebar /> </div> - <div class="flex-1 min-w-0 px-8 py-8"> + <div class="flex-1 min-w-0 px-4 py-6 md:px-8 md:py-8"> <NuxtPage /> </div> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/settings.vue` around lines 9 - 14, The two-column layout should switch to a single column on small screens and only apply the sticky, full-height sidebar on larger viewports: update the wrapper div around SettingsSidebar and NuxtPage to use responsive flex classes (e.g., flex-col on small screens and flex-row on md/up) and adjust the sidebar container (SettingsSidebar) to use md:sticky and md:h-screen while making it h-auto/relative on small screens; also ensure padding/margins on the content container (the div containing NuxtPage) use responsive utilities (e.g., smaller px/py on mobile) so the settings content doesn’t get compressed on narrow viewports.server/database/schema/app.ts (1)
182-185: Add a target+time index for comment listing paths.For paginated comment timelines, include
created_atin the target index to avoid extra sort cost.📈 Proposed index enhancement
}, (t) => ([ index('comment_organization_id_idx').on(t.organizationId), index('comment_target_idx').on(t.targetType, t.targetId), + index('comment_target_created_at_idx').on(t.targetType, t.targetId, t.createdAt), index('comment_author_id_idx').on(t.authorId), ]))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/database/schema/app.ts` around lines 182 - 185, The comment table needs a composite index that includes created_at to optimize paginated timelines; add a new index (e.g., name it 'comment_target_created_at_idx') on the same table index block alongside index('comment_target_idx') that calls .on(t.targetType, t.targetId, t.createdAt) so queries that filter by targetType/targetId and order/seek by created_at can use the index; update the schema where the indexes are defined (the index(...) calls for the comment table) and ensure the migration is generated/applied.app/pages/dashboard/settings/account.vue (1)
63-65: Block password submit when minimum policy is not met.The current submit gate allows requests even when strength is “Too short,” causing unnecessary failed calls.
🔧 Proposed fix
const passwordsMatch = computed(() => newPassword.value === confirmPassword.value && newPassword.value.length > 0, ) +const meetsPasswordPolicy = computed(() => newPassword.value.length >= 8) @@ async function handleChangePassword() { - if (!passwordsMatch.value || !currentPassword.value) return + if (!passwordsMatch.value || !currentPassword.value || !meetsPasswordPolicy.value) return @@ <button - :disabled="isChangingPassword || !passwordsMatch || !currentPassword" + :disabled="isChangingPassword || !passwordsMatch || !currentPassword || !meetsPasswordPolicy" class="inline-flex items-center gap-2 rounded-lg bg-brand-600 px-4 py-2 text-sm font-medium text-white hover:bg-brand-700 transition-colors disabled:opacity-50 disabled:cursor-not-allowed" `@click`="handleChangePassword" >Also applies to: 85-87, 343-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/settings/account.vue` around lines 63 - 65, The submit gate currently only checks passwordsMatch (computed using newPassword and confirmPassword) but allows submission when the password strength is "Too short"; update the submit checks (where passwordsMatch is used and the submit handlers around the other occurrences) to also require the password meets the minimum policy by validating either passwordStrength.value !== 'Too short' or newPassword.value.length >= MIN_PASSWORD_LENGTH (use your existing strength variable or a constant), so both passwordsMatch and the minimum-strength/length check must pass before allowing the request.server/database/migrations/0006_breezy_hairball.sql (1)
29-35: Add composite indexes for org-scoped chronological reads.The queries use
organization_idfiltering +created_atordering. Composite indexes matching these patterns will improve pagination performance:
activity_loglist: WHEREorganization_id = ?ORDER BYcreated_at DESCcommentlist: WHEREorganization_id = ?ANDtarget_type = ?ANDtarget_id = ?ORDER BYcreated_at DESC⚙️ Suggested patch
CREATE INDEX "activity_log_organization_id_idx" ON "activity_log" USING btree ("organization_id");--> statement-breakpoint CREATE INDEX "activity_log_actor_id_idx" ON "activity_log" USING btree ("actor_id");--> statement-breakpoint CREATE INDEX "activity_log_resource_idx" ON "activity_log" USING btree ("resource_type","resource_id");--> statement-breakpoint CREATE INDEX "activity_log_created_at_idx" ON "activity_log" USING btree ("created_at");--> statement-breakpoint +CREATE INDEX "activity_log_org_created_at_idx" ON "activity_log" USING btree ("organization_id","created_at");--> statement-breakpoint CREATE INDEX "comment_organization_id_idx" ON "comment" USING btree ("organization_id");--> statement-breakpoint CREATE INDEX "comment_target_idx" ON "comment" USING btree ("target_type","target_id");--> statement-breakpoint CREATE INDEX "comment_author_id_idx" ON "comment" USING btree ("author_id"); +CREATE INDEX "comment_org_target_created_at_idx" ON "comment" USING btree ("organization_id","target_type","target_id","created_at");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/database/migrations/0006_breezy_hairball.sql` around lines 29 - 35, Add composite btree indexes to support queries that filter by organization_id and order by created_at: create an index on activity_log (organization_id, created_at DESC) — e.g. name it activity_log_organization_created_at_idx — and create an index on comment (organization_id, target_type, target_id, created_at DESC) — e.g. name it comment_organization_target_created_at_idx; update the migration (near the existing CREATE INDEX statements for activity_log and comment) to add these two CREATE INDEX statements so the planner can use the composite indexes for the listed WHERE ... ORDER BY patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/composables/usePermission.ts`:
- Around line 45-57: The fetchRole() logic must reset role before each org
change and avoid races from out-of-order responses: set role.value = null at the
start of fetchRole(), then perform the
authClient.organization.getActiveMemberRole() call inside a try/catch; use a
monotonic request token (e.g., a module-scoped requestCounter incremented before
each fetch and captured as localRequestId inside fetchRole) or capture the
current activeOrgState.value.data?.id and compare it when the response returns;
only assign role.value = data?.role if the localRequestId matches the latest
token (or the org id still matches) and on error leave role.value as null.
Ensure watch(...) still calls fetchRole() with immediate: true so the token
logic handles quick org switches.
In `@app/pages/dashboard/settings/account.vue`:
- Around line 259-266: The password visibility icon buttons are icon-only and
lack accessible names; update the toggle button for showCurrentPassword (and the
similar toggle around lines 284-291) to include an accessible label and state by
adding a dynamic aria-label and aria-pressed (or title) that reflects the
action, e.g. aria-label set to "Show current password" when showCurrentPassword
is false and "Hide current password" when true, and set aria-pressed to the
boolean showCurrentPassword so screen readers can understand the control state;
ensure both the current-password toggle (referencing showCurrentPassword,
EyeOff, Eye) and the other password-toggle button use the same pattern.
In `@app/pages/dashboard/settings/index.vue`:
- Around line 37-45: The org API calls (authClient.organization.update and
authClient.organization.delete) are treated as throwing but actually return {
error } in resolved results; update the code to capture the result object (e.g.,
const result = await authClient.organization.update(...)) and check if
(result.error) before setting saveSuccess.value or performing redirects; on
error, set an appropriate error state and avoid the success toggle/redirect,
mirroring existing patterns used in members.vue, account.vue, and
useCurrentOrg.ts.
In `@app/pages/dashboard/settings/members.vue`:
- Line 451: The modal backdrop click handler only clears memberToRemove but
leaves the previous removeError intact, causing stale errors to appear on
reopen; update the backdrop close behavior to also clear removeError — either
replace the inline handler `@click.self`="memberToRemove = null" with a call to a
new method (e.g., closeRemoveModal) or expand the inline handler to set both
memberToRemove = null and removeError = null so both state variables are reset
when the modal is dismissed via backdrop.
- Around line 230-235: The icon-only buttons lack accessible names; add an
accessible label to each affected button (the one that runs "showInviteForm =
false; resetInviteForm()" and the buttons at the other two spots) by either
adding an appropriate aria-label (e.g., aria-label="Close invite form") or
including a visually-hidden text node (e.g., <span class="sr-only">Close</span>)
inside the button so screen readers get context; ensure the label text describes
the action (Close, Invite, Remove, etc.) and use the same approach for the
buttons that render the X component and the other icon-only buttons referenced.
- Around line 185-192: The document-level click listener added in onMounted is
never removed; change the inline listener to a named handler (e.g., const
onDocClick = (e: MouseEvent) => { ... }) and register it with
document.addEventListener inside onMounted, then remove it in onUnmounted via
document.removeEventListener using the same handler reference so closeDropdown
won't be called multiple times on re-entry; ensure the handler references the
same logic currently using target.closest('[data-member-actions]') and calls
closeDropdown() when appropriate.
In `@server/api/candidates/`[id]/documents/index.post.ts:
- Around line 167-174: The code calls recordActivity using properties of created
but static analysis warns created may be undefined; add a guard after the insert
that checks if created is truthy (e.g., if (!created) { handle error/return
early or throw an HttpError/Response with 500 }) before accessing created.id,
created.originalFilename or created.type, and ensure recordActivity is only
invoked when created is defined so TypeScript narrows the type and the edge case
of no rows is handled.
In `@server/api/candidates/index.post.ts`:
- Around line 43-50: The call to recordActivity uses properties on created
without ensuring created is defined; add a guard that validates created is
truthy before accessing created.id, created.firstName, or created.lastName
(e.g., if (!created) return or skip logging), then only call recordActivity with
resourceId: created.id and metadata using created.firstName/lastName when
created exists; update the block around recordActivity in index.post.ts to
perform this check to avoid runtime errors.
In `@server/api/comments/`[id].delete.ts:
- Line 31: The DELETE currently calls db.delete(comment).where(eq(comment.id,
id)) which is not tenant-scoped; update the mutation to include the organization
constraint in the same where clause (e.g. add eq(comment.orgId, orgId) together
with eq(comment.id, id)) so the delete is executed only for the org returned by
your pre-check; ensure you reference the same orgId variable used in the
pre-check and keep both predicates combined in the db.delete(...).where(...)
call to avoid check/delete race conditions.
In `@server/api/comments/`[id].patch.ts:
- Around line 36-50: The update must include authorization constraints in the
WHERE clause and explicitly handle the case when no rows are updated: modify the
db.update(comment)...where(eq(comment.id, id)) call to add the org/author
predicates (e.g., and(eq(comment.authorId, currentUserId), eq(comment.orgId,
currentOrgId)) or the appropriate fields your schema uses) so the DB enforces
ownership/organization atomically, keep the same .returning(...) shape, then
check the result (the updated variable/array) and if no row was returned throw
or return a 404/403 error (e.g., throw new Error('Not found or not authorized'))
so callers get an explicit failure when zero rows are affected.
In `@server/api/jobs/index.post.ts`:
- Around line 46-53: The call to recordActivity is accessing created.id and
created.title without ensuring created exists; add a guard before calling
recordActivity (in the handler where created is produced) to check that created
is defined and only call recordActivity if it is, or handle the error/return
early if not; reference the variables and functions involved (created,
recordActivity, orgId, session.user.id) so you locate the spot and either wrap
the recordActivity call in an if (created) { ... } or return an appropriate
error path before accessing created.id/created.title.
In `@server/database/migrations/0006_breezy_hairball.sql`:
- Line 26: The ALTER TABLE statement adds a foreign key constraint
activity_log_actor_id_user_id_fk on activity_log(actor_id) with ON DELETE
cascade which will delete audit records when a user is removed; change the
constraint to remove ON DELETE cascade (use ON DELETE NO ACTION or RESTRICT) so
actor deletions do not cascade to activity_log and preserve the immutable audit
trail; update the migration SQL in the ALTER TABLE statement that references the
constraint name activity_log_actor_id_user_id_fk and keep ON UPDATE NO ACTION
as-is.
In `@server/database/migrations/meta/0006_snapshot.json`:
- Around line 789-801: The foreign-key constraint
activity_log_actor_id_user_id_fk currently uses onDelete: "cascade" which
deletes activity_log rows when a user is removed; change the constraint to
either onDelete: "restrict" to prevent user deletion while logs exist or to
onDelete: "set null" and make activity_log.actor_id nullable so deleting a user
preserves the audit record; update the migration definition for
activity_log_actor_id_user_id_fk (tableFrom: "activity_log", tableTo: "user",
columnsFrom: ["actor_id"], columnsTo: ["id"]) to use the chosen onDelete
behavior and adjust the actor_id column nullability accordingly, then
regenerate/apply the migration so the DB schema matches.
In `@server/database/schema/app.ts`:
- Around line 196-204: The activity_log currently cascades deletes via actorId
which allows audit rows to be removed; update the actorId reference in the
activityLog table so it no longer cascades: make actorId nullable (remove
.notNull()) and change the references(...) option from onDelete: 'cascade' to
onDelete: 'set null' (reference is the actorId column and the user.id foreign
key) so that when a user is removed the audit entry is preserved with actorId
set to null.
In `@server/utils/auth.ts`:
- Around line 101-112: The sendInvitationEmail function currently logs PII
(data.email, data.inviter.user.name, data.inviter.user.email,
data.organization.name) which risks exposing sensitive data; update
sendInvitationEmail to avoid logging raw PII in production by: (1) gating the
verbose console.info behind a dev-only flag (e.g., NODE_ENV === 'development' or
a config value) or (2) replacing PII with non-identifying tokens/IDs (e.g.,
data.id, organization.id, inviter.user.id) or redacted strings before logging,
and ensure the inviteLink is only logged in dev; keep the baseURL and inviteLink
generation intact but do not write raw user emails/names/org names to logs in
non-dev environments.
In `@server/utils/recordActivity.ts`:
- Around line 1-2: At the top of server/utils/recordActivity.ts add an import
for the database client/export named db alongside the existing activityLog
import so the db symbol used in the recordActivity logic is available; locate
the file-level imports where activityLog is imported and add the missing import
for db from whatever module exports the database client in this codebase, then
run a quick type-check to ensure references to db (used around the
recordActivity function) resolve.
In `@server/utils/requirePermission.ts`:
- Around line 44-47: requirePermission currently allows calls with an empty or
missing PermissionRequest which can silently bypass authorization; add a runtime
guard at the top of the requirePermission function to fail-closed: validate that
the permissions argument is present and non-empty (handle undefined, null, empty
array/collection) and immediately reject the request (throw/createError or
return a 403/unauthorized response) when it is not. Reference the
requirePermission function and the PermissionRequest/AuthSessionWithActiveOrg
types when making the check so the guard executes before any auth/session logic
that uses event.
In `@TEAM-COLLABORATION.md`:
- Around line 33-67: Update the three fenced code blocks that start with the
ASCII diagrams so they include a fence language (e.g., text) to satisfy
markdownlint MD040: replace the opening triple backticks for the block that
begins with
"┌──────────────────────────────────────────────────────────────────┐" (the
CLIENT/SERVER/DATABASE diagram), the block that begins with "Request arrives"
(the request/handler/session flow), and the block that begins with "shared/"
(the permissions/shared files list) so each opens with ```text instead of ```,
preserving the block content unchanged.
In `@TESTING-SECURITY.md`:
- Around line 62-710: The unlabeled fenced code blocks in TESTING-SECURITY.md
(e.g., the block containing "TEST-AUTH-001: Hit every API route..." and other
repeated ``` blocks) are causing markdownlint MD040 warnings; update every
unlabeled ``` fence to include an appropriate language identifier (use text for
plain prose blocks, ts for TypeScript snippets, bash for shell examples) so each
fenced block is labeled consistently (apply the same change pattern shown in the
example replacing ``` with ```text or other relevant language) across the whole
document.
---
Outside diff comments:
In `@server/api/applications/index.post.ts`:
- Around line 53-59: The insert call using the destructuring const [created]
from db.insert(...).returning may yield undefined; add an immediate guard after
that insert to check if created is truthy (e.g., if (!created) { return or throw
a controlled error/response }) before any usage of created.id, then proceed to
use created.id safely so the type is narrowed and TS18048 is resolved; update
the code path that follows the insert (the block referencing created.id) to rely
on that guard.
---
Nitpick comments:
In `@app/pages/dashboard/settings.vue`:
- Around line 9-14: The two-column layout should switch to a single column on
small screens and only apply the sticky, full-height sidebar on larger
viewports: update the wrapper div around SettingsSidebar and NuxtPage to use
responsive flex classes (e.g., flex-col on small screens and flex-row on md/up)
and adjust the sidebar container (SettingsSidebar) to use md:sticky and
md:h-screen while making it h-auto/relative on small screens; also ensure
padding/margins on the content container (the div containing NuxtPage) use
responsive utilities (e.g., smaller px/py on mobile) so the settings content
doesn’t get compressed on narrow viewports.
In `@app/pages/dashboard/settings/account.vue`:
- Around line 63-65: The submit gate currently only checks passwordsMatch
(computed using newPassword and confirmPassword) but allows submission when the
password strength is "Too short"; update the submit checks (where passwordsMatch
is used and the submit handlers around the other occurrences) to also require
the password meets the minimum policy by validating either
passwordStrength.value !== 'Too short' or newPassword.value.length >=
MIN_PASSWORD_LENGTH (use your existing strength variable or a constant), so both
passwordsMatch and the minimum-strength/length check must pass before allowing
the request.
In `@server/database/migrations/0006_breezy_hairball.sql`:
- Around line 29-35: Add composite btree indexes to support queries that filter
by organization_id and order by created_at: create an index on activity_log
(organization_id, created_at DESC) — e.g. name it
activity_log_organization_created_at_idx — and create an index on comment
(organization_id, target_type, target_id, created_at DESC) — e.g. name it
comment_organization_target_created_at_idx; update the migration (near the
existing CREATE INDEX statements for activity_log and comment) to add these two
CREATE INDEX statements so the planner can use the composite indexes for the
listed WHERE ... ORDER BY patterns.
In `@server/database/schema/app.ts`:
- Around line 182-185: The comment table needs a composite index that includes
created_at to optimize paginated timelines; add a new index (e.g., name it
'comment_target_created_at_idx') on the same table index block alongside
index('comment_target_idx') that calls .on(t.targetType, t.targetId,
t.createdAt) so queries that filter by targetType/targetId and order/seek by
created_at can use the index; update the schema where the indexes are defined
(the index(...) calls for the comment table) and ensure the migration is
generated/applied.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
TEAM-COLLABORATION.mdTESTING-SECURITY.mdapp/components/AppSidebar.vueapp/components/SettingsSidebar.vueapp/composables/usePermission.tsapp/layouts/dashboard.vueapp/pages/dashboard/settings.vueapp/pages/dashboard/settings/account.vueapp/pages/dashboard/settings/index.vueapp/pages/dashboard/settings/members.vueapp/utils/auth-client.tsserver/api/activity-log/index.get.tsserver/api/applications/[id].get.tsserver/api/applications/[id].patch.tsserver/api/applications/index.get.tsserver/api/applications/index.post.tsserver/api/candidates/[id].delete.tsserver/api/candidates/[id].get.tsserver/api/candidates/[id].patch.tsserver/api/candidates/[id]/documents/index.post.tsserver/api/candidates/index.get.tsserver/api/candidates/index.post.tsserver/api/comments/[id].delete.tsserver/api/comments/[id].patch.tsserver/api/comments/index.get.tsserver/api/comments/index.post.tsserver/api/dashboard/stats.get.tsserver/api/documents/[id].delete.tsserver/api/documents/[id]/download.get.tsserver/api/documents/[id]/preview.get.tsserver/api/jobs/[id].delete.tsserver/api/jobs/[id].get.tsserver/api/jobs/[id].patch.tsserver/api/jobs/[id]/questions/[questionId].delete.tsserver/api/jobs/[id]/questions/[questionId].patch.tsserver/api/jobs/[id]/questions/index.get.tsserver/api/jobs/[id]/questions/index.post.tsserver/api/jobs/[id]/questions/reorder.put.tsserver/api/jobs/index.get.tsserver/api/jobs/index.post.tsserver/database/migrations/0006_breezy_hairball.sqlserver/database/migrations/meta/0006_snapshot.jsonserver/database/migrations/meta/_journal.jsonserver/database/schema/app.tsserver/utils/auth.tsserver/utils/recordActivity.tsserver/utils/requirePermission.tsserver/utils/schemas/activityLog.tsserver/utils/schemas/comment.tsshared/permissions.ts
Comment on lines
+45
to
+57
| async function fetchRole() { | ||
| const { data, error } = await authClient.organization.getActiveMemberRole() | ||
| if (!error) { | ||
| role.value = data?.role ?? null | ||
| } | ||
| } | ||
|
|
||
| // Fetch on mount and whenever the active org changes | ||
| watch( | ||
| () => activeOrgState.value.data?.id, | ||
| () => fetchRole(), | ||
| { immediate: true }, | ||
| ) |
There was a problem hiding this comment.
Reset and de-race role fetches on org changes.
Current logic can keep an old role on fetch failure and is vulnerable to out-of-order responses during fast org switches.
🔧 Proposed fix
+let fetchSeq = 0
+
async function fetchRole() {
- const { data, error } = await authClient.organization.getActiveMemberRole()
- if (!error) {
- role.value = data?.role ?? null
- }
+ const seq = ++fetchSeq
+ role.value = null
+ try {
+ const { data, error } = await authClient.organization.getActiveMemberRole()
+ if (seq !== fetchSeq) return
+ role.value = error ? null : (data?.role ?? null)
+ }
+ catch {
+ if (seq === fetchSeq) role.value = null
+ }
}
@@
watch(
() => activeOrgState.value.data?.id,
- () => fetchRole(),
+ () => { void fetchRole() },
{ immediate: true },
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function fetchRole() { | |
| const { data, error } = await authClient.organization.getActiveMemberRole() | |
| if (!error) { | |
| role.value = data?.role ?? null | |
| } | |
| } | |
| // Fetch on mount and whenever the active org changes | |
| watch( | |
| () => activeOrgState.value.data?.id, | |
| () => fetchRole(), | |
| { immediate: true }, | |
| ) | |
| let fetchSeq = 0 | |
| async function fetchRole() { | |
| const seq = ++fetchSeq | |
| role.value = null | |
| try { | |
| const { data, error } = await authClient.organization.getActiveMemberRole() | |
| if (seq !== fetchSeq) return | |
| role.value = error ? null : (data?.role ?? null) | |
| } | |
| catch { | |
| if (seq === fetchSeq) role.value = null | |
| } | |
| } | |
| // Fetch on mount and whenever the active org changes | |
| watch( | |
| () => activeOrgState.value.data?.id, | |
| () => { void fetchRole() }, | |
| { immediate: true }, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/composables/usePermission.ts` around lines 45 - 57, The fetchRole() logic
must reset role before each org change and avoid races from out-of-order
responses: set role.value = null at the start of fetchRole(), then perform the
authClient.organization.getActiveMemberRole() call inside a try/catch; use a
monotonic request token (e.g., a module-scoped requestCounter incremented before
each fetch and captured as localRequestId inside fetchRole) or capture the
current activeOrgState.value.data?.id and compare it when the response returns;
only assign role.value = data?.role if the localRequestId matches the latest
token (or the org id still matches) and on error leave role.value as null.
Ensure watch(...) still calls fetchRole() with immediate: true so the token
logic handles quick org switches.
Comment on lines
+259
to
+266
| <button | ||
| type="button" | ||
| class="absolute right-2.5 top-1/2 -translate-y-1/2 text-surface-400 hover:text-surface-600 dark:hover:text-surface-300 transition-colors" | ||
| @click="showCurrentPassword = !showCurrentPassword" | ||
| > | ||
| <EyeOff v-if="showCurrentPassword" class="size-4" /> | ||
| <Eye v-else class="size-4" /> | ||
| </button> |
There was a problem hiding this comment.
Add accessible labels to password visibility toggle buttons.
These are icon-only controls and currently lack accessible names.
♿ Proposed fix
<button
type="button"
class="absolute right-2.5 top-1/2 -translate-y-1/2 text-surface-400 hover:text-surface-600 dark:hover:text-surface-300 transition-colors"
+ :aria-label="showCurrentPassword ? 'Hide current password' : 'Show current password'"
`@click`="showCurrentPassword = !showCurrentPassword"
>
@@
<button
type="button"
class="absolute right-2.5 top-1/2 -translate-y-1/2 text-surface-400 hover:text-surface-600 dark:hover:text-surface-300 transition-colors"
+ :aria-label="showNewPassword ? 'Hide new password' : 'Show new password'"
`@click`="showNewPassword = !showNewPassword"
>Also applies to: 284-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/pages/dashboard/settings/account.vue` around lines 259 - 266, The
password visibility icon buttons are icon-only and lack accessible names; update
the toggle button for showCurrentPassword (and the similar toggle around lines
284-291) to include an accessible label and state by adding a dynamic aria-label
and aria-pressed (or title) that reflects the action, e.g. aria-label set to
"Show current password" when showCurrentPassword is false and "Hide current
password" when true, and set aria-pressed to the boolean showCurrentPassword so
screen readers can understand the control state; ensure both the
current-password toggle (referencing showCurrentPassword, EyeOff, Eye) and the
other password-toggle button use the same pattern.
Comment on lines
+37
to
+45
| try { | ||
| await authClient.organization.update({ | ||
| data: { | ||
| name: orgName.value.trim(), | ||
| slug: orgSlug.value.trim().toLowerCase(), | ||
| }, | ||
| }) | ||
| saveSuccess.value = true | ||
| setTimeout(() => { saveSuccess.value = false }, 3000) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the Better Auth client initialization and imports
find . -type f -name "*.vue" -o -name "*.ts" -o -name "*.js" | head -20
git ls-files | grep -E "\.(vue|ts|js)$" | head -30Repository: reqcore-inc/reqcore
Length of output: 1755
🏁 Script executed:
# Search for authClient.organization usage patterns
rg "authClient\.organization" --type vue --type ts --type js -A 5 -B 2Repository: reqcore-inc/reqcore
Length of output: 90
🏁 Script executed:
# Look for result.error checks mentioned in the review
rg "result\.error" --type vue --type ts --type js -B 3 -A 2Repository: reqcore-inc/reqcore
Length of output: 90
🏁 Script executed:
# Find the members and account files mentioned in the review
fd "members\|account" --type f | grep -E "\.(vue|ts|js)$"Repository: reqcore-inc/reqcore
Length of output: 45
🏁 Script executed:
# Check if there's Better Auth type definitions or documentation
fd "better.*auth\|auth.*types" --type f | grep -E "\.(ts|d\.ts|vue)$"
rg "better-auth\|@better-auth" --type json | head -20Repository: reqcore-inc/reqcore
Length of output: 45
🏁 Script executed:
# Read the actual file in question
cat -n app/pages/dashboard/settings/index.vue | head -100Repository: reqcore-inc/reqcore
Length of output: 3620
🏁 Script executed:
# Search for authClient.organization without file type filter
rg "authClient\.organization" -A 5 -B 2Repository: reqcore-inc/reqcore
Length of output: 8125
🏁 Script executed:
# Search for result.error pattern without file type filter
rg "result\.error|result\?" -A 2 -B 2 | head -100Repository: reqcore-inc/reqcore
Length of output: 5727
🏁 Script executed:
# Check package.json for Better Auth version
cat package.json | grep -A 2 -B 2 "better-auth\|auth"Repository: reqcore-inc/reqcore
Length of output: 233
🏁 Script executed:
# Find member and account related files
fd "member\|account" --type f | grep -E "\.vue$|\.ts$|\.js$"Repository: reqcore-inc/reqcore
Length of output: 45
🏁 Script executed:
# Search for Better Auth type definitions or imports
rg "from.*better-auth\|import.*auth" | head -30Repository: reqcore-inc/reqcore
Length of output: 45
Add error checks to organization API calls before success/redirect.
The Better Auth organization client returns errors in resolved promises with a { error } field, not by throwing. Both organization.update() and organization.delete() lack these checks, causing false success states (line 44) and failed redirects (line 77). The rest of the codebase consistently checks if (result.error) in similar calls (members.vue, account.vue, useCurrentOrg.ts).
🔧 Proposed fix
try {
- await authClient.organization.update({
+ const result = await authClient.organization.update({
data: {
name: orgName.value.trim(),
slug: orgSlug.value.trim().toLowerCase(),
},
})
+ if (result.error) throw new Error(String(result.error.message ?? 'Failed to update organization'))
saveSuccess.value = true
setTimeout(() => { saveSuccess.value = false }, 3000)
}
@@
try {
- await authClient.organization.delete({
+ const result = await authClient.organization.delete({
organizationId: activeOrg.value!.id,
})
+ if (result.error) throw new Error(String(result.error.message ?? 'Failed to delete organization'))
const localePath = useLocalePath()
window.location.href = localePath('/onboarding/create-org')
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| await authClient.organization.update({ | |
| data: { | |
| name: orgName.value.trim(), | |
| slug: orgSlug.value.trim().toLowerCase(), | |
| }, | |
| }) | |
| saveSuccess.value = true | |
| setTimeout(() => { saveSuccess.value = false }, 3000) | |
| try { | |
| const result = await authClient.organization.update({ | |
| data: { | |
| name: orgName.value.trim(), | |
| slug: orgSlug.value.trim().toLowerCase(), | |
| }, | |
| }) | |
| if (result.error) throw new Error(String(result.error.message ?? 'Failed to update organization')) | |
| saveSuccess.value = true | |
| setTimeout(() => { saveSuccess.value = false }, 3000) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/pages/dashboard/settings/index.vue` around lines 37 - 45, The org API
calls (authClient.organization.update and authClient.organization.delete) are
treated as throwing but actually return { error } in resolved results; update
the code to capture the result object (e.g., const result = await
authClient.organization.update(...)) and check if (result.error) before setting
saveSuccess.value or performing redirects; on error, set an appropriate error
state and avoid the success toggle/redirect, mirroring existing patterns used in
members.vue, account.vue, and useCurrentOrg.ts.
Comment on lines
+230
to
+235
| <button | ||
| class="p-1 rounded-md text-surface-400 hover:text-surface-600 dark:hover:text-surface-300 hover:bg-surface-100 dark:hover:bg-surface-800 transition-colors" | ||
| @click="showInviteForm = false; resetInviteForm()" | ||
| > | ||
| <X class="size-4" /> | ||
| </button> |
There was a problem hiding this comment.
Add accessible names to icon-only buttons.
Buttons on Line 230, Line 295, and Line 379 have no accessible label, so assistive tech users won’t get action context.
♿ Proposed fix
<button
class="p-1 rounded-md text-surface-400 hover:text-surface-600 dark:hover:text-surface-300 hover:bg-surface-100 dark:hover:bg-surface-800 transition-colors"
+ aria-label="Close invite form"
`@click`="showInviteForm = false; resetInviteForm()"
>
@@
<button class="text-danger-500 hover:text-danger-700 transition-colors" `@click`="roleUpdateError = ''">
+ aria-label="Dismiss role update error"
<X class="size-4" />
</button>
@@
<button
class="p-1.5 rounded-md text-surface-400 hover:text-surface-600 dark:hover:text-surface-300 hover:bg-surface-100 dark:hover:bg-surface-800 transition-colors"
+ aria-label="Open member actions"
`@click.stop`="toggleDropdown(m.id)"
>Also applies to: 295-297, 379-384
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/pages/dashboard/settings/members.vue` around lines 230 - 235, The
icon-only buttons lack accessible names; add an accessible label to each
affected button (the one that runs "showInviteForm = false; resetInviteForm()"
and the buttons at the other two spots) by either adding an appropriate
aria-label (e.g., aria-label="Close invite form") or including a visually-hidden
text node (e.g., <span class="sr-only">Close</span>) inside the button so screen
readers get context; ensure the label text describes the action (Close, Invite,
Remove, etc.) and use the same approach for the buttons that render the X
component and the other icon-only buttons referenced.
Comment on lines
+1
to
+2
| import { activityLog } from '../database/schema' | ||
|
|
There was a problem hiding this comment.
Import db before use.
db is referenced on Line 21 but never imported, which breaks this utility.
🐛 Proposed fix
import { activityLog } from '../database/schema'
+import { db } from './db'Also applies to: 21-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/utils/recordActivity.ts` around lines 1 - 2, At the top of
server/utils/recordActivity.ts add an import for the database client/export
named db alongside the existing activityLog import so the db symbol used in the
recordActivity logic is available; locate the file-level imports where
activityLog is imported and add the missing import for db from whatever module
exports the database client in this codebase, then run a quick type-check to
ensure references to db (used around the recordActivity function) resolve.
Comment on lines
+44
to
+47
| export async function requirePermission( | ||
| event: H3Event, | ||
| permissions: PermissionRequest, | ||
| ): Promise<AuthSessionWithActiveOrg> { |
There was a problem hiding this comment.
Fail closed on empty permission requests.
Add a runtime guard so route misconfiguration cannot silently bypass intended authorization semantics.
🔧 Proposed fix
export async function requirePermission(
event: H3Event,
permissions: PermissionRequest,
): Promise<AuthSessionWithActiveOrg> {
+ const hasRequestedPermission = Object.values(permissions)
+ .some(actions => Array.isArray(actions) && actions.length > 0)
+ if (!hasRequestedPermission) {
+ throw createError({
+ statusCode: 500,
+ statusMessage: 'Misconfigured route: empty permission request',
+ })
+ }
+
// ── Step 1: Authenticate ──
const session = await auth.api.getSession({
headers: event.headers,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function requirePermission( | |
| event: H3Event, | |
| permissions: PermissionRequest, | |
| ): Promise<AuthSessionWithActiveOrg> { | |
| export async function requirePermission( | |
| event: H3Event, | |
| permissions: PermissionRequest, | |
| ): Promise<AuthSessionWithActiveOrg> { | |
| const hasRequestedPermission = Object.values(permissions) | |
| .some(actions => Array.isArray(actions) && actions.length > 0) | |
| if (!hasRequestedPermission) { | |
| throw createError({ | |
| statusCode: 500, | |
| statusMessage: 'Misconfigured route: empty permission request', | |
| }) | |
| } | |
| // ── Step 1: Authenticate ── | |
| const session = await auth.api.getSession({ | |
| headers: event.headers, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/utils/requirePermission.ts` around lines 44 - 47, requirePermission
currently allows calls with an empty or missing PermissionRequest which can
silently bypass authorization; add a runtime guard at the top of the
requirePermission function to fail-closed: validate that the permissions
argument is present and non-empty (handle undefined, null, empty
array/collection) and immediately reject the request (throw/createError or
return a 403/unauthorized response) when it is not. Reference the
requirePermission function and the PermissionRequest/AuthSessionWithActiveOrg
types when making the check so the guard executes before any auth/session logic
that uses event.
Comment on lines
+33
to
+67
| ``` | ||
| ┌──────────────────────────────────────────────────────────────────┐ | ||
| │ CLIENT (Nuxt 4 — app/ directory) │ | ||
| │ │ | ||
| │ auth-client.ts ──► organizationClient({ ac, roles }) │ | ||
| │ usePermission() ──► checkRolePermission() (cosmetic UI gating) │ | ||
| │ useCurrentOrg() ──► useActiveOrganization() (org switching) │ | ||
| └────────────────────────────┬─────────────────────────────────────┘ | ||
| │ HTTP requests | ||
| ▼ | ||
| ┌──────────────────────────────────────────────────────────────────┐ | ||
| │ SERVER (Nitro — server/ directory) │ | ||
| │ │ | ||
| │ Every API route: │ | ||
| │ requirePermission(event, { resource: ['action'] }) │ | ||
| │ 1. Authenticate (401) │ | ||
| │ 2. Verify active org (403) │ | ||
| │ 3. hasPermission against AC (403) │ | ||
| │ │ | ||
| │ shared/permissions.ts ◄── single source of truth for all roles │ | ||
| │ auth.ts ──► betterAuth + organization({ ac, roles }) │ | ||
| │ recordActivity() ──► fire-and-forget audit logging │ | ||
| └────────────────────────────┬─────────────────────────────────────┘ | ||
| │ | ||
| ▼ | ||
| ┌──────────────────────────────────────────────────────────────────┐ | ||
| │ DATABASE (PostgreSQL via Drizzle ORM) │ | ||
| │ │ | ||
| │ Better Auth tables: user, session, account, verification, │ | ||
| │ organization, member, invitation │ | ||
| │ ATS tables: job, candidate, application, document, │ | ||
| │ job_question, question_response │ | ||
| │ Collaboration tables: comment, activity_log │ | ||
| └──────────────────────────────────────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add fence languages to clear markdownlint MD040 warnings.
Line 33, Line 235, and Line 567 start fenced blocks without a language identifier. Please annotate them (e.g., text) to satisfy linting consistently.
🧩 Suggested patch
-```
+```text
┌──────────────────────────────────────────────────────────────────┐
│ CLIENT (Nuxt 4 — app/ directory) │
...
└──────────────────────────────────────────────────────────────────┘- +text
Request arrives
│
▼
...
Return session object (with guaranteed activeOrganizationId)
-```
+```text
shared/
permissions.ts ← Single source of truth for ALL permissions
...
useCurrentOrg.ts ← Org list, active org, switch/create org
</details>
Also applies to: 235-254, 567-607
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @TEAM-COLLABORATION.md around lines 33 - 67, Update the three fenced code
blocks that start with the ASCII diagrams so they include a fence language
(e.g., text) to satisfy markdownlint MD040: replace the opening triple backticks
for the block that begins with
"┌──────────────────────────────────────────────────────────────────┐" (the
CLIENT/SERVER/DATABASE diagram), the block that begins with "Request arrives"
(the request/handler/session flow), and the block that begins with "shared/"
(the permissions/shared files list) so each opens with text instead of ,
preserving the block content unchanged.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+62
to
+710
| ``` | ||
| TEST-AUTH-001: Hit every API route without a session cookie → expect 401 on all. | ||
| ``` | ||
|
|
||
| | # | Test Case | Method | URL | Expected | | ||
| |---|-----------|--------|-----|----------| | ||
| | A001 | No cookie — list jobs | GET | `/api/jobs` | 401 | | ||
| | A002 | No cookie — create job | POST | `/api/jobs` | 401 | | ||
| | A003 | No cookie — list comments | GET | `/api/comments?targetType=job&targetId=<uuid>` | 401 | | ||
| | A004 | No cookie — create comment | POST | `/api/comments` | 401 | | ||
| | A005 | No cookie — update comment | PATCH | `/api/comments/<uuid>` | 401 | | ||
| | A006 | No cookie — delete comment | DELETE | `/api/comments/<uuid>` | 401 | | ||
| | A007 | No cookie — list activity | GET | `/api/activity-log` | 401 | | ||
| | A008 | No cookie — list candidates | GET | `/api/candidates` | 401 | | ||
| | A009 | No cookie — create candidate | POST | `/api/candidates` | 401 | | ||
| | A010 | No cookie — list applications | GET | `/api/applications` | 401 | | ||
| | A011 | No cookie — create application | POST | `/api/applications` | 401 | | ||
| | A012 | No cookie — dashboard stats | GET | `/api/dashboard/stats` | 401 | | ||
| | A013 | No cookie — upload document | POST | `/api/candidates/<uuid>/documents` | 401 | | ||
| | A014 | No cookie — delete document | DELETE | `/api/documents/<uuid>` | 401 | | ||
| | A015 | No cookie — download document | GET | `/api/documents/<uuid>/download` | 401 | | ||
| | A016 | No cookie — preview document | GET | `/api/documents/<uuid>/preview` | 401 | | ||
|
|
||
| ### 2.2 Invalid/expired session | ||
|
|
||
| **What could go wrong:** A tampered or expired session cookie is accepted. | ||
|
|
||
| ``` | ||
| TEST-AUTH-002: Send a request with a garbage session cookie → expect 401. | ||
| TEST-AUTH-003: Send a request with an expired session → expect 401. | ||
| TEST-AUTH-004: Log out, then reuse the old session cookie → expect 401. | ||
| ``` | ||
|
|
||
| ### 2.3 Session after org removal | ||
|
|
||
| **What could go wrong:** A user is removed from an org but their session still has `activeOrganizationId` set to that org. They continue making API calls. | ||
|
|
||
| ``` | ||
| TEST-AUTH-005: Remove a member from Org A. Before they refresh their session, | ||
| send a request with their old session → expect 403. | ||
| Better Auth's hasPermission should reject because they're no longer a member. | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 3. Authorization & RBAC Vulnerabilities | ||
|
|
||
| ### 3.1 Privilege escalation — member tries admin/owner actions | ||
|
|
||
| This is the most critical category. Every action that members cannot do must be tested. | ||
|
|
||
| ``` | ||
| TEST-RBAC-001: Member tries to create a job → expect 403. | ||
| TEST-RBAC-002: Member tries to update a job → expect 403. | ||
| TEST-RBAC-003: Member tries to delete a job → expect 403. | ||
| TEST-RBAC-004: Member tries to delete a candidate → expect 403. | ||
| TEST-RBAC-005: Member tries to delete an application → expect 403. | ||
| TEST-RBAC-006: Member tries to delete a document → expect 403. | ||
| TEST-RBAC-007: Member tries to update a comment → expect 403. | ||
| TEST-RBAC-008: Member tries to delete a comment → expect 403. | ||
| TEST-RBAC-009: Member tries to invite a new member → expect 403. | ||
| TEST-RBAC-010: Member tries to remove a member → expect 403. | ||
| TEST-RBAC-011: Member tries to change a member's role → expect 403. | ||
| TEST-RBAC-012: Member tries to delete the organization → expect 403. | ||
| TEST-RBAC-013: Member tries to update org settings → expect 403. | ||
| ``` | ||
|
|
||
| ### 3.2 Privilege escalation — admin tries owner-only actions | ||
|
|
||
| ``` | ||
| TEST-RBAC-014: Admin tries to delete the organization → expect 403. | ||
| TEST-RBAC-015: Admin tries to transfer ownership → expect 403. | ||
| ``` | ||
|
|
||
| ### 3.3 Positive authorization tests (confirm allowed actions work) | ||
|
|
||
| ``` | ||
| TEST-RBAC-016: Member can read jobs → expect 200. | ||
| TEST-RBAC-017: Member can create a candidate → expect 201. | ||
| TEST-RBAC-018: Member can read candidates → expect 200. | ||
| TEST-RBAC-019: Member can update a candidate → expect 200. | ||
| TEST-RBAC-020: Member can create an application → expect 201. | ||
| TEST-RBAC-021: Member can update an application → expect 200. | ||
| TEST-RBAC-022: Member can create a comment → expect 201. | ||
| TEST-RBAC-023: Member can read comments → expect 200. | ||
| TEST-RBAC-024: Member can read the activity log → expect 200. | ||
| TEST-RBAC-025: Member can upload a document → expect 201. | ||
| TEST-RBAC-026: Member can read/download a document → expect 200. | ||
| TEST-RBAC-027: Admin can do everything a member can, plus create/update/delete jobs → expect 2xx. | ||
| TEST-RBAC-028: Owner can do everything, including org deletion → expect 2xx. | ||
| ``` | ||
|
|
||
| ### 3.4 No active organization | ||
|
|
||
| **What could go wrong:** A user is authenticated but has no `activeOrganizationId` set. They try to access data. | ||
|
|
||
| ``` | ||
| TEST-RBAC-029: Authenticated user with no active org → hit any API route → expect 403 "No active organization". | ||
| ``` | ||
|
|
||
| ### 3.5 Fabricated permission claims | ||
|
|
||
| **What could go wrong:** The `requirePermission` utility receives an empty permission object `{}` and passes because there's nothing to deny. | ||
|
|
||
| ``` | ||
| TEST-RBAC-030: Verify that requirePermission with empty permissions {} still enforces | ||
| authentication and active org checks. | ||
| (Note: This is a code-level concern — ensure no route accidentally passes {}.) | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 4. Organization Isolation Vulnerabilities | ||
|
|
||
| This is the second most critical category. All data is scoped to an organization. If the scoping is broken, users in Org A can see data from Org B. | ||
|
|
||
| ### 4.1 Cross-tenant data access (IDOR) | ||
|
|
||
| **What could go wrong:** A user in Org A knows the UUID of a resource in Org B. They request it directly. | ||
|
|
||
| ``` | ||
| TEST-IDOR-001: User in Org A fetches GET /api/jobs/<org-b-job-id> → expect 404 (not 200 with Org B data). | ||
| TEST-IDOR-002: User in Org A fetches GET /api/candidates/<org-b-candidate-id> → expect 404. | ||
| TEST-IDOR-003: User in Org A fetches GET /api/applications/<org-b-application-id> → expect 404. | ||
| TEST-IDOR-004: User in Org A fetches GET /api/documents/<org-b-document-id>/download → expect 404. | ||
| TEST-IDOR-005: User in Org A fetches GET /api/documents/<org-b-document-id>/preview → expect 404. | ||
| TEST-IDOR-006: User in Org A tries PATCH /api/comments/<org-b-comment-id> → expect 404. | ||
| TEST-IDOR-007: User in Org A tries DELETE /api/comments/<org-b-comment-id> → expect 404. | ||
| ``` | ||
|
|
||
| ### 4.2 Cross-tenant list leakage | ||
|
|
||
| **What could go wrong:** A list endpoint doesn't filter by `organizationId` and returns data from all orgs. | ||
|
|
||
| ``` | ||
| TEST-IDOR-008: Create jobs in Org A and Org B. User in Org A lists jobs → only Org A jobs returned. | ||
| TEST-IDOR-009: Create comments in Org A and Org B on same target type. User in Org A lists comments → only Org A comments. | ||
| TEST-IDOR-010: Create activity in Org A and Org B. User in Org A lists activity → only Org A activity. | ||
| TEST-IDOR-011: Create candidates in Org A and Org B. List in Org A → only Org A candidates. | ||
| ``` | ||
|
|
||
| ### 4.3 Cross-tenant write via request body manipulation | ||
|
|
||
| **What could go wrong:** The POST body includes an `organizationId` field and the server uses it instead of the session's `activeOrganizationId`. | ||
|
|
||
| ``` | ||
| TEST-IDOR-012: POST /api/jobs with body { organizationId: "<org-b-id>", title: "Hacked" } | ||
| → job should be created in Org A (from session), not Org B. | ||
| TEST-IDOR-013: POST /api/comments with body { organizationId: "<org-b-id>", ... } | ||
| → comment should be created in Org A. | ||
| TEST-IDOR-014: POST /api/candidates with body { organizationId: "<org-b-id>", ... } | ||
| → candidate should be created in Org A. | ||
| ``` | ||
|
|
||
| ### 4.4 Comment on cross-tenant target | ||
|
|
||
| **What could go wrong:** A user in Org A creates a comment on a candidate that belongs to Org B. The comment `targetId` is a valid UUID, but in the wrong org. | ||
|
|
||
| ``` | ||
| TEST-IDOR-015: User in Org A posts comment with targetType=candidate, targetId=<org-b-candidate-id> | ||
| → expect 404 "candidate not found" (the target existence check filters by orgId). | ||
| TEST-IDOR-016: Same test for targetType=application with Org B application ID → expect 404. | ||
| TEST-IDOR-017: Same test for targetType=job with Org B job ID → expect 404. | ||
| ``` | ||
|
|
||
| ### 4.5 Activity log cross-tenant resource filter | ||
|
|
||
| **What could go wrong:** The activity log `resourceId` filter parameter is used to probe for existence of resources in other orgs. | ||
|
|
||
| ``` | ||
| TEST-IDOR-018: GET /api/activity-log?resourceType=job&resourceId=<org-b-job-id> | ||
| → expect empty results (not an error that leaks "resource exists in another org"). | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 5. Comments — Attack Surface | ||
|
|
||
| ### 5.1 Authorization edge cases | ||
|
|
||
| ``` | ||
| TEST-CMT-001: Member creates a comment → expect 201. | ||
| TEST-CMT-002: Member tries to PATCH their own comment → expect 403 (member role lacks comment:update). | ||
| TEST-CMT-003: Admin edits their OWN comment → expect 200. | ||
| TEST-CMT-004: Admin tries to edit ANOTHER user's comment → expect 403 "You can only edit your own comments". | ||
| (This is the authorship check, not the role check.) | ||
| TEST-CMT-005: Owner tries to edit another user's comment → expect 403 "You can only edit your own comments". | ||
| (Even owners can't edit other people's words — but they CAN delete them.) | ||
| TEST-CMT-006: Admin deletes another user's comment → expect 204 (admin has comment:delete). | ||
| TEST-CMT-007: Owner deletes another user's comment → expect 204. | ||
| TEST-CMT-008: Member tries to delete their own comment → expect 403 (member lacks comment:delete). | ||
| ``` | ||
|
|
||
| ### 5.2 Comment on deleted target | ||
|
|
||
| **What could go wrong:** A candidate is deleted, but existing comments referencing that candidate remain queryable. Or a user tries to post a comment on a deleted target. | ||
|
|
||
| ``` | ||
| TEST-CMT-009: Delete a candidate. Then POST a comment with targetType=candidate, targetId=<deleted-id> | ||
| → expect 404 "candidate not found". | ||
| TEST-CMT-010: Delete a candidate that has comments. GET comments for that target | ||
| → decide on expected behavior: | ||
| Option A: return empty (comments cascade-deleted with candidate) | ||
| Option B: return comments (orphaned but still visible) | ||
| NOTE: Currently, comments are NOT cascade-deleted with the target because | ||
| targetId is not a foreign key — it's a plain text field. This is a design | ||
| decision to consider. Orphaned comments won't cause errors but may confuse users. | ||
| ``` | ||
|
|
||
| ### 5.3 Comment body content | ||
|
|
||
| **What could go wrong:** Malicious content in the comment body. | ||
|
|
||
| ``` | ||
| TEST-CMT-011: Create comment with HTML: <script>alert('xss')</script> | ||
| → stored body should contain the raw string (no execution). | ||
| → when rendered on the client, must be escaped (Vue's {{ }} does this by default). | ||
| TEST-CMT-012: Create comment with SQL injection payload: '; DROP TABLE comment; -- | ||
| → expect 201 (Drizzle uses parameterized queries — payload is stored as literal text). | ||
| TEST-CMT-013: Create comment with 10,000 characters (max allowed) → expect 201. | ||
| TEST-CMT-014: Create comment with 10,001 characters → expect 422 (Zod rejects). | ||
| TEST-CMT-015: Create comment with empty body "" → expect 422 (min 1 char). | ||
| TEST-CMT-016: Create comment with body of only whitespace " " → expect 201 (currently allowed). | ||
| Consider: Should we trim and reject whitespace-only bodies? | ||
| TEST-CMT-017: Create comment with unicode/emoji: "Great candidate! 🎉👍" → expect 201. | ||
| TEST-CMT-018: Create comment with zero-width characters → expect 201 (stored as-is). | ||
| Consider: Should we strip zero-width characters? | ||
| ``` | ||
|
|
||
| ### 5.4 Comment ID manipulation | ||
|
|
||
| ``` | ||
| TEST-CMT-019: PATCH /api/comments/<non-existent-uuid> → expect 404. | ||
| TEST-CMT-020: PATCH /api/comments/not-a-uuid → expect 422 (Zod UUID validation). | ||
| TEST-CMT-021: DELETE /api/comments/<non-existent-uuid> → expect 404. | ||
| TEST-CMT-022: DELETE /api/comments/not-a-uuid → expect 422. | ||
| ``` | ||
|
|
||
| ### 5.5 Comment pagination edge cases | ||
|
|
||
| ``` | ||
| TEST-CMT-023: GET /api/comments with page=0 → expect 422 (positive integer required). | ||
| TEST-CMT-024: GET /api/comments with page=-1 → expect 422. | ||
| TEST-CMT-025: GET /api/comments with limit=0 → expect 422 (min 1). | ||
| TEST-CMT-026: GET /api/comments with limit=101 → expect 422 (max 100). | ||
| TEST-CMT-027: GET /api/comments with page=999999 (beyond data) → expect 200 with empty data array and correct total. | ||
| TEST-CMT-028: GET /api/comments without targetType → expect 422. | ||
| TEST-CMT-029: GET /api/comments without targetId → expect 422. | ||
| TEST-CMT-030: GET /api/comments with targetType=invalid → expect 422. | ||
| TEST-CMT-031: GET /api/comments with targetId=not-a-uuid → expect 422. | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 6. Activity Log — Attack Surface | ||
|
|
||
| ### 6.1 Immutability | ||
|
|
||
| **What could go wrong:** Someone creates, modifies, or deletes activity log entries. | ||
|
|
||
| ``` | ||
| TEST-ACT-001: Verify there is no POST /api/activity-log endpoint → expect 404 or 405. | ||
| TEST-ACT-002: Verify there is no PATCH /api/activity-log/:id endpoint → expect 404 or 405. | ||
| TEST-ACT-003: Verify there is no DELETE /api/activity-log/:id endpoint → expect 404 or 405. | ||
| TEST-ACT-004: Attempt to POST to /api/activity-log with a valid body → expect 404/405 (no route). | ||
| ``` | ||
|
|
||
| ### 6.2 Completeness | ||
|
|
||
| **What could go wrong:** An action happens but no activity log entry is recorded. | ||
|
|
||
| ``` | ||
| TEST-ACT-005: Create a job → verify activity_log has entry: action=created, resourceType=job. | ||
| TEST-ACT-006: Update a job → verify activity_log has entry: action=updated, resourceType=job. | ||
| TEST-ACT-007: Delete a job → verify activity_log has entry: action=deleted, resourceType=job. | ||
| TEST-ACT-008: Change job status (draft → published) → verify activity_log: action=status_changed, metadata contains from/to. | ||
| TEST-ACT-009: Create a candidate → verify activity_log: action=created, resourceType=candidate. | ||
| TEST-ACT-010: Update a candidate → verify activity_log: action=updated, resourceType=candidate. | ||
| TEST-ACT-011: Delete a candidate → verify activity_log: action=deleted, resourceType=candidate. | ||
| TEST-ACT-012: Create an application → verify activity_log: action=created, resourceType=application. | ||
| TEST-ACT-013: Change application status → verify activity_log: action=status_changed, metadata has from/to. | ||
| TEST-ACT-014: Upload a document → verify activity_log: action=created, resourceType=document. | ||
| TEST-ACT-015: Delete a document → verify activity_log: action=deleted, resourceType=document. | ||
| TEST-ACT-016: Create a comment → verify activity_log: action=comment_added, resourceType matches targetType. | ||
| TEST-ACT-017: Delete a comment → verify activity_log: action=deleted, resourceType=comment. | ||
| ``` | ||
|
|
||
| ### 6.3 Actor accuracy | ||
|
|
||
| **What could go wrong:** The `actorId` in the log entry doesn't match the user who performed the action. | ||
|
|
||
| ``` | ||
| TEST-ACT-018: User A creates a job. Verify actorId === User A's ID, not some default or null. | ||
| TEST-ACT-019: User B (different user, same org) deletes that job. Verify actorId === User B's ID. | ||
| ``` | ||
|
|
||
| ### 6.4 Fire-and-forget resilience | ||
|
|
||
| **What could go wrong:** If `recordActivity()` throws, the primary operation fails. | ||
|
|
||
| ``` | ||
| TEST-ACT-020: Simulate a database error during activity logging (e.g., constraint violation). | ||
| Verify the primary operation (e.g., job creation) still succeeds. | ||
| Verify the error is logged to stderr. | ||
| ``` | ||
|
|
||
| ### 6.5 Activity log pagination and filtering | ||
|
|
||
| ``` | ||
| TEST-ACT-021: GET /api/activity-log with page=0 → expect 422. | ||
| TEST-ACT-022: GET /api/activity-log with limit=101 → expect 422. | ||
| TEST-ACT-023: GET /api/activity-log with resourceId=not-a-uuid → expect 422. | ||
| TEST-ACT-024: GET /api/activity-log with resourceType=job&resourceId=<valid-uuid> → returns only matching entries. | ||
| TEST-ACT-025: GET /api/activity-log with resourceType only (no resourceId) → returns all entries for that type. | ||
| ``` | ||
|
|
||
| ### 6.6 Metadata injection | ||
|
|
||
| **What could go wrong:** The JSONB `metadata` field could contain unexpectedly large payloads or malicious content. | ||
|
|
||
| ``` | ||
| TEST-ACT-026: Verify metadata values are derived from server-side data (not user input). | ||
| The metadata is set by the server routes, not from request bodies. | ||
| This is a code review check, not a runtime test. | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 7. Invitation System — Attack Surface | ||
|
|
||
| ### 7.1 Invitation enumeration | ||
|
|
||
| **What could go wrong:** An attacker can enumerate valid invitation IDs to discover which emails have been invited. | ||
|
|
||
| ``` | ||
| TEST-INV-001: Call GET /organization/get-invitation with a random UUID → expect 404 (not a different error that leaks info). | ||
| TEST-INV-002: Call GET /organization/get-invitation with a valid but expired invitation → expect appropriate error. | ||
| ``` | ||
|
|
||
| ### 7.2 Invitation acceptance by wrong user | ||
|
|
||
| **What could go wrong:** User X receives an invitation email for user@example.com, but User Y (with a different email) tries to accept it. | ||
|
|
||
| ``` | ||
| TEST-INV-003: Send invitation to alice@example.com. Log in as bob@example.com. | ||
| Try to accept the invitation → expect rejection (Better Auth should enforce email match). | ||
| ``` | ||
|
|
||
| ### 7.3 Invitation replay | ||
|
|
||
| **What could go wrong:** An invitation is accepted, but the attacker replays the accept request to get re-added after removal. | ||
|
|
||
| ``` | ||
| TEST-INV-004: Accept invitation → becomes member. Get removed from org. | ||
| Try to accept the same invitation again → expect failure (invitation status is no longer pending). | ||
| ``` | ||
|
|
||
| ### 7.4 Invitation expiration | ||
|
|
||
| ``` | ||
| TEST-INV-005: Create invitation. Wait for expiration (or manipulate timestamp in test DB). | ||
| Try to accept → expect failure. | ||
| ``` | ||
|
|
||
| ### 7.5 Role escalation via invitation | ||
|
|
||
| **What could go wrong:** An admin invites someone as "owner" to escalate their own privileges. | ||
|
|
||
| ``` | ||
| TEST-INV-006: Admin invites a new user with role=owner → expect 403 (admins can't create owners). | ||
| Verify Better Auth enforces this at the plugin level. | ||
| ``` | ||
|
|
||
| ### 7.6 Re-invitation cancellation | ||
|
|
||
| ``` | ||
| TEST-INV-007: Invite alice@example.com. Invite alice@example.com again. | ||
| Verify the first invitation is cancelled (cancelPendingInvitationsOnReInvite: true). | ||
| Old invitation ID should no longer be acceptable. | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 8. Input Validation Vulnerabilities | ||
|
|
||
| ### 8.1 UUID parameter tampering | ||
|
|
||
| **What could go wrong:** Route parameters or query strings accept non-UUID values, potentially causing SQL errors or unexpected behavior. | ||
|
|
||
| ``` | ||
| TEST-VAL-001: GET /api/jobs/not-a-uuid → expect 422 (or 404 if no param validation — check both). | ||
| TEST-VAL-002: PATCH /api/comments/../../etc/passwd → expect 422. | ||
| TEST-VAL-003: DELETE /api/comments/' OR '1'='1 → expect 422. | ||
| TEST-VAL-004: GET /api/comments?targetId=null → expect 422. | ||
| TEST-VAL-005: GET /api/comments?targetId=undefined → expect 422. | ||
| ``` | ||
|
|
||
| ### 8.2 Request body type coercion | ||
|
|
||
| **What could go wrong:** Sending unexpected types (number instead of string, array instead of object). | ||
|
|
||
| ``` | ||
| TEST-VAL-006: POST /api/comments with body: 123 (number, not object) → expect 422. | ||
| TEST-VAL-007: POST /api/comments with body: [1, 2, 3] (array) → expect 422. | ||
| TEST-VAL-008: POST /api/comments with body: { body: 123 } (number instead of string) → expect 422. | ||
| TEST-VAL-009: POST /api/comments with body: { targetType: "user" } (invalid enum value) → expect 422. | ||
| TEST-VAL-010: POST /api/comments with extra fields: { body: "hi", evil: true } → extra fields should be stripped by Zod. | ||
| ``` | ||
|
|
||
| ### 8.3 Content-Type manipulation | ||
|
|
||
| ``` | ||
| TEST-VAL-011: POST /api/comments with Content-Type: text/plain → expect 400 or 422. | ||
| TEST-VAL-012: POST /api/comments with Content-Type: application/xml → expect 400 or 422. | ||
| ``` | ||
|
|
||
| ### 8.4 Prototype pollution | ||
|
|
||
| ``` | ||
| TEST-VAL-013: POST /api/comments with body: { "__proto__": { "isAdmin": true }, "body": "test", ... } | ||
| → __proto__ should be ignored. Zod's strict parsing should strip unknown keys. | ||
| TEST-VAL-014: POST /api/comments with body: { "constructor": { "prototype": { ... } } } | ||
| → should be stripped or rejected. | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 9. Rate Limiting & Denial of Service | ||
|
|
||
| ### 9.1 Comment spam | ||
|
|
||
| **What could go wrong:** An authenticated user creates thousands of comments in a loop. | ||
|
|
||
| ``` | ||
| TEST-DOS-001: Create 100 comments in rapid succession → expect rate limiter to kick in (429) in production. | ||
| Note: Rate limiting is disabled in development (NODE_ENV !== 'production'). | ||
| TEST-DOS-002: Verify the write rate limiter (80/min) applies to POST /api/comments. | ||
| ``` | ||
|
|
||
| ### 9.2 Large pagination requests | ||
|
|
||
| **What could go wrong:** Requesting limit=100 with deeply nested joins on large datasets causes slow queries. | ||
|
|
||
| ``` | ||
| TEST-DOS-003: GET /api/activity-log?limit=100 on an org with 100,000 activity entries → measure response time. | ||
| Should return in < 500ms with proper indexing. | ||
| TEST-DOS-004: GET /api/comments?limit=100&page=1 with many comments → measure response time. | ||
| ``` | ||
|
|
||
| ### 9.3 Activity log flooding | ||
|
|
||
| **What could go wrong:** Since `recordActivity` is fire-and-forget, a flood of mutations could fill the activity_log table. | ||
|
|
||
| ``` | ||
| TEST-DOS-005: Consider whether activity_log needs a retention policy (e.g., delete entries older than 1 year). | ||
| This is a design concern, not a code bug — but monitor table size. | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 10. Data Leakage & Information Disclosure | ||
|
|
||
| ### 10.1 Error message leakage | ||
|
|
||
| **What could go wrong:** Error responses include stack traces, SQL queries, or internal details. | ||
|
|
||
| ``` | ||
| TEST-LEAK-001: Trigger a 404 on /api/comments/<org-b-comment-id> → response body must NOT leak | ||
| the comment exists in another org. Should just say "Comment not found". | ||
| TEST-LEAK-002: Trigger a 422 with invalid input → error should describe the validation issue | ||
| but not include internal paths, table names, or schema details. | ||
| TEST-LEAK-003: Trigger a 500 (e.g., database connection failure) → response must NOT include | ||
| the database connection string or stack trace in production. | ||
| ``` | ||
|
|
||
| ### 10.2 Response body leakage | ||
|
|
||
| **What could go wrong:** API responses include fields the user shouldn't see. | ||
|
|
||
| ``` | ||
| TEST-LEAK-004: GET /api/comments → verify response does NOT include organizationId | ||
| (it's implicit from the session — exposing it aids cross-tenant attacks). | ||
| Currently: organizationId IS included in the response. Consider removing it. | ||
| TEST-LEAK-005: GET /api/activity-log → verify metadata doesn't contain sensitive data | ||
| (passwords, tokens, PII beyond what's needed for the audit trail). | ||
| TEST-LEAK-006: GET /api/comments → verify authorEmail is appropriate to expose. | ||
| In a team tool, showing email is fine. But if comments are ever | ||
| visible to external parties, this leaks internal email addresses. | ||
| ``` | ||
|
|
||
| ### 10.3 Timing attacks on resource existence | ||
|
|
||
| **What could go wrong:** The response time for "resource not found in this org" vs "resource doesn't exist at all" differs, allowing an attacker to enumerate valid UUIDs. | ||
|
|
||
| ``` | ||
| TEST-LEAK-007: Measure response time for: | ||
| (a) PATCH /api/comments/<org-b-comment-id> → 404 (exists in other org) | ||
| (b) PATCH /api/comments/<completely-fake-uuid> → 404 (doesn't exist at all) | ||
| Both should take approximately equal time (within noise). | ||
| Currently: Both go through the same db.query.comment.findFirst() path, so timing should be similar. | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 11. Race Conditions & TOCTOU | ||
|
|
||
| ### 11.1 Double-submit comment creation | ||
|
|
||
| **What could go wrong:** A user double-clicks "Post Comment" and two identical comments are created. | ||
|
|
||
| ``` | ||
| TEST-RACE-001: Send two POST /api/comments requests simultaneously with identical bodies. | ||
| Both should succeed (no unique constraint on body+target). | ||
| Decision: Is this acceptable? Options: | ||
| - Accept duplicates (current behavior) | ||
| - Add client-side debounce | ||
| - Add a deduplication window on the server (e.g., same author+target+body within 5 seconds) | ||
| ``` | ||
|
|
||
| ### 11.2 Edit-while-delete race | ||
|
|
||
| **What could go wrong:** User A starts editing a comment. User B deletes it. User A submits the edit. | ||
|
|
||
| ``` | ||
| TEST-RACE-002: Delete a comment. Then immediately PATCH the same comment ID. | ||
| → expect 404 (the findFirst check will not find the deleted comment). | ||
| Currently correct: the PATCH route fetches the comment first. | ||
| ``` | ||
|
|
||
| ### 11.3 TOCTOU on comment ownership | ||
|
|
||
| **What could go wrong:** The PATCH route checks `authorId === session.user.id`, then updates. But between the check and the update, the comment's ownership hypothetically changes. | ||
|
|
||
| ``` | ||
| TEST-RACE-003: This is a theoretical concern. Comment authorId is immutable (never updated). | ||
| No action needed — but document the invariant: authorId must never be updateable. | ||
| Verify: The updateCommentSchema only allows { body: string }. authorId cannot be sent. | ||
| ``` | ||
|
|
||
| ### 11.4 Organization deletion during request processing | ||
|
|
||
| **What could go wrong:** Org A is deleted while a member of Org A is mid-request creating a comment. | ||
|
|
||
| ``` | ||
| TEST-RACE-004: Delete Org A. Simultaneously create a comment from Org A. | ||
| → expect either 201 (created before cascade) or a DB error caught by the route. | ||
| The FK cascade will eventually clean up, but the in-flight request might fail. | ||
| This is acceptable — the user will see an error, and the data will be consistent. | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 12. Integration Test Plan | ||
|
|
||
| These tests run against a real database (test PostgreSQL instance) with the Nitro server, testing the full request → middleware → handler → DB → response pipeline. | ||
|
|
||
| ### Test file structure | ||
|
|
||
| ``` | ||
| test/ | ||
| integration/ | ||
| auth/ | ||
| require-permission.test.ts # Tests for the requirePermission utility | ||
| comments/ | ||
| comments-crud.test.ts # Happy path CRUD | ||
| comments-authorization.test.ts # RBAC enforcement | ||
| comments-isolation.test.ts # Cross-org isolation | ||
| comments-validation.test.ts # Input validation edge cases | ||
| activity-log/ | ||
| activity-log-read.test.ts # Happy path + pagination | ||
| activity-log-completeness.test.ts # Verify all mutations log activity | ||
| activity-log-immutability.test.ts # Verify no write endpoints exist | ||
| rbac/ | ||
| role-permissions.test.ts # Each role × each action matrix | ||
| cross-tenant.test.ts # Org isolation for all resources | ||
| ``` | ||
|
|
||
| ### Test helpers needed | ||
|
|
||
| ```ts | ||
| // Create test users with specific roles in specific orgs | ||
| async function createTestUser(role: 'owner' | 'admin' | 'member', orgId: string): Promise<TestUser> | ||
|
|
||
| // Create a full org with one of each resource for testing | ||
| async function seedOrg(): Promise<{ org, owner, admin, member, job, candidate, application, comment }> | ||
|
|
||
| // Make authenticated requests as a specific user | ||
| async function apiAs(user: TestUser, method: string, url: string, body?: object): Promise<Response> | ||
| ``` | ||
|
|
||
| ### Test execution order | ||
|
|
||
| 1. Seed two orgs (Org A and Org B) with users at each role level. | ||
| 2. Run all tests against both orgs to verify isolation. | ||
| 3. Tear down test data. | ||
|
|
||
| --- | ||
|
|
||
| ## 13. E2E Test Plan | ||
|
|
||
| End-to-end tests using Playwright that verify the full user flow through the browser. | ||
|
|
||
| ### E2E-COLLAB-001: Comment lifecycle | ||
|
|
||
| ``` | ||
| 1. Sign up → create org → create job → create candidate. | ||
| 2. Navigate to candidate detail page. | ||
| 3. Type a comment in the comment box → click "Post". | ||
| 4. Verify comment appears in the list with author name and timestamp. | ||
| 5. Click "Edit" on the comment → change text → save. | ||
| 6. Verify updated text appears. | ||
| 7. Click "Delete" → confirm. | ||
| 8. Verify comment is removed from the list. | ||
| ``` | ||
|
|
||
| ### E2E-COLLAB-002: Activity log visibility | ||
|
|
||
| ``` | ||
| 1. As owner: create a job, create a candidate, create an application. | ||
| 2. Navigate to activity log page. | ||
| 3. Verify entries appear: "created job", "created candidate", "created application". | ||
| 4. Click filter by resourceType=job → only job entries shown. | ||
| ``` | ||
|
|
||
| ### E2E-COLLAB-003: Member permission restrictions | ||
|
|
||
| ``` | ||
| 1. As owner: create org, create job, invite member@test.com with role=member. | ||
| 2. Log in as member. | ||
| 3. Verify: can see jobs list, but "New Job" button is hidden. | ||
| 4. Navigate to candidates → verify can create a candidate. | ||
| 5. Post a comment on the candidate. | ||
| 6. Verify: "Edit" and "Delete" buttons are NOT shown on the comment (usePermission gates them). | ||
| 7. Verify: navigating directly to /api/jobs (POST) via fetch returns 403. | ||
| ``` | ||
|
|
||
| ### E2E-COLLAB-004: Org switching isolation | ||
|
|
||
| ``` | ||
| 1. Create Org A with job "Alpha". | ||
| 2. Create Org B with job "Beta". | ||
| 3. Switch to Org A → verify only "Alpha" visible. | ||
| 4. Switch to Org B → verify only "Beta" visible. | ||
| 5. Ensure no data leaks between orgs during switching. | ||
| ``` | ||
|
|
||
| ### E2E-COLLAB-005: Invitation flow | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks (markdownlint MD040).
There are repeated unlabeled fenced blocks (starting at Line 62 and throughout the doc), which will keep lint warnings noisy.
🛠️ Example fix pattern
-```
+```text
TEST-AUTH-001: Hit every API route without a session cookie → expect 401 on all.
-```
+```Apply the same pattern to each unlabeled block (text, ts, or bash depending on content).
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 62-62: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 99-99: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 113-113: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 131-131: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 138-138: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 158-158: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 166-166: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 182-182: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 196-196: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 207-207: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 220-220: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 231-231: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 242-242: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 259-259: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 275-275: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 293-293: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 302-302: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 322-322: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 333-333: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 353-353: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 362-362: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 370-370: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 382-382: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 396-396: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 405-405: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 414-414: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 421-421: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 430-430: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 437-437: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 451-451: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 463-463: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 473-473: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 480-480: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 495-495: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 505-505: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 515-515: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 528-528: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 541-541: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 556-556: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 572-572: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 585-585: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 595-595: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 605-605: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 620-620: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 666-666: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 679-679: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 688-688: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 700-700: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 710-710: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TESTING-SECURITY.md` around lines 62 - 710, The unlabeled fenced code blocks
in TESTING-SECURITY.md (e.g., the block containing "TEST-AUTH-001: Hit every API
route..." and other repeated ``` blocks) are causing markdownlint MD040
warnings; update every unlabeled ``` fence to include an appropriate language
identifier (use text for plain prose blocks, ts for TypeScript snippets, bash
for shell examples) so each fenced block is labeled consistently (apply the same
change pattern shown in the example replacing ``` with ```text or other relevant
language) across the whole document.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/api/candidates/index.post.ts (1)
47-54: Same async intent issue: explicitly handlerecordActivitypromise.At Line 47, prefer
await recordActivity(...)(orvoidif intentionally non-blocking) so behavior is explicit and consistent.Suggested change
- recordActivity({ + await recordActivity({ organizationId: orgId, actorId: session.user.id, action: 'created', resourceType: 'candidate', resourceId: created.id, metadata: { name: `${created.firstName} ${created.lastName}` }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/candidates/index.post.ts` around lines 47 - 54, The call to recordActivity in the candidate creation handler is currently unhandled; make the async intent explicit by either awaiting recordActivity(...) if activity recording must complete before responding, or prefixing with void recordActivity(...) if it should run fire-and-forget—update the invocation in the POST handler (the recordActivity call that uses created.id and metadata with created.firstName/lastName) accordingly so the promise is no longer silently ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/api/candidates/index.post.ts`:
- Around line 47-54: The call to recordActivity in the candidate creation
handler is currently unhandled; make the async intent explicit by either
awaiting recordActivity(...) if activity recording must complete before
responding, or prefixing with void recordActivity(...) if it should run
fire-and-forget—update the invocation in the POST handler (the recordActivity
call that uses created.id and metadata with created.firstName/lastName)
accordingly so the promise is no longer silently ignored.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/api/applications/index.post.tsserver/api/candidates/[id]/documents/index.post.tsserver/api/candidates/index.post.tsserver/api/jobs/index.post.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/api/applications/index.post.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
server/utils/auth.ts (1)
101-113:⚠️ Potential issue | 🟠 MajorInvitation handler still leaks sensitive data and silently skips real delivery.
Line 106 through Line 112 logs raw PII plus the full invitation link/token, and Line 104 confirms there is no actual provider yet. In non-dev environments, this is both a compliance/security risk and a reliability gap for invites.
🔧 Suggested patch (dev-only logging + fail-fast outside dev)
async sendInvitationEmail(data) { const inviteLink = `${baseURL}/auth/accept-invitation/${data.id}` + const isDev = import.meta.dev - // TODO: Wire up a real email provider (Resend, SES, etc.) - // For now, log the invitation link so dev/testing works. - console.info( - `[Reqcore] Invitation email → ${data.email} | ` + - `Invited by ${data.inviter.user.name} (${data.inviter.user.email}) | ` + - `Org: ${data.organization.name} | ` + - `Role: ${data.role} | ` + - `Link: ${inviteLink}`, - ) + // TODO: Wire up a real email provider (Resend, SES, etc.) + if (isDev) { + console.info('[Reqcore] Invitation email (dev only)', { + invitationId: data.id, + role: data.role, + inviteLink, + }) + return + } + + // Avoid silent success until provider integration exists. + throw new Error('Invitation email provider is not configured') },If useful, I can draft a concrete Resend/SES implementation next.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/auth.ts` around lines 101 - 113, The sendInvitationEmail method currently logs raw PII and the full invite link and silently skips delivery; update it to (1) use an injected email provider abstraction (e.g., an emailProvider/send method) and call that to send the real message from sendInvitationEmail, (2) if no provider is configured, throw or return an explicit error (fail-fast) rather than silently logging in non-dev environments, and (3) restrict any local logging to development only and redact PII (mask data.email and omit the full token/link or replace with a truncated placeholder) so that the function name sendInvitationEmail, baseURL usage, and data.inviter.user / data.organization fields are preserved but not exposed in logs.app/composables/usePermission.ts (1)
45-57:⚠️ Potential issue | 🟠 MajorPrevent stale role state and out-of-order overwrites during org switches.
fetchRole()currently leaves the previous role intact on error and has no request sequencing. Rapid org changes can apply an older response after a newer one.🔧 Proposed fix
+let fetchSeq = 0 + async function fetchRole() { - const { data, error } = await authClient.organization.getActiveMemberRole() - if (!error) { - role.value = data?.role ?? null - } + const seq = ++fetchSeq + role.value = null + try { + const { data, error } = await authClient.organization.getActiveMemberRole() + if (seq !== fetchSeq) return + role.value = error ? null : (data?.role ?? null) + } + catch { + if (seq === fetchSeq) role.value = null + } } @@ watch( () => activeOrgState.value.data?.id, - () => fetchRole(), + () => { void fetchRole() }, { immediate: true }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/usePermission.ts` around lines 45 - 57, fetchRole leaves the prior role on error and allows out-of-order responses to overwrite newer state; modify fetchRole and the watcher so that on any failure you explicitly clear role.value (set to null) and add simple request sequencing: maintain a request counter/token (e.g., currentRequestId) that you increment before each authClient.organization.getActiveMemberRole() call and capture in the closure, then only assign role.value = data?.role ?? null if the captured token matches the latest currentRequestId; update both fetchRole and the watch (watching activeOrgState.value.data?.id) to use this token logic to prevent stale/older responses from applying.
🧹 Nitpick comments (2)
app/pages/dashboard/jobs/[id]/index.vue (1)
24-24: Consider moving import to the top of the script block.The import statement is placed mid-script under a section comment. For consistency and readability, group it with the other imports at lines 2-4.
♻️ Suggested refactor
Move the import to the top of the file:
<script setup lang="ts"> import { Pencil, Trash2, MapPin, Clock, Calendar, UserPlus } from 'lucide-vue-next' import { z } from 'zod' import { usePreviewReadOnly } from '~/composables/usePreviewReadOnly' +import { JOB_STATUS_TRANSITIONS } from '~~/shared/status-transitions' definePageMeta({Then remove it from line 24:
// ───────────────────────────────────────────── // Status transitions // ───────────────────────────────────────────── -import { JOB_STATUS_TRANSITIONS } from '~~/shared/status-transitions' const transitionLabels: Record<string, string> = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/jobs/`[id]/index.vue at line 24, Move the mid-script import of JOB_STATUS_TRANSITIONS into the main import group at the top of the script block so all imports are consistently grouped (i.e., place "import { JOB_STATUS_TRANSITIONS } from '~~/shared/status-transitions'" with the existing imports near the top and remove the duplicate statement from line 24); ensure there are no leftover comments or references that assume the old location and that any linter/formatter rules still pass after relocating the import.app/pages/dashboard/applications/[id].vue (1)
27-27: Consider moving import to the top of the script block.Same suggestion as the jobs page — group this import with the others at lines 2-3 for consistency.
♻️ Suggested refactor
Move the import to the top of the file:
<script setup lang="ts"> import { ArrowLeft, User, Briefcase, Calendar, Clock, Hash, FileText, MessageSquare } from 'lucide-vue-next' import { usePreviewReadOnly } from '~/composables/usePreviewReadOnly' +import { APPLICATION_STATUS_TRANSITIONS } from '~~/shared/status-transitions' definePageMeta({Then remove it from line 27:
// ───────────────────────────────────────────── // Status transitions // ───────────────────────────────────────────── -import { APPLICATION_STATUS_TRANSITIONS } from '~~/shared/status-transitions' const transitionLabels: Record<string, string> = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/applications/`[id].vue at line 27, The import APPLICATION_STATUS_TRANSITIONS is placed mid-script; move the import statement "import { APPLICATION_STATUS_TRANSITIONS } from '~~/shared/status-transitions'" up into the main import group at the top of the script block (group with the other imports) and remove the duplicate from line 27 so the script imports are consistent and linting/order is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/composables/usePermission.ts`:
- Around line 45-57: fetchRole leaves the prior role on error and allows
out-of-order responses to overwrite newer state; modify fetchRole and the
watcher so that on any failure you explicitly clear role.value (set to null) and
add simple request sequencing: maintain a request counter/token (e.g.,
currentRequestId) that you increment before each
authClient.organization.getActiveMemberRole() call and capture in the closure,
then only assign role.value = data?.role ?? null if the captured token matches
the latest currentRequestId; update both fetchRole and the watch (watching
activeOrgState.value.data?.id) to use this token logic to prevent stale/older
responses from applying.
In `@server/utils/auth.ts`:
- Around line 101-113: The sendInvitationEmail method currently logs raw PII and
the full invite link and silently skips delivery; update it to (1) use an
injected email provider abstraction (e.g., an emailProvider/send method) and
call that to send the real message from sendInvitationEmail, (2) if no provider
is configured, throw or return an explicit error (fail-fast) rather than
silently logging in non-dev environments, and (3) restrict any local logging to
development only and redact PII (mask data.email and omit the full token/link or
replace with a truncated placeholder) so that the function name
sendInvitationEmail, baseURL usage, and data.inviter.user / data.organization
fields are preserved but not exposed in logs.
---
Nitpick comments:
In `@app/pages/dashboard/applications/`[id].vue:
- Line 27: The import APPLICATION_STATUS_TRANSITIONS is placed mid-script; move
the import statement "import { APPLICATION_STATUS_TRANSITIONS } from
'~~/shared/status-transitions'" up into the main import group at the top of the
script block (group with the other imports) and remove the duplicate from line
27 so the script imports are consistent and linting/order is preserved.
In `@app/pages/dashboard/jobs/`[id]/index.vue:
- Line 24: Move the mid-script import of JOB_STATUS_TRANSITIONS into the main
import group at the top of the script block so all imports are consistently
grouped (i.e., place "import { JOB_STATUS_TRANSITIONS } from
'~~/shared/status-transitions'" with the existing imports near the top and
remove the duplicate statement from line 24); ensure there are no leftover
comments or references that assume the old location and that any
linter/formatter rules still pass after relocating the import.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/components/CandidateDetailSidebar.vueapp/composables/usePermission.tsapp/pages/dashboard/applications/[id].vueapp/pages/dashboard/jobs/[id]/index.vueapp/pages/dashboard/jobs/[id]/pipeline.vueapp/pages/dashboard/jobs/[id]/swipe.vueapp/pages/jobs/[slug]/index.vueapp/pages/jobs/index.vueapp/utils/auth-client.tsserver/utils/auth.tsserver/utils/requirePermission.tsserver/utils/schemas/application.tsserver/utils/schemas/job.tsshared/status-transitions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/utils/requirePermission.ts
…pdate job search to prevent pattern injection
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/api/comments/[id].delete.ts (1)
46-46:⚠️ Potential issue | 🟠 MajorScope the DELETE statement to organization in the same mutation.
At Line 46, deletion is still keyed only by
id. Keep tenant scoping in the mutation itself and assert a row was actually deleted to avoid check/delete race gaps.🔧 Proposed fix
- await db.delete(comment).where(eq(comment.id, id)) + const [deleted] = await db.delete(comment) + .where(and( + eq(comment.id, id), + eq(comment.organizationId, orgId), + )) + .returning({ id: comment.id }) + + if (!deleted) { + throw createError({ statusCode: 404, statusMessage: 'Comment not found' }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/comments/`[id].delete.ts at line 46, Scope the DELETE to the tenant by adding an organization/tenant predicate to the same where clause (e.g. change db.delete(comment).where(eq(comment.id, id)) to include eq(comment.organizationId, organizationId) or the appropriate orgId from context) and capture the delete result; assert that a row was actually deleted (check result.rowCount / affectedRows / numDeleted depending on your DB client) and throw an appropriate error (404 or permission error) if nothing was deleted to avoid check/delete race conditions.
🧹 Nitpick comments (1)
server/api/public/jobs/index.get.ts (1)
38-40: Consider centralizing LIKE escaping in a small helper.The same escaping logic now exists in two branches; extracting it reduces drift risk and keeps behavior consistent across filters.
♻️ Suggested refactor
+const escapeLikePattern = (value: string) => value.replace(/[%_\\]/g, '\\$&') + export default defineEventHandler(async (event) => { const query = await getValidatedQuery(event, publicJobsQuerySchema.parse) @@ if (query.search) { - const escaped = query.search.replace(/[%_\\]/g, '\\$&') + const escaped = escapeLikePattern(query.search) const pattern = `%${escaped}%` @@ if (query.location) { - const escapedLoc = query.location.replace(/[%_\\]/g, '\\$&') + const escapedLoc = escapeLikePattern(query.location) conditions.push(ilike(job.location, `%${escapedLoc}%`)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/public/jobs/index.get.ts` around lines 38 - 40, Extract the regex-based LIKE-escaping into a small helper (e.g., escapeLike or escapeLikeMeta) and replace the inline usage where you build the condition (the current const escapedLoc = query.location.replace(/[%_\\]/g, '\\$&') and its sibling branch) with calls to that helper, then use ilike(job.location, `%${escapeLike(query.location)}%`) to keep behavior consistent; define the helper near the top of the module (or in a shared util) and ensure it accepts a string, returns the escaped string, and is used in both branches that build the ilike condition for job.location.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/api/public/jobs/`[slug]/apply.post.ts:
- Around line 105-112: Replace the unchecked Zod parse with a safeParse and
throw a 400 createError on validation failure: call
publicApplicationSchema.safeParse(...) using the same prepared input (the object
currently passed to parse) and if result.success is false, throw createError({
statusCode: 400, message: result.error.message }) so the multipart path mirrors
the JSON path's explicit 400 handling; update uses of validated to use
result.data when success is true.
---
Duplicate comments:
In `@server/api/comments/`[id].delete.ts:
- Line 46: Scope the DELETE to the tenant by adding an organization/tenant
predicate to the same where clause (e.g. change
db.delete(comment).where(eq(comment.id, id)) to include
eq(comment.organizationId, organizationId) or the appropriate orgId from
context) and capture the delete result; assert that a row was actually deleted
(check result.rowCount / affectedRows / numDeleted depending on your DB client)
and throw an appropriate error (404 or permission error) if nothing was deleted
to avoid check/delete race conditions.
---
Nitpick comments:
In `@server/api/public/jobs/index.get.ts`:
- Around line 38-40: Extract the regex-based LIKE-escaping into a small helper
(e.g., escapeLike or escapeLikeMeta) and replace the inline usage where you
build the condition (the current const escapedLoc =
query.location.replace(/[%_\\]/g, '\\$&') and its sibling branch) with calls to
that helper, then use ilike(job.location, `%${escapeLike(query.location)}%`) to
keep behavior consistent; define the helper near the top of the module (or in a
shared util) and ensure it accepts a string, returns the escaped string, and is
used in both branches that build the ilike condition for job.location.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/api/candidates/index.get.tsserver/api/comments/[id].delete.tsserver/api/public/jobs/[slug]/apply.post.tsserver/api/public/jobs/index.get.tsshared/permissions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/api/candidates/index.get.ts
Comment on lines
+105
to
+112
| const validated = publicApplicationSchema.parse({ | ||
| firstName: fields.firstName?.trim() ?? '', | ||
| lastName: fields.lastName?.trim() ?? '', | ||
| email: fields.email?.trim() ?? '', | ||
| phone: fields.phone?.trim() || undefined, | ||
| website: fields.website || undefined, | ||
| responses: rawResponses, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for ZodError handling patterns in the codebase
echo "=== Checking ZodError handling patterns ==="
rg -n --type=ts "ZodError|instanceof\s+ZodError" server utils || echo "No explicit ZodError handling found"
echo ""
echo "=== Checking for createError with 400 status ==="
rg -n --type=ts "createError\(\{\s*statusCode:\s*400" server || echo "No createError 400 patterns found"
echo ""
echo "=== Checking safeParse usage ==="
rg -n --type=ts "safeParse\(" server/api/public || echo "No safeParse in public API found"
echo ""
echo "=== Checking .parse() usage in public API ==="
rg -n --type=ts "\.parse\(" server/api/public/jobsRepository: reqcore-inc/reqcore
Length of output: 1794
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at the target file to understand context
echo "=== Target file content (lines 90-120) ==="
wc -l server/api/public/jobs/[slug]/apply.post.ts
head -120 "server/api/public/jobs/[slug]/apply.post.ts" | tail -35Repository: reqcore-inc/reqcore
Length of output: 1194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for global error handlers or middleware that might catch ZodError
echo "=== Looking for error handling middleware ==="
fd -e ts -e js server | xargs grep -l "ZodError\|error.*handler\|middleware" 2>/dev/null | head -20
# Check if there's a validated-body helper in utils
echo ""
echo "=== Looking for validated-body or similar helpers ==="
rg -n "validated.*body|parseBody|validateBody" server utils --type tsRepository: reqcore-inc/reqcore
Length of output: 110
Use safeParse with explicit createError(400) for Zod validation in multipart path.
The multipart validation at line 105 uses publicApplicationSchema.parse() without error handling, which will throw an uncaught ZodError and bubble as a 500. This is inconsistent with the same endpoint's JSON parsing (line 98–100), which wraps errors in a try-catch and throws createError({statusCode: 400}). The codebase shows no global middleware catching ZodError, so explicit error handling is required.
Proposed fix
- const validated = publicApplicationSchema.parse({
+ const parsed = publicApplicationSchema.safeParse({
firstName: fields.firstName?.trim() ?? '',
lastName: fields.lastName?.trim() ?? '',
email: fields.email?.trim() ?? '',
phone: fields.phone?.trim() || undefined,
website: fields.website || undefined,
responses: rawResponses,
})
+ if (!parsed.success) {
+ throw createError({
+ statusCode: 400,
+ statusMessage: 'Invalid application payload',
+ data: parsed.error.flatten(),
+ })
+ }
+ const validated = parsed.data📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const validated = publicApplicationSchema.parse({ | |
| firstName: fields.firstName?.trim() ?? '', | |
| lastName: fields.lastName?.trim() ?? '', | |
| email: fields.email?.trim() ?? '', | |
| phone: fields.phone?.trim() || undefined, | |
| website: fields.website || undefined, | |
| responses: rawResponses, | |
| }) | |
| const parsed = publicApplicationSchema.safeParse({ | |
| firstName: fields.firstName?.trim() ?? '', | |
| lastName: fields.lastName?.trim() ?? '', | |
| email: fields.email?.trim() ?? '', | |
| phone: fields.phone?.trim() || undefined, | |
| website: fields.website || undefined, | |
| responses: rawResponses, | |
| }) | |
| if (!parsed.success) { | |
| throw createError({ | |
| statusCode: 400, | |
| statusMessage: 'Invalid application payload', | |
| data: parsed.error.flatten(), | |
| }) | |
| } | |
| const validated = parsed.data |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/api/public/jobs/`[slug]/apply.post.ts around lines 105 - 112, Replace
the unchecked Zod parse with a safeParse and throw a 400 createError on
validation failure: call publicApplicationSchema.safeParse(...) using the same
prepared input (the object currently passed to parse) and if result.success is
false, throw createError({ statusCode: 400, message: result.error.message }) so
the multipart path mirrors the JSON path's explicit 400 handling; update uses of
validated to use result.data when success is true.
…rch functionality, and improved role checks
There was a problem hiding this comment.
♻️ Duplicate comments (4)
app/pages/dashboard/settings/members.vue (2)
515-515:⚠️ Potential issue | 🟡 MinorClear
removeErrorwhen closing the modal via backdrop.Line 515 closes the modal but keeps previous error text, so stale errors can reappear on reopen.
🔧 Proposed fix
- <div v-if="memberToRemove" class="fixed inset-0 z-[100] flex items-center justify-center bg-black/50 backdrop-blur-sm" `@click.self`="memberToRemove = null"> + <div + v-if="memberToRemove" + class="fixed inset-0 z-[100] flex items-center justify-center bg-black/50 backdrop-blur-sm" + `@click.self`="memberToRemove = null; removeError = ''" + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/settings/members.vue` at line 515, The modal backdrop click currently sets memberToRemove = null but leaves stale removeError text; update the backdrop close handler (the element with `@click.self` on the div that checks memberToRemove) to also clear removeError (e.g., set removeError = null when closing via backdrop). Ensure any other backdrop/close paths that set memberToRemove (such as cancel buttons or programmatic closes) also clear removeError to avoid showing previous errors on reopen.
275-280:⚠️ Potential issue | 🟠 MajorAdd accessible names to icon-only action buttons.
Line 275, Line 340, and Line 432 still expose icon-only buttons without accessible names, so screen readers won’t announce the action context.
♿ Proposed fix
<button class="p-1 rounded-md text-surface-400 hover:text-surface-600 dark:hover:text-surface-300 hover:bg-surface-100 dark:hover:bg-surface-800 transition-colors" + aria-label="Close invite form" `@click`="showInviteForm = false; resetInviteForm()" > <X class="size-4" /> </button> @@ - <button class="text-danger-500 hover:text-danger-700 transition-colors" `@click`="roleUpdateError = ''"> + <button + class="text-danger-500 hover:text-danger-700 transition-colors" + aria-label="Dismiss role update error" + `@click`="roleUpdateError = ''" + > <X class="size-4" /> </button> @@ <button class="p-1.5 rounded-md text-surface-400 hover:text-surface-600 dark:hover:text-surface-300 hover:bg-surface-100 dark:hover:bg-surface-800 transition-colors" + :aria-label="`Open actions for ${m.user.name}`" `@click.stop`="toggleDropdown(m.id)" > <MoreHorizontal class="size-4" /> </button>Also applies to: 340-342, 432-437
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/settings/members.vue` around lines 275 - 280, Several icon-only action buttons (the close button that sets showInviteForm = false and calls resetInviteForm(), plus the other icon-only buttons around the member actions using the X and similar icon components) lack accessible names; add an accessible name by either adding an aria-label/aria-labelledby attribute with a concise description (e.g., aria-label="Close invite form" for the X button, or aria-label="Remove member" / "Edit member" for the member action buttons) or include a visually-hidden label element (class="sr-only") inside the button. Update the specific buttons that render the X icon and the other icon-only actions so screen readers receive a clear action description while preserving existing click handlers (e.g., keep showInviteForm and resetInviteForm calls intact).app/pages/dashboard/settings/index.vue (1)
62-70:⚠️ Potential issue | 🔴 CriticalHandle Better Auth
{ error }results before success/redirect.Line 68 and Line 104 currently assume throws-only failure handling. If these calls resolve with
{ error }, you can show “Changes saved” or navigate away after a failed operation.🔧 Proposed fix
try { - await authClient.organization.update({ + const result = await authClient.organization.update({ data: { name: trimmedName, slug: trimmedSlug, }, }) + if (result.error) { + throw new Error(String(result.error.message ?? 'Failed to update organization')) + } saveSuccess.value = true setTimeout(() => { saveSuccess.value = false }, 3000) } @@ try { - await authClient.organization.delete({ + const result = await authClient.organization.delete({ organizationId: activeOrg.value!.id, }) + if (result.error) { + throw new Error(String(result.error.message ?? 'Failed to delete organization')) + } // Use navigateTo for Nuxt-managed navigation, then force reload // so all cached org state is cleared. const localePath = useLocalePath() await navigateTo(localePath('/onboarding/create-org'), { external: true })For Better Auth organization client docs: do `organization.update(...)` and `organization.delete(...)` throw on failure, or return a resolved object containing `{ error }`? Please include the current official doc links.Also applies to: 98-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/settings/index.vue` around lines 62 - 70, The update and delete calls (authClient.organization.update and authClient.organization.delete) currently assume failures will throw and immediately set saveSuccess or navigate; instead capture the resolved response and check for a returned error property before marking success or redirecting—if response.error exists, set an error state (e.g., saveError or deleteError), do not set saveSuccess or navigate, and surface the error message to the user; update the code paths in the save/update handler and deleteOrganization handler to inspect the response object and only set saveSuccess or perform router.push when response.error is falsy.app/composables/usePermission.ts (1)
45-53:⚠️ Potential issue | 🟠 MajorDe-race role fetching across rapid org switches.
Line 49 can resolve after a newer org switch and overwrite
rolewith stale data; rejected calls also aren’t handled. This is the same unresolved race noted earlier.🔧 Proposed fix
export function usePermission(permissions: PermissionRequest) { const role = ref<string | null>(null) + let fetchSeq = 0 // Fetch the active member's role and re-fetch when org changes const activeOrgState = authClient.useActiveOrganization() async function fetchRole() { - // Reset immediately to avoid stale role from previous org (race condition) + const seq = ++fetchSeq role.value = null - const { data, error } = await authClient.organization.getActiveMemberRole() - if (!error) { - role.value = data?.role ?? null + try { + const { data, error } = await authClient.organization.getActiveMemberRole() + if (seq !== fetchSeq) return + role.value = error ? null : (data?.role ?? null) + } + catch { + if (seq === fetchSeq) role.value = null } } @@ if (import.meta.client) { watch( () => activeOrgState.value.data?.id, - () => fetchRole(), + () => { void fetchRole() }, { immediate: true }, ) }Also applies to: 59-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/usePermission.ts` around lines 45 - 53, fetchRole currently races: an earlier fetch can resolve after a newer org switch and overwrite role.value, and rejected calls aren't handled; fix by adding a per-call identifier (e.g., a closure-scoped incrementing fetchId or token) that you increment right before calling authClient.organization.getActiveMemberRole(), capture in the local scope, and after the await only assign role.value if the captured id matches the latest id; also properly handle rejections by checking the error result and not touching role.value on error (and optionally log the error). Apply the identical pattern to the other fetch block at lines 59-63 so both fetches (the one using authClient.organization.getActiveMemberRole and the similar membership fetch) ignore stale or failed responses.
🧹 Nitpick comments (1)
app/pages/dashboard/settings/members.vue (1)
522-529: Add dialog semantics to the remove-member modal.The modal lacks explicit dialog semantics, which hurts assistive-tech navigation/context.
♿ Proposed refactor
- <div v-if="memberToRemove" class="w-full max-w-md bg-white dark:bg-surface-900 rounded-2xl border border-surface-200 dark:border-surface-800 shadow-2xl p-6"> + <div + v-if="memberToRemove" + role="dialog" + aria-modal="true" + aria-labelledby="remove-member-title" + class="w-full max-w-md bg-white dark:bg-surface-900 rounded-2xl border border-surface-200 dark:border-surface-800 shadow-2xl p-6" + > @@ - <h3 class="text-base font-semibold text-surface-900 dark:text-surface-100">Remove member</h3> + <h3 id="remove-member-title" class="text-base font-semibold text-surface-900 dark:text-surface-100">Remove member</h3>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/settings/members.vue` around lines 522 - 529, The remove-member modal currently controlled by memberToRemove needs proper dialog semantics: add role="dialog" and aria-modal="true" to the modal container div, ensure the heading element used ("Remove member" h3) has a unique id (e.g., removeMemberTitle) and set aria-labelledby on the dialog to that id, add aria-describedby referencing the paragraph under the heading (assign it a unique id like removeMemberDesc), make the modal container focusable (tabindex="-1") and programmatically move focus into it when memberToRemove becomes truthy, and ensure the modal close control has an appropriate aria-label so screen readers can close the dialog.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/composables/usePermission.ts`:
- Around line 45-53: fetchRole currently races: an earlier fetch can resolve
after a newer org switch and overwrite role.value, and rejected calls aren't
handled; fix by adding a per-call identifier (e.g., a closure-scoped
incrementing fetchId or token) that you increment right before calling
authClient.organization.getActiveMemberRole(), capture in the local scope, and
after the await only assign role.value if the captured id matches the latest id;
also properly handle rejections by checking the error result and not touching
role.value on error (and optionally log the error). Apply the identical pattern
to the other fetch block at lines 59-63 so both fetches (the one using
authClient.organization.getActiveMemberRole and the similar membership fetch)
ignore stale or failed responses.
In `@app/pages/dashboard/settings/index.vue`:
- Around line 62-70: The update and delete calls (authClient.organization.update
and authClient.organization.delete) currently assume failures will throw and
immediately set saveSuccess or navigate; instead capture the resolved response
and check for a returned error property before marking success or redirecting—if
response.error exists, set an error state (e.g., saveError or deleteError), do
not set saveSuccess or navigate, and surface the error message to the user;
update the code paths in the save/update handler and deleteOrganization handler
to inspect the response object and only set saveSuccess or perform router.push
when response.error is falsy.
In `@app/pages/dashboard/settings/members.vue`:
- Line 515: The modal backdrop click currently sets memberToRemove = null but
leaves stale removeError text; update the backdrop close handler (the element
with `@click.self` on the div that checks memberToRemove) to also clear
removeError (e.g., set removeError = null when closing via backdrop). Ensure any
other backdrop/close paths that set memberToRemove (such as cancel buttons or
programmatic closes) also clear removeError to avoid showing previous errors on
reopen.
- Around line 275-280: Several icon-only action buttons (the close button that
sets showInviteForm = false and calls resetInviteForm(), plus the other
icon-only buttons around the member actions using the X and similar icon
components) lack accessible names; add an accessible name by either adding an
aria-label/aria-labelledby attribute with a concise description (e.g.,
aria-label="Close invite form" for the X button, or aria-label="Remove member" /
"Edit member" for the member action buttons) or include a visually-hidden label
element (class="sr-only") inside the button. Update the specific buttons that
render the X icon and the other icon-only actions so screen readers receive a
clear action description while preserving existing click handlers (e.g., keep
showInviteForm and resetInviteForm calls intact).
---
Nitpick comments:
In `@app/pages/dashboard/settings/members.vue`:
- Around line 522-529: The remove-member modal currently controlled by
memberToRemove needs proper dialog semantics: add role="dialog" and
aria-modal="true" to the modal container div, ensure the heading element used
("Remove member" h3) has a unique id (e.g., removeMemberTitle) and set
aria-labelledby on the dialog to that id, add aria-describedby referencing the
paragraph under the heading (assign it a unique id like removeMemberDesc), make
the modal container focusable (tabindex="-1") and programmatically move focus
into it when memberToRemove becomes truthy, and ensure the modal close control
has an appropriate aria-label so screen readers can close the dialog.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/composables/usePermission.tsapp/pages/dashboard/settings.vueapp/pages/dashboard/settings/account.vueapp/pages/dashboard/settings/index.vueapp/pages/dashboard/settings/members.vue
🚧 Files skipped from review as they are similar to previous changes (2)
- app/pages/dashboard/settings.vue
- app/pages/dashboard/settings/account.vue
…and cancellation functionality
- Created `inviteLink.ts` schema for generating shareable invite links with role, max uses, and expiry duration. - Created `joinRequest.ts` schema for submitting join requests to organizations and searching organizations by slug.
…ing, creating, and revoking links
…ipeline stages and integrate pipeline data into job listings
…links - Updated the jobs index page to redirect to the main dashboard, maintaining old bookmarks and links. - Adjusted navigation links in the new job creation page to point to the main dashboard instead of the jobs section.
…e design adjustments
… with improved responsiveness and visual hierarchy
…yout for better user experience
…enhance layout for improved user experience
…oved scrollable layout
… to dashboard after job creation
This was referenced Mar 10, 2026
9 tasks
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
Type of change
Validation
DCO
Signed-off-by) viagit commit -sSummary by CodeRabbit
New Features
Security / Access Control
Database
Documentation & Testing