Skip to content

Commit 1b80ae2

Browse files
committed
fix(admin): validate retention workspace targets on the Admin API too
retentionOverrides and per-workspace PII rules both name a workspace, and neither field is a foreign key. The settings UI rejected ids belonging to another organization; the Admin API did not, so the two paths could persist different data for the same org. Extracts the check as getForeignWorkspaceTargetsReason and points both routes at it, so they cannot drift apart again.
1 parent d12c871 commit 1b80ae2

4 files changed

Lines changed: 125 additions & 26 deletions

File tree

apps/sim/app/api/organizations/[id]/data-retention/route.ts

Lines changed: 13 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
22
import { db } from '@sim/db'
33
import type { DataRetentionSettings } from '@sim/db/schema'
4-
import { member, organization, workspace } from '@sim/db/schema'
4+
import { member, organization } from '@sim/db/schema'
55
import { createLogger } from '@sim/logger'
6-
import { and, eq, inArray } from 'drizzle-orm'
6+
import { and, eq } from 'drizzle-orm'
77
import { type NextRequest, NextResponse } from 'next/server'
88
import {
99
type OrganizationRetentionValues,
@@ -13,7 +13,10 @@ import { parseRequest, validationErrorResponse } from '@/lib/api/server'
1313
import { getSession } from '@/lib/auth'
1414
import { CLEANUP_CONFIG } from '@/lib/billing/cleanup-dispatcher'
1515
import { isOrganizationOnEnterprisePlan } from '@/lib/billing/core/subscription'
16-
import { getPiiRedactionDenialReason } from '@/lib/billing/retention'
16+
import {
17+
getForeignWorkspaceTargetsReason,
18+
getPiiRedactionDenialReason,
19+
} from '@/lib/billing/retention'
1720
import { isBillingEnabled } from '@/lib/core/config/env-flags'
1821
import { isFeatureEnabled } from '@/lib/core/config/feature-flags'
1922
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
@@ -220,27 +223,13 @@ export const PUT = withRouteHandler(
220223
merged.retentionOverrides = body.retentionOverrides
221224
}
222225

223-
const targetedWorkspaceIds = new Set<string>()
224-
for (const override of body.retentionOverrides ?? []) {
225-
targetedWorkspaceIds.add(override.workspaceId)
226-
}
227-
for (const rule of body.piiRedaction?.rules ?? []) {
228-
if (rule.workspaceId) targetedWorkspaceIds.add(rule.workspaceId)
229-
}
230-
if (targetedWorkspaceIds.size > 0) {
231-
const ids = [...targetedWorkspaceIds]
232-
const orgWorkspaces = await db
233-
.select({ id: workspace.id })
234-
.from(workspace)
235-
.where(and(eq(workspace.organizationId, organizationId), inArray(workspace.id, ids)))
236-
const known = new Set(orgWorkspaces.map((row) => row.id))
237-
const unknown = ids.filter((id) => !known.has(id))
238-
if (unknown.length > 0) {
239-
return NextResponse.json(
240-
{ error: `Override targets workspaces outside this organization: ${unknown.join(', ')}` },
241-
{ status: 400 }
242-
)
243-
}
226+
const foreignTargetsReason = await getForeignWorkspaceTargetsReason({
227+
organizationId,
228+
retentionOverrides: body.retentionOverrides,
229+
piiRedaction: body.piiRedaction,
230+
})
231+
if (foreignTargetsReason) {
232+
return NextResponse.json({ error: foreignTargetsReason }, { status: 400 })
244233
}
245234

246235
const [updated] = await db

apps/sim/app/api/v1/admin/organizations/[id]/data-retention/route.ts

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,17 @@ import { createLogger } from '@sim/logger'
2323
import { eq } from 'drizzle-orm'
2424
import { adminV1UpdateOrganizationDataRetentionContract } from '@/lib/api/contracts/v1/admin'
2525
import { parseRequest } from '@/lib/api/server'
26-
import { getPiiRedactionDenialReason } from '@/lib/billing/retention'
26+
import {
27+
getForeignWorkspaceTargetsReason,
28+
getPiiRedactionDenialReason,
29+
} from '@/lib/billing/retention'
2730
import { isFeatureEnabled } from '@/lib/core/config/feature-flags'
2831
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
2932
import { withAdminAuthParams } from '@/app/api/v1/admin/middleware'
3033
import {
3134
adminInvalidJsonResponse,
3235
adminValidationErrorResponse,
36+
badRequestResponse,
3337
forbiddenResponse,
3438
internalErrorResponse,
3539
notFoundResponse,
@@ -105,6 +109,20 @@ export const PATCH = withRouteHandler(
105109
merged.retentionOverrides = body.retentionOverrides
106110
}
107111

112+
/**
113+
* Same ownership check the settings UI applies. Neither `workspaceId`
114+
* field is a foreign key, so without it the Admin API could persist an
115+
* override naming another organization's workspace.
116+
*/
117+
const foreignTargetsReason = await getForeignWorkspaceTargetsReason({
118+
organizationId,
119+
retentionOverrides: body.retentionOverrides,
120+
piiRedaction: body.piiRedaction,
121+
})
122+
if (foreignTargetsReason) {
123+
return badRequestResponse(foreignTargetsReason)
124+
}
125+
108126
await db
109127
.update(organization)
110128
.set({ dataRetentionSettings: merged, updatedAt: new Date() })

apps/sim/lib/billing/retention.test.ts

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@
22
* @vitest-environment node
33
*/
44
import type { DataRetentionSettings, PiiRedactionRule } from '@sim/db/schema'
5-
import { describe, expect, it } from 'vitest'
5+
import { queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing'
6+
import { afterAll, beforeEach, describe, expect, it } from 'vitest'
67
import {
78
DEFAULT_PII_REDACTION,
9+
getForeignWorkspaceTargetsReason,
810
getPiiRedactionDenialReason,
911
resolveEffectivePiiRedaction,
1012
resolveEffectiveRetentionHours,
@@ -466,3 +468,51 @@ describe('getPiiRedactionDenialReason', () => {
466468
).toBeNull()
467469
})
468470
})
471+
472+
describe('getForeignWorkspaceTargetsReason', () => {
473+
beforeEach(resetDbChainMock)
474+
afterAll(resetDbChainMock)
475+
476+
it('skips the lookup entirely when nothing targets a workspace', async () => {
477+
await expect(
478+
getForeignWorkspaceTargetsReason({
479+
organizationId: 'org-1',
480+
retentionOverrides: [],
481+
piiRedaction: { rules: [{ workspaceId: null }] },
482+
})
483+
).resolves.toBeNull()
484+
})
485+
486+
it('accepts overrides whose workspaces belong to the organization', async () => {
487+
queueTableRows(schemaMock.workspace, [{ id: 'ws-1' }, { id: 'ws-2' }])
488+
489+
await expect(
490+
getForeignWorkspaceTargetsReason({
491+
organizationId: 'org-1',
492+
retentionOverrides: [{ workspaceId: 'ws-1' }, { workspaceId: 'ws-2' }],
493+
})
494+
).resolves.toBeNull()
495+
})
496+
497+
it('rejects an override naming a workspace the organization does not own', async () => {
498+
queueTableRows(schemaMock.workspace, [{ id: 'ws-1' }])
499+
500+
await expect(
501+
getForeignWorkspaceTargetsReason({
502+
organizationId: 'org-1',
503+
retentionOverrides: [{ workspaceId: 'ws-1' }, { workspaceId: 'ws-foreign' }],
504+
})
505+
).resolves.toContain('ws-foreign')
506+
})
507+
508+
it('also checks workspaces named by PII rules, not just overrides', async () => {
509+
queueTableRows(schemaMock.workspace, [])
510+
511+
await expect(
512+
getForeignWorkspaceTargetsReason({
513+
organizationId: 'org-1',
514+
piiRedaction: { rules: [{ workspaceId: 'ws-foreign' }] },
515+
})
516+
).resolves.toContain('ws-foreign')
517+
})
518+
})

apps/sim/lib/billing/retention.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1+
import { db } from '@sim/db'
12
import type { CustomPiiPattern, DataRetentionSettings, PiiStagePolicy } from '@sim/db/schema'
3+
import { workspace } from '@sim/db/schema'
4+
import { and, eq, inArray } from 'drizzle-orm'
25
import {
36
coercePiiLanguage,
47
DEFAULT_PII_LANGUAGE,
@@ -214,3 +217,42 @@ export function getPiiRedactionDenialReason(params: {
214217
? 'Granular PII redaction (workflow input and block outputs) is not enabled for this organization'
215218
: null
216219
}
220+
221+
/**
222+
* Rejects retention settings that point at workspaces the organization does not
223+
* own. Returns `null` when every referenced workspace belongs to it.
224+
*
225+
* Both `retentionOverrides` and per-workspace `piiRedaction` rules name a
226+
* workspace, and neither is a foreign key — an id that belongs to another
227+
* organization would persist silently and then be applied by
228+
* `resolveEffectiveRetentionHours` / `resolveEffectivePiiRedaction` to whatever
229+
* workspace later matched it. Shared so the settings API and the Admin API
230+
* cannot accept different data for the same organization.
231+
*/
232+
export async function getForeignWorkspaceTargetsReason(params: {
233+
organizationId: string
234+
retentionOverrides?: Array<{ workspaceId: string }> | null
235+
piiRedaction?: PiiRedactionRulesLike | null
236+
}): Promise<string | null> {
237+
const targeted = new Set<string>()
238+
for (const override of params.retentionOverrides ?? []) {
239+
if (override?.workspaceId) targeted.add(override.workspaceId)
240+
}
241+
for (const rule of params.piiRedaction?.rules ?? []) {
242+
if (rule.workspaceId) targeted.add(rule.workspaceId)
243+
}
244+
if (targeted.size === 0) return null
245+
246+
const ids = [...targeted]
247+
const owned = await db
248+
.select({ id: workspace.id })
249+
.from(workspace)
250+
.where(and(eq(workspace.organizationId, params.organizationId), inArray(workspace.id, ids)))
251+
252+
const known = new Set(owned.map((row) => row.id))
253+
const unknown = ids.filter((id) => !known.has(id))
254+
255+
return unknown.length > 0
256+
? `Override targets workspaces outside this organization: ${unknown.join(', ')}`
257+
: null
258+
}

0 commit comments

Comments
 (0)