Fix/broken authorization #1297
Fix/broken authorization #1297
Conversation
WalkthroughAdds a centralized Security model and middleware, enforces explicit event.locals.security.require*() checks across many server routes, refactors role and permission helpers to use Security, introduces an ESLint rule requiring these security calls, updates hooks/error handling, and adjusts UI and public endpoints accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Router as SvelteKit Router
participant Hooks as hooks.server.ts
participant Security as populateSecurityInfo
participant Handler as Route Handler
participant DB as DatabaseReads
Client->>Router: HTTP request
Router->>Hooks: handle(event)
activate Hooks
Hooks->>Security: populateSecurityInfo(event)
activate Security
Security->>DB: load session, memberships, roles
DB-->>Security: session & role data
Security-->>Hooks: set event.locals.security
deactivate Security
Hooks-->>Router: continue
deactivate Hooks
Router->>Handler: execute load/action/GET
activate Handler
Handler->>Handler: event.locals.security.require*(...)
alt Authorized
Handler->>DB: query / mutate
DB-->>Handler: result
Handler-->>Client: 2xx response
else Not authorized
Handler-->>Client: 4xx/5xx error (thrown by require*)
end
deactivate Handler
sequenceDiagram
autonumber
participant Dev
participant ESLint
participant Rule as require-security-check
Dev->>ESLint: run lint
ESLint->>Rule: analyze exported server handlers
Rule->>Rule: search for event.locals.security.require*() in handler bodies
alt missing call
Rule-->>Dev: diagnostic + suggestions (requireAuthenticated / requireNothing) with autofix
else present
Rule-->>Dev: OK
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120–180 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
src/routes/(unauthenticated)/api/products/[product_id]/files/published/[type]/+server.ts (1)
14-33: Add locals.security.requireNothing() to exported route handlers (lint will fail otherwise)Add
localsto the handler signature and calllocals.security.requireNothing().-export async function HEAD({ params, request }) { +export async function HEAD({ params, request, locals }) { + locals.security.requireNothing(); const ifModifiedSince = request.headers.get('If-Modified-Since') ?? ''; const res = await _getHeaders(params.product_id, params.type); if (!res) return error(404);Verification run (script emitted
awk: ... division by zero attempted) flagged these handlers as also missing thelocals.securitycall:
- src/routes/(authenticated)/tasks/sse/+server.ts:5
- src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts:6
- src/routes/(unauthenticated)/api/products/[package]/published/+server.ts:15
- src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts:16
- src/routes/(unauthenticated)/api/products/[product_id]/files/published/[type]/+server.ts:5
- src/routes/(unauthenticated)/api/products/[product_id]/files/published/[type]/+server.ts:14
Fix the handlers above (add
localsand calllocals.security.requireNothing()) and re-run verification.src/routes/(authenticated)/workflow-instances/[product_id]/+page.server.ts (1)
48-49: Throw HttpErrors — use throw error(...), not return error(...)Returning error(...) in SvelteKit server code won’t produce the intended HTTP response; throw error(status) to abort and set the status.
File: src/routes/(authenticated)/workflow-instances/[product_id]/+page.server.ts — lines 48, 53, 111
Apply:
- if (!product) return error(404); + if (!product) throw error(404); @@ - if (!(flow && snap)) return error(404); + if (!(flow && snap)) throw error(404); @@ - if (!QueueConnected()) return error(503); + if (!QueueConnected()) throw error(503);src/routes/(authenticated)/admin/settings/organizations/edit/+page.server.ts (1)
27-40: Avoid echoing secrets into forms.Prefilling BuildEngineApiAccessToken exposes a secret in HTML. Prefer leaving it blank and only updating when a new value is submitted.
const form = await superValidate( { id: data.Id, name: data.Name, contact: data.ContactEmail, websiteUrl: data.WebsiteUrl, buildEngineUrl: data.BuildEngineUrl, - buildEngineApiAccessToken: data.BuildEngineApiAccessToken, + // Do not prefill secrets; leave blank, update only if provided on submit + buildEngineApiAccessToken: '', logoUrl: data.LogoUrl, useDefaultBuildEngine: data.UseDefaultBuildEngine ?? true, publicByDefault: data.PublicByDefault ?? false }, valibot(editSchema) );src/routes/(authenticated)/organizations/[id=idNumber]/settings/groups/+page.server.ts (2)
1-2: Incorrect import source for fail.fail must come from @sveltejs/kit, not sveltekit-superforms. Current import will break on runtime.
-import { fail, superValidate } from 'sveltekit-superforms'; +import { fail } from '@sveltejs/kit'; +import { superValidate } from 'sveltekit-superforms';
1-48: Replacefailimports from 'sveltekit-superforms' withfailfrom '@sveltejs/kit'Multiple files import
failfrom 'sveltekit-superforms' — change toimport { fail } from '@sveltejs/kit'. Update the following files:
- src/routes/(unauthenticated)/request-access-for-organization/+page.server.ts
- src/routes/(authenticated)/workflow-instances/+page.server.ts
- src/routes/(authenticated)/workflow-instances/[product_id]/+page.server.ts
- src/routes/(authenticated)/tasks/[product_id]/+page.server.ts
- src/routes/(authenticated)/users/[id=idNumber]/settings/profile/+page.server.ts
- src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts
- src/routes/(authenticated)/projects/new/[id=idNumber]/+page.server.ts
- src/routes/(authenticated)/organizations/[id=idNumber]/settings/stores/+page.server.ts
- src/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.server.ts
- src/routes/(authenticated)/organizations/[id=idNumber]/settings/info/+page.server.ts
- src/routes/(authenticated)/organizations/[id=idNumber]/settings/infrastructure/+page.server.ts
- src/routes/(authenticated)/organizations/[id=idNumber]/settings/groups/+page.server.ts
- src/routes/(authenticated)/projects/import/[id=idNumber]/+page.server.ts
- src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts
- src/routes/(authenticated)/projects/[id=idNumber]/edit/+page.server.ts
- src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts
- src/routes/(authenticated)/directory/+page.server.ts
- src/routes/(authenticated)/users/[id=idNumber]/settings/roles/+page.server.ts
src/routes/(authenticated)/users/[id=idNumber]/settings/profile/+page.server.ts (1)
69-87: Replace legacy event.locals.auth() usages with event.locals.securityOnly call event.locals.auth() in src/auth.ts (populateSecurityInfo) where it seeds locals.security; everywhere else read from event.locals.security and use security.userId for “self” checks.
- const user = (await event.locals.auth())!.user; + const actingUserId = event.locals.security.userId; const subjectId = parseInt(event.params.id); @@ - // You cannot change lock state of yourself, and if you are editing someone else, you are either org admin or superadmin - IsLocked: subjectId === user.userId ? undefined : !form.data.active + // You cannot change lock state of yourself. If editing someone else, gate above ensures admin rights. + IsLocked: subjectId === actingUserId ? undefined : !form.data.activeFiles/locations to update (found by the grep you ran):
- src/routes/(authenticated)/users/[id=idNumber]/settings/profile/+page.server.ts — apply diff above (line ~69).
- src/routes/(authenticated)/tasks/+page.server.ts:6 — replace (await event.locals.auth())!.user.userId with event.locals.security.userId.
- src/routes/+layout.server.ts:5 — replace const uData = await event.locals.auth(); return { session: uData! }; with using event.locals.security.
- src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts:15 — replace const session = await event.locals.auth(); with reading event.locals.security (handle undefined).
- src/auth.ts:236 — keep event.locals.auth() here (this is the bootstrap that populates locals.security).
src/routes/(authenticated)/admin/settings/stores/edit/+page.server.ts (1)
28-39: Fix options shape mismatch between stores/new and stores/edit
- Issue: src/routes/(authenticated)/admin/settings/stores/new/+page.server.ts returns options as { storeType: [...] } (lines ~17–19) while the edit flow expects data.options to be an array (src/routes/(authenticated)/admin/settings/stores/edit/+page.svelte — lines ~59–61; edit server returns options as an array at src/.../stores/edit/+page.server.ts — lines ~28–39).
- Action: make the shape consistent — either return options as an array in new (const options = await DatabaseReads.storeTypes.findMany(); return { form, options };) or change the edit consumer to iterate data.options.storeType. Update the server or svelte file(s) accordingly.
src/routes/(unauthenticated)/(auth)/login/+page.server.ts (1)
8-15: Invite cookie ignored for unauthenticated users — move check before auth gate.Currently the inviteToken redirect only runs when the user is already authenticated. That breaks invite flows for users who land on /login with only the cookie set.
Apply this diff:
export const load: PageServerLoad = async (event) => { - event.locals.security.requireNothing(); - if (event.locals.security.userId && QueueConnected()) { - if (event.cookies.get('inviteToken')) { - const inviteToken = event.cookies.get('inviteToken')!; - return redirect(302, localizeHref('/invitations/organization-membership?t=' + inviteToken)); - } - return redirect(302, localizeHref('/tasks')); - } + event.locals.security.requireNothing(); + const inviteToken = event.cookies.get('inviteToken'); + if (inviteToken) { + return redirect(302, localizeHref('/invitations/organization-membership?t=' + inviteToken)); + } + if (event.locals.security.userId && QueueConnected()) { + return redirect(302, localizeHref('/tasks')); + } return { serviceAvailable: QueueConnected() }; }src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (1)
300-319: editOwnerGroup lacks invariants; enforce same‑org group and owner’s membership.Before updating, ensure:
- Group belongs to the project’s org.
- New owner is a member of the chosen group and of the org.
- Optional: prevent cross‑org owner changes.
Apply this diff:
- event.locals.security.requireProjectWriteAccess( - await DatabaseReads.projects.findUniqueOrThrow({ - where: { Id: parseInt(event.params.id) }, - select: { OwnerId: true, OrganizationId: true } - }) - ); + const projectId = parseInt(event.params.id); + if (isNaN(projectId) || projectId + '' !== event.params.id) return fail(404); + const minimal = await DatabaseReads.projects.findUniqueOrThrow({ + where: { Id: projectId }, + select: { OwnerId: true, OrganizationId: true } + }); + event.locals.security.requireProjectWriteAccess(minimal); @@ - const success = await DatabaseWrites.projects.update(projectId, { + // Validate invariants + const groupOk = await DatabaseReads.groups.findFirst({ + where: { Id: form.data.group, OwnerId: minimal.OrganizationId } + }); + const ownerOk = await DatabaseReads.groupMemberships.findFirst({ + where: { GroupId: form.data.group, UserId: form.data.owner } + }); + const ownerInOrg = await DatabaseReads.organizationMemberships.findFirst({ + where: { OrganizationId: minimal.OrganizationId, UserId: form.data.owner } + }); + if (!groupOk || !ownerOk || !ownerInOrg) return fail(403, { form, ok: false }); + + const success = await DatabaseWrites.projects.update(projectId, { GroupId: form.data.group, OwnerId: form.data.owner });
🧹 Nitpick comments (69)
eslint-rules/tsconfig.json (1)
2-7: Tighten TS config for rule authoring (Node types, strictness, and includes).Add Node/Eslint types and enable strict mode to avoid subtle typing gaps in the custom rule; also scope files explicitly.
{ "compilerOptions": { "target": "esnext", "module": "esnext", "moduleResolution": "node", - "skipLibCheck": true + "skipLibCheck": true, + "strict": true, + "lib": ["ES2022"], + "types": ["node", "eslint"] } } +, +"include": ["**/*.ts"]src/routes/(unauthenticated)/api/products/[package]/published/+server.ts (1)
15-20: Add Content-Type for JSON response.Set
application/json; charset=utf-8for clients/CDNs to treat the payload correctly.- return res - ? new Response(JSON.stringify(res), { status: 200 }) - : new Response(null, { status: 404 }); + return res + ? new Response(JSON.stringify(res), { + status: 200, + headers: { 'Content-Type': 'application/json; charset=utf-8' } + }) + : new Response(null, { status: 404 });src/routes/(authenticated)/projects/new/[id=idNumber]/+page.server.ts (2)
12-14: Validate orgId before invoking the role check to avoid NaN propagation.Do the parse/validate first; fail fast on bad params.
-export const load = (async ({ locals, params }) => { - locals.security.requireHasRole(parseInt(params.id), RoleId.AppBuilder, true); +export const load = (async ({ locals, params }) => { + const orgId = parseInt(params.id); + if (isNaN(orgId)) return error(404); + locals.security.requireHasRole(orgId, RoleId.AppBuilder, true);
55-56: Mirror the NaN guard in actions before checks.Minor resilience improvement; prevents odd paths inside
requireHasRole.- event.locals.security.requireHasRole(parseInt(event.params.id), RoleId.AppBuilder, true); - const organizationId = parseInt(event.params.id); + const organizationId = parseInt(event.params.id); + if (isNaN(organizationId)) return error(404); + event.locals.security.requireHasRole(organizationId, RoleId.AppBuilder, true);src/routes/(authenticated)/products/[id]/files/+page.server.ts (1)
82-93: Duplicate authorization fetch in action; factor and 404 early.Same fetch/guard pattern as load; 404 if missing and avoid passing undefined.
Apply:
- page: async ({ request, params, locals }) => { - locals.security.requireProjectWriteAccess( - ( - await DatabaseReads.products.findUnique({ - where: { Id: params.id }, - select: { - Project: { select: { OwnerId: true, OrganizationId: true } } - } - }) - )?.Project - ); + page: async ({ request, params, locals }) => { + const prod = await DatabaseReads.products.findUnique({ + where: { Id: params.id }, + select: { Project: { select: { OwnerId: true, OrganizationId: true } } } + }); + if (!prod?.Project) throw error(404); + locals.security.requireProjectWriteAccess(prod.Project);Optionally extract a small helper to DRY this pattern.
src/lib/projects/components/ProjectActionMenu.svelte (3)
66-66: Passing session.user after security overhaul — verify presence.If session is no longer populated in page.data, these checks will break. Prefer a user field exposed by layout or fall back.
Apply:
- {#if allowActions && canArchive(project, page.data.session.user, parseInt(page.params.id))} + {#if allowActions && canArchive(project, page.data.user ?? page.data.session?.user, parseInt(page.params.id, 10))}Repeat similar adjustment for the calls below.
80-80: Same concern as above for canReactivate; add radix.Apply:
- {#if allowReactivate && canReactivate(project, page.data.session.user, parseInt(page.params.id))} + {#if allowReactivate && canReactivate(project, page.data.user ?? page.data.session?.user, parseInt(page.params.id, 10))}
97-97: canClaimProject call — update arg source + small parseInt nit.Keep arg order; switch to user fallback and explicit radix.
Apply:
- {#if canClaimProject(page.data.session.user, project.OwnerId, orgId, project.GroupId, userGroups)} + {#if canClaimProject(page.data.user ?? page.data.session?.user, project.OwnerId, orgId, project.GroupId, userGroups)}eslint-rules/require-security-check.ts (3)
133-145: Include OPTIONS/HEAD handlers to avoid gapsEndpoint guard misses OPTIONS/HEAD; add them to the verb list.
Apply this diff:
- ['POST', 'GET', 'PUT', 'DELETE', 'PATCH'].includes(node.declaration.id.name) + ['POST', 'GET', 'PUT', 'DELETE', 'PATCH', 'OPTIONS', 'HEAD'].includes(node.declaration.id.name)- ['POST', 'GET', 'PUT', 'DELETE', 'PATCH'].includes(decl.id.name) + ['POST', 'GET', 'PUT', 'DELETE', 'PATCH', 'OPTIONS', 'HEAD'].includes(decl.id.name)
47-66: Reduce false positives from shadowed “locals” identifiersCurrently accepts locals.security.* anywhere; consider restricting to event.locals.security.* or verifying the base chain ends at Identifier('event') when present.
78-79: Docs: mention endpoints in descriptionTiny wording tweak so rule docs reflect +server.ts handlers too.
Apply this diff:
- description: 'Require calling event.locals.security.* in every server load/action' + description: 'Require calling event.locals.security.* in every server load/action and +server.ts endpoint'src/routes/+layout.server.ts (1)
6-6: Avoid non‑null assertion on sessionauth() may return null on public routes; prefer returning the nullable value rather than asserting.
Apply this diff:
- return { session: uData! }; + return { session: uData };src/routes/(authenticated)/organizations/[id=idNumber]/settings/info/+page.server.ts (2)
8-8: Use explicit radix with parseIntPrevents octal quirks and keeps intent clear.
Apply this diff:
- event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + event.locals.security.requireAdminOfOrg(Number.parseInt(event.params.id, 10));
23-23: Consistently parse org id with base 10Minor consistency/readability improvement.
Apply this diff:
- event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + event.locals.security.requireAdminOfOrg(Number.parseInt(event.params.id, 10));- Id: parseInt(event.params.id) + Id: Number.parseInt(event.params.id, 10)Also applies to: 28-28
src/routes/(authenticated)/tasks/+page.server.ts (1)
5-7: Use security.userId instead of calling auth() againYou already required authentication; avoid a second auth() fetch and the non‑null assertion.
Apply this diff:
export const load = (async (event) => { event.locals.security.requireAuthenticated(); - const userTasks = await getUserTasks((await event.locals.auth())!.user.userId); + const userTasks = await getUserTasks(event.locals.security.userId!); return { userTasks }; }) satisfies PageServerLoad;src/routes/(authenticated)/organizations/[id=idNumber]/settings/+page.server.ts (1)
8-9: Use “throw redirect(...)”, not “return redirect(...)”.redirect() throws; returning it is redundant/unreachable and can confuse readers/tools.
- return redirect(308, localizeHref(`/organizations/${event.params.id}/settings/info`)); + throw redirect(308, localizeHref(`/organizations/${event.params.id}/settings/info`));src/routes/(authenticated)/organizations/[id=idNumber]/settings/stores/+page.server.ts (1)
14-14: Parse orgId once; prefer Number() over parseInt.Small cleanup and avoids repeated parsing.
-export const load = (async (event) => { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); +export const load = (async (event) => { + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const { organization } = await event.parent(); … }) satisfies PageServerLoad; export const actions = { async default(event) { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const form = await superValidate(event.request, valibot(toggleStoreSchema)); if (!form.valid) return fail(400, { form, ok: false }); await DatabaseWrites.organizationStores.toggleForOrg( - parseInt(event.params.id), + orgId, form.data.storeId, form.data.enabled );Also applies to: 32-40
src/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.server.ts (1)
18-35: Coalesce orgId parsing and reuse.Minor cleanup for consistency and readability.
-export const load = (async (event) => { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); +export const load = (async (event) => { + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const { organization } = await event.parent(); const setOrgProductDefs = new Set( ( await DatabaseReads.organizationProductDefinitions.findMany({ where: { - OrganizationId: organization.Id + OrganizationId: organization.Id } }) ).map((p) => p.ProductDefinitionId) ); … }) satisfies PageServerLoad; export const actions = { async togglePublic(event) { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const form = await superValidate(event.request, valibot(togglePublicSchema)); if (!form.valid) return fail(400, { form, ok: false }); await DatabaseWrites.organizations.update({ where: { - Id: parseInt(event.params.id) + Id: orgId }, data: { PublicByDefault: form.data.publicByDefault } }); return { form, ok: true }; }, async toggleProduct(event) { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const form = await superValidate(event.request, valibot(toggleProductSchema)); if (!form.valid) return fail(400, { form, ok: false }); await DatabaseWrites.organizationProductDefinitions.toggleForOrg( - parseInt(event.params.id), + orgId, form.data.prodDefId, form.data.enabled ); return { form, ok: true }; } }Also applies to: 39-51, 53-64
src/routes/(authenticated)/organizations/[id=idNumber]/settings/infrastructure/+page.server.ts (1)
8-19: Coalesce orgId parsing and reuse.Minor consistency improvement; reduces repetition.
-export const load = (async (event) => { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); +export const load = (async (event) => { + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const { organization } = await event.parent(); const form = await superValidate( { buildEngineUrl: organization.BuildEngineUrl, buildEngineApiAccessToken: organization.BuildEngineApiAccessToken, useDefaultBuildEngine: organization.UseDefaultBuildEngine ?? false }, valibot(infrastructureSchema) ); return { form }; }) satisfies PageServerLoad; export const actions = { async default(event) { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const form = await superValidate(event.request, valibot(infrastructureSchema)); if (!form.valid) return fail(400, { form, ok: false }); await DatabaseWrites.organizations.update({ where: { - Id: parseInt(event.params.id) + Id: orgId }, data: { BuildEngineApiAccessToken: form.data.buildEngineApiAccessToken, BuildEngineUrl: form.data.buildEngineUrl, UseDefaultBuildEngine: form.data.useDefaultBuildEngine } }); return { form, ok: true }; } } satisfies Actions;Also applies to: 23-38
src/lib/server/bullmq/BullBoard.ts (1)
40-42: Guard bullboard nullity and remove non-null assertion; security ordering is already correct.Security/auth (populateSecurityInfo via authSequence) runs before bullboardHandle in src/hooks.server.ts — only change needed is to check bullboard truthiness instead of using the non-null assertion.
-export const bullboardHandle: Handle = async ({ event, resolve }) => { - if (event.url.pathname.match(/^\/admin\/jobs($|\/)/) && event.locals.security.isSuperAdmin) { - return bullboard!.fetch(event.request); - } - return resolve(event); -}; +export const bullboardHandle: Handle = async ({ event, resolve }) => { + if ( + bullboard && + event.url.pathname.match(/^\/admin\/jobs($|\/)/) && + event.locals.security.isSuperAdmin + ) { + return bullboard.fetch(event.request); + } + return resolve(event); +};eslint.config.js (1)
27-33: Scope the security rule to server endpoints to reduce noiseOptional: apply the rule only to server handlers to avoid any chance of false positives and speed up linting.
{ rules: { 'no-unused-vars': 'off', '@typescript-eslint/no-unused-vars': [ 'warn', { args: 'none' } ], '@typescript-eslint/consistent-type-imports': 'warn', 'import/order': [ 'warn', { groups: [['builtin', 'external', 'internal'], 'parent', 'sibling'], alphabetize: { order: 'asc' }, named: true } ], 'import/no-unresolved': 'off', - 'sveltekit-sec/require-security-check': 'error' + // moved to server-only override below } }, + { + files: ['**/+server.ts', '**/*.server.ts'], + rules: { + 'sveltekit-sec/require-security-check': 'error' + } + },Also applies to: 71-71
src/routes/(authenticated)/admin/settings/workflow-definitions/edit/+page.server.ts (1)
15-16: Super-admin gate added at function start — good; tighten id parsing and use throw redirectGate is correctly first. Consider stricter numeric parsing and idiomatic redirect.
-export const load = (async ({ url, locals }) => { - locals.security.requireSuperAdmin(); - const id = parseInt(url.searchParams.get('id') ?? ''); - if (isNaN(id)) { - return redirect(302, localizeHref('/admin/settings/workflow-definitions')); - } +export const load = (async ({ url, locals }) => { + locals.security.requireSuperAdmin(); + const idParam = url.searchParams.get('id'); + if (!/^\d+$/.test(idParam ?? '')) { + throw redirect(302, localizeHref('/admin/settings/workflow-definitions')); + } + const id = Number(idParam);Also consider changing remaining
return redirect(...)tothrow redirect(...)for clarity.src/routes/(authenticated)/admin/settings/workflow-definitions/+page.server.ts (1)
6-7: LGTM: centralized security check at top of loadPattern matches other admin routes. Minor: the header comment (Line 1) looks stale and points to
src/routes/+page.server.ts. Not blocking.src/routes/(authenticated)/tasks/sse/+server.ts (1)
6-7: Auth flow migrated to Security — good; consider typing and event namingLooks correct and early in the handler. Optional: switch to typed RequestHandler and name the param
eventfor clarity.+import type { RequestHandler } from '@sveltejs/kit'; - -export async function POST(request) { - request.locals.security.requireAuthenticated(); - const userId = request.locals.security.userId; +export const POST: RequestHandler = async (event) => { + event.locals.security.requireAuthenticated(); + const userId = event.locals.security.userId;src/routes/(authenticated)/admin/settings/organizations/edit/+page.server.ts (2)
17-19: Minor: be explicit with radix when parsing IDs.Use parseInt(..., 10) (or Number(...)) for clarity and consistency with other routes.
- const id = parseInt(url.searchParams.get('id') ?? ''); + const id = Number.parseInt(url.searchParams.get('id') ?? '', 10);
55-64: Only update the API token when provided.Prevents accidental token wipe and aligns with not echoing secrets.
- data: { - Name: form.data.name, - BuildEngineApiAccessToken: form.data.buildEngineApiAccessToken, - BuildEngineUrl: form.data.buildEngineUrl, - LogoUrl: form.data.logoUrl, - ContactEmail: form.data.contact, - PublicByDefault: form.data.publicByDefault, - UseDefaultBuildEngine: form.data.useDefaultBuildEngine, - WebsiteUrl: form.data.websiteUrl - } + data: { + Name: form.data.name, + ...(form.data.buildEngineApiAccessToken?.trim() + ? { BuildEngineApiAccessToken: form.data.buildEngineApiAccessToken } + : {}), + BuildEngineUrl: form.data.buildEngineUrl, + LogoUrl: form.data.logoUrl, + ContactEmail: form.data.contact, + PublicByDefault: form.data.publicByDefault, + UseDefaultBuildEngine: form.data.useDefaultBuildEngine, + WebsiteUrl: form.data.websiteUrl + }src/routes/(authenticated)/organizations/[id=idNumber]/settings/groups/+page.server.ts (3)
13-15: Validate and reuse orgId once.parseInt on event.params.id without validation risks NaN flowing into guards/queries. Parse once, validate, and reuse.
-export const load = (async (event) => { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); +export const load = (async (event) => { + const orgId = Number.parseInt(event.params.id, 10); + if (!Number.isFinite(orgId)) return fail(400, { ok: false }); + event.locals.security.requireAdminOfOrg(orgId); const { organization } = await event.parent();
23-31: Reuse validated orgId in addGroup.Prevents duplicative parsing and avoids passing NaN to createGroup.
- async addGroup(event) { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + async addGroup(event) { + const orgId = Number.parseInt(event.params.id, 10); + if (!Number.isFinite(orgId)) return fail(400, { ok: false }); + event.locals.security.requireAdminOfOrg(orgId); const form = await superValidate(event.request, valibot(addGroupSchema)); if (!form.valid) return fail(400, { form, ok: false }); const group = await DatabaseWrites.groups.createGroup( form.data.name, form.data.abbreviation, - parseInt(event.params.id) + orgId );
34-47: Reuse validated orgId in deleteGroup.Same rationale as addGroup; also keeps hidden-field tampering check intact.
- async deleteGroup(event) { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + async deleteGroup(event) { + const orgId = Number.parseInt(event.params.id, 10); + if (!Number.isFinite(orgId)) return fail(400, { ok: false }); + event.locals.security.requireAdminOfOrg(orgId); const form = await superValidate(event.request, valibot(deleteSchema)); if (!form.valid) return fail(400, { form, ok: false }); if ( // if user modified hidden values !(await DatabaseReads.groups.findFirst({ - where: { Id: form.data.id, OwnerId: parseInt(event.params.id) } + where: { Id: form.data.id, OwnerId: orgId } })) ) { return fail(403, { form, ok: false }); }src/routes/(authenticated)/users/+page.svelte (1)
14-15: UI gate updated to isAdminForAny: LGTM.Ensure isAdminForAny safely handles undefined roles; if not, default to new Map().
- {#if isAdminForAny(data.session?.user.roles)} + {#if isAdminForAny(data.session?.user.roles ?? new Map())}Also applies to: 68-79
src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts (1)
10-12: Harden cookie attributes.Set httpOnly, secure, and sameSite to reduce risk of token leakage and CSRF.
- if (event.cookies.get('inviteToken')) { - event.cookies.set('inviteToken', '', { path: '/' }); - } + if (event.cookies.get('inviteToken')) { + event.cookies.set('inviteToken', '', { path: '/', httpOnly: true, secure: true, sameSite: 'lax' }); + } ... - event.cookies.set('inviteToken', inviteToken, { - path: '/' - }); + event.cookies.set('inviteToken', inviteToken, { path: '/', httpOnly: true, secure: true, sameSite: 'lax' });Also applies to: 20-22
src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts (2)
17-20: Good: security context plumbed to data fetch.Emitting getProjectDetails(id, security) is the right shape after the guard. Consider reusing the parsed id variable for consistency.
24-43: Confirm policy: continued access after revocation.Comment states viewers keep receiving updates until reload. Verify this is an approved policy; otherwise, add a permission recheck inside updateCb before emitting.
src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts (1)
21-35: Minor: reuse subject fetch to avoid double query.You fetch the subject again after the gate. Consider hoisting the first fetch and reusing it for both the gate and subject payload.
src/routes/(authenticated)/organizations/[id=idNumber]/+layout.server.ts (1)
8-8: Validate id before authorization call.If id is NaN, requireAdminOfOrg(NaN) may throw a confusing error. Guard early.
- event.locals.security.requireAdminOfOrg(id); + if (Number.isNaN(id)) return redirect(302, localizeHref('/organizations')); + event.locals.security.requireAdminOfOrg(id);src/routes/(authenticated)/directory/[id]/+page.server.ts (2)
7-9: Comment contradicts route grouping.Route lives under (authenticated), but the comment says “anyone can view.” Either move this out of the authenticated group or update the comment/policy.
26-31: Optional: guard membership lookup when unauth (if made public).If this route becomes public, ensure you skip the membership query when there’s no userId.
- const allowDownloads = !!( - projectCheck.AllowDownloads || - (await DatabaseReads.organizationMemberships.findFirst({ - where: { OrganizationId: projectCheck.OrganizationId, UserId: locals.security.userId } - })) - ); + const uid = locals.security.userId; + const member = uid + ? await DatabaseReads.organizationMemberships.findFirst({ + where: { OrganizationId: projectCheck.OrganizationId, UserId: uid } + }) + : null; + const allowDownloads = !!(projectCheck.AllowDownloads || member);src/routes/(authenticated)/users/[id=idNumber]/settings/profile/+page.server.ts (2)
21-31: Good: narrow edit rights to self or org‑admins.Gate aligns with PR goals. Consider fetching subject once and reusing for both the gate and form population to avoid double queries.
55-65: Action gate mirrors load; good.Same note about double queries applies here.
src/routes/(authenticated)/admin/settings/stores/edit/+page.server.ts (1)
22-28: Prefer findUnique for primary key lookup.Id is unique; findUnique communicates intent and avoids surprises.
- const data = await DatabaseReads.stores.findFirst({ - where: { - Id: id - } - }); + const data = await DatabaseReads.stores.findUnique({ where: { Id: id } });src/routes/(authenticated)/admin/settings/store-types/new/+page.server.ts (1)
19-20: Consider renaming action "new" to "create".Avoids reserved-word confusion and is semantically clearer. If you adopt this, remember to update forms and clients.
Apply locally:
-export const actions = { - async new({ request, locals }) { +export const actions = { + async create({ request, locals }) {src/routes/(authenticated)/admin/settings/stores/new/+page.server.ts (1)
14-16: Good: super-admin guard early in load.Auth happens before any DB/form work; consistent with the refactor.
urlis unused; drop it from the load signature.-export const load = (async ({ url, locals }) => { +export const load = (async ({ locals }) => {src/routes/(authenticated)/projects/[filter=projectSelector]/+page.server.ts (2)
10-21: Use SvelteKit cookies API instead of parsing headers.Cleaner and less error-prone than manual regex.
- const cookieHeader = event.request.headers.get('cookie'); - if (cookieHeader) { - const match = cookieHeader.match(/orgLastSelected=([^;]+)/); - if (match) { - const orgLastSelected = decodeURIComponent(match[1]); - // Only use if it matches an org in the list - if ( - !isNaN(Number(orgLastSelected)) && - event.locals.security.organizationMemberships.includes(Number(orgLastSelected)) - ) { - orgRedirect = Number(orgLastSelected); - } - } - } + const last = event.cookies.get('orgLastSelected'); + if (last) { + const parsed = Number(last); + if (!Number.isNaN(parsed) && event.locals.security.organizationMemberships.includes(parsed)) { + orgRedirect = parsed; + } + }
6-8: Auth gate + redirect logic look good.Requires auth, validates cookie against memberships, then redirects.
If users can exist without orgs, consider
requireMemberOfAnyOrg()for a clearer failure path.Also applies to: 25-27
src/routes/(authenticated)/projects/[id=idNumber]/edit/+page.server.ts (1)
78-85: Queue gating is redundant/conflicting — pick one place.Unconditional 503 at Line 84 makes the owner-change-specific gate at Line 93 unreachable. Either:
- keep only the owner-change gate (allow non-owner edits when queue is down), or
- keep only the unconditional gate (block all edits when queue is down).
Provide preference and I’ll send a follow-up diff.
Also applies to: 93-94
src/routes/(unauthenticated)/(auth)/login/+page.server.ts (1)
20-21: Wrap signIn to run an explicit security check; drop the ESLint suppression.Ensures runtime hook sees securityHandled and avoids relying on a disabled rule.
Apply this diff:
-// eslint-disable-next-line sveltekit-sec/require-security-check -export const actions: Actions = { default: signIn }; +export const actions: Actions = { + default: async (event) => { + event.locals.security.requireNothing(); + return signIn(event); + } +};src/routes/(authenticated)/admin/settings/store-types/edit/+page.server.ts (1)
21-26: Prefer findUnique for primary key lookups.Slightly clearer and avoids accidental non-unique matches.
- const data = await DatabaseReads.storeTypes.findFirst({ - where: { - Id: id - } - }); + const data = await DatabaseReads.storeTypes.findUnique({ + where: { Id: id } + });src/routes/(authenticated)/admin/settings/workflow-definitions/new/+page.server.ts (1)
13-19: Naming nit: options.storeType is plural data.Consider renaming to storeTypes for clarity; adjust consumer accordingly.
src/routes/(authenticated)/admin/settings/product-definitions/new/+page.server.ts (1)
30-31: Remove unused cookies parameter.Avoids linter noise and clarifies intent.
- async new({ cookies, request, locals }) { + async new({ request, locals }) {src/routes/(authenticated)/tasks/[product_id]/+page.server.ts (3)
33-36: Gate may be too strict: verifyCanViewTask only allows SuperAdmin or assigned UserTask.OrgAdmins or project owners/authors without an explicit UserTask will be blocked. Confirm this is intended.
Possible broadened check (if desired):
- allow project owner
- allow authors
- allow org admins of the project’s org
I can draft a safe revision if you confirm the desired policy.
136-145: Minor: redundant undefined check and overfetch in nested select.requireAuthenticated guarantees userId; also limit nested selection to UserId.
- .filter((a) => { - if (locals.security.userId === undefined) return false; + .filter((a) => { switch (a[0].meta?.user) { case RoleId.AppBuilder: return locals.security.userId === product.Project.Owner.Id; case RoleId.Author: return product.Project.Authors.map((a) => a.UserId).includes(locals.security.userId); case RoleId.OrgAdmin: - return product.Project.Organization.UserRoles.map((u) => u.UserId).includes( + return product.Project.Organization.UserRoles.map((u) => u.UserId).includes( locals.security.userId ); default: return false; } })And in the load query:
Organization: { select: { UserRoles: { where: { RoleId: RoleId.OrgAdmin - } + }, + select: { UserId: true } } } },
233-248: verifyCanViewTask: confirm policy scope.If broader access is required (owners/authors/org-admins), expand here to keep policy centralized.
src/routes/(authenticated)/users/+page.server.ts (4)
91-101: Extract admin org IDs once (utility) — repeated pattern elsewhere.Consider a small helper to derive OrgAdmin orgIds from roles.
121-124: Telemetry: JSON.stringify(Map) yields "{}".Convert roles Map to plain object before stringifying.
- 'auth.load.user.roles': JSON.stringify(locals.security.roles), + 'auth.load.user.roles': JSON.stringify(Object.fromEntries(locals.security.roles)),
167-177: Add select to slim payload for subject org lookup.No need to fetch full rows.
- await DatabaseReads.organizationMemberships.findMany({ - where: { UserId: form.data.user }, - distinct: 'OrganizationId' - }) + await DatabaseReads.organizationMemberships.findMany({ + where: { UserId: form.data.user }, + distinct: 'OrganizationId', + select: { OrganizationId: true } + })
195-223: Telemetry: same Map→JSON issue in action span attributes.- 'auth.load.user.roles': JSON.stringify(event.locals.security.roles), + 'auth.load.user.roles': JSON.stringify(Object.fromEntries(event.locals.security.roles)),src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte (1)
116-121: Pass a consistent security/session source.You mix
page.data.session.user(reactive) anddata.session.user(initial). Prefer one (recommendpage.data.session.user) to avoid stale role reads.Example:
- selectedProjects.every((p) => canArchive(p, page.data.session.user, organizationId)) + selectedProjects.every((p) => canArchive(p, page.data.session.user, organizationId))And similarly keep
canReactivateSelectedand other usages aligned withpage.data....src/hooks.server.ts (2)
100-111: Ensure 'Auth Hooks' span ends on errors (no leaks).If
authSequencethrows before callingresolve, the span never ends. Wrap with try/finally and drop the resolve wrapper.Apply:
- async function hookTelementry({ event, resolve }) { - return tracer.startActiveSpan('Auth Hooks', async (span) => { - // Call the auth sequence - const ret = await authSequence({ - event, - resolve: (...args) => { - span.end(); - return resolve(...args); - } - }); - return ret; - }); - }, + async function hookTelementry({ event, resolve }) { + return tracer.startActiveSpan('Auth Hooks', async (span) => { + try { + return await authSequence({ event, resolve }); + } catch (e) { + span.recordException(e as Error); + span.setStatus({ code: SpanStatusCode.ERROR, message: e instanceof Error ? e.message : String(e) }); + throw e; + } finally { + span.end(); + } + }); + },
170-175: Avoid duplicate logging in handleError.
console.errorduplicates OTEL logs; drop it to reduce noise.- console.error('Error occurred:', error);src/lib/utils/roles.ts (1)
6-12: Doc nit: roles param description is outdated.Docs say
[OrganizationId, RoleId][]; it’s aMap<number, RoleId[]>.- * @param roles [OrganizationId, RoleId][] + * @param roles Map<OrganizationId, RoleId[]>src/lib/projects/index.ts (1)
170-178: Guard unauthenticated SecurityLike correctly.
!!securityis always true (object exists). CheckuserIdpresence before comparing owner.Apply:
- return ( - !!security && - (projectOwnerId === security.userId || isAdminForOrg(organizationId, security.roles)) - ); + return ( + security?.userId != null && + (projectOwnerId === security.userId || isAdminForOrg(organizationId, security.roles)) + );src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (2)
61-67: Repeat: parse and reuse projectId before auth/DB; avoidId: NaNacross actions.Each action fetches the project with
parseInt(event.params.id)before any validation. Parse once, validate, and reuse. Apply the same fix to all actions in this file.Apply this pattern here and in other actions:
- event.locals.security.requireProjectWriteAccess( - await DatabaseReads.projects.findUniqueOrThrow({ - where: { Id: parseInt(event.params.id) }, - select: { OwnerId: true, OrganizationId: true } - }) - ); + const projectId = parseInt(event.params.id); + if (isNaN(projectId) || projectId + '' !== event.params.id) return fail(404); + event.locals.security.requireProjectWriteAccess( + await DatabaseReads.projects.findUniqueOrThrow({ + where: { Id: projectId }, + select: { OwnerId: true, OrganizationId: true } + }) + );
116-117: Prefer throwing errors over returning them for clarity/typing.Use
throw error(503)instead ofreturn error(503)in actions.Example:
- if (!QueueConnected()) return error(503); + if (!QueueConnected()) throw error(503);Apply similarly in other actions.
Also applies to: 137-138, 167-168, 185-186, 204-205, 271-276, 293-298
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts (2)
210-212: Ineffectiveeverycheck; rely on throwingrequireProjectWriteAccess.
requireProjectWriteAccessthrows on failure; it doesn’t return a boolean. The currentevery(...)always returns true on success and never reaches thefail(403)on failure.Apply this diff:
- if (!projects.every((p) => event.locals.security.requireProjectWriteAccess(p))) { - return fail(403); - } + projects.forEach((p) => event.locals.security.requireProjectWriteAccess(p));
239-243: Consistency: prefer fail(403) over throwing here (or standardize).Elsewhere in this file you return
fail(4xx). Choose one approach for 4xx; recommendfail(403)to keep superforms flow consistent.Apply this diff:
- ) { - throw error(403); - } + ) { + return fail(403, { form, ok: false }); + }src/auth.ts (4)
79-88: Trace logs are cleared before recording; reorder to preserve values.You null out tokenStatus/currentInviteToken before logging them, so traces always show null.
Apply this diff:
- tokenStatus = null; - currentInviteToken = null; - trace.getActiveSpan()?.addEvent('signIn callback completed', { + const ts = tokenStatus; + const inv = currentInviteToken; + tokenStatus = null; + currentInviteToken = null; + trace.getActiveSpan()?.addEvent('signIn callback completed', { 'auth.signIn.profile': profile ? profile.email + ' - ' + profile.sub : 'unknown', - 'auth.signIn.tokenStatus': tokenStatus ?? 'null', - 'auth.signIn.currentInviteToken': currentInviteToken ?? 'null', + 'auth.signIn.tokenStatus': ts ?? 'null', + 'auth.signIn.currentInviteToken': inv ?? 'null', 'auth.signIn.user': JSON.stringify(user), 'auth.signIn.userExists': userExists });
266-272: Cookie deletion should expire the cookie, not just blank its value.Setting an empty value may not reliably clear the session cookie in all clients.
Apply this diff:
- event.cookies.set('authjs.session-token', '', { path: '/' }); + event.cookies.set('authjs.session-token', '', { + path: '/', + expires: new Date(0) + }); @@ - event.cookies.set('authjs.session-token', '', { path: '/' }); + event.cookies.set('authjs.session-token', '', { + path: '/', + expires: new Date(0) + });
148-156: Preserve return URL on auth redirect.Improves UX by returning users to the original page post-login.
- throw redirect(302, localizeHref('/login')); + const returnTo = encodeURIComponent(this.event.url.pathname + this.event.url.search); + throw redirect(302, localizeHref(`/login?returnTo=${returnTo}`));
115-121: Fetch only needed fields for roles.Minor efficiency and payload reduction.
- const userRoles = await DatabaseReads.userRoles.findMany({ - where: { - UserId: token.userId as number - } - }); + const userRoles = await DatabaseReads.userRoles.findMany({ + where: { UserId: token.userId as number }, + select: { OrganizationId: true, RoleId: true } + });
src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts
Outdated
Show resolved
Hide resolved
src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts
Show resolved
Hide resolved
src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts
Show resolved
Hide resolved
src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
I like how this is looking, for the most part.
2 things:
-
I am not entirely sold on using Map. I understand why you chose it. It works without issue on the server side, since we control the version of NodeJS that we are using. However, there is no guarantee that it will work as expected on the client side, especially since
Iterator.prototype.some()is marked as newly available and may not be available on all browsers yet. It may be that we would want to do something a al #1139 for this. -
This doesn't yet account for all of the changes I made in #1275.
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts
Outdated
Show resolved
Hide resolved
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts
Outdated
Show resolved
Hide resolved
src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts
Show resolved
Hide resolved
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (1)
202-231: addAuthor allows non‑group/non‑Author users; enforce invariants server‑side.UI filters are not sufficient; validate target user is in the project’s group and has Author role, and prevent duplicates.
async addAuthor(event) { - event.locals.security.requireProjectWriteAccess( - await DatabaseReads.projects.findUniqueOrThrow({ - where: { Id: parseInt(event.params.id) }, - select: { OwnerId: true, OrganizationId: true } - }) - ); + const projectId = parseInt(event.params.id); + const minimal = await DatabaseReads.projects.findUniqueOrThrow({ + where: { Id: projectId }, + select: { OwnerId: true, OrganizationId: true, GroupId: true } + }); + event.locals.security.requireProjectWriteAccess(minimal); if (!QueueConnected()) return error(503); const form = await superValidate(event.request, valibot(addAuthorSchema)); if (!form.valid) return fail(400, { form, ok: false }); - const projectId = parseInt(event.params.id); - if ( - // if user modified hidden values - !(await DatabaseReads.organizationMemberships.findFirst({ - where: { - UserId: form.data.author, - Organization: { - Projects: { - some: { Id: projectId } - } - } - } - })) - ) { + // Target must be in project group AND have Author role in project org AND not already an author. + const canAdd = await DatabaseReads.users.findFirst({ + where: { + Id: form.data.author, + GroupMemberships: { some: { GroupId: minimal.GroupId } }, + UserRoles: { + some: { OrganizationId: minimal.OrganizationId, RoleId: RoleId.Author } + }, + Authors: { none: { ProjectId: projectId } } + }, + select: { Id: true } + }); + if (!canAdd) { return fail(403, { form, ok: false }); } // ISSUE: #1101 Appears that CanUpdate is not used const author = await DatabaseWrites.authors.create({ ProjectId: projectId, UserId: form.data.author });
🧹 Nitpick comments (8)
src/auth.ts (4)
79-87: Trace fields nulled before logging; move resets after addEvent.Currently always logs
nullfortokenStatusandcurrentInviteToken.Apply:
- } finally { - trace.getActiveSpan()?.addEvent('signIn callback completed', { + } finally { + trace.getActiveSpan()?.addEvent('signIn callback completed', { 'auth.signIn.profile': profile ? profile.email + ' - ' + profile.sub : 'unknown', 'auth.signIn.tokenStatus': tokenStatus ?? 'null', 'auth.signIn.currentInviteToken': currentInviteToken ?? 'null', 'auth.signIn.user': JSON.stringify(user), 'auth.signIn.userExists': userExists - }); - } + }); + tokenStatus = null; + currentInviteToken = null; + }
120-125: De‑dupe role entries when building the roles map.Avoid repeated RoleIds per org.
- for (const { OrganizationId, RoleId } of userRoles) { - if (!rolesMap.has(OrganizationId)) rolesMap.set(OrganizationId, []); - rolesMap.get(OrganizationId)!.push(RoleId); - } + for (const { OrganizationId, RoleId } of userRoles) { + const arr = rolesMap.get(OrganizationId) ?? []; + if (!arr.includes(RoleId)) arr.push(RoleId); + rolesMap.set(OrganizationId, arr); + }
148-154: Preserve return URL on auth redirect.Attach
returnToto the login redirect.requireAuthenticated() { this.securityHandled = true; if (!this.userId || !this.organizationMemberships || !this.roles) { // Redirect to login - // TODO: preserve the original URL to return to after login - throw redirect(302, localizeHref('/login')); + const returnTo = this.event.url.pathname + this.event.url.search; + const login = new URL(localizeHref('/login'), this.event.url); + login.searchParams.set('returnTo', returnTo); + throw redirect(302, login.toString()); } }
285-292: Avoid logging PII (email) and large role payloads.Prefer IDs; omit or truncate roles to reduce log volume and PII risk.
- trace.getActiveSpan()?.addEvent('checkUserExistsHandle completed', { - 'auth.userId': security?.userId, - 'auth.user': user ? user.Email + ' - ' + user.Id : 'null', - 'auth.user.OrganizationMemberships': - user?.OrganizationMemberships.map((o) => o.OrganizationId).join(', ') ?? 'null', - 'auth.user.roles': user ? JSON.stringify([...security.roles.entries()]) : 'null', - 'auth.user.IsLocked': user ? user.IsLocked + '' : 'null' - }); + trace.getActiveSpan()?.addEvent('checkUserExistsHandle completed', { + 'auth.userId': security?.userId, + 'auth.user.dbId': user?.Id ?? 'null', + 'auth.user.orgs': user?.OrganizationMemberships.map((o) => o.OrganizationId).join(', ') ?? 'null', + 'auth.user.roleOrgsCount': security?.roles ? security.roles.size : 0, + 'auth.user.IsLocked': user ? String(user.IsLocked) : 'null' + });src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts (2)
79-88: Redundant param validation;[orgId=idNumber]already guarantees a valid positive integer.Drop the extra
isNaNand string-equality checks; keep the existence check.- const orgId = parseInt(params.orgId); - if ( - isNaN(orgId) || - !(orgId + '' === params.orgId) || - !(await DatabaseReads.organizations.findFirst({ where: { Id: orgId } })) - ) { + const orgId = parseInt(params.orgId); + if (!(await DatabaseReads.organizations.findFirst({ where: { Id: orgId } }))) { return redirect(302, localizeHref(`/projects/${params.filter}`)); }
206-223: Authorization check viaeveryis misleading; it always returns true if no throw.The guard methods throw on failure and otherwise return
this(truthy), soeveryis redundant. Validate in a simple pass and rely on throws.- const allowAction = - form.data.operation === 'claim' - ? (p: (typeof projects)[0]) => event.locals.security.requireProjectClaimable(groups, p) - : (p: (typeof projects)[0]) => event.locals.security.requireProjectWriteAccess(p); - - if (!projects.every(allowAction)) { - return fail(403); - } + for (const p of projects) { + if (form.data.operation === 'claim') { + event.locals.security.requireProjectClaimable(groups, p); + } else { + event.locals.security.requireProjectWriteAccess(p); + } + }src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (2)
114-121: Minor: placeQueueConnected()checks consistently after auth but before DB work.Current order is fine; consider standardizing across actions for clarity.
262-267: Same consistency note fordeleteReviewer.Align placement of the queue check with other actions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app.d.ts(1 hunks)src/auth.ts(3 hunks)src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts(7 hunks)src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts(13 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-09-12T14:07:02.200Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:285-303
Timestamp: 2025-09-12T14:07:02.200Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to optimize filterAvailableActions by creating Sets at the caller level rather than inside the function, so the function would take Set<number> arguments instead of arrays for better performance and separation of concerns.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts
📚 Learning: 2025-09-22T15:01:28.442Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/lib/projects/index.ts:185-197
Timestamp: 2025-09-22T15:01:28.442Z
Learning: The functions in src/lib/projects/index.ts (canModifyProject, canClaimProject, canArchive, canReactivate) are used both server-side and client-side in Svelte components, so any changes must be compatible with both environments. Map iterator methods like .values().some() may not work consistently in all client environments.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/auth.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts
📚 Learning: 2025-09-10T13:46:43.497Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1275
File: src/auth.ts:240-245
Timestamp: 2025-09-10T13:46:43.497Z
Learning: In src/auth.ts, the products branch intentionally uses params.id as a string for database lookups, not as a number. This is the correct behavior and should not be changed to numeric parsing.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts
📚 Learning: 2025-09-17T21:07:18.139Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1275
File: src/lib/projects/index.ts:183-201
Timestamp: 2025-09-17T21:07:18.139Z
Learning: In the AppBuilder portal project, org admins must be in the project group to claim/take ownership of projects. This is enforced both in client-side logic (canClaimProject function) and server-side validation.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts
📚 Learning: 2025-09-22T15:00:33.837Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts:37-45
Timestamp: 2025-09-22T15:00:33.837Z
Learning: The idNumber param matcher in src/params/idNumber.ts validates that parameters are positive integers with exact string matching (no leading zeros), ensuring parseInt() calls in route handlers will never return NaN when using [id=idNumber] route parameters.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts
📚 Learning: 2025-09-22T15:00:33.837Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts:37-45
Timestamp: 2025-09-22T15:00:33.837Z
Learning: In SvelteKit applications, param matchers like idNumber in src/params/idNumber.ts handle parameter validation at the routing level, ensuring that route handlers only execute with valid parameters. Manual validation in load functions is redundant when param matchers are used.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts
📚 Learning: 2025-09-22T15:01:42.995Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/auth.ts:13-13
Timestamp: 2025-09-22T15:01:42.995Z
Learning: The codebase already successfully uses Map objects in session data that are serialized to the client via SvelteKit's devalue. Evidence: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte line 154 uses data.session?.user.roles.get(organizationId), proving Maps work fine in client-side Svelte components when passed through session data.
Applied to files:
src/auth.ts
📚 Learning: 2025-09-22T13:48:06.537Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/auth.ts:145-146
Timestamp: 2025-09-22T13:48:06.537Z
Learning: Node.js 22+ with V8 12.4+ supports Iterator Helpers including Iterator.prototype.some(), which means Map.values().some() and other iterator methods work directly without needing Array.from() conversion. This was added through the Iterator Helpers proposal in 2024.
Applied to files:
src/auth.ts
📚 Learning: 2025-03-27T13:00:13.162Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1079
File: source/SIL.AppBuilder.Portal/node-server/job-executors/recurring.ts:33-36
Timestamp: 2025-03-27T13:00:13.162Z
Learning: In the SIL.AppBuilder.Portal project running on Node 22, the code pattern using Set iterator methods like `.values().map().toArray()` is supported and works as expected in the backend environment.
Applied to files:
src/auth.ts
📚 Learning: 2025-09-22T15:01:42.995Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/auth.ts:13-13
Timestamp: 2025-09-22T15:01:42.995Z
Learning: SvelteKit uses the devalue library for serialization, which supports Map and Set objects. This means Maps can be safely stored in session data and will be properly serialized/deserialized for client-side usage, unlike standard JSON serialization which would lose Map objects.
Applied to files:
src/auth.ts
📚 Learning: 2025-09-22T14:59:53.346Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts:214-221
Timestamp: 2025-09-22T14:59:53.346Z
Learning: In the AppBuilder portal, bulk project actions in the route `/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts` are organization-scoped by design. The route parameter `[orgId=idNumber]` ensures all operations happen within a single organization context, so using the route-level orgId for all projects in bulk actions like `doProjectAction` is correct and safe.
Applied to files:
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts
🧬 Code graph analysis (4)
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (3)
src/routes/(authenticated)/projects/[id=idNumber]/project.ts (1)
getProjectDetails(10-284)src/lib/server/bullmq/queues.ts (1)
QueueConnected(86-86)src/lib/projects/server.ts (2)
userGroupsForOrg(106-120)doProjectAction(122-161)
src/auth.ts (1)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
src/app.d.ts (1)
src/auth.ts (1)
Security(135-265)
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts (2)
src/lib/utils/roles.ts (1)
isAdminForOrg(9-12)src/lib/projects/server.ts (3)
userGroupsForOrg(106-120)projectFilter(8-100)doProjectAction(122-161)
🔇 Additional comments (3)
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts (1)
232-246: Status code choice for missing products.Mismatched counts currently throw 403. Consider 404 to avoid implying permission issues, or keep 403 to avoid ID enumeration. Pick one consistently across routes.
- { - throw error(403); - } + { + // Decide: 404 (not found) vs 403 (tampering). Align with project/id route. + throw error(404); + }src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (1)
36-48: RedundantNaNcheck;[id=idNumber]matcher already enforces numeric id.Remove the extra validation.
); - if (isNaN(parseInt(params.id))) throw error(404, 'Not Found'); return { projectData: await getProjectDetails(parseInt(params.id), locals.security),src/app.d.ts (1)
20-20: .d.ts cannot declare default parameter values; make the flag optional.Use an optional boolean instead of
= true.Apply this diff:
- requireHasRole(organizationId: number, roleId: number, orOrgAdmin = true): this | never; + requireHasRole(organizationId: number, roleId: number, orOrgAdmin?: boolean): this | never;
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(unauthenticated)/(auth)/login/LoginScreen.svelte (1)
3-22: Clear the polling interval when the component unmounts
setIntervalkeeps running even after this component is torn down (for example, if the user navigates away during an outage). That dangling timer will still triggerlocation.reload()every 2 seconds and unexpectedly yank the user back to the login page. Please store the interval handle and clear it in anonDestroyhook.- import { browser } from '$app/environment'; + import { browser } from '$app/environment'; + import { onDestroy } from 'svelte'; @@ - if (!serviceAvailable && browser) { - let timeout = setInterval(() => { + let reloadInterval: ReturnType<typeof setInterval> | undefined; + + if (!serviceAvailable && browser) { + reloadInterval = setInterval(() => { // Reload every 2 seconds to check if the service is back up if (serviceAvailable) { - clearInterval(timeout); + clearInterval(reloadInterval); + reloadInterval = undefined; } else { location.reload(); } }, 2000); } + + onDestroy(() => { + if (reloadInterval) { + clearInterval(reloadInterval); + } + });
🧹 Nitpick comments (7)
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte (1)
152-155: Guard roles when session may be absent; avoid potential TS/runtime issues.In an authenticated layout this is usually fine, but TS sees roles as possibly undefined. Use a fallback and optional chain on user.
-const canModifyProjects = $derived( - isAdminForOrg(organizationId, data.session?.user.roles) || - data.session?.user.roles.some(([o, r]) => r === RoleId.AppBuilder && o === organizationId) -); +const canModifyProjects = $derived( + isAdminForOrg(organizationId, data.session?.user?.roles ?? []) || + (data.session?.user?.roles ?? []).some( + ([o, r]) => r === RoleId.AppBuilder && o === organizationId + ) +);src/lib/utils/roles.ts (1)
9-12: OK: org admin check over tuple arrayLogic is clear and efficient. Minor: the double negation on orgRoles is redundant.
- return !!orgRoles && (orgRoles.includes(RoleId.SuperAdmin) || orgRoles.includes(RoleId.OrgAdmin)); + return orgRoles.includes(RoleId.SuperAdmin) || orgRoles.includes(RoleId.OrgAdmin);src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts (1)
82-84: Consider 404 instead of redirect when org not foundRedirecting to listing may mask an invalid orgId; 404 better signals a bad path.
- if (!(await DatabaseReads.organizations.findFirst({ where: { Id: orgId } }))) { - return redirect(302, localizeHref(`/projects/${params.filter}`)); - } + if (!(await DatabaseReads.organizations.findFirst({ where: { Id: orgId } }))) { + throw error(404, 'Organization not found'); + }src/auth.ts (1)
306-327: Nit: de-dupe roles when building roleMapIf duplicates exist in security.roles tuples, roleMap may include repeated RoleIds. Optional, but can save downstream checks.
- for (const [orgId, roleId] of security.roles) { - if (!roleMap.has(orgId)) roleMap.set(orgId, []); - roleMap.get(orgId)!.push(roleId); - } + for (const [orgId, roleId] of security.roles) { + const arr = roleMap.get(orgId) ?? []; + if (!arr.includes(roleId)) arr.push(roleId); + roleMap.set(orgId, arr); + }src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts (1)
7-11: SSE should likely require read access, not write. Confirm policy.Most viewers of an SSE stream only need read permission; restricting to write blocks legitimate observers. If policy allows, switch to read-access + group check.
Apply these changes:
-import { DatabaseReads } from '$lib/server/database'; +import { DatabaseReads } from '$lib/server/database'; +import { userGroupsForOrg } from '$lib/projects/server'; +import { error } from '@sveltejs/kit';- const project = await DatabaseReads.projects.findUniqueOrThrow({ where: { Id: id } }); - request.locals.security.requireProjectWriteAccess(project); + const project = await DatabaseReads.projects.findUniqueOrThrow({ where: { Id: id } }); + const groups = await userGroupsForOrg(request.locals.security.userId, project.OrganizationId); + request.locals.security.requireProjectReadAccess(groups, project);Please confirm whether read access is sufficient for this endpoint.
Also applies to: 1-5
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (1)
37-47: Remove redundant NaN check; parse once; return 404 on missing project.[id=idNumber] matcher guarantees a valid positive integer, so the
isNaNcheck is dead code and happens after a DB call. Also, prefer explicit 404 if the project doesn’t exist rather than lettingfindUniqueOrThrowbubble a generic error.Apply this diff:
export const load = (async ({ locals, params }) => { - locals.security.requireProjectReadAccess( - await DatabaseReads.groupMemberships.findMany({ - where: { UserId: locals.security.userId }, - select: { GroupId: true } - }), - await DatabaseReads.projects.findUniqueOrThrow({ - where: { Id: parseInt(params.id) }, - select: { OwnerId: true, OrganizationId: true, GroupId: true } - }) - ); - if (isNaN(parseInt(params.id))) throw error(404, 'Not Found'); + const projectId = parseInt(params.id); + const [groups, minimal] = await Promise.all([ + DatabaseReads.groupMemberships.findMany({ + where: { UserId: locals.security.userId }, + select: { GroupId: true } + }), + DatabaseReads.projects.findUnique({ + where: { Id: projectId }, + select: { OwnerId: true, OrganizationId: true, GroupId: true } + }) + ]); + if (!minimal) throw error(404, 'Not Found'); + locals.security.requireProjectReadAccess(groups, minimal); return { - projectData: await getProjectDetails(parseInt(params.id), locals.security.sessionForm), + projectData: await getProjectDetails(projectId, locals.security.sessionForm),Also applies to: 49-50
src/lib/projects/index.ts (1)
192-197: Clarify claim policy (AppBuilder vs OrgAdmin) and align implementation.Docstring says “AppBuilder or OrgAdmin,” but implementation only allows OrgAdmin (via canModifyProject). Confirm desired policy; if AppBuilder should be allowed, include it explicitly.
Option A (allow AppBuilder):
export function canClaimProject( security: Session['user'], projectOwnerId: number, organizationId: number, projectGroupId: number, userGroupIds: number[] ) { if (security.userId === projectOwnerId) return false; if (security.roles.some(([_, r]) => r === RoleId.SuperAdmin)) return true; - return ( - canModifyProject(security, projectOwnerId, organizationId) && - userGroupIds.includes(projectGroupId) - ); + const isOrgAdmin = isAdminForOrg(organizationId, security.roles); + const hasAppBuilder = + security.roles.some(([orgId, r]) => orgId === organizationId && r === RoleId.AppBuilder); + return userGroupIds.includes(projectGroupId) && (isOrgAdmin || hasAppBuilder); }Option B (OrgAdmin only): update the comment to match current behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
eslint-rules/require-security-check.ts(1 hunks)package.json(2 hunks)src/app.d.ts(1 hunks)src/auth.ts(4 hunks)src/hooks.server.ts(4 hunks)src/lib/projects/index.ts(3 hunks)src/lib/projects/server.ts(4 hunks)src/lib/utils/roles.ts(1 hunks)src/routes/(authenticated)/+layout.server.ts(3 hunks)src/routes/(authenticated)/admin/settings/product-definitions/edit/+page.server.ts(2 hunks)src/routes/(authenticated)/products/[id]/files/+page.server.ts(2 hunks)src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts(7 hunks)src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte(4 hunks)src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts(13 hunks)src/routes/(authenticated)/projects/[id=idNumber]/project.ts(5 hunks)src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts(2 hunks)src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts(2 hunks)src/routes/(unauthenticated)/(auth)/login/+page.server.ts(2 hunks)src/routes/(unauthenticated)/(auth)/login/LoginScreen.svelte(2 hunks)src/routes/(unauthenticated)/(auth)/signout/+page.server.ts(1 hunks)src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts(2 hunks)src/routes/+layout.server.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/routes/+layout.server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(unauthenticated)/(auth)/login/+page.server.ts
🧰 Additional context used
🧠 Learnings (16)
📚 Learning: 2025-09-12T14:07:02.200Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:285-303
Timestamp: 2025-09-12T14:07:02.200Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to optimize filterAvailableActions by creating Sets at the caller level rather than inside the function, so the function would take Set<number> arguments instead of arrays for better performance and separation of concerns.
Applied to files:
src/routes/(unauthenticated)/(auth)/signout/+page.server.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/products/[id]/files/+page.server.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.sveltesrc/routes/(authenticated)/admin/settings/product-definitions/edit/+page.server.tssrc/routes/(authenticated)/+layout.server.ts
📚 Learning: 2025-09-22T15:01:28.442Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/lib/projects/index.ts:185-197
Timestamp: 2025-09-22T15:01:28.442Z
Learning: The functions in src/lib/projects/index.ts (canModifyProject, canClaimProject, canArchive, canReactivate) are used both server-side and client-side in Svelte components, so any changes must be compatible with both environments. Map iterator methods like .values().some() may not work consistently in all client environments.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/project.tssrc/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.tssrc/lib/utils/roles.tssrc/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/products/[id]/files/+page.server.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/sse/+server.tssrc/lib/projects/index.tssrc/lib/projects/server.tssrc/auth.ts
📚 Learning: 2025-09-25T03:51:13.786Z
Learnt from: 7dev7urandom
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts:7-8
Timestamp: 2025-09-25T03:51:13.786Z
Learning: In the organization invitation flow in src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts, the checkInviteErrors function properly handles null/undefined invite tokens by accepting inviteToken?: string | null and having an explicit guard clause if (!inviteToken) return { error: 'not found' }, making the non-null assertion on searchParams.get('t')! safe since error handling is centralized.
Applied to files:
src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.tssrc/auth.ts
📚 Learning: 2025-09-22T14:59:53.346Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts:214-221
Timestamp: 2025-09-22T14:59:53.346Z
Learning: In the AppBuilder portal, bulk project actions in the route `/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts` are organization-scoped by design. The route parameter `[orgId=idNumber]` ensures all operations happen within a single organization context, so using the route-level orgId for all projects in bulk actions like `doProjectAction` is correct and safe.
Applied to files:
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.sveltesrc/lib/projects/server.ts
📚 Learning: 2025-09-17T21:07:18.139Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1275
File: src/lib/projects/index.ts:183-201
Timestamp: 2025-09-17T21:07:18.139Z
Learning: In the AppBuilder portal project, org admins must be in the project group to claim/take ownership of projects. This is enforced both in client-side logic (canClaimProject function) and server-side validation.
Applied to files:
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.sveltesrc/lib/projects/index.tssrc/lib/projects/server.ts
📚 Learning: 2025-09-22T15:00:33.837Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts:37-45
Timestamp: 2025-09-22T15:00:33.837Z
Learning: The idNumber param matcher in src/params/idNumber.ts validates that parameters are positive integers with exact string matching (no leading zeros), ensuring parseInt() calls in route handlers will never return NaN when using [id=idNumber] route parameters.
Applied to files:
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts
📚 Learning: 2025-09-22T13:48:06.537Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/auth.ts:145-146
Timestamp: 2025-09-22T13:48:06.537Z
Learning: Node.js 22+ with V8 12.4+ supports Iterator Helpers including Iterator.prototype.some(), which means Map.values().some() and other iterator methods work directly without needing Array.from() conversion. This was added through the Iterator Helpers proposal in 2024.
Applied to files:
src/lib/utils/roles.tssrc/auth.ts
📚 Learning: 2025-09-22T15:01:42.995Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/auth.ts:13-13
Timestamp: 2025-09-22T15:01:42.995Z
Learning: The codebase already successfully uses Map objects in session data that are serialized to the client via SvelteKit's devalue. Evidence: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte line 154 uses data.session?.user.roles.get(organizationId), proving Maps work fine in client-side Svelte components when passed through session data.
Applied to files:
src/lib/utils/roles.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.sveltesrc/auth.ts
📚 Learning: 2025-09-22T15:00:43.076Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts:7-10
Timestamp: 2025-09-22T15:00:43.076Z
Learning: In user settings layouts, use the `adminOrgs` function from `$lib/users/server` to determine admin access rather than throwing 403 errors upfront. The `adminOrgs` function creates a filter that returns organizations where the subject user is a member AND the current user is an admin of that organization.
Applied to files:
src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts
📚 Learning: 2025-09-12T14:57:08.647Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:252-264
Timestamp: 2025-09-12T14:57:08.647Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to use transition.target[0] prediction mechanism for determining post-send state rather than calling flow.state() after flow.send() due to race condition concerns. The prediction approach is sufficient for redirect decisions between staying on task page vs going to projects page.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/products/[id]/files/+page.server.ts
📚 Learning: 2025-09-10T13:46:43.497Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1275
File: src/auth.ts:240-245
Timestamp: 2025-09-10T13:46:43.497Z
Learning: In src/auth.ts, the products branch intentionally uses params.id as a string for database lookups, not as a number. This is the correct behavior and should not be changed to numeric parsing.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts
📚 Learning: 2025-09-22T15:00:33.837Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts:37-45
Timestamp: 2025-09-22T15:00:33.837Z
Learning: In SvelteKit applications, param matchers like idNumber in src/params/idNumber.ts handle parameter validation at the routing level, ensuring that route handlers only execute with valid parameters. Manual validation in load functions is redundant when param matchers are used.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts
📚 Learning: 2025-09-22T15:01:28.442Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/lib/projects/index.ts:185-197
Timestamp: 2025-09-22T15:01:28.442Z
Learning: In the AppBuilder portal project, there's a policy inconsistency in canClaimProject: the docstring says "AppBuilder or OrgAdmin roles" but the implementation only allows OrgAdmin (via canModifyProject → isAdminForOrg). The UI code suggests AppBuilder users should have some project modification rights. This needs clarification from the maintainer.
Applied to files:
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.sveltesrc/lib/projects/index.ts
📚 Learning: 2025-09-25T03:51:13.786Z
Learnt from: 7dev7urandom
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts:7-8
Timestamp: 2025-09-25T03:51:13.786Z
Learning: In the organization invitation flow in src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts, the checkInviteErrors function handles validation of invite tokens including null/undefined cases, so non-null assertion on searchParams.get('t') is safe because error handling is centralized in checkInviteErrors.
Applied to files:
src/auth.ts
📚 Learning: 2025-03-27T13:00:13.162Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1079
File: source/SIL.AppBuilder.Portal/node-server/job-executors/recurring.ts:33-36
Timestamp: 2025-03-27T13:00:13.162Z
Learning: In the SIL.AppBuilder.Portal project running on Node 22, the code pattern using Set iterator methods like `.values().map().toArray()` is supported and works as expected in the backend environment.
Applied to files:
src/auth.ts
📚 Learning: 2025-09-22T15:01:42.995Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/auth.ts:13-13
Timestamp: 2025-09-22T15:01:42.995Z
Learning: SvelteKit uses the devalue library for serialization, which supports Map and Set objects. This means Maps can be safely stored in session data and will be properly serialized/deserialized for client-side usage, unlike standard JSON serialization which would lose Map objects.
Applied to files:
src/auth.ts
🧬 Code graph analysis (16)
src/routes/(unauthenticated)/(auth)/signout/+page.server.ts (1)
src/routes/(unauthenticated)/(auth)/login/+page.server.ts (1)
actions(20-25)
src/routes/(authenticated)/projects/[id=idNumber]/project.ts (3)
src/lib/projects/index.ts (1)
canModifyProject(170-179)src/lib/products/index.ts (1)
getProductActions(10-47)src/lib/projects/server.ts (1)
userGroupsForOrg(109-123)
src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts (2)
src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts (1)
POST(6-45)src/routes/(authenticated)/tasks/sse/+server.ts (1)
POST(5-31)
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts (3)
src/auth.ts (1)
Security(138-277)src/lib/projects/index.ts (2)
bulkProjectActionSchema(149-152)projectSearchSchema(72-79)src/lib/projects/server.ts (3)
userGroupsForOrg(109-123)projectFilter(8-100)doProjectAction(125-164)
src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts (3)
src/routes/(authenticated)/+layout.server.ts (1)
load(12-71)src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/users/server.ts (1)
adminOrgs(12-29)
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (4)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/routes/(authenticated)/projects/[id=idNumber]/project.ts (1)
getProjectDetails(11-285)src/lib/server/bullmq/queues.ts (1)
QueueConnected(86-86)src/lib/projects/server.ts (2)
userGroupsForOrg(109-123)doProjectAction(125-164)
src/routes/(authenticated)/products/[id]/files/+page.server.ts (2)
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (1)
load(36-55)src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
src/routes/(authenticated)/admin/settings/product-definitions/edit/+page.server.ts (6)
src/routes/(authenticated)/admin/jobs/[...path]/+page.server.ts (1)
load(3-6)src/routes/(authenticated)/admin/settings/build-engines/+page.server.ts (1)
load(6-11)src/routes/(authenticated)/admin/settings/organizations/+page.server.ts (1)
load(6-11)src/routes/(authenticated)/admin/settings/product-definitions/+page.server.ts (1)
load(6-18)src/routes/(authenticated)/admin/settings/organizations/new/+page.server.ts (1)
load(10-17)src/routes/(authenticated)/admin/settings/organizations/edit/+page.server.ts (1)
load(15-42)
src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts (2)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/routes/(authenticated)/projects/[id=idNumber]/project.ts (1)
getProjectDetails(11-285)
eslint-rules/require-security-check.ts (1)
eslint-rules/require-security-check.js (4)
val(12-12)result(15-15)result(21-21)nodeIsSecurityCall(28-51)
src/hooks.server.ts (3)
src/lib/otel/index.ts (2)
error(46-53)OTEL(56-112)src/auth.ts (2)
populateSecurityInfo(279-336)organizationInviteHandle(342-365)src/lib/server/bullmq/BullBoard.ts (1)
bullboardHandle(39-44)
src/lib/projects/index.ts (1)
src/lib/utils/roles.ts (1)
isAdminForOrg(9-12)
src/routes/(authenticated)/+layout.server.ts (1)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
src/lib/projects/server.ts (3)
src/auth.ts (1)
Security(138-277)src/lib/utils/roles.ts (1)
isAdminForOrg(9-12)src/lib/projects/index.ts (1)
canClaimProject(185-198)
src/app.d.ts (1)
src/auth.ts (1)
Security(138-277)
src/auth.ts (1)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
🔇 Additional comments (28)
src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts (2)
16-17: Explicitly allowing the unauthenticated flow is spot on.Calling
locals.security.requireNothing()keeps the handler compliant with the new security contract without interfering with the Auth0 bearer-token path. Nicely done.
105-112: Thanks for switching the role gate to the JWT subject.This closes the privilege-escalation hole we talked about earlier—write access is now derived from the authenticated user’s own roles instead of the browser session.
src/routes/(authenticated)/products/[id]/files/+page.server.ts (1)
9-17: Throw on missing Project before invoking the guard.
if (!project) error(404);just creates the HttpError but doesn’t throw it, so execution falls through andrequireProjectWriteAccessstill receivesundefined. That recreates the original bug and yields a 500 instead of a 404 when the product is missing. Throw the 404 before calling the guard.-export const load = (async ({ params, locals }) => { - const project = ( - await DatabaseReads.products.findUnique({ - where: { Id: params.id }, - select: { Project: { select: { OwnerId: true, OrganizationId: true } }, Id: true } - }) - )?.Project; - if (!project) error(404); - locals.security.requireProjectWriteAccess(project); +export const load = (async ({ params, locals }) => { + const productRecord = await DatabaseReads.products.findUnique({ + where: { Id: params.id }, + select: { Project: { select: { OwnerId: true, OrganizationId: true } }, Id: true } + }); + if (!productRecord?.Project) throw error(404); + locals.security.requireProjectWriteAccess(productRecord.Project);package.json (1)
58-60: Good: typescript-eslint toolchain aligned to ^8.44.0Upgrading parser/plugin/utils and root meta to the same version removes prior mismatch risks. Looks solid.
Also applies to: 94-94
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte (3)
111-111: Use radix with parseInt (and keep param-naming consistent).parseInt without a radix is brittle; pass 10. This also resolves a prior comment about the orgId param usage.
- $pageForm.organizationId = parseInt(navigation.to!.params!.orgId); + $pageForm.organizationId = parseInt(navigation.to!.params!.orgId, 10); ... -const organizationId = $derived(parseInt(page.params.orgId)); +const organizationId = $derived(parseInt(page.params.orgId, 10));Also applies to: 114-114
117-121: Passing user object to canArchive/canReactivate: LGTMThe calls now align with updated signatures that take a user object rather than session. Good.
416-424: canClaimProject user arg change: LGTMSwitching to pass data.session.user matches the new helper signatures.
eslint-rules/require-security-check.ts (2)
238-258: Autofix insertion can land outside braces for empty blocks; insert after “{”.This was previously flagged; still present. Use range-based insertion immediately after the opening brace.
- fix: (fixer) => - fixer.insertTextBefore( - bs.body[0] || bs, - 'event.locals.security.requireAuthenticated();\n' + - ((functionExport.id as TSESTree.Identifier).name === 'actions' - ? ' ' - : ' ') - ) + fix: (fixer) => + fixer.insertTextAfterRange( + [bs.range[0], bs.range[0] + 1], + '\n event.locals.security.requireAuthenticated();' + ) }, { messageId: 'suggestNoAuthCall', - fix: (fixer) => - fixer.insertTextBefore( - bs.body[0] || bs, - 'event.locals.security.requireNothing();\n' + - ((functionExport.id as TSESTree.Identifier).name === 'actions' - ? ' ' - : ' ') - ) + fix: (fixer) => + fixer.insertTextAfterRange( + [bs.range[0], bs.range[0] + 1], + '\n event.locals.security.requireNothing();' + )
21-42: Critical: walkFind can call callback on null and iterate null; TypeScript will also error.
- callback(node) is invoked when node can be null/undefined.
- for..in over node when node can be null throws at runtime.
- Accessing node[key] without narrowing violates types.
Harden the traversal and always return a value.
-function walkFind( - node: TSESTree.Node | undefined | null, - callback: (node: TSESTree.Node) => boolean, - maxDepth = 10 -): TSESTree.Node | null { - if (maxDepth <= 0) return null; - if (node && typeof node !== 'object') return null; - if (callback(node)) return node; - for (const key in node) { - if (key === 'parent') continue; - const val = node[key]; - if (Array.isArray(val)) { - for (const child of val) { - const result = walkFind(child, callback, maxDepth - 1); - if (result) return result; - } - } else { - const result = walkFind(val, callback, maxDepth - 1); - if (result) return result; - } - } - return null; -} +function walkFind( + node: TSESTree.Node | null | undefined, + callback: (node: TSESTree.Node) => boolean, + maxDepth = 10 +): TSESTree.Node | null { + if (maxDepth <= 0) return null; + if (!node || typeof node !== 'object') return null; + if (callback(node)) return node; + for (const key in node as Record<string, unknown>) { + if (key === 'parent') continue; + const val = (node as unknown as Record<string, unknown>)[key]; + if (Array.isArray(val)) { + for (const child of val as unknown[]) { + const result = walkFind(child as TSESTree.Node, callback, maxDepth - 1); + if (result) return result; + } + } else if (val && typeof val === 'object') { + const result = walkFind(val as TSESTree.Node, callback, maxDepth - 1); + if (result) return result; + } + } + return null; +}src/lib/utils/roles.ts (1)
18-20: OK: tuple-based helpersThese replacements avoid Map iterator pitfalls on the client. Looks good.
Also applies to: 24-26, 30-32
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts (3)
22-41: Security-aware whereStatements: LGTMCorrectly gates by super/OrgAdmin and falls back to group membership + org-scoped filter.
79-85: Good: org membership or super-admin required at loadThis matches the route’s authenticated scope and prevents data disclosure.
225-229: Redundant orgId validation for idNumber paramThe [orgId=idNumber] matcher guarantees a valid positive integer. The isNaN/string-equality checks are unnecessary.
- const orgId = parseInt(event.params.orgId!); - if (isNaN(orgId) || !(orgId + '' === event.params.orgId)) return fail(404); + const orgId = parseInt(event.params.orgId!, 10);src/app.d.ts (1)
21-21: Fix for default parameter in .d.ts: LGTMSwitching orOrgAdmin to optional resolves invalid default-in-declaration.
src/routes/(authenticated)/admin/settings/product-definitions/edit/+page.server.ts (1)
20-22: Correctly tightened to requireSuperAdminBoth load and action now enforce super-admin. Good alignment with other admin routes.
Also applies to: 53-55
src/auth.ts (1)
18-22: Session shape and population: LGTMroles stored as tuple array is client-serializable and consistent with usage across the app.
Also applies to: 120-129
src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts (1)
29-29: Good use ofadminOrgsfor editable-scope filtering.This aligns with the established pattern and avoids over-restrictive gating.
src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts (2)
17-20: Initial emit path looks correct.Emits are scoped with the validated id and current security session; error short-circuit is handled.
7-11: Guard against invalid project id before hitting the DB; avoid NaN → 5xx.Parse and validate the id first, fetch the project, then run the guard.
Apply this diff:
- request.locals.security.requireProjectWriteAccess( - await DatabaseReads.projects.findUniqueOrThrow({ - where: { Id: parseInt(request.params.id) } - }) - ); + const id = parseInt(request.params.id); + if (Number.isNaN(id)) throw error(400); + const project = await DatabaseReads.projects.findUniqueOrThrow({ where: { Id: id } }); + request.locals.security.requireProjectWriteAccess(project);Add missing import:
+import { error } from '@sveltejs/kit';src/routes/(authenticated)/+layout.server.ts (1)
13-15: Solid migration to security context.Auth guard +
secusage for queries are consistent with the new Security API.Confirm
event.locals.auth()is guaranteed non-null afterrequireAuthenticated()in all adapters.Also applies to: 27-31, 42-45
src/lib/projects/server.ts (2)
128-129: Parameter rename tosecurityand ownership update are consistent.Claim flow updates OwnerId with the current user; good alignment with Security.
Also applies to: 158-162
101-107: verifyCanCreateProject signature and callers validated
All calls passevent.locals.securitythenorganizationIdin the correct order; no updates required.src/routes/(authenticated)/projects/[id=idNumber]/project.ts (1)
1-1: All getProjectDetails call sites updated; no bare userId passed.src/hooks.server.ts (2)
61-62: Heartbeat: 503 on DB outage is correct.Throwing
error(503, ...)here is appropriate and will route throughhandleError.
69-69: Auth sequence composition looks good.
sequence(authRouteHandle, populateSecurityInfo)is the right order for these hooks.src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (2)
331-355: Project action auth branching looks correct.Claim vs. write access split is clear; groups prefetch for claim is correct and reused in doProjectAction.
202-207: Enforce group membership and Author role when adding authors.Action permits adding any org member. Must restrict to users in the project’s group with the Author role, and avoid duplicates. Also prefetch GroupId.
Apply this diff:
async addAuthor(event) { - event.locals.security.requireProjectWriteAccess( - await DatabaseReads.projects.findUniqueOrThrow({ - where: { Id: parseInt(event.params.id) }, - select: { OwnerId: true, OrganizationId: true } - }) - ); + const projectId = parseInt(event.params.id); + const minimal = await DatabaseReads.projects.findUniqueOrThrow({ + where: { Id: projectId }, + select: { OwnerId: true, OrganizationId: true, GroupId: true } + }); + event.locals.security.requireProjectWriteAccess(minimal); if (!QueueConnected()) return error(503); const form = await superValidate(event.request, valibot(addAuthorSchema)); if (!form.valid) return fail(400, { form, ok: false }); - const projectId = parseInt(event.params.id); - if ( - // if user modified hidden values - !(await DatabaseReads.organizationMemberships.findFirst({ - where: { - UserId: form.data.author, - Organization: { - Projects: { - some: { Id: projectId } - } - } - } - })) - ) { - return fail(403, { form, ok: false }); - } + // Validate: in project group, has Author role in org, and not already an Author + const canAdd = await DatabaseReads.users.findFirst({ + where: { + Id: form.data.author, + GroupMemberships: { some: { GroupId: minimal.GroupId } }, + UserRoles: { some: { OrganizationId: minimal.OrganizationId, RoleId: RoleId.Author } }, + Authors: { none: { ProjectId: projectId } } + }, + select: { Id: true } + }); + if (!canAdd) return fail(403, { form, ok: false });Also applies to: 211-241
src/lib/projects/index.ts (1)
171-178: canModifyProject: server/client-compatible roles check is good.Using
isAdminForOrg(organizationId, security.roles)avoids Map iterator pitfalls on the client.
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts
Outdated
Show resolved
Hide resolved
src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts
Show resolved
Hide resolved
7dev7urandom
force-pushed
the
fix/broken-authorization
branch
from
264100d to
7858ff1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (4)
37-47: Remove redundant id validation; [id=idNumber] already guarantees a valid integer.Param matcher enforces numeric id; this check is dead code. Parse once if needed.
Apply this diff:
locals.security.requireProjectReadAccess( await DatabaseReads.groupMemberships.findMany({ where: { UserId: locals.security.userId }, select: { GroupId: true } }), await DatabaseReads.projects.findUniqueOrThrow({ where: { Id: parseInt(params.id) }, select: { OwnerId: true, OrganizationId: true, GroupId: true } }) ); - if (isNaN(parseInt(params.id))) throw error(404, 'Not Found');
65-71: Prefer 404 on missing project; avoid Prisma “OrThrow” for auth prefetch.Use
findUnique+ explicit 404 before calling guards. Keeps error semantics consistent and avoids leaking ORM errors.Apply the pattern below (shown for one block; replicate in the other ranges):
- event.locals.security.requireProjectWriteAccess( - await DatabaseReads.projects.findUniqueOrThrow({ - where: { Id: parseInt(event.params.id) }, - select: { OwnerId: true, OrganizationId: true } - }) - ); + { + const minimal = await DatabaseReads.projects.findUnique({ + where: { Id: parseInt(event.params.id) }, + select: { OwnerId: true, OrganizationId: true } + }); + if (!minimal) throw error(404, 'Not Found'); + event.locals.security.requireProjectWriteAccess(minimal); + }Also applies to: 82-87, 114-121, 135-141, 165-171, 183-188, 202-207, 245-250, 262-267, 283-288
120-120: Throw 503 instead of returning it.SvelteKit expects
throw error(503). Returning it risks inconsistent handling.Apply this diff:
- if (!QueueConnected()) return error(503); + if (!QueueConnected()) throw error(503);
312-319: Queue connectivity check here seems unnecessary.Owner change doesn’t enqueue work in this action. Consider removing the 503 guard to avoid blocking benign updates.
Apply this diff:
- if (!QueueConnected()) return error(503);src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts (2)
80-83: Use throw redirect for clarity.
redirectthrows; usingthrowis the idiomatic pattern in SvelteKit.Apply this diff:
- if (!(await DatabaseReads.organizations.findFirst({ where: { Id: orgId } }))) { - return redirect(302, localizeHref(`/projects/${params.filter}`)); - } + if (!(await DatabaseReads.organizations.findFirst({ where: { Id: orgId } }))) { + throw redirect(302, localizeHref(`/projects/${params.filter}`)); + }
226-229: Drop redundant orgId validation; param matcher already enforces it.
[orgId=idNumber]guarantees a valid positive integer.Apply this diff:
- const orgId = parseInt(event.params.orgId!); - if (isNaN(orgId) || !(orgId + '' === event.params.orgId)) return fail(404); + const orgId = parseInt(event.params.orgId!);src/lib/projects/index.ts (1)
185-198: Align claim policy with docstring (include AppBuilder) or fix the doc.Doc says “AppBuilder or OrgAdmin,” but implementation only allows OrgAdmin. Either include AppBuilder or update the comment/UI.
Option A (include AppBuilder) — minimal change:
export function canClaimProject( security: Session['user'], projectOwnerId: number, organizationId: number, projectGroupId: number, userGroupIds: number[] ) { if (security.userId === projectOwnerId) return false; - if (security.roles.some(([_, r]) => r === RoleId.SuperAdmin)) return true; - return ( - canModifyProject(security, projectOwnerId, organizationId) && - userGroupIds.includes(projectGroupId) - ); + if (security.roles.some(([_, r]) => r === RoleId.SuperAdmin)) return true; + const hasAppBuilder = security.roles.some( + ([org, r]) => org === organizationId && r === RoleId.AppBuilder + ); + return userGroupIds.includes(projectGroupId) && + (hasAppBuilder || isAdminForOrg(organizationId, security.roles)); }If AppBuilder should not be permitted, update the docstring to avoid confusion.
src/routes/(unauthenticated)/(auth)/login/LoginScreen.svelte (1)
13-22: Tidy the polling interval.We never clear this interval on component teardown, so navigating away leaves a stray timer hammering
location.reload(). Wrap it inonDestroyto cleanly cancel when the component unmounts (and keep the handle outside the block for reuse).- if (!serviceAvailable && browser) { - let timeout = setInterval(() => { + let timeout: ReturnType<typeof setInterval> | undefined; + if (!serviceAvailable && browser) { + timeout = setInterval(() => { // Reload every 2 seconds to check if the service is back up if (serviceAvailable) { clearInterval(timeout); } else { location.reload(); } }, 2000); } + onDestroy(() => { + if (timeout) clearInterval(timeout); + });src/app.d.ts (1)
10-43: ExposesecurityHandledon the ambient Security type.
Line 21 currently lacks thesecurityHandledfield even though the runtime class mutates it, forcing consumers (e.g., hooks.server.ts) to suppress the type error. Adding the property lets us drop the@ts-expect-errorand keeps the ambient declaration aligned with the implementation.public readonly organizationMemberships: number[]; public readonly roles: Map<number, number[]>; + public securityHandled: boolean; public readonly sessionForm: Session['user'];src/routes/(authenticated)/projects/[id=idNumber]/project.ts (1)
1-1: Type source OK, but consider decoupling from @auth/sveltekit long‑termImport is type‑only, fine. For durability, consider a local alias (e.g., SecurityUser) so downstream code isn’t tightly coupled to the auth package type surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
eslint-rules/require-security-check.ts(1 hunks)package.json(2 hunks)src/app.d.ts(1 hunks)src/auth.ts(4 hunks)src/hooks.server.ts(4 hunks)src/lib/projects/index.ts(3 hunks)src/lib/projects/server.ts(4 hunks)src/lib/utils/roles.ts(1 hunks)src/routes/(authenticated)/admin/settings/product-definitions/edit/+page.server.ts(2 hunks)src/routes/(authenticated)/products/[id]/files/+page.server.ts(2 hunks)src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts(7 hunks)src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte(4 hunks)src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts(13 hunks)src/routes/(authenticated)/projects/[id=idNumber]/project.ts(5 hunks)src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts(2 hunks)src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts(2 hunks)src/routes/(unauthenticated)/(auth)/login/+page.server.ts(2 hunks)src/routes/(unauthenticated)/(auth)/login/LoginScreen.svelte(2 hunks)src/routes/(unauthenticated)/(auth)/signout/+page.server.ts(1 hunks)src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts
- src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts
- src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte
👮 Files not reviewed due to content moderation or server errors (3)
- package.json
- src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts
- src/routes/(authenticated)/admin/settings/product-definitions/edit/+page.server.ts
🧰 Additional context used
🧠 Learnings (15)
📚 Learning: 2025-09-22T15:01:28.442Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/lib/projects/index.ts:185-197
Timestamp: 2025-09-22T15:01:28.442Z
Learning: The functions in src/lib/projects/index.ts (canModifyProject, canClaimProject, canArchive, canReactivate) are used both server-side and client-side in Svelte components, so any changes must be compatible with both environments. Map iterator methods like .values().some() may not work consistently in all client environments.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/project.tssrc/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.tssrc/lib/utils/roles.tssrc/lib/projects/index.tssrc/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/auth.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.tssrc/routes/(authenticated)/products/[id]/files/+page.server.tssrc/lib/projects/server.ts
📚 Learning: 2025-09-25T03:51:13.786Z
Learnt from: 7dev7urandom
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts:7-8
Timestamp: 2025-09-25T03:51:13.786Z
Learning: In the organization invitation flow in src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts, the checkInviteErrors function properly handles null/undefined invite tokens by accepting inviteToken?: string | null and having an explicit guard clause if (!inviteToken) return { error: 'not found' }, making the non-null assertion on searchParams.get('t')! safe since error handling is centralized.
Applied to files:
src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.tssrc/routes/(unauthenticated)/(auth)/login/+page.server.tssrc/auth.ts
📚 Learning: 2025-09-22T13:48:06.537Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/auth.ts:145-146
Timestamp: 2025-09-22T13:48:06.537Z
Learning: Node.js 22+ with V8 12.4+ supports Iterator Helpers including Iterator.prototype.some(), which means Map.values().some() and other iterator methods work directly without needing Array.from() conversion. This was added through the Iterator Helpers proposal in 2024.
Applied to files:
src/lib/utils/roles.tssrc/auth.ts
📚 Learning: 2025-09-22T15:01:42.995Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/auth.ts:13-13
Timestamp: 2025-09-22T15:01:42.995Z
Learning: The codebase already successfully uses Map objects in session data that are serialized to the client via SvelteKit's devalue. Evidence: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte line 154 uses data.session?.user.roles.get(organizationId), proving Maps work fine in client-side Svelte components when passed through session data.
Applied to files:
src/lib/utils/roles.tssrc/auth.ts
📚 Learning: 2025-09-12T14:57:08.647Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:252-264
Timestamp: 2025-09-12T14:57:08.647Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to use transition.target[0] prediction mechanism for determining post-send state rather than calling flow.state() after flow.send() due to race condition concerns. The prediction approach is sufficient for redirect decisions between staying on task page vs going to projects page.
Applied to files:
src/routes/(unauthenticated)/(auth)/login/+page.server.ts
📚 Learning: 2025-09-12T14:07:02.200Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:285-303
Timestamp: 2025-09-12T14:07:02.200Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to optimize filterAvailableActions by creating Sets at the caller level rather than inside the function, so the function would take Set<number> arguments instead of arrays for better performance and separation of concerns.
Applied to files:
src/routes/(unauthenticated)/(auth)/login/+page.server.tssrc/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/admin/settings/product-definitions/edit/+page.server.tssrc/routes/(unauthenticated)/(auth)/signout/+page.server.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.tssrc/routes/(authenticated)/products/[id]/files/+page.server.ts
📚 Learning: 2025-09-25T03:51:13.786Z
Learnt from: 7dev7urandom
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts:7-8
Timestamp: 2025-09-25T03:51:13.786Z
Learning: In the organization invitation flow in src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts, the checkInviteErrors function handles validation of invite tokens including null/undefined cases, so non-null assertion on searchParams.get('t') is safe because error handling is centralized in checkInviteErrors.
Applied to files:
src/routes/(unauthenticated)/(auth)/login/+page.server.tssrc/auth.ts
📚 Learning: 2025-09-22T15:01:28.442Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/lib/projects/index.ts:185-197
Timestamp: 2025-09-22T15:01:28.442Z
Learning: In the AppBuilder portal project, there's a policy inconsistency in canClaimProject: the docstring says "AppBuilder or OrgAdmin roles" but the implementation only allows OrgAdmin (via canModifyProject → isAdminForOrg). The UI code suggests AppBuilder users should have some project modification rights. This needs clarification from the maintainer.
Applied to files:
src/lib/projects/index.ts
📚 Learning: 2025-09-17T21:07:18.139Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1275
File: src/lib/projects/index.ts:183-201
Timestamp: 2025-09-17T21:07:18.139Z
Learning: In the AppBuilder portal project, org admins must be in the project group to claim/take ownership of projects. This is enforced both in client-side logic (canClaimProject function) and server-side validation.
Applied to files:
src/lib/projects/index.tssrc/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.tssrc/lib/projects/server.ts
📚 Learning: 2025-09-22T14:59:53.346Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts:214-221
Timestamp: 2025-09-22T14:59:53.346Z
Learning: In the AppBuilder portal, bulk project actions in the route `/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts` are organization-scoped by design. The route parameter `[orgId=idNumber]` ensures all operations happen within a single organization context, so using the route-level orgId for all projects in bulk actions like `doProjectAction` is correct and safe.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.tssrc/lib/projects/server.ts
📚 Learning: 2025-09-10T13:46:43.497Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1275
File: src/auth.ts:240-245
Timestamp: 2025-09-10T13:46:43.497Z
Learning: In src/auth.ts, the products branch intentionally uses params.id as a string for database lookups, not as a number. This is the correct behavior and should not be changed to numeric parsing.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts
📚 Learning: 2025-09-22T15:00:33.837Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts:37-45
Timestamp: 2025-09-22T15:00:33.837Z
Learning: The idNumber param matcher in src/params/idNumber.ts validates that parameters are positive integers with exact string matching (no leading zeros), ensuring parseInt() calls in route handlers will never return NaN when using [id=idNumber] route parameters.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts
📚 Learning: 2025-09-22T15:00:33.837Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts:37-45
Timestamp: 2025-09-22T15:00:33.837Z
Learning: In SvelteKit applications, param matchers like idNumber in src/params/idNumber.ts handle parameter validation at the routing level, ensuring that route handlers only execute with valid parameters. Manual validation in load functions is redundant when param matchers are used.
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts
📚 Learning: 2025-03-27T13:00:13.162Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1079
File: source/SIL.AppBuilder.Portal/node-server/job-executors/recurring.ts:33-36
Timestamp: 2025-03-27T13:00:13.162Z
Learning: In the SIL.AppBuilder.Portal project running on Node 22, the code pattern using Set iterator methods like `.values().map().toArray()` is supported and works as expected in the backend environment.
Applied to files:
src/auth.ts
📚 Learning: 2025-09-22T15:01:42.995Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/auth.ts:13-13
Timestamp: 2025-09-22T15:01:42.995Z
Learning: SvelteKit uses the devalue library for serialization, which supports Map and Set objects. This means Maps can be safely stored in session data and will be properly serialized/deserialized for client-side usage, unlike standard JSON serialization which would lose Map objects.
Applied to files:
src/auth.ts
🧬 Code graph analysis (13)
src/routes/(authenticated)/projects/[id=idNumber]/project.ts (3)
src/lib/projects/index.ts (1)
canModifyProject(170-179)src/lib/products/index.ts (1)
getProductActions(10-47)src/lib/projects/server.ts (1)
userGroupsForOrg(109-123)
src/app.d.ts (1)
src/auth.ts (1)
Security(138-277)
src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts (2)
src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts (1)
POST(6-45)src/routes/(authenticated)/tasks/sse/+server.ts (1)
POST(5-31)
src/hooks.server.ts (3)
src/lib/otel/index.ts (2)
error(46-53)OTEL(56-112)src/auth.ts (2)
populateSecurityInfo(279-336)organizationInviteHandle(342-365)src/lib/server/bullmq/BullBoard.ts (1)
bullboardHandle(39-44)
src/routes/(unauthenticated)/(auth)/login/+page.server.ts (2)
src/routes/(unauthenticated)/(auth)/signout/+page.server.ts (1)
actions(10-15)src/auth.ts (1)
signIn(51-97)
src/lib/projects/index.ts (1)
src/lib/utils/roles.ts (1)
isAdminForOrg(9-12)
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (4)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/routes/(authenticated)/projects/[id=idNumber]/project.ts (1)
getProjectDetails(11-285)src/lib/server/bullmq/queues.ts (1)
QueueConnected(86-86)src/lib/projects/server.ts (2)
userGroupsForOrg(109-123)doProjectAction(125-164)
src/routes/(authenticated)/admin/settings/product-definitions/edit/+page.server.ts (4)
src/routes/(authenticated)/admin/settings/organizations/edit/+page.server.ts (1)
load(15-42)src/routes/(authenticated)/admin/settings/product-definitions/+page.server.ts (1)
load(6-18)src/routes/(authenticated)/admin/settings/product-definitions/new/+page.server.ts (1)
load(19-27)src/routes/(authenticated)/admin/settings/store-types/edit/+page.server.ts (1)
load(15-36)
src/auth.ts (1)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
src/routes/(unauthenticated)/(auth)/signout/+page.server.ts (1)
src/routes/(unauthenticated)/(auth)/login/+page.server.ts (1)
actions(20-25)
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts (2)
src/auth.ts (1)
Security(138-277)src/lib/projects/server.ts (3)
userGroupsForOrg(109-123)projectFilter(8-100)doProjectAction(125-164)
src/routes/(authenticated)/products/[id]/files/+page.server.ts (2)
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (1)
load(36-55)src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
src/lib/projects/server.ts (3)
src/auth.ts (1)
Security(138-277)src/lib/utils/roles.ts (1)
isAdminForOrg(9-12)src/lib/projects/index.ts (1)
canClaimProject(185-198)
🔇 Additional comments (17)
eslint-rules/require-security-check.ts (2)
25-39: GuardwalkFindfrom null nodes before invoking callbacks
walkFindstill calls the callback withnull/primitive nodes, so callbacks like(n) => n.type === 'BlockStatement'will explode at runtime when ESLint traverses optional AST fields (e.g.,alternate,body). Restore the earlier guard so we bail out before invoking the callback or iterating non-objects.if (maxDepth <= 0) return null; - if (node && typeof node !== 'object') return null; - if (callback(node)) return node; - for (const key in node) { + if (!node || typeof node !== 'object') return null; + if (callback(node)) return node; + const record = node as Record<string, unknown>; + for (const key in record) { if (key === 'parent') continue; - const val = node[key]; + const val = record[key]; if (Array.isArray(val)) { for (const child of val) { const result = walkFind(child, callback, maxDepth - 1); if (result) return result; } } else { - const result = walkFind(val, callback, maxDepth - 1); + if (!val || typeof val !== 'object') continue; + const result = walkFind(val as TSESTree.Node, callback, maxDepth - 1); if (result) return result; } } return null;
236-257: Fix autofix insertion point so it lands inside the block
insertTextBefore(bs.body[0] || bs, …)still emits the suggestion before the opening{when the block is empty, producing invalid code. Insert after the{instead (and reuse anindenthelper) so the quick-fix yields syntactically correct output.- context.report({ + const indent = + (functionExport.id as TSESTree.Identifier).name === 'actions' ? ' ' : ' '; + context.report({ node: bs.parent, messageId: 'missingSecurityCheck', suggest: [ { messageId: 'suggestAuthCall', fix: (fixer) => - fixer.insertTextBefore( - bs.body[0] || bs, - 'event.locals.security.requireAuthenticated();\n' + - ((functionExport.id as TSESTree.Identifier).name === 'actions' - ? ' ' - : ' ') - ) + fixer.insertTextAfterRange( + [bs.range[0], bs.range[0] + 1], + `\n${indent}event.locals.security.requireAuthenticated();` + ) }, { messageId: 'suggestNoAuthCall', fix: (fixer) => - fixer.insertTextBefore( - bs.body[0] || bs, - 'event.locals.security.requireNothing();\n' + - ((functionExport.id as TSESTree.Identifier).name === 'actions' - ? ' ' - : ' ') - ) + fixer.insertTextAfterRange( + [bs.range[0], bs.range[0] + 1], + `\n${indent}event.locals.security.requireNothing();` + ) } ] });src/auth.ts (1)
139-157: Fix nullrolescrash inSecurityconstructor
populateSecurityInfocreatesnew Security(event, null, null, null)for unauthenticated requests (see Line 331). Because the constructor still assumesroles/organizationMemberships/userIdare non-null, the first unauthenticated hit (e.g./login) blows up withTypeError: Cannot read properties of undefined (reading 'some')whenroles?.values()returnsundefined, and the very next line dereferencesthis.roles.entries(). Nothing downstream ever runs, so every anonymous route currently 500s. Please make the constructor (and corresponding typings) nullable-aware before iterating, then remove the@ts-expect-error.- public readonly userId: number, - public readonly organizationMemberships: number[], - public readonly roles: Map<number, RoleId[]> + public readonly userId: number | null, + public readonly organizationMemberships: number[] | null, + public readonly roles: Map<number, RoleId[]> | null ) { - this.isSuperAdmin = roles?.values().some((r) => r.includes(RoleId.SuperAdmin)) ?? false; - this.sessionForm = { - userId: this.userId, - roles: [ - ...this.roles - .entries() - .map(([orgId, roleIds]) => roleIds.map((r) => [orgId, r]) as [number, RoleId][]) - ].flat(1) - }; + const roleEntries = this.roles ? [...this.roles.entries()] : []; + this.isSuperAdmin = roleEntries.some(([, ids]) => ids.includes(RoleId.SuperAdmin)); + this.sessionForm = { + userId: this.userId ?? 0, + roles: roleEntries.flatMap(([orgId, ids]) => + ids.map((roleId) => [orgId, roleId] as [number, RoleId]) + ) + };Follow-up: update
src/app.d.tsto match the nullable fields and swap the sites that read fromthis.roles/this.organizationMembershipsto use a narrowed/local reference afterrequireAuthenticated()so the compiler stays happy once the types are corrected.src/lib/projects/server.ts (3)
101-107: LGTM: verifyCanCreateProject logic matches policy.Org Admin or AppBuilder allowed via
isAdminForOrg(...) || roles.get(orgId)?.includes(RoleId.AppBuilder).
156-163: LGTM: doProjectAction uses Security context correctly.Claim path uses
sessionFormand setsOwnerIdtosecurity.userId.
1-7: Add missing type import for Security.
verifyCanCreateProjectanddoProjectActionreferenceSecuritybut it's not imported; TS will error.Apply this diff:
import type { Prisma } from '@prisma/client'; import { RoleId } from '$lib/prisma'; import { type ProjectForAction, canClaimProject } from '$lib/projects'; import { BullMQ, getQueues } from '$lib/server/bullmq'; import { DatabaseReads, DatabaseWrites } from '$lib/server/database'; import { isAdminForOrg } from '$lib/utils/roles'; +import type { Security } from '../../auth';src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (1)
201-243: Enforce group membership and Author role when adding authors.Server currently allows users outside the project group and without Author role. Match UI and prevent privilege escalation.
Apply this diff:
- const projectId = parseInt(event.params.id); - if ( - // if user modified hidden values - !(await DatabaseReads.organizationMemberships.findFirst({ - where: { - UserId: form.data.author, - Organization: { - Projects: { - some: { Id: projectId } - } - } - } - })) - ) { - return fail(403, { form, ok: false }); - } + const projectId = parseInt(event.params.id); + // Validate user is in the project group and has Author role in the org + const minimal = await DatabaseReads.projects.findUnique({ + where: { Id: projectId }, + select: { OrganizationId: true, GroupId: true } + }); + if (!minimal) return fail(404, { form, ok: false }); + const canAdd = await DatabaseReads.users.findFirst({ + where: { + Id: form.data.author, + GroupMemberships: { some: { GroupId: minimal.GroupId } }, + UserRoles: { some: { OrganizationId: minimal.OrganizationId, RoleId: RoleId.Author } }, + Authors: { none: { ProjectId: projectId } } + }, + select: { Id: true } + }); + if (!canAdd) return fail(403, { form, ok: false });src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts (2)
25-39: LGTM: whereStatements integrates Security correctly.SuperAdmin/Admin bypass and group-scope for others look correct.
202-209: Don’t use Array.every with guard methods that throw.Guard throws abort the loop; the boolean result is unused. Iterate and let throws propagate (or catch explicitly).
Apply this diff:
- const allowAction = - form.data.operation === 'claim' - ? (p: (typeof projects)[0]) => event.locals.security.requireProjectClaimable(groups, p) - : (p: (typeof projects)[0]) => event.locals.security.requireProjectWriteAccess(p); - - if (!projects.every(allowAction)) { - return fail(403); - } + const allowAction = + form.data.operation === 'claim' + ? (p: (typeof projects)[0]) => event.locals.security.requireProjectClaimable(groups, p) + : (p: (typeof projects)[0]) => event.locals.security.requireProjectWriteAccess(p); + for (const p of projects) allowAction(p); // throws on first unauthorized projectsrc/lib/projects/index.ts (1)
171-179: LGTM: canModifyProject aligns with new roles shape.Owner or OrgAdmin via
isAdminForOrg(security.roles).src/routes/(unauthenticated)/(auth)/signout/+page.server.ts (1)
10-14: Sign-out redirect regression resolvedReturning the action result ensures AuthJS can drive the redirect/response again. Thanks for wiring
requireNothing()alongside the guard.src/routes/(authenticated)/products/[id]/files/+page.server.ts (1)
82-92: Return 404 before hitting the guard.We’re still piping
?.Projectstraight intorequireProjectWriteAccess. When the product doesn’t exist, we end up invoking the guard withundefined, which throws “Project is required…” instead of a clean 404 for the missing product. Please mirror the load handler: fetch once,throw error(404)if absent, then pass the definite Project to the guard.page: async ({ request, params, locals }) => { - locals.security.requireProjectWriteAccess( - ( - await DatabaseReads.products.findUnique({ - where: { Id: params.id }, - select: { - Project: { select: { OwnerId: true, OrganizationId: true } } - } - }) - )?.Project - ); + const productRecord = await DatabaseReads.products.findUnique({ + where: { Id: params.id }, + select: { + Project: { select: { OwnerId: true, OrganizationId: true } } + } + }); + if (!productRecord?.Project) throw error(404); + locals.security.requireProjectWriteAccess(productRecord.Project);src/routes/(authenticated)/projects/[id=idNumber]/project.ts (3)
16-16: LGTM: Tracing attribute now tied to authenticated identityUsing userSession.userId is consistent with the new security context.
271-273: LGTM: Org group lookup uses authenticated user identitySwapping to userSession.userId keeps behavior consistent under the new model.
201-217: LGTM: Centralized permission check + action gating
All getProjectDetails calls pass the security user object (locals.security.sessionForm) as the second argument—no numeric userId usages remain.src/routes/(unauthenticated)/(auth)/login/+page.server.ts (2)
8-15: LGTM: Security gating is explicit and consistentrequireNothing() at entry and redirecting authenticated users aligns with the new Security API.
21-25: LGTM: Action wrapped with security guarddefault action uses requireNothing() before signIn; consistent with signout and the broader refactor.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/routes/(authenticated)/directory/[id]/+page.server.ts (4)
21-23: Prefer 404 over 403 for private projects to avoid enumerationReturning 404 prevents leaking the existence of private resources to unauthorized users. Align this with your global policy.
- if (!projectCheck.IsPublic) return error(403); + if (!projectCheck.IsPublic) return error(404);Please confirm if other routes follow the same pattern.
28-31: Reduce payload for membership probe (+ consider index)Select only a scalar to minimize round‑trip and memory for the existence check. Ensure a composite index on (OrganizationId, UserId) exists for this hot path.
- (await DatabaseReads.organizationMemberships.findFirst({ - where: { OrganizationId: projectCheck.OrganizationId, UserId: locals.security.userId } - })) + (await DatabaseReads.organizationMemberships.findFirst({ + where: { OrganizationId: projectCheck.OrganizationId, UserId: locals.security.userId }, + select: { Id: true } + }))Operational: verify an index like
CREATE INDEX ON OrganizationMemberships (OrganizationId, UserId);
83-89: Prisma select: dynamic boolean for Url — verify typings
Url: allowDownloadsrelies on Prisma acceptingfalsein select. If TS complains (expectstrue), switch to conditional spread.Alternative pattern:
select: { ArtifactType: true, FileSize: true, DateUpdated: true, ...(allowDownloads && { Url: true }) }
105-114: Parallelize final queries to reduce latencyThe project and session group queries run sequentially. Use Promise.all after computing allowDownloads.
Example:
const [project, sessionGroupRows] = await Promise.all([ DatabaseReads.projects.findUniqueOrThrow({ where: { Id: id }, select: { /* same select as today */ } }), DatabaseReads.groupMemberships.findMany({ where: { UserId: locals.security.userId }, select: { GroupId: true } }) ]); return { project, allowDownloads, sessionGroups: sessionGroupRows.map((gm) => gm.GroupId) };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/projects/server.ts(4 hunks)src/routes/(authenticated)/directory/[id]/+page.server.ts(4 hunks)src/routes/(authenticated)/directory/[id]/+page.svelte(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-09-22T14:59:49.356Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/directory/[id]/+page.server.ts:10-11
Timestamp: 2025-09-22T14:59:49.356Z
Learning: In the appbuilder-portal codebase, calling SvelteKit's error() function already throws - using `return error(...)` vs `throw error(...)` doesn't matter, and technically the return statement isn't even needed since the function throws.
Applied to files:
src/routes/(authenticated)/directory/[id]/+page.server.ts
📚 Learning: 2025-09-12T14:02:53.478Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:205-263
Timestamp: 2025-09-12T14:02:53.478Z
Learning: In the appbuilder-portal codebase, use plain `redirect()` calls instead of `throw redirect()` - the throw is not necessary in their SvelteKit setup.
Applied to files:
src/routes/(authenticated)/directory/[id]/+page.server.ts
📚 Learning: 2025-09-22T15:01:28.442Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/lib/projects/index.ts:185-197
Timestamp: 2025-09-22T15:01:28.442Z
Learning: The functions in src/lib/projects/index.ts (canModifyProject, canClaimProject, canArchive, canReactivate) are used both server-side and client-side in Svelte components, so any changes must be compatible with both environments. Map iterator methods like .values().some() may not work consistently in all client environments.
Applied to files:
src/lib/projects/server.tssrc/routes/(authenticated)/directory/[id]/+page.svelte
📚 Learning: 2025-09-22T14:59:53.346Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts:214-221
Timestamp: 2025-09-22T14:59:53.346Z
Learning: In the AppBuilder portal, bulk project actions in the route `/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts` are organization-scoped by design. The route parameter `[orgId=idNumber]` ensures all operations happen within a single organization context, so using the route-level orgId for all projects in bulk actions like `doProjectAction` is correct and safe.
Applied to files:
src/lib/projects/server.ts
📚 Learning: 2025-09-17T21:07:18.139Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1275
File: src/lib/projects/index.ts:183-201
Timestamp: 2025-09-17T21:07:18.139Z
Learning: In the AppBuilder portal project, org admins must be in the project group to claim/take ownership of projects. This is enforced both in client-side logic (canClaimProject function) and server-side validation.
Applied to files:
src/lib/projects/server.ts
📚 Learning: 2025-09-22T15:01:42.995Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/auth.ts:13-13
Timestamp: 2025-09-22T15:01:42.995Z
Learning: The codebase already successfully uses Map objects in session data that are serialized to the client via SvelteKit's devalue. Evidence: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte line 154 uses data.session?.user.roles.get(organizationId), proving Maps work fine in client-side Svelte components when passed through session data.
Applied to files:
src/routes/(authenticated)/directory/[id]/+page.svelte
📚 Learning: 2025-09-22T15:01:06.464Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/directory/[id]/+page.svelte:24-24
Timestamp: 2025-09-22T15:01:06.464Z
Learning: In directory view pages, canModifyProject should be replaced with canViewProject for more appropriate permission checking, as mentioned in PR #1275. The directory should allow viewing projects with read access, not just modify access. However, canViewProject function doesn't exist yet and needs to be implemented.
Applied to files:
src/routes/(authenticated)/directory/[id]/+page.svelte
📚 Learning: 2025-09-22T15:01:28.442Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/lib/projects/index.ts:185-197
Timestamp: 2025-09-22T15:01:28.442Z
Learning: In the AppBuilder portal project, there's a policy inconsistency in canClaimProject: the docstring says "AppBuilder or OrgAdmin roles" but the implementation only allows OrgAdmin (via canModifyProject → isAdminForOrg). The UI code suggests AppBuilder users should have some project modification rights. This needs clarification from the maintainer.
Applied to files:
src/routes/(authenticated)/directory/[id]/+page.svelte
📚 Learning: 2025-09-22T15:01:06.464Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/directory/[id]/+page.svelte:24-24
Timestamp: 2025-09-22T15:01:06.464Z
Learning: PR #1275 "Allow members of project group to view but not edit" was supposed to introduce separate view and edit permissions, including a canViewProject function, but this wasn't fully implemented across all components. The directory view should use view permissions rather than modify permissions to determine if project names should be clickable links.
Applied to files:
src/routes/(authenticated)/directory/[id]/+page.svelte
🧬 Code graph analysis (2)
src/routes/(authenticated)/directory/[id]/+page.server.ts (1)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
src/lib/projects/server.ts (3)
src/auth.ts (1)
Security(138-277)src/lib/utils/roles.ts (1)
isAdminForOrg(9-12)src/lib/projects/index.ts (1)
canClaimProject(185-198)
🔇 Additional comments (3)
src/lib/projects/server.ts (1)
1-7: ImportSecurityto restore type checking.
Securityis referenced below but not imported, so TypeScript will fail to compile this module. Please add the missing type import.import { type ProjectForAction, canClaimProject } from '$lib/projects'; import { BullMQ, getQueues } from '$lib/server/bullmq'; import { DatabaseReads, DatabaseWrites } from '$lib/server/database'; import { isAdminForOrg } from '$lib/utils/roles'; +import type { Security } from '../../auth';src/routes/(authenticated)/directory/[id]/+page.server.ts (2)
40-41: Including GroupId looks goodHelpful for client logic; ensure the UI handles nulls.
108-112: Confirm locals.security.userId type matches DB (number vs string)Ensure
locals.security.userIdis a number; Prisma expectsintfor UserId. If it’s a string, coerce once at the source.
7dev7urandom
force-pushed
the
fix/broken-authorization
branch
2 times, most recently
from
4f3e5a9 to
30fd283
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte (1)
416-424: Clarify AppBuilder modify/claim policy
- Server’s
canModifyProjectonly grants owners & OrgAdmins (not AppBuilder), butcanClaimProjectdoc says AppBuilder should be allowed—implementation ignores AppBuilder. Either update server guards to include AppBuilder or remove AppBuilder affordances in the UI.- No need for
data.session?.user—data.session.useris always defined in authenticated routes and matchescanClaimProject’sSession['user']parameter.src/routes/(authenticated)/projects/new/[id=idNumber]/+page.server.ts (1)
12-60: Avoid double-checking the same role
requireHasRole(..., RoleId.AppBuilder, true)already ensures the user is AppBuilder (or org admin/super admin). Immediately callingverifyCanCreateProjectrepeats the same logic and returns 403 for the exact condition the guard just enforced. This extra check is redundant, adds maintenance overhead, and creates an unnecessary second source of truth. Please remove theverifyCanCreateProjectcall (or refactor it) so the handler relies on the centralized guard as intended.src/routes/(unauthenticated)/request-access-for-organization/+page.server.ts (1)
13-26: Reintroduce the load hook for form initializationThe previous
loadexported asuperValidateform, which the Svelte page still imports asform. With that hook removed, the page now receivesundefinedand the UI fails. Please restore the load function (withlocals.security.requireNothing()inside) so the form is initialized before rendering.src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts (2)
48-52: Return 401 when JWT subject is unknown instead of 500Unknown or missing user mapping should be treated as unauthorized, not server error.
- if (user.length !== 1) { - // Should never happen - // (unless the user doesn't exist?) - return error(500, `Internal Server Error`); - } + if (user.length !== 1) { + return error(401, `Unauthorized`); + }
262-266: Enforce issuer and audience in JWT validation
- In decryptJwtWithAuth0 (src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts lines 262–266), call
jwtVerify(jwt, async (h) => (await secrets).get(h.kid!), { issuer: \https://${env.AUTH0_DOMAIN}/`, audience: env.AUTH0_AUDIENCE } )`- Add AUTH0_AUDIENCE to your .env, CI workflows (.github/workflows/main.yml) and docker-compose.yml.
🧹 Nitpick comments (39)
src/routes/+page.server.ts (1)
7-8: Usethrow redirect(...)for clarity.SvelteKit’s
redirectthrows; usingthrowmakes intent explicit and avoids accidental continuation if the API ever changes.- if (event.locals.security.userId) redirect(302, localizeHref('/tasks')); + if (event.locals.security.userId) throw redirect(302, localizeHref('/tasks'));src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts (1)
16-19: Deduplicateidparsing and reuse the validated value.After validating
idonce, keep and reuse it for the initial emit and subsequent updates.- const { id: strId } = request.params; + // `id` already parsed and validated above return produce(async function start({ emit, lock }) { - const id = parseInt(strId); + // use the validated `id` from the outer scope // User will be allowed to see project updates until they reload // even if their permission is revoked during the SSE connection. - const { error } = emit( + const { error } = emit( 'projectData', - JSON.stringify(await getProjectDetails(id, request.locals.security.sessionForm)) + JSON.stringify(await getProjectDetails(id, request.locals.security.sessionForm)) );Also applies to: 21-25, 33-33
src/lib/server/bullmq/BullBoard.ts (1)
39-44: Guardbullboardnull and centralize the gate in the handle.
- Add a runtime null-guard for
bullboard(safe in build/prerender contexts).- Keep the super-admin check; optionally return 404 for non‑admins to avoid endpoint enumeration.
export const bullboardHandle: Handle = async ({ event, resolve }) => { - if (event.url.pathname.match(/^\/admin\/jobs($|\/)/) && event.locals.security.isSuperAdmin) { - return bullboard!.fetch(event.request); - } - return resolve(event); + if (!bullboard) return resolve(event); + if (event.url.pathname.match(/^\/admin\/jobs($|\/)/)) { + if (event.locals.security.isSuperAdmin) { + return bullboard.fetch(event.request); + } + // Optionally conceal existence: + // return new Response('Not found', { status: 404 }); + } + return resolve(event); };src/routes/(authenticated)/organizations/[id=idNumber]/settings/infrastructure/+page.server.ts (1)
8-9: ParseorgIdonce per handler and reuse.Improves readability and avoids repeated parsing.
export const load = (async (event) => { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + const orgId = parseInt(event.params.id, 10); + event.locals.security.requireAdminOfOrg(orgId); const { organization } = await event.parent(); const form = await superValidate( { buildEngineUrl: organization.BuildEngineUrl, buildEngineApiAccessToken: organization.BuildEngineApiAccessToken, useDefaultBuildEngine: organization.UseDefaultBuildEngine ?? false }, valibot(infrastructureSchema) ); return { form }; }) satisfies PageServerLoad; export const actions = { async default(event) { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + const orgId = parseInt(event.params.id, 10); + event.locals.security.requireAdminOfOrg(orgId); const form = await superValidate(event.request, valibot(infrastructureSchema)); if (!form.valid) return fail(400, { form, ok: false }); await DatabaseWrites.organizations.update({ where: { - Id: parseInt(event.params.id) + Id: orgId }, data: { BuildEngineApiAccessToken: form.data.buildEngineApiAccessToken, BuildEngineUrl: form.data.buildEngineUrl, UseDefaultBuildEngine: form.data.useDefaultBuildEngine } }); return { form, ok: true }; } } satisfies Actions;Also applies to: 23-29
src/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.server.ts (3)
18-19: Avoid parseInt; derive orgId once and reuse.The
[id=idNumber]matcher guarantees numeric; preferNumber()to avoid partial parses and reuse the value.Apply:
-export const load = (async (event) => { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); +export const load = (async (event) => { + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId);Based on learnings
38-45: DRY orgId handling in action; reuse validated value.Compute once, reuse for both guard and update.
Apply:
async togglePublic(event) { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const form = await superValidate(event.request, valibot(togglePublicSchema)); if (!form.valid) return fail(400, { form, ok: false }); await DatabaseWrites.organizations.update({ where: { - Id: parseInt(event.params.id) + Id: orgId },Based on learnings
53-60: Same here: compute orgId once for guard and write.Prevents repeated parsing and partial parse edge cases.
Apply:
async toggleProduct(event) { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const form = await superValidate(event.request, valibot(toggleProductSchema)); if (!form.valid) return fail(400, { form, ok: false }); await DatabaseWrites.organizationProductDefinitions.toggleForOrg( - parseInt(event.params.id), + orgId, form.data.prodDefId, form.data.enabled );Based on learnings
src/routes/(unauthenticated)/api/products/[package]/published/+server.ts (1)
18-21: Add caching headers to reduce load and latency.Public, unauthenticated manifest is a strong cache candidate.
Apply:
- return res - ? new Response(JSON.stringify(res), { status: 200 }) - : new Response(null, { status: 404 }); + return res + ? new Response(JSON.stringify(res), { + status: 200, + headers: { 'Cache-Control': 'public, max-age=300, s-maxage=300, stale-while-revalidate=60', 'Content-Type': 'application/json' } + }) + : new Response(null, { + status: 404, + headers: { 'Cache-Control': 'public, max-age=60' } + });src/routes/(authenticated)/directory/[id]/+page.svelte (1)
8-8: Verify client-safe import for RoleId.Importing from
$lib/prismain a Svelte component can pull server-only Prisma code into the client bundle. Ensure$lib/prismaexports RoleId from a browser-safe module, or move RoleId constants to a shared isomorphic module (e.g.,$lib/roles).Do you want a quick PR-ready refactor to extract RoleId into a tiny shared module?
src/routes/+layout.server.ts (1)
4-6: Don’t lie to the type system withuData!at the root layout.Root layout runs for unauthenticated routes too; asserting non-null risks misuse down the tree. Keep
session’s nullable type here and let the authenticated segment (/(authenticated)) assert non-null if desired.- event.locals.security.requireNothing(); + event.locals.security.requireNothing(); const uData = await event.locals.auth(); - return { session: uData! }; + return { session: uData };eslint.config.js (1)
10-10: Nice: custom rule wired in. Consider scoping to server files to cut noise.The rule already guards by filename, but adding an override limited to server files reduces traversal and false positives.
Here’s an example override to apply the rule only to server handlers:
{ - rules: { + files: ['**/*.server.ts', '**/+server.ts', '**/+page.server.ts', '**/+layout.server.ts'], + rules: { 'no-unused-vars': 'off', '@typescript-eslint/no-unused-vars': [ 'warn', { args: 'none' } ], '@typescript-eslint/consistent-type-imports': 'warn', 'import/order': [ 'warn', { groups: [['builtin', 'external', 'internal'], 'parent', 'sibling'], alphabetize: { order: 'asc' }, named: true } ], 'import/no-unresolved': 'off', - 'sveltekit-sec/require-security-check': 'error' + 'sveltekit-sec/require-security-check': 'error' } },Also applies to: 27-33, 71-71
src/routes/(authenticated)/organizations/[id=idNumber]/settings/groups/+page.server.ts (1)
14-14: DRY the orgId parsing and use Number() for clarity.You parse
event.params.idthree times withparseInt. Parse once per handler and reuse; preferNumber()for base-10 numeric route params.Apply these localized changes:
export const load = (async (event) => { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const { organization } = await event.parent(); return { - groups: await DatabaseReads.groups.findMany({ where: { OwnerId: organization.Id } }), + groups: await DatabaseReads.groups.findMany({ where: { OwnerId: organization.Id } }), form: await superValidate(valibot(addGroupSchema)) }; }) satisfies PageServerLoad; export const actions = { async addGroup(event) { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const form = await superValidate(event.request, valibot(addGroupSchema)); if (!form.valid) return fail(400, { form, ok: false }); const group = await DatabaseWrites.groups.createGroup( form.data.name, form.data.abbreviation, - parseInt(event.params.id) + orgId ); return { form, ok: true, createdId: group.Id }; }, async deleteGroup(event) { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const form = await superValidate(event.request, valibot(deleteSchema)); if (!form.valid) return fail(400, { form, ok: false }); if ( // if user modified hidden values !(await DatabaseReads.groups.findFirst({ - where: { Id: form.data.id, OwnerId: parseInt(event.params.id) } + where: { Id: form.data.id, OwnerId: orgId } })) ) { return fail(403, { form, ok: false }); } return { form, ok: await DatabaseWrites.groups.deleteGroup(form.data.id) }; } } satisfies Actions;Based on learnings
Also applies to: 24-24, 35-35
src/routes/(authenticated)/tasks/sse/+server.ts (1)
5-8: Minor: prefer RequestEvent naming/destructure for clarityUsing the conventional event param and destructuring locals improves readability and consistency across routes.
-export async function POST(request) { - request.locals.security.requireAuthenticated(); - const userId = request.locals.security.userId; +export async function POST({ locals }) { + locals.security.requireAuthenticated(); + const userId = locals.security.userId;src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts (1)
20-22: Harden inviteToken cookie flagsSet secure/httpOnly/sameSite to reduce XSS and CSRF risks; JS access isn’t needed server-side.
- event.cookies.set('inviteToken', inviteToken, { - path: '/' - }); + event.cookies.set('inviteToken', inviteToken, { + path: '/', + httpOnly: true, + sameSite: 'lax', + secure: true + });src/routes/(authenticated)/projects/[filter=projectSelector]/+page.server.ts (1)
16-21: Minor: parse cookie once and reuseSlight simplification and avoids double Number() calls.
- if ( - !isNaN(Number(orgLastSelected)) && - event.locals.security.organizationMemberships.includes(Number(orgLastSelected)) - ) { - orgRedirect = Number(orgLastSelected); - } + const last = Number.parseInt(orgLastSelected, 10); + if (!Number.isNaN(last) && event.locals.security.organizationMemberships.includes(last)) { + orgRedirect = last; + }src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts (2)
133-146: Avoid including PII in 403 error payloadsThe 403 payload returns the user’s name and email. Prefer a generic message to minimize PII exposure.
- title: `Project id=${projectId}, user='${user[0].Name}' with email='${user[0].Email}' does not have permission to access` + title: `Project id=${projectId}: user does not have permission to access`
208-226: Sanitize the Use header to a known allowlistLimit Use to known values and reuse the constant for Download. This also lets you drop the eslint-disable on TOKEN_USE_DOWNLOAD.
- let use = readOnly ? 'ReadOnly Access' : 'ReadWrite Access'; - - if (tokenUse) { - use = tokenUse; - } + const allowedUses = new Set([TOKEN_USE_UPLOAD, TOKEN_USE_DOWNLOAD]); + let use = readOnly ? 'ReadOnly Access' : 'ReadWrite Access'; + if (tokenUse && allowedUses.has(tokenUse)) { + use = tokenUse; + }src/routes/(authenticated)/directory/[id]/+page.server.ts (3)
7-8: Fix the misleading comment or remove itThe route requires authentication; “Anyone can view a public project” is inaccurate here.
85-86: Return 404 for private projects to avoid existence leaksPrefer 404 over 403 when a project isn’t public.
- if (!project.IsPublic) return error(403); + if (!project.IsPublic) return error(404);
95-103: Avoid type mismatch when scrubbing URLsIf Url is typed as string, setting it to null may break consumers. Use an empty string or remove the property.
- if (!allowDownloads) { - project.Products.forEach((p) => { - p.ProductPublications.forEach((pp) => { - pp.ProductBuild.ProductArtifacts.forEach((pa) => { - pa.Url = null; - }); - }); - }); - } + if (!allowDownloads) { + project.Products.forEach((p) => { + p.ProductPublications.forEach((pp) => { + pp.ProductBuild.ProductArtifacts.forEach((pa) => { + pa.Url = ''; + }); + }); + }); + }src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts (1)
6-19: Fetch the subject first, then enforce admin-of-orgsIf the subject doesn’t exist, the current order may pass an empty org list into requireAdminOfOrgIn (implementation-dependent). Safer to 404 first, then enforce org-admin check.
- const userId = Number(params.id); - if (isNaN(userId)) return error(404); - if (locals.security.userId !== userId) { - locals.security.requireAdminOfOrgIn( - await DatabaseReads.organizationMemberships - .findMany({ - where: { UserId: userId }, - select: { OrganizationId: true } - }) - .then((orgs) => orgs.map((o) => o.OrganizationId)) - ); - } - const subject = await DatabaseReads.users.findUniqueOrThrow({ + const userId = Number(params.id); + if (isNaN(userId)) return error(404); + const subject = await DatabaseReads.users.findUniqueOrThrow({ where: { Id: userId }, select: { Id: true, Name: true } }); + if (locals.security.userId !== userId) { + locals.security.requireAdminOfOrgIn( + await DatabaseReads.organizationMemberships + .findMany({ + where: { UserId: userId }, + select: { OrganizationId: true } + }) + .then((orgs) => orgs.map((o) => o.OrganizationId)) + ); + }src/routes/(authenticated)/directory/+page.server.ts (1)
10-11: Confirm directory auth policy (now gated).Both load and page action now require authentication. If the directory is intended to be publicly browsable for public projects, this is a behavior change. Confirm expected policy and adjust accordingly; otherwise, consider updating any stale comments suggesting public access.
Also applies to: 63-65
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (3)
37-48: Call requireAuthenticated before DB reads in loader.Avoid doing two DB calls for unauthenticated requests; short‑circuit first.
Apply:
const projectId = Number(params.id); if (isNaN(projectId)) throw error(404, 'Not Found'); + locals.security.requireAuthenticated(); locals.security.requireProjectReadAccess( await DatabaseReads.groupMemberships.findMany({
115-121: Don’t block deleteReviewer on queue connectivity.This action doesn’t enqueue any jobs; gating on
QueueConnected()unnecessarily prevents reviewer removal during queue downtime.Apply:
- if (!QueueConnected()) return error(503);
66-71: Reduce duplication: factor minimal project fetch.The same
findUnique({ select: { OwnerId, OrganizationId(, GroupId) } })appears across actions. Consider a tiny helper to fetch once and reuse; it will also make it easier to include GroupId consistently where needed.Also applies to: 83-88, 115-121, 136-141, 166-171, 184-189, 203-208, 261-266, 278-283, 299-304
src/routes/(authenticated)/users/[id=idNumber]/settings/roles/+page.server.ts (1)
20-27: Authenticate before computing subject orgs.Call
requireAuthenticated()first to avoid DB work for unauthenticated requests;requireAdminOfOrgInwill still perform role checks afterwards.Apply:
export const load = (async ({ params, locals }) => { const subjectId = parseInt(params.id); + locals.security.requireAuthenticated(); locals.security.requireAdminOfOrgIn( @@ export const actions = { async default(event) { + event.locals.security.requireAuthenticated(); event.locals.security.requireAdminOfOrgIn(Also applies to: 49-56
src/routes/(authenticated)/projects/[id=idNumber]/edit/+page.server.ts (1)
18-33: Non-null assertion on project is unnecessary and brittle.
requireProjectWriteAccess(project)already 404s whenprojectis null. The!at Line 31 isn’t needed and can hide nulls during refactors.- }))!; - locals.security.requireProjectWriteAccess(project); + })); + locals.security.requireProjectWriteAccess(project);src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts (3)
200-206: Use a for…of to fail fast on first unauthorized project.
forEachignores return and is less clear when the callback throws; a simple loop is clearer and stops on first denial.- projects.forEach( - form.data.operation === 'claim' - ? (p: (typeof projects)[0]) => event.locals.security.requireProjectClaimable(groups, p) - : (p: (typeof projects)[0]) => event.locals.security.requireProjectWriteAccess(p) - ); + const allow = form.data.operation === 'claim' + ? (p: (typeof projects)[0]) => event.locals.security.requireProjectClaimable(groups, p) + : (p: (typeof projects)[0]) => event.locals.security.requireProjectWriteAccess(p); + for (const p of projects) allow(p);
223-229: Throw 503 instead of returning; idNumber matcher makes NaN guard redundant.
error()must be thrown. Also,[orgId=idNumber]guarantees a valid positive integer, so the NaN check can be removed.- const orgId = parseInt(event.params.orgId!); - if (isNaN(orgId) || !(orgId + '' === event.params.orgId)) return fail(404); + const orgId = parseInt(event.params.orgId!); + // idNumber matcher guarantees a valid numeric orgId- if (!QueueConnected()) return error(503); + if (!QueueConnected()) throw error(503);Based on learnings
79-84: Micro: avoid double parse and use a single orgId variable.You parse
orgIdtwice (Lines 80 and 82). Parse once and reuse.- locals.security.requireMemberOfOrgOrSuperAdmin(parseInt(params.orgId)); - const orgId = parseInt(params.orgId); + const orgId = parseInt(params.orgId); + locals.security.requireMemberOfOrgOrSuperAdmin(orgId);src/lib/utils/roles.ts (1)
9-15: Simplify and short‑circuit SuperAdmin first.
!!orgRolesis always true; also check SuperAdmin before computing org roles.-export function isAdminForOrg(orgId: number, roles: ClientRolesArray): boolean { - const orgRoles = roles.filter(([oId]) => oId === orgId).map(([, r]) => r); - return ( - !!orgRoles && - (roles.some(([_, r]) => r === RoleId.SuperAdmin) || orgRoles.includes(RoleId.OrgAdmin)) - ); -} +export function isAdminForOrg(orgId: number, roles: ClientRolesArray): boolean { + if (roles.some(([, r]) => r === RoleId.SuperAdmin)) return true; + return roles.some(([oId, r]) => oId === orgId && r === RoleId.OrgAdmin); +}src/auth.ts (1)
86-88: Consider preserving returnTo URL during sign-in.After successful authentication, users might lose their intended destination if they were redirected to login.
Consider preserving the returnTo URL parameter through the authentication flow to redirect users back to their intended destination after successful login.
src/routes/(authenticated)/+layout.svelte (1)
152-183: Guard against undefined roles in client checks.If roles can be momentarily undefined during hydration, isAdminForAny should handle it. Consider a defensive default.
Apply this diff:
-{#if isAdminForAny(page.data.session?.user.roles)} +{#if isAdminForAny(page.data.session?.user.roles ?? [])}Ensure isAdminForAny’s signature accepts an empty array (ClientRolesArray) without changing semantics.
src/routes/(authenticated)/users/+page.svelte (1)
68-79: Defensive default for roles to avoid runtime edge cases.Mirror the layout change to prevent any transient undefined.
Apply this diff:
-{#if isAdminForAny(data.session?.user.roles)} +{#if isAdminForAny(data.session?.user.roles ?? [])}Confirm roles is always present under (authenticated), or keep the defensive default.
src/routes/(authenticated)/users/[id=idNumber]/settings/profile/+page.server.ts (2)
27-27: Preferthrow error(404)for clarity.
error(404)throws already, butreturn error(404)is misleading. Usethrow error(404)for readability and consistency with our other handlers.- if (!user) return error(404); + if (!user) throw error(404);
81-82: Confirm Prisma ignoresundefinedon update.You rely on
undefinedto skip updatingIsLockedfor self-edits. Ensure Prisma’s TS types acceptIsLocked?: booleanand the runtime ignoresundefined, or switch to conditional object spread.- IsLocked: user.Id === event.locals.security.userId ? undefined : !form.data.active + ...(user.Id === event.locals.security.userId ? {} : { IsLocked: !form.data.active })src/routes/(authenticated)/users/+page.server.ts (1)
219-223: Same telemetry issue in action: stringify Map -> entries.Mirror the load-handler fix so roles are observable.
- span.setAttributes({ - 'auth.load.userId': event.locals.security.userId, - 'auth.load.user.roles': JSON.stringify(event.locals.security.roles), + span.setAttributes({ + 'auth.load.userId': event.locals.security.userId, + 'auth.load.user.roles': JSON.stringify(Array.from(event.locals.security.roles.entries())), 'auth.load.user.isSuper': isSuper, 'auth.load.user.orgIds': JSON.stringify(orgIds) });src/routes/(authenticated)/products/[id]/files/+page.server.ts (2)
15-16: Preferthrow error(404)for consistency.
error(404)throws; usingthrowmakes intent explicit and matches our style elsewhere.- if (!project) error(404); + if (!project) throw error(404);
9-16: Minor: avoid double product fetch.You can select both the authZ fields and the page fields in one query to cut a DB round-trip.
- const project = ( - await DatabaseReads.products.findUnique({ - where: { Id: params.id }, - select: { Project: { select: { OwnerId: true, OrganizationId: true } }, Id: true } - }) - )?.Project; + const product = await DatabaseReads.products.findUnique({ + where: { Id: params.id }, + select: { + WorkflowBuildId: true, + ProductDefinition: { select: { Name: true } }, + Project: { select: { Id: true, Name: true, OwnerId: true, OrganizationId: true } } + } + }); - if (!project) error(404); - locals.security.requireProjectWriteAccess(project); + if (!product?.Project) throw error(404); + locals.security.requireProjectWriteAccess(product.Project); @@ - const product = await DatabaseReads.products.findUnique({ - where: { - Id: params.id - }, - select: { - WorkflowBuildId: true, - ProductDefinition: { - select: { - Name: true - } - }, - Project: { - select: { - Id: true, - Name: true - } - } - } - });Also applies to: 54-72
src/routes/(authenticated)/admin/settings/organizations/new/+page.server.ts
Outdated
Show resolved
Hide resolved
src/routes/(authenticated)/organizations/[id=idNumber]/settings/stores/+page.server.ts
Show resolved
Hide resolved
src/routes/(authenticated)/organizations/[id=idNumber]/settings/stores/+page.server.ts
Show resolved
Hide resolved
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts
Show resolved
Hide resolved
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts
Show resolved
Hide resolved
src/routes/(authenticated)/projects/import/[id=idNumber]/+page.server.ts
Show resolved
Hide resolved
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts
Show resolved
Hide resolved
Transition a couple routes to new security system
Fix new coderabbit issues canModifyProject -> canViewProject
Fix 400 codes -> 404 Handle 404 errors correctly
Add orgId check in user groups Sanitize redirect URL Destroy LoginScreen poll interval Remove unneeded query Fix issue with ProjectFilterSelector Fix issue with ProjectFilterSelector Fix project claimability check Fix role stringification for OTEL in /users
FyreByrd
force-pushed
the
fix/broken-authorization
branch
from
30fd283 to
b4488d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/routes/(authenticated)/tasks/[product_id]/+page.server.ts (1)
31-311: Import theSecuritytype
verifyCanViewTasknow referencesSecurity, but this file doesn’t import that symbol. TypeScript will flag “Cannot find name 'Security'”. Add the missing import (e.g.,import type { Security } from '$src/auth'depending on your alias).-import { WorkflowAction, WorkflowState, artifactLists } from '$lib/workflowTypes'; +import { WorkflowAction, WorkflowState, artifactLists } from '$lib/workflowTypes'; +import type { Security } from '$src/auth';src/routes/(authenticated)/projects/import/[id=idNumber]/+page.server.ts (1)
198-205: Validate group belongs to organization before writes.form.data.group can be tampered; ensure the group is owned by organizationId to prevent cross‑org linkage and data integrity issues.
Apply this diff before creating the import:
if (!errors.length) { + // Ensure selected group belongs to the organization + const validGroup = await DatabaseReads.groups.findFirst({ + where: { Id: form.data.group, OwnerId: organizationId }, + select: { Id: true } + }); + if (!validGroup) return error(400); + const existingProjects = ( await DatabaseReads.projects.findMany({Also applies to: 210-217
🧹 Nitpick comments (36)
src/routes/(unauthenticated)/api/products/[package]/published/+server.ts (1)
18-21: Set JSON content-type (or use SvelteKit json helper).The Response lacks a content-type header. Prefer json() for correctness and simplicity.
Apply:
+import { json } from '@sveltejs/kit'; @@ - return res - ? new Response(JSON.stringify(res), { status: 200 }) - : new Response(null, { status: 404 }); + return res ? json(res) : new Response(null, { status: 404 });src/routes/(authenticated)/organizations/[id=idNumber]/settings/infrastructure/+page.server.ts (2)
8-8: Avoid repeated parsing; compute orgId once.Minor clarity/DRY improvement: parse once and reuse.
-export const load = (async (event) => { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); +export const load = (async (event) => { + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId);
23-29: Reuse orgId in action; single source of truth.Same refactor here; also reuse in the update where clause.
async default(event) { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); @@ - where: { - Id: parseInt(event.params.id) - }, + where: { Id: orgId },src/routes/(authenticated)/organizations/[id=idNumber]/settings/groups/+page.server.ts (1)
14-14: Use Number() and (optionally) guard NaN for orgId parsingPrefer Number(...) over parseInt(...) to avoid partial parses (e.g., '10abc' -> 10). Optionally guard against NaN for defense-in-depth.
Apply this minimal change:
- event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + event.locals.security.requireAdminOfOrg(Number(event.params.id));Repeat similarly in actions:
- event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + event.locals.security.requireAdminOfOrg(Number(event.params.id));If you want to hard‑fail on invalid ids, also add a guard:
+ const orgId = Number(event.params.id); + if (Number.isNaN(orgId)) throw error(400, 'Invalid organization id'); - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + event.locals.security.requireAdminOfOrg(orgId);And import error once:
// at top of file import { error } from '@sveltejs/kit';Also applies to: 24-24, 35-35
src/routes/(authenticated)/admin/settings/stores/+page.server.ts (1)
6-7: LGTM; optional: destructure event for consistencyAuthorization is correctly enforced. For consistency with other routes, consider destructuring.
-export const load = (async (event) => { - event.locals.security.requireSuperAdmin(); +export const load = (async ({ locals }) => { + locals.security.requireSuperAdmin();eslint.config.js (1)
10-10: Registering custom security rule looks good; scope it to server files to reduce false positivesPlugin wiring is correct. To avoid flagging client-only files, scope the rule to server handlers via overrides.
Change the global rule to off:
- 'sveltekit-sec/require-security-check': 'error' + 'sveltekit-sec/require-security-check': 'off'Then add an overrides block enabling it only for server endpoints:
// add a new config block (below existing ones) { files: [ '**/+page.server.ts', '**/+server.ts', '**/*.server.ts' ], rules: { 'sveltekit-sec/require-security-check': 'error' } }Also applies to: 26-33, 71-71
src/lib/server/bullmq/BullBoard.ts (1)
40-41: Use requireSuperAdmin() for explicit authorization and consistent failuresUse the centralized guard instead of a boolean check; return 403 on unauthorized rather than falling through.
-export const bullboardHandle: Handle = async ({ event, resolve }) => { - if (event.url.pathname.match(/^\/admin\/jobs($|\/)/) && event.locals.security.isSuperAdmin) { - return bullboard!.fetch(event.request); +export const bullboardHandle: Handle = async ({ event, resolve }) => { + if (event.url.pathname.match(/^\/admin\/jobs($|\/)/)) { + event.locals.security.requireSuperAdmin(); + return bullboard ? bullboard.fetch(event.request) : resolve(event); } return resolve(event); };src/routes/(authenticated)/projects/[filter=projectSelector]/+page.server.ts (1)
6-7: Good: explicit auth check; minor type-safety nit on default orgAuth enforcement is correct. Consider typing the default to reflect possible absence when the user has no orgs.
Apply this minimal tweak:
- let orgRedirect = event.locals.security.organizationMemberships[0]; + let orgRedirect: number | undefined = event.locals.security.organizationMemberships[0];src/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.server.ts (3)
18-19: Parse orgId once; reuse for consistency and clarityYou parse event.params.id multiple times below. Parse once and reuse to reduce repetition and potential drift.
Apply:
-export const load = (async (event) => { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); +export const load = (async (event) => { + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const { organization } = await event.parent();And reuse orgId anywhere needed in this function.
38-49: Actions: reuse a single parsed orgIdSame suggestion for togglePublic: parse once, reuse for the security check and DB calls.
Apply:
async togglePublic(event) { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const form = await superValidate(event.request, valibot(togglePublicSchema)); if (!form.valid) return fail(400, { form, ok: false }); await DatabaseWrites.organizations.update({ where: { - Id: parseInt(event.params.id) + Id: orgId }, data: { PublicByDefault: form.data.publicByDefault // seed for Project.IsPublic when creating a new project } }); return { form, ok: true }; },
52-60: Actions: reuse parsed orgId in toggleProductMirror the consolidation here.
Apply:
async toggleProduct(event) { - event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + const orgId = Number(event.params.id); + event.locals.security.requireAdminOfOrg(orgId); const form = await superValidate(event.request, valibot(toggleProductSchema)); if (!form.valid) return fail(400, { form, ok: false }); await DatabaseWrites.organizationProductDefinitions.toggleForOrg( - parseInt(event.params.id), + orgId, form.data.prodDefId, form.data.enabled );src/routes/(unauthenticated)/request-access-for-organization/+page.server.ts (1)
7-11: Tighten validation and make the email job idempotent/resilient.
- Validate url as a proper URL (prevents junk input).
- Add job options (jobId, attempts, backoff) to avoid duplicate emails and handle transient failures.
Apply:
const requestSchema = v.object({ organizationName: v.pipe(v.string(), v.nonEmpty()), email: v.pipe(v.string(), v.nonEmpty(), v.email()), - url: v.pipe(v.string(), v.nonEmpty()) + url: v.pipe(v.string(), v.nonEmpty(), v.url()) }); @@ - await getQueues().Emails.add('Email SuperAdmins about new org ' + form.data.organizationName, { - type: BullMQ.JobType.Email_NotifySuperAdminsOfNewOrganizationRequest, - email: form.data.email, - organizationName: form.data.organizationName, - url: form.data.url - }); + await getQueues().Emails.add( + 'Email SuperAdmins about new org ' + form.data.organizationName, + { + type: BullMQ.JobType.Email_NotifySuperAdminsOfNewOrganizationRequest, + email: form.data.email, + organizationName: form.data.organizationName, + url: form.data.url + }, + { + jobId: `org-request:${form.data.email}:${form.data.organizationName}`, + attempts: 3, + backoff: { type: 'exponential', delay: 5000 }, + removeOnComplete: true, + removeOnFail: 100 + } + );Optional: consider basic rate‑limiting or a CAPTCHA on this unauthenticated endpoint to reduce spam.
Also applies to: 18-23
src/routes/(authenticated)/admin/settings/organizations/edit/+page.server.ts (1)
45-65: Guard against ID tampering; align URL id and form id before update.Even for super-admins, matching the URL id to the posted id prevents accidental edits and keeps route semantics strict.
Apply:
-export const actions = { - async edit({ request, locals }) { +export const actions = { + async edit({ request, locals, url }) { locals.security.requireSuperAdmin(); const form = await superValidate(request, valibot(editSchema)); if (!form.valid) { return fail(400, { form, ok: false }); } + const idFromUrl = parseInt(url.searchParams.get('id') ?? ''); + if (!Number.isFinite(idFromUrl) || idFromUrl !== form.data.id) { + return fail(400, { form, ok: false }); + } await DatabaseWrites.organizations.update({ where: { - Id: form.data.id + Id: idFromUrl }, data: { Name: form.data.name, BuildEngineApiAccessToken: form.data.buildEngineApiAccessToken, BuildEngineUrl: form.data.buildEngineUrl, LogoUrl: form.data.logoUrl, ContactEmail: form.data.contact, PublicByDefault: form.data.publicByDefault, UseDefaultBuildEngine: form.data.useDefaultBuildEngine, WebsiteUrl: form.data.websiteUrl } });src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts (1)
20-24: Harden inviteToken cookie flags.Set httpOnly, sameSite, and secure to reduce XSS/CSRF risk; secure can depend on protocol.
Apply:
- event.cookies.set('inviteToken', inviteToken, { - path: '/' - }); + event.cookies.set('inviteToken', inviteToken, { + path: '/', + httpOnly: true, + sameSite: 'lax', + secure: event.url.protocol === 'https:' + });src/routes/(authenticated)/directory/[id]/+page.server.ts (3)
7-8: Fix misleading comment: route requires auth.The page enforces authentication upfront; update the comment to reflect that only authenticated users may view public projects.
- // Anyone can view a public project, even if not an org member - // But we need to check for org membership if downloads are allowed + // Authenticated users can view a public project even if not an org member. + // Org membership additionally allows downloads when applicable.
85-86: Prefer 404 over 403 to avoid existence leaks.Returning 404 for non-public projects avoids leaking project existence to non-members.
- if (!project.IsPublic) return error(403); + if (!project.IsPublic) return error(404);Based on learnings
95-103: Avoid mutating DB result; map a safe view instead.Mutating the fetched object in-place can fight types and surprises downstream. Build a derived object with Url nulled instead.
- if (!allowDownloads) { - project.Products.forEach((p) => { - p.ProductPublications.forEach((pp) => { - pp.ProductBuild.ProductArtifacts.forEach((pa) => { - pa.Url = null; - }); - }); - }); - } + const projectOut = allowDownloads + ? project + : { + ...project, + Products: project.Products.map((p) => ({ + ...p, + ProductPublications: p.ProductPublications.map((pp) => ({ + ...pp, + ProductBuild: { + ...pp.ProductBuild, + ProductArtifacts: pp.ProductBuild.ProductArtifacts.map((pa) => ({ + ...pa, + Url: null + })) + } + })) + })) + };And return projectOut instead of project:
- project, + project: projectOut,src/lib/utils/roles.ts (2)
9-14: Simplify logic and short‑circuit for SuperAdmin.Remove redundant !!orgRoles and return early when user is SuperAdmin.
-export function isAdminForOrg(orgId: number, roles: ClientRolesArray): boolean { - const orgRoles = roles.filter(([oId]) => oId === orgId).map(([, r]) => r); - return ( - !!orgRoles && - (roles.some(([_, r]) => r === RoleId.SuperAdmin) || orgRoles.includes(RoleId.OrgAdmin)) - ); -} +export function isAdminForOrg(orgId: number, roles: ClientRolesArray): boolean { + if (roles.some(([, r]) => r === RoleId.SuperAdmin)) return true; + const orgRoles = roles.filter(([oId]) => oId === orgId).map(([, r]) => r); + return orgRoles.includes(RoleId.OrgAdmin); +}
21-22: Deduplicate org IDs in orgsForRole.Avoid returning duplicate org IDs if roles contain repeated entries.
-export function orgsForRole(role: RoleId, roles: ClientRolesArray): number[] { - return roles.filter(([, r]) => r === role).map(([orgId]) => orgId); -} +export function orgsForRole(role: RoleId, roles: ClientRolesArray): number[] { + const s = new Set<number>(); + for (const [orgId, r] of roles) if (r === role) s.add(orgId); + return [...s]; +}eslint-rules/require-security-check.js (1)
60-68: Align docs and suggestions with locals.security usage.Messages currently mention event.locals; the rule and fixer match locals.security.
- description: 'Require calling event.locals.security.* in every server load/action' + description: 'Require calling locals.security.* in every server load/action' @@ - suggestAuthCall: 'Require authentication by adding "event.locals.security.requireAuthenticated();" to the start of the function body.', - suggestNoAuthCall: 'Mark this function as not requiring any auth by adding "event.locals.security.requireNothing();" to the start of the function body.' + suggestAuthCall: 'Require authentication by adding "locals.security.requireAuthenticated();" to the start of the function body.', + suggestNoAuthCall: 'Mark this function as not requiring any auth by adding "locals.security.requireNothing();" to the start of the function body.'src/auth.ts (2)
301-308: Log roles array directly; current logging uses Array.entries() shape.security.roles is an array; spreading entries logs [index, value] pairs, which is noisy. Prefer the array.
- 'auth.user.roles': user ? JSON.stringify([...security.roles.entries()]) : 'null', + 'auth.user.roles': user ? JSON.stringify(security.roles) : 'null',
128-128: Optional: de-duplicate roles when building session payload.If userRoles contains redundant rows, avoid duplicates in session.user.roles.
- session.user.roles = userRoles.map(({ OrganizationId, RoleId }) => [OrganizationId, RoleId]); + const seen = new Set<string>(); + session.user.roles = userRoles + .map(({ OrganizationId, RoleId }) => [OrganizationId, RoleId] as [number, RoleId]) + .filter(([o, r]) => (seen.has(`${o}:${r}`) ? false : (seen.add(`${o}:${r}`), true)));src/routes/(authenticated)/organizations/[id=idNumber]/settings/info/+page.server.ts (1)
8-8: Security gate added — specify radix in parseInt (or use Number) to avoid implicit base.Use explicit base-10 for clarity and to avoid surprises if input ever contains leading zeros.
- event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + event.locals.security.requireAdminOfOrg(parseInt(event.params.id, 10));- event.locals.security.requireAdminOfOrg(parseInt(event.params.id)); + event.locals.security.requireAdminOfOrg(parseInt(event.params.id, 10));Optional: compute once per handler to avoid double parsing:
const orgId = Number(event.params.id); // ... event.locals.security.requireAdminOfOrg(orgId);Also applies to: 23-23
src/routes/(authenticated)/+layout.server.ts (1)
17-20: Minor: avoid passingwhere: undefined.Prefer an empty filter for super admins to make the intent explicit and keep the query object well-formed.
- where: sec.isSuperAdmin - ? undefined + where: sec.isSuperAdmin + ? {} : { Id: { in: sec.organizationMemberships } },src/routes/(authenticated)/projects/[id=idNumber]/+page.svelte (1)
48-55: Guard session access with optional chaining to avoid client-side crashesSession may be transiently undefined during hydration; use optional chaining for resilience and consistency with other pages.
Apply this diff:
const canEdit = $derived( canModifyProject( - data.session.user, + data.session?.user, projectData?.project.OwnerId ?? -1, projectData?.project.Organization.Id ?? -1 ) ); const canClaim = $derived( canClaimProject( - data.session.user, + data.session?.user, projectData?.project.OwnerId ?? -1, projectData?.project.Organization.Id ?? -1, projectData?.project.GroupId ?? -1, projectData?.userGroups ?? [] ) );Please confirm that (authenticated) routes always provide session in data during SSR; if not guaranteed, this change prevents rare client throws.
Also applies to: 56-63
src/routes/(authenticated)/+layout.svelte (1)
154-185: Defensive access for roles in templatesUse optional chaining for session access to avoid runtime errors during hydration and to match usage elsewhere (e.g., users page). Based on learnings.
Apply this diff:
- {#if isAdminForAny(data.session.user.roles)} + {#if isAdminForAny(data.session?.user.roles)} <li> <a class="rounded-none" class:active-menu-item={isUrlActive('/projects/active')} href={activeOrgUrl('/projects/active')} onclick={closeDrawer} > {m.sidebar_activeProjects()} </a> </li> <li> <a class="rounded-none" class:active-menu-item={isUrlActive('/users')} href={localizeHref('/users')} onclick={closeDrawer} > {m.sidebar_users()} </a> </li> <li> <a class="rounded-none" class:active-menu-item={isUrlActive('/organizations')} href={activeOrgUrl('/organizations')} onclick={closeDrawer} > {m.sidebar_orgSettings()} </a> </li> {/if} - {#if isSuperAdmin(data.session.user.roles)} + {#if isSuperAdmin(data.session?.user.roles)} <li> <a class="rounded-none" class:active-menu-item={isUrlActive('/admin/settings')} href={localizeHref('/admin/settings/organizations')} onclick={closeDrawer} > {m.sidebar_adminSettings()} </a> </li>Also applies to: 186-218
src/lib/projects/sse.ts (1)
198-205: Minor: use Set for membership test when filtering product definitionsFor many products/definitions, Set lookup reduces repeated O(n) includes checks.
Example refactor:
- const projectProductDefinitionIds = project.Products.map((p) => p.ProductDefinition.Id); + const projectProductDefinitionIds = new Set(project.Products.map((p) => p.ProductDefinition.Id)); ... - productsToAdd: productDefinitions.filter( - (pd) => !projectProductDefinitionIds.includes(pd.Id) - ), + productsToAdd: productDefinitions.filter((pd) => !projectProductDefinitionIds.has(pd.Id)),Also applies to: 220-224
src/routes/(authenticated)/tasks/+page.server.ts (1)
5-7: Use Security context for userId and avoid redundant auth call (possible duplicate fetch).Prefer
event.locals.security.userIdover re-awaitinglocals.auth(). Also, the authenticated layout already fetchesuserTasks(see src/routes/(authenticated)/+layout.server.ts); consider consuming it viaparent()to avoid a duplicate DB read.Apply this minimal change:
export const load = (async (event) => { event.locals.security.requireAuthenticated(); - const userTasks = await getUserTasks((await event.locals.auth())!.user.userId); + const userTasks = await getUserTasks(event.locals.security.userId); return { userTasks }; }) satisfies PageServerLoad;Optional: remove the query here and read from parent data instead if the page doesn’t need to refetch.
src/lib/projects/components/ProjectActionMenu.svelte (1)
71-71: Confirm updated function signatures and null-safety of session.user.Switching to pass
page.data.session.usermatches the “user-first” API; ensure all call sites were updated andpage.data.sessionis always present in this authenticated group to avoid runtime nulls.Optionally guard with optional chaining or expose
userdirectly in layout data to avoid deep property access.Also applies to: 84-84, 102-102
src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts (1)
8-16: Confirm requireProjectReadAccess handles null project; parse id once.findUnique may return null; ensure requireProjectReadAccess tolerates null to avoid runtime errors. Also parse the id once and reuse.
Apply this diff to parse once and reuse:
export async function POST(request) { - request.locals.security.requireProjectReadAccess( - await DatabaseReads.groupMemberships.findMany({ - where: { UserId: request.locals.security.userId }, - select: { GroupId: true } - }), - await DatabaseReads.projects.findUnique({ - where: { Id: parseInt(request.params.id) } - }) - ); - const { id: strId } = request.params; + const id = parseInt(request.params.id); + request.locals.security.requireProjectReadAccess( + await DatabaseReads.groupMemberships.findMany({ + where: { UserId: request.locals.security.userId }, + select: { GroupId: true } + }), + await DatabaseReads.projects.findUnique({ + where: { Id: id } + }) + );src/routes/(authenticated)/projects/import/[id=idNumber]/+page.server.ts (2)
81-82: Redundant NaN check.[id=idNumber] matcher guarantees a valid positive integer, so isNaN check is unnecessary.
Apply this diff to simplify:
- const organizationId = parseInt(event.params.id); - - if (isNaN(organizationId)) return error(404); + const organizationId = parseInt(event.params.id);As per coding guidelines
83-84: Duplicate permission checks.After requireHasRole(..., true), verifyCanCreateProject is redundant and performs the same policy. Remove to reduce complexity.
Apply this diff:
- if (!verifyCanCreateProject(event.locals.security, organizationId)) return error(403);src/hooks.server.ts (1)
99-104: Typo: “hookTelementry” → “hookTelemetry”.Minor naming nit.
- async function hookTelementry({ event, resolve }) { + async function hookTelemetry({ event, resolve }) {src/routes/(authenticated)/projects/[id=idNumber]/edit/+page.server.ts (1)
18-33: Avoid non-null assertion on project; let guard handle 404findUnique can return null; requireProjectWriteAccess already handles null with 404. Drop the non-null assertion to match the method’s signature and avoid masking nulls.
Apply:
- }))!; + }));src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts (2)
173-185: Gate queue only for operations that need it (archive/reactivate), not claimQueue availability isn’t needed for claim; moving the check after form parse avoids blocking claim during outages.
Apply:
- if (!QueueConnected()) return error(503); - - const form = await superValidate(event.request, valibot(bulkProjectActionSchema)); + const form = await superValidate(event.request, valibot(bulkProjectActionSchema)); if ( !form.valid || !form.data.operation || (!form.data.projects?.length && form.data.projectId === null) ) { return fail(400, { form, ok: false }); } + if (form.data.operation !== 'claim' && !QueueConnected()) return error(503);
223-226: Redundant orgId validation with idNumber matcherThe [orgId=idNumber] matcher guarantees a valid positive integer and exact string match. This check can be removed.
Apply:
- const orgId = parseInt(event.params.orgId!); - if (isNaN(orgId) || !(orgId + '' === event.params.orgId)) return fail(404); + const orgId = parseInt(event.params.orgId!);Based on learnings
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/routes/(unauthenticated)/(auth)/login/LoginScreen.svelte (1)
16-25: Optional: null out interval after clearing when service returns.Not required, but setting timeout = null after clear makes intent explicit and prevents accidental double-clears if this logic evolves.
Apply this diff:
- if (serviceAvailable) { - if (timeout !== null) clearInterval(timeout); + if (serviceAvailable) { + if (timeout !== null) { + clearInterval(timeout); + timeout = null; + }eslint-rules/require-security-check.js (3)
30-56: Broaden detection to cover optional chaining (?.).Calls like locals?.security?.requireAuthenticated() are missed. Support ChainExpression/OptionalMemberExpression.
Apply this diff to unwrap ChainExpression and handle OptionalMemberExpression:
function blockStatementIncludesSecurityCall(block) { const nodeIsSecurityCall = (node) => { - // Valid check if the node is a CallExpression of the form: + // Valid check if the node is a CallExpression/ChainExpression of the form: // *.locals.security.require*() // or locals.security.require*() - if (node?.type === 'CallExpression' && node.callee.type === 'MemberExpression') { - const obj = node.callee.object; - const prop = node.callee.property; + // Also allow optional chaining (?.) + if (!node) return false; + const unwrapChain = (n) => (n && n.type === 'ChainExpression' ? n.expression : n); + if (node.type === 'CallExpression' || node.type === 'ChainExpression') { + const call = node.type === 'CallExpression' ? node : unwrapChain(node); + if (!call || call.type !== 'CallExpression') return false; + const callee = unwrapChain(call.callee); + if (!callee || callee.type !== 'MemberExpression') return false; + const obj = callee.object; + const prop = callee.property; - if (obj.type === 'MemberExpression' && + if (obj && obj.type === 'MemberExpression' && // object is either *.locals or just locals - ((obj.object.type === 'MemberExpression' && - obj.object.property.type === 'Identifier' && - obj.object.property.name === 'locals') || - (obj.object.type === 'Identifier' && obj.object.name === 'locals')) && + (((obj.object.type === 'MemberExpression' && + obj.object.property.type === 'Identifier' && + obj.object.property.name === 'locals') || + (obj.object.type === 'Identifier' && obj.object.name === 'locals'))) && // locals.security obj.property.type === 'Identifier' && obj.property.name === 'security' && prop.type === 'Identifier' && prop.name.startsWith('require') // ex. requireAuthenticated, requireRole, etc. ) { return true; } } return false; }; return !!block.body.find((n) => !!walkFind(n, nodeIsSecurityCall, 6)); }
57-71: Align rule docs/messages with detection: prefer locals.security.Docs say event.locals..., but detection/message mention locals.security. Make it consistent and parameter-agnostic.
Apply this diff:
- docs: { - description: 'Require calling event.locals.security.* in every server load/action' - }, + docs: { + description: 'Require calling locals.security.require*() (or event.locals.security.require*()) in every server load/action' + }, schema: [], messages: { - missingSecurityCheck: 'Missing call to requireAuthenticated() or similar security check. This must be of the form locals.security.require*()', + missingSecurityCheck: 'Missing call to requireAuthenticated() or similar security check. Use locals.security.require*() (or event.locals.security.require*()).', unexpectedFunction: 'Unexpected export function type {{type}}. Expected ArrowFunctionExpression, FunctionExpression, TSSatisfiesExpression or ObjectExpression.', - suggestAuthCall: 'Require authentication by adding "event.locals.security.requireAuthenticated();" to the start of the function body.', - suggestNoAuthCall: 'Mark this function as not requiring any auth by adding "event.locals.security.requireNothing();" to the start of the function body.' + suggestAuthCall: 'Require authentication by adding "locals.security.requireAuthenticated();" to the start of the function body.', + suggestNoAuthCall: 'Mark this function as not requiring any auth by adding "locals.security.requireNothing();" to the start of the function body.' }
83-85: Optional: include JavaScript server files.If your repo has .server.js/+server.js, extend the filename check.
Apply this diff:
- if (!context.filename.endsWith('.server.ts') && !context.filename.endsWith('+server.ts')) + if ( + !context.filename.endsWith('.server.ts') && + !context.filename.endsWith('+server.ts') && + !context.filename.endsWith('.server.js') && + !context.filename.endsWith('+server.js') + ) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
eslint-rules/require-security-check.js(1 hunks)src/lib/projects/components/ProjectActionMenu.svelte(3 hunks)src/lib/projects/components/ProjectFilterSelector.svelte(1 hunks)src/routes/(authenticated)/directory/[id=idNumber]/+page.server.ts(1 hunks)src/routes/(authenticated)/directory/[id=idNumber]/+page.svelte(2 hunks)src/routes/(authenticated)/projects/[id=idNumber]/ProductCard.svelte(1 hunks)src/routes/(unauthenticated)/(auth)/login/LoginScreen.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/projects/components/ProjectFilterSelector.svelte
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-12T14:07:02.200Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:285-303
Timestamp: 2025-09-12T14:07:02.200Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to optimize filterAvailableActions by creating Sets at the caller level rather than inside the function, so the function would take Set<number> arguments instead of arrays for better performance and separation of concerns.
Applied to files:
src/routes/(authenticated)/directory/[id=idNumber]/+page.server.ts
📚 Learning: 2025-09-17T21:07:18.139Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1275
File: src/lib/projects/index.ts:183-201
Timestamp: 2025-09-17T21:07:18.139Z
Learning: In the AppBuilder portal project, org admins must be in the project group to claim/take ownership of projects. This is enforced both in client-side logic (canClaimProject function) and server-side validation.
Applied to files:
src/lib/projects/components/ProjectActionMenu.svelte
📚 Learning: 2025-09-22T15:01:06.464Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/directory/[id]/+page.svelte:24-24
Timestamp: 2025-09-22T15:01:06.464Z
Learning: In directory view pages, canModifyProject should be replaced with canViewProject for more appropriate permission checking, as mentioned in PR #1275. The directory should allow viewing projects with read access, not just modify access. However, canViewProject function doesn't exist yet and needs to be implemented.
Applied to files:
src/routes/(authenticated)/directory/[id=idNumber]/+page.svelte
📚 Learning: 2025-09-22T15:01:06.464Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/directory/[id]/+page.svelte:24-24
Timestamp: 2025-09-22T15:01:06.464Z
Learning: PR #1275 "Allow members of project group to view but not edit" was supposed to introduce separate view and edit permissions, including a canViewProject function, but this wasn't fully implemented across all components. The directory view should use view permissions rather than modify permissions to determine if project names should be clickable links.
Applied to files:
src/routes/(authenticated)/directory/[id=idNumber]/+page.svelte
📚 Learning: 2025-09-25T20:03:59.355Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte:152-155
Timestamp: 2025-09-25T20:03:59.355Z
Learning: In the AppBuilder portal, there are two different roles properties with different data structures: `data.session.user.roles` is an array of tuples `[number, RoleId][]` (defined in Session interface), while `event.locals.security.roles` is a `Map<number, RoleId[]>` (defined in Security class). When accessing roles from session data in Svelte components, use array methods like `.some()` with destructuring syntax `([o, r])`.
Applied to files:
src/routes/(authenticated)/directory/[id=idNumber]/+page.svelte
🧬 Code graph analysis (1)
src/routes/(authenticated)/directory/[id=idNumber]/+page.server.ts (1)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
🔇 Additional comments (7)
src/routes/(authenticated)/projects/[id=idNumber]/ProductCard.svelte (1)
211-223: Reinstate a session null check before accessing rolesDropping the optional chain means any render where
page.data.sessionis missing will now throw before the check runs. The component still treatspage.data.sessionas optional farther down (Line 272), so it looks like we do hit such states. Please either add a guard or revert to the optional access so this menu doesn’t crash when the session isn’t present.If you opt for the guard, this diff keeps the runtime safe:
- {#if isAdminForOrg(project.Organization.Id, page.data.session!.user.roles)} + {#if page.data.session && isAdminForOrg(project.Organization.Id, page.data.session.user.roles)} <li class="w-full rounded-none"> <button class="text-nowrap" onclick={() => updateProductModal?.showModal()}> {m.products_properties_title()} </button> </li> {/if} - {#if isSuperAdmin(page.data.session!.user.roles) && !!product.WorkflowInstance} + {#if page.data.session && isSuperAdmin(page.data.session.user.roles) && !!product.WorkflowInstance} <li class="w-full rounded-none"> <a href={localizeHref(`/workflow-instances/${product.Id}`)}> {m.common_workflow()} </a> </li> {/if}src/routes/(authenticated)/directory/[id=idNumber]/+page.svelte (1)
20-28: Fix role iteration: values are arrays, so the predicate never matches.
data.session.user.rolesentries are shaped like[orgId, RoleId[]]. The current destructuring treats the second slot as a singleRoleId, sorole === RoleId.SuperAdminwill always be false (and the OrgAdmin check won’t look inside the array). As a result, Super Admins and Org Admins lose access to the project link. Please iterate the role arrays instead.- return ( - data.session.user.roles.some( - ([org, role]) => - (org === project.Organization.Id && role === RoleId.OrgAdmin) || - role === RoleId.SuperAdmin - ) || data.sessionGroups.includes(project.GroupId) - ); + const canViewFromRole = data.session.user.roles.some( + ([org, roles]) => + org === project.Organization.Id && roles?.includes(RoleId.OrgAdmin) + ); + + const isSuperAdmin = data.session.user.roles.some(([, roles]) => + roles?.includes(RoleId.SuperAdmin) + ); + + return canViewFromRole || isSuperAdmin || data.sessionGroups.includes(project.GroupId);⛔ Skipped due to learnings
Learnt from: FyreByrd PR: sillsdev/appbuilder-portal#1297 File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte:152-155 Timestamp: 2025-09-25T20:03:59.355Z Learning: In the AppBuilder portal, there are two different roles properties with different data structures: `data.session.user.roles` is an array of tuples `[number, RoleId][]` (defined in Session interface), while `event.locals.security.roles` is a `Map<number, RoleId[]>` (defined in Security class). When accessing roles from session data in Svelte components, use array methods like `.some()` with destructuring syntax `([o, r])`.Learnt from: FyreByrd PR: sillsdev/appbuilder-portal#1275 File: src/lib/projects/index.ts:183-201 Timestamp: 2025-09-17T21:07:18.139Z Learning: In the AppBuilder portal project, org admins must be in the project group to claim/take ownership of projects. This is enforced both in client-side logic (canClaimProject function) and server-side validation.src/lib/projects/components/ProjectActionMenu.svelte (1)
71-71: Guard against missing session.user before archiving
- In ProjectActionMenu.svelte, replace the non-null assertion with an explicit guard:
{#if allowActions && page.data.session?.user && canArchive(project, page.data.session.user, orgId)}- In src/lib/projects/index.ts, update canArchive’s signature to accept
Session['user'] | undefinedand return false whensecurityis undefined:export function canArchive( project: Pick<ProjectForAction, 'OwnerId' | 'DateArchived'>, security: Session['user'] | undefined, orgId: number ): boolean { if (!security) return false; return !project.DateArchived && canModifyProject(security, project.OwnerId, orgId); }This ensures no runtime crash if the session is absent.
⛔ Skipped due to learnings
Learnt from: FyreByrd PR: sillsdev/appbuilder-portal#1275 File: src/routes/(authenticated)/projects/[filter=projectSelector]/[id]/+page.svelte:202-241 Timestamp: 2025-09-15T15:13:50.486Z Learning: In the project listing page, the `canArchiveSelected` permission check is intentionally used for both archive operations and rebuild/republish operations because these operations require the same authorization conditions.src/routes/(unauthenticated)/(auth)/login/LoginScreen.svelte (1)
3-4: Nice cleanup: lifecycle + typing fixes applied.Importing onDestroy/browser, using ReturnType, and adding teardown all look good.
Also applies to: 8-9, 27-32
eslint-rules/require-security-check.js (3)
166-169: Good: only push truthy block statements.Prevents undefined entries in blockStatements.
4-29: Fix: walkFind can crash on null/primitive nodes.For null/undefined, we call callback(node) and iterate keys, which throws. Guard early and only traverse objects.
Apply this diff:
-function walkFind(node, callback, maxDepth = 10) { - if (maxDepth <= 0) - return null; - if (node && typeof node !== 'object') - return null; - if (callback(node)) - return node; - for (const key in node) { - if (key === 'parent') - continue; - const val = node[key]; - if (Array.isArray(val)) { - for (const child of val) { - const result = walkFind(child, callback, maxDepth - 1); - if (result) - return result; - } - } - else { - const result = walkFind(val, callback, maxDepth - 1); - if (result) - return result; - } - } - return null; -} +function walkFind(node, callback, maxDepth = 10) { + if (maxDepth <= 0 || node == null) return null; + if (typeof node !== 'object') return null; + if (callback(node)) return node; + for (const key in node) { + if (key === 'parent') continue; + const val = node[key]; + if (Array.isArray(val)) { + for (const child of val) { + const result = walkFind(child, callback, maxDepth - 1); + if (result) return result; + } + } else if (val != null && typeof val === 'object') { + const result = walkFind(val, callback, maxDepth - 1); + if (result) return result; + } + } + return null; +}
179-204: Fixer can generate invalid code on empty blocks; also prefer locals.security in suggestions.insertTextBefore(bs) places text before the opening brace when bs.body is empty, producing invalid syntax. Insert just after the opening brace and use locals.security to be parameter-agnostic.
Apply this diff:
- blockStatements.forEach((bs) => { - if (!blockStatementIncludesSecurityCall(bs)) { + blockStatements.forEach((bs) => { + if (!bs || bs.type !== 'BlockStatement' || !blockStatementIncludesSecurityCall(bs)) { // No security check found in this function's block statement // Flag an error on the parent node of the block statement (the whole function) context.report({ - node: bs.parent, + node: (bs && bs.parent) || node.declaration, messageId: 'missingSecurityCheck', suggest: [ { - messageId: 'suggestAuthCall', - fix: (fixer) => fixer.insertTextBefore(bs.body[0] || bs, 'event.locals.security.requireAuthenticated();\n' + - (functionExport.id.name === 'actions' - ? ' ' - : ' ')) + messageId: 'suggestAuthCall', + fix: (fixer) => { + const indent = functionExport?.id?.name === 'actions' ? ' ' : ' '; + // Insert just after '{' + return fixer.insertTextAfterRange( + [bs.range[0], bs.range[0] + 1], + `${indent}locals.security.requireAuthenticated();\n` + ); + } }, { messageId: 'suggestNoAuthCall', - fix: (fixer) => fixer.insertTextBefore(bs.body[0] || bs, 'event.locals.security.requireNothing();\n' + - (functionExport.id.name === 'actions' - ? ' ' - : ' ')) + fix: (fixer) => { + const indent = functionExport?.id?.name === 'actions' ? ' ' : ' '; + return fixer.insertTextAfterRange( + [bs.range[0], bs.range[0] + 1], + `${indent}locals.security.requireNothing();\n` + ); + } } ] }); } });
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts (1)
89-90: Actions must throw, not return, Kit errors.Use throw error(403) or return fail(403, ...). Returning error(...) won’t behave as intended.
- return error(403); + throw error(403);
🧹 Nitpick comments (5)
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts (5)
59-67: Avoid redundant admin check in action (micro perf).This repeats the membership fetch + requireAdminOfOrgIn that the subsequent DB guard also enforces. You can drop this block and rely on the single authoritative DB check below. Keep if you want defense‑in‑depth.
92-97: Derive orgId server‑side from groupId to avoid trusting client input.Even with the guard, prefer deriving orgId from groupId and pass that to toggleForOrg. This keeps write parameters server‑trusted while keeping the same number of DB round trips.
- const ok = await DatabaseWrites.groupMemberships.toggleForOrg( - form.data.orgId, - subjectId, - form.data.groupId, - form.data.enabled - ); + // reuse the guard query to fetch the authoritative orgId + const org = await DatabaseReads.organizations.findFirst({ + where: { + AND: [ + adminOrgs( + subjectId, + event.locals.security.userId, + event.locals.security.isSuperAdmin + ), + { Groups: { some: { Id: form.data.groupId } } } + ] + }, + select: { Id: true } + }); + if (!org) throw error(403); + + const ok = await DatabaseWrites.groupMemberships.toggleForOrg( + org.Id, + subjectId, + form.data.groupId, + form.data.enabled + );
17-17: Use explicit radix for parseInt (or Number()).Prevent subtlety with leading zeros and improve clarity.
- const subjectId = parseInt(params.id); + const subjectId = Number.parseInt(params.id, 10);- const subjectId = parseInt(event.params.id); + const subjectId = Number.parseInt(event.params.id, 10);Also applies to: 59-59
59-67: Optional: reuse subject membership from load via hidden field removal.To reduce queries, you can skip the pre-check here and rely on the single organizations.findFirst guard that already validates admin scope, subject membership, and group ownership. Keep as is if you prioritize explicit checks.
54-57: Track DB composite PK improvement in an issue.The TODO about composite PKs for many‑to‑many is valid; consider opening/linked issue to plan the migration.
Do you want me to draft a migration outline (Prisma schema, backfill, and rollout plan)?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/(authenticated)/products/[id]/files/+page.server.ts(2 hunks)src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(authenticated)/products/[id]/files/+page.server.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-12T14:07:02.200Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:285-303
Timestamp: 2025-09-12T14:07:02.200Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to optimize filterAvailableActions by creating Sets at the caller level rather than inside the function, so the function would take Set<number> arguments instead of arrays for better performance and separation of concerns.
Applied to files:
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts
📚 Learning: 2025-09-25T19:44:14.845Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/import/[id=idNumber]/+page.server.ts:24-26
Timestamp: 2025-09-25T19:44:14.845Z
Learning: The `requireHasRole(organizationId, roleId, orOrgAdmin)` method in the Security class allows organization admins to pass role checks when the third parameter `orOrgAdmin` is set to `true`, even if they don't have the specific role being checked for.
Applied to files:
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts
🧬 Code graph analysis (1)
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts (2)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/users/server.ts (1)
adminOrgs(12-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-and-test
- GitHub Check: typecheck-lint
🔇 Additional comments (3)
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts (3)
29-29: LGTM: query scoped by security context.Using adminOrgs(subjectId, locals.security.userId, locals.security.isSuperAdmin) correctly limits results to orgs where actor is admin and subject is a member.
75-87: Nice: orgId is now bound in the authorization check (resolves prior concern).
This ties the admin scope to the specific org and ensures the group belongs to that org. Good hardening.
18-25: requireAdminOfOrgIn(empty) correctly denies access and toggleForOrg enforces server-side org scoping. No changes needed.
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(authenticated)/users/[id=idNumber]/settings/roles/+page.server.ts (1)
20-55: Remove the blanketrequireAdminOfOrgInchecksThe new
requireAdminOfOrgIn(user.OrganizationMemberships.map(...))calls in both load and action force the current admin to be an OrgAdmin for every org the subject belongs to. That’s too restrictive: if the subject is in orgs A and B and I’m only an admin of A, I now get a 403 and can’t manage the roles I legitimately control. We should instead filter accessible orgs viaadminOrgs(...)(as the layout already does) and only 403 when that filtered set is empty, letting admins operate on the orgs they actually administer.A possible fix is to reuse the
adminOrgsquery to derive accessible org IDs and guard on an empty result, e.g.:- locals.security.requireAdminOfOrgIn(user.OrganizationMemberships.map((o) => o.OrganizationId)); + const manageableOrgs = await DatabaseReads.organizations.findMany({ + where: adminOrgs(subjectId, locals.security.userId, locals.security.isSuperAdmin), + select: { Id: true } + }); + if (manageableOrgs.length === 0) throw error(403);(Apply the same pattern in the action.) This keeps hidden-field tampering protection while avoiding needless lockouts. Based on learnings
🧹 Nitpick comments (2)
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts (2)
63-65: Guard against empty org list passed to requireAdminOfOrgIn.If the subject exists but has zero org memberships, this becomes
requireAdminOfOrgIn([]). Confirm its semantics (deny vs allow) and handle explicitly to avoid ambiguity (e.g., short‑circuit to empty view or throw 403).
18-21: Reduce duplication: extract “fetchSubjectOr404” helper.Both load and action fetch the same subject data and 404 on null. Consider a small local helper to DRY and keep checks consistent.
+async function fetchSubjectOr404(id: number) { + const subject = await DatabaseReads.users.findUnique({ + where: { Id: id }, + include: { OrganizationMemberships: { select: { OrganizationId: true } } } + }); + if (!subject) throw error(404, 'User not found'); + return subject; +}Then:
- const user = await DatabaseReads.users.findUnique({ ... }); - if (!user) throw error(404, 'User not found'); - locals.security.requireAdminOfOrgIn(user.OrganizationMemberships.map((o) => o.OrganizationId)); + const subject = await fetchSubjectOr404(subjectId); + locals.security.requireAdminOfOrgIn(subject.OrganizationMemberships.map((o) => o.OrganizationId));Also applies to: 57-61
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts(2 hunks)src/routes/(authenticated)/users/[id=idNumber]/settings/roles/+page.server.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-22T14:59:53.366Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts:214-221
Timestamp: 2025-09-22T14:59:53.366Z
Learning: In the AppBuilder portal, bulk project actions in the route `/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts` are organization-scoped by design. The route parameter `[orgId=idNumber]` ensures all operations happen within a single organization context, so using the route-level orgId for all projects in bulk actions like `doProjectAction` is correct and safe.
Applied to files:
src/routes/(authenticated)/users/[id=idNumber]/settings/roles/+page.server.ts
📚 Learning: 2025-09-12T14:07:02.200Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:285-303
Timestamp: 2025-09-12T14:07:02.200Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to optimize filterAvailableActions by creating Sets at the caller level rather than inside the function, so the function would take Set<number> arguments instead of arrays for better performance and separation of concerns.
Applied to files:
src/routes/(authenticated)/users/[id=idNumber]/settings/roles/+page.server.tssrc/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts
📚 Learning: 2025-09-22T15:00:43.129Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts:7-10
Timestamp: 2025-09-22T15:00:43.129Z
Learning: In user settings layouts, use the `adminOrgs` function from `$lib/users/server` to determine admin access rather than throwing 403 errors upfront. The `adminOrgs` function creates a filter that returns organizations where the subject user is a member AND the current user is an admin of that organization.
Applied to files:
src/routes/(authenticated)/users/[id=idNumber]/settings/roles/+page.server.tssrc/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts
📚 Learning: 2025-09-25T19:44:14.845Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/import/[id=idNumber]/+page.server.ts:24-26
Timestamp: 2025-09-25T19:44:14.845Z
Learning: The `requireHasRole(organizationId, roleId, orOrgAdmin)` method in the Security class allows organization admins to pass role checks when the third parameter `orOrgAdmin` is set to `true`, even if they don't have the specific role being checked for.
Applied to files:
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts
📚 Learning: 2025-09-25T03:51:13.814Z
Learnt from: 7dev7urandom
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts:7-8
Timestamp: 2025-09-25T03:51:13.814Z
Learning: In the organization invitation flow in src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts, the checkInviteErrors function properly handles null/undefined invite tokens by accepting inviteToken?: string | null and having an explicit guard clause if (!inviteToken) return { error: 'not found' }, making the non-null assertion on searchParams.get('t')! safe since error handling is centralized.
Applied to files:
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts
🧬 Code graph analysis (2)
src/routes/(authenticated)/users/[id=idNumber]/settings/roles/+page.server.ts (2)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/users/server.ts (1)
adminOrgs(12-29)
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts (2)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/users/server.ts (1)
adminOrgs(12-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (2)
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts (2)
76-81: Good fix: orgId is now bound in the authorization predicate.Passing
form.data.orgIdintoadminOrgs(...)plus theGroups.somejoin prevents cross‑org toggles. This addresses the earlier concern about mismatched orgId.
22-23: Throw Kit errors instead of returning them.Use
throw error(...)in load;return error(...)won’t behave as intended in SvelteKit.- if (!user) return error(404); + if (!user) throw error(404, 'User not found');
src/routes/(authenticated)/users/[id=idNumber]/settings/groups/+page.server.ts
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores