Skip to content

Commit e2f389e

Browse files
committed
fix(interfaces): close a public output leak and three correctness gaps
Findings from a full-branch audit pass. Security: - The public form-submit route returned the workflow run's entire `output` to an anonymous visitor. A form module has no `outputConfigs`, so unlike the chat path there is nothing redacting it — for an agent block that means `toolCalls.list`: the literal arguments sent to every tool and the literal responses. No client consumed it. Dropped from the public contract; the authenticated in-app response is unchanged. - The interface share PUT validated the org policy against `authType ?? 'public'` while `upsertResourceShare` persists `authType ?? stored ?? 'public'`, so a bare re-enable was checked against the wrong mode in both directions. Now mirrors the file route, whose comment names this exact bug. - The public *file* OTP route compared codes with `!==`. The interfaces route added `safeCompare`; brought the files route up to it. Correctness: - A combined name+layout PATCH pre-validated with no `previous`, making the pre-flight stricter than the write it guards — renaming an interface that referenced an archived table would 400 on a layout the write accepts. - `DataRow`'s memo comparator observed `renderCellEditor` unconditionally. That callback is rebuilt on every keystroke that opens an editor and on every in-flight save, so every mounted row re-rendered where one needed to. Gated on the editing row, replacing the gate the editor-injection refactor dropped. - `CellContent` derives both the overlay and the dimmed cell from the editor's presence, so "editing with no editor" — a blank, unreadable cell — is unreachable rather than merely unused. CI: - `check:resources` was in package.json but ran nowhere. Added to test-build. Cleanup: deleted an unused `tableResourceId` and a duplicate `PendingTagIndicator`; consolidated the interface row mapper and the form submit-label default onto one definition each; completed a `vi.mock` factory missing three error classes the route narrows on with `instanceof`; corrected stale doc references and a comment asserting a Tailwind limitation that does not exist.
1 parent 3cecc86 commit e2f389e

33 files changed

Lines changed: 190 additions & 143 deletions

File tree

.github/workflows/test-build.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,9 @@ jobs:
123123
- name: API contract boundary audit
124124
run: bun run check:api-validation:strict
125125

126+
- name: Resource view boundary audit
127+
run: bun run check:resources
128+
126129
- name: Desktop bridge contract audit
127130
run: bun run check:desktop-bridge
128131

apps/sim/app/(landing)/components/share-link-button/share-link-button.tsx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,15 +54,15 @@ export function ShareLinkButton({ title, kind }: ShareLinkButtonProps) {
5454
</DropdownMenuTrigger>
5555
<DropdownMenuContent align='end'>
5656
<DropdownMenuItem onSelect={() => copy(window.location.href)}>
57-
<Duplicate className='size-4' />
57+
<Duplicate />
5858
{copied ? 'Copied!' : 'Copy link'}
5959
</DropdownMenuItem>
6060
<DropdownMenuItem onSelect={() => openShare(buildXShareUrl(composePost('@simdotai')))}>
61-
<XIcon className='size-4' />
61+
<XIcon />
6262
Post on X
6363
</DropdownMenuItem>
6464
<DropdownMenuItem onSelect={() => openShare(buildLinkedInPostUrl(composePost('Sim')))}>
65-
<LinkedInIcon className='size-4' />
65+
<LinkedInIcon />
6666
Post on LinkedIn
6767
</DropdownMenuItem>
6868
</DropdownMenuContent>

apps/sim/app/api/files/public/[token]/otp/route.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { createLogger } from '@sim/logger'
2+
import { safeCompare } from '@sim/security/compare'
23
import { normalizeEmail } from '@sim/utils/string'
34
import type { NextRequest } from 'next/server'
45
import { NextResponse } from 'next/server'
@@ -164,7 +165,7 @@ export const PUT = withRouteHandler(
164165
)
165166
}
166167

167-
if (storedOTP !== otp) {
168+
if (!safeCompare(storedOTP, otp)) {
168169
const result = await incrementOTPAttempts('file', resolved.share.id, email, storedValue)
169170
if (result === 'locked') {
170171
return NextResponse.json(

apps/sim/app/api/interfaces/[interfaceId]/modules/[moduleId]/submit/route.test.ts

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,10 @@ const {
1919
mockValidateFormSubmission,
2020
InterfaceConflictErrorMock,
2121
InterfaceLayoutErrorMock,
22+
InterfaceNotArchivedErrorMock,
23+
InterfaceNotFoundErrorMock,
2224
InterfaceStaleWriteErrorMock,
25+
InterfaceWorkspaceArchivedErrorMock,
2326
InvalidModuleReferenceErrorMock,
2427
} = vi.hoisted(() => {
2528
class InterfaceConflictErrorMock extends Error {
@@ -39,14 +42,27 @@ const {
3942
class InvalidModuleReferenceErrorMock extends Error {
4043
readonly code = 'INVALID_MODULE_REFERENCE' as const
4144
}
45+
/**
46+
* Not thrown anywhere in this suite, and still stubbed. `@/app/api/interfaces/utils`
47+
* — which the route imports — narrows on all three with `instanceof`, so omitting
48+
* one from the factory leaves it `undefined` and turns that narrowing into a
49+
* TypeError the moment this route maps a domain error. Cheaper to keep the
50+
* factory complete than to rediscover that as a 500.
51+
*/
52+
class InterfaceNotFoundErrorMock extends Error {}
53+
class InterfaceNotArchivedErrorMock extends Error {}
54+
class InterfaceWorkspaceArchivedErrorMock extends Error {}
4255
return {
4356
mockExecuteWorkflow: vi.fn(),
4457
mockGetInterfaceById: vi.fn(),
4558
mockReleaseExecutionSlot: vi.fn(),
4659
mockValidateFormSubmission: vi.fn(),
4760
InterfaceConflictErrorMock,
4861
InterfaceLayoutErrorMock,
62+
InterfaceNotArchivedErrorMock,
63+
InterfaceNotFoundErrorMock,
4964
InterfaceStaleWriteErrorMock,
65+
InterfaceWorkspaceArchivedErrorMock,
5066
InvalidModuleReferenceErrorMock,
5167
}
5268
})
@@ -61,6 +77,9 @@ vi.mock('@/lib/interfaces', () => ({
6177
InterfaceConflictError: InterfaceConflictErrorMock,
6278
InterfaceLayoutError: InterfaceLayoutErrorMock,
6379
InterfaceStaleWriteError: InterfaceStaleWriteErrorMock,
80+
InterfaceNotFoundError: InterfaceNotFoundErrorMock,
81+
InterfaceNotArchivedError: InterfaceNotArchivedErrorMock,
82+
InterfaceWorkspaceArchivedError: InterfaceWorkspaceArchivedErrorMock,
6483
InvalidModuleReferenceError: InvalidModuleReferenceErrorMock,
6584
}))
6685

@@ -73,7 +92,10 @@ vi.mock('@/lib/billing/calculations/usage-reservation', () => ({
7392
}))
7493

7594
vi.mock('@/lib/execution/preprocessing', () => executionPreprocessingMock)
76-
vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: vi.fn() }))
95+
vi.mock('@/lib/posthog/server', () => ({
96+
captureServerEvent: vi.fn(),
97+
getPostHogClient: vi.fn(() => null),
98+
}))
7799

78100
vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock)
79101

apps/sim/app/api/interfaces/[interfaceId]/route.test.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,10 @@ describe('PATCH /api/interfaces/[interfaceId]', () => {
299299
await expect(response.json()).resolves.toEqual({
300300
error: 'Table "tbl-1" was not found in this workspace',
301301
})
302-
expect(mockValidateLayout).toHaveBeenCalledWith('ws-1', EMPTY_LAYOUT)
302+
// The committed layout is passed as `previous` so the pre-flight grandfathers
303+
// exactly what the write does — otherwise a rename would 400 on a reference
304+
// (an archived table, say) that `updateInterfaceLayout` would have accepted.
305+
expect(mockValidateLayout).toHaveBeenCalledWith('ws-1', EMPTY_LAYOUT, buildDefinition().layout)
303306
expect(mockRenameInterface).not.toHaveBeenCalled()
304307
expect(mockUpdateInterfaceLayout).not.toHaveBeenCalled()
305308
expect(mockRecordAudit).not.toHaveBeenCalled()

apps/sim/app/api/interfaces/[interfaceId]/route.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,16 @@ export const PATCH = withRouteHandler(
110110
*/
111111
const layoutSharesRequest = body.name !== undefined || body.description !== undefined
112112
if (body.layout !== undefined && layoutSharesRequest) {
113-
await validateLayout(access.definition.workspaceId, body.layout)
113+
/**
114+
* Passed the committed layout as `previous` for the same reason
115+
* `updateInterfaceLayout` does: references already stored are
116+
* grandfathered, so an interface whose table was archived stays
117+
* renameable. Without it this pre-flight is stricter than the write it
118+
* is guarding, and renaming would 400 on a layout the write would have
119+
* accepted. It is only a fail-fast — the authoritative check still runs
120+
* inside the transaction against a `FOR UPDATE` re-read.
121+
*/
122+
await validateLayout(access.definition.workspaceId, body.layout, access.definition.layout)
114123
}
115124

116125
let result: InterfaceDefinition = access.definition

apps/sim/app/api/interfaces/[interfaceId]/share/route.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,11 +105,19 @@ export const PUT = withRouteHandler(
105105
// master on/off and the per-auth-type allow-list); disabling is always
106106
// allowed so users can still un-share after the policy is turned on.
107107
if (isActive) {
108+
// Validate the auth type that will ACTUALLY be persisted. upsertResourceShare
109+
// falls back to the existing share's authType when none is passed, so a bare
110+
// re-enable must be checked against that stored mode — not 'public' — or a
111+
// now-disallowed password/email/sso share could be silently reactivated, and
112+
// a still-permitted one could be falsely rejected by a group that only bars
113+
// 'public'.
114+
const existingShare = await getShareForResource('interface', interfaceId)
115+
const effectiveAuthType = authType ?? existingShare?.authType ?? 'public'
108116
try {
109117
await validatePublicInterfaceSharing(
110118
session.user.id,
111119
definition.workspaceId,
112-
authType ?? 'public'
120+
effectiveAuthType
113121
)
114122
} catch (error) {
115123
if (error instanceof PublicInterfaceSharingNotAllowedError) {

apps/sim/app/api/interfaces/public/[token]/modules/[moduleId]/submit/route.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,6 @@ export const POST = withRouteHandler(
182182
success: true,
183183
data: {
184184
executionId: run.metadata?.executionId ?? executionId,
185-
output: run.output,
186185
},
187186
})
188187
} finally {

apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/index.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ export {
3131
} from '@/components/chat/special-tags/parse'
3232
export {
3333
CredentialDisplay,
34-
PendingTagIndicator,
3534
SpecialTags,
3635
WorkspaceResourceDisplay,
3736
} from './special-tags'

apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.tsx

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ import type {
2727
WorkspaceResourceTagData,
2828
WorkspaceResourceTagType,
2929
} from '@/components/chat/special-tags/parse'
30-
import { ThinkingLoader } from '@/components/ui'
3130
import { useSession } from '@/lib/auth/auth-client'
3231
import { buildHostedUpgradeUrl, HOSTED_BILLING_SETTINGS_URL } from '@/lib/billing/upgrade-reasons'
3332
import { canManageWorkspaceBilling } from '@/lib/billing/workspace-permissions'
@@ -124,22 +123,6 @@ export function SpecialTags({
124123
}
125124
}
126125

127-
interface PendingTagIndicatorProps {
128-
/** Activity phrase next to the loader; crossfades on change. */
129-
label: string
130-
}
131-
132-
/**
133-
* Renders the turn-level activity shimmer.
134-
*/
135-
export function PendingTagIndicator({ label }: PendingTagIndicatorProps) {
136-
return (
137-
<div className='animate-stream-fade-in py-2'>
138-
<ThinkingLoader size={20} startVariant='corners' label={label} labelRatio={0.7} />
139-
</div>
140-
)
141-
}
142-
143126
interface OptionsDisplayProps {
144127
data: OptionsTagData
145128
onSelect?: (id: string) => void

0 commit comments

Comments
 (0)