Skip to content

Commit 3cedac8

Browse files
authored
fix(security): authz, IDOR, and abuse-prevention fixes (#4944)
* fix(knowledge): require write access for batch chunk operations The PATCH /api/knowledge/[id]/documents/[documentId]/chunks handler performs enable/disable/delete operations but authorized callers with only read-level access (checkDocumentAccess). This let read-only workspace members destroy or disable indexed chunks. Switch to checkDocumentWriteAccess (write/admin required), matching the sibling POST/PUT/DELETE chunk mutation endpoints. * fix(env): restrict decrypted workspace env vars to secret admins GET /api/workspaces/:id/environment returned decrypted workspace environment variables to any member, including read-only collaborators, leaking API tokens, database URLs, and other secrets. Mask workspace variable values for non-admin viewers while preserving the variable names, so editor autocomplete and conflict detection keep working. A value is revealed only when the caller is a credential admin of that key, or — for legacy keys with no per-secret ACL — holds workspace admin permission. This mirrors the per-key edit gating already enforced by PUT/DELETE: if you can administer a secret, you can read it. Personal variables and execution-time resolution are unchanged. * fix(files): block cross-tenant deletion via client-controlled context POST /api/files/delete trusted a client-supplied `context`, letting any authenticated user delete another tenant's file by naming an arbitrary key with `context: "og-images"`. verifyFileAccess() short-circuited the three public contexts (profile-pictures, og-images, workspace-logos) to `true` before any ownership/requireWrite check. - Derive the storage context strictly from the trusted key prefix in the delete route; reject a supplied `context` that disagrees with the key. - Gate the public-context short-circuit to reads only. Destructive ops (requireWrite) now prove ownership via verifyPublicAssetWriteAccess: workspace-logos require write/admin on the bound workspace, profile-pictures require an exact owner match, og-images always deny. Reads of public assets are unchanged. * fix(telegram): verify X-Telegram-Bot-Api-Secret-Token on inbound webhooks Telegram triggers accepted any forged update from anyone who knew the webhook URL path: verifyAuth was a no-op that always returned null, and setWebhook registered no secret_token. Generate a per-webhook secret in createSubscription, register it with Telegram as secret_token, and persist it to providerConfig. verifyAuth now fails closed — rejects when no token is configured, when the X-Telegram-Bot-Api-Secret-Token header is absent, or when it does not match via constant-time safeCompare. * fix(security): pin DNS for Agiloft directExecution and Grafana update tools The Agiloft directExecution tools (read/create/search/update/delete/lock/ saved_search/select/get_choice_line_id/remove_attachment/attachment_info) and the Grafana update_dashboard/update_alert_rule postProcess hooks issued outbound HTTP to a fully user-controlled host (instanceUrl/baseUrl) via the global fetch(), guarded only by the synchronous validateExternalUrl() — which never resolves DNS, so a hostname resolving to an internal/reserved IP passed validation (SSRF). Route all of these through the codebase's standard SSRF-safe path: - Agiloft: moved executeAgiloftRequest into utils.server.ts where the existing pinned helpers live. It now resolves+validates the instance URL once and pins every hop (login, operation, logout) to that IP via secureFetchWithPinnedIP. The 11 tool configs now import it from utils.server; URL builders stay in the client-safe utils.ts. - Grafana: the postProcess POST/PUT now uses validateUrlWithDNS + secureFetchWithPinnedIP, matching the already-pinned initial GET. This completes the Agiloft SSRF pinning started in #4639 (which covered the attach/retrieve API routes) by closing the directExecution path, and extends the same guard to the Grafana update tools. * fix(api): enforce workspace allowPersonalApiKeys policy on v1 surface The external v1 API authenticated API keys without evaluating the per-workspace allowPersonalApiKeys setting, so a personal API key could read and mutate a workspace's resources (workflows, tables, files, knowledge, logs) even when the workspace had explicitly disabled personal keys. The same control is already enforced on the workflow-execution surface. Enforce the policy in checkWorkspaceScope (covering validateWorkspaceAccess too): reject personal keys with 403 when the workspace has allowPersonalApiKeys=false. checkWorkspaceScope becomes async; all v1 route callsites updated to await it. * fix(billing): close usage-cap admission race with atomic reservation The server-side usage-limit gate read already-recorded cost, but cost is only written when an execution finishes. A burst of concurrent executions all observed the same pre-burst usage, all passed the cap, and all ran — collectively spending far past the limit before any cost landed in the ledger (free-tier abuse / hard-cap defeat). manual/chat triggers also skip rate limiting, removing the only throttle. Add an atomic check-then-reserve admission step (Redis Lua) that bounds in-flight, un-costed executions per billing entity by both a per-plan concurrency cap and remaining usage headroom, so recordedUsage + reservedSlots * estimate <= limit always holds. The slot is released at execution completion via LoggingSession (skipped on pause; TTL self-heals crashes). Runs for all trigger types, covering the previously-unthrottled manual/chat paths. Fails open when billing is disabled or Redis is unavailable, matching the rate limiter — a Redis blip can't turn into an execution outage, and the recorded-usage gate still runs. * fix(workflows): validate folderId belongs to workflow's workspace on create/update/reorder Reject a folderId that references a folder in a different workspace (or an archived/non-existent folder) before writing it to workflow.folderId. Previously create, update, and reorder only checked workspace permission on the workflow and the folder's lock status, never that the folder lived in the workflow's own workspace, allowing a dangling cross-workspace folder reference. Adds isFolderInWorkspace/assertFolderInWorkspace + FolderNotFoundError to @sim/workflow-authz (mirroring assertTargetFolderMutable in the duplicate path), enforced in performCreateWorkflow, performUpdateWorkflow, and the reorder route. Invalid folders now return 400. * fix(folders): validate parentId against workspace on create/update/reorder Folder write endpoints accepted a caller-supplied parentId and persisted it without verifying the parent existed in the same workspace, and the create and reorder paths had no cycle guard. A workspace member with write access could reparent a folder to a foreign-workspace folder, a non-existent id, or (via reorder) into a cycle, hiding the folder and its workflows from all members. - performCreateFolder: reject self-parenting and validate the parent exists in the workspace and is not archived (mirrors the duplicate route). - performUpdateFolder: add the same workspace/archived parent check alongside the existing circular-reference guard. - folders/reorder: validate every target parent against the workspace, detect cycles in the resulting parent graph (catches batch cycles), and normalize falsy parentId to null to prevent orphaning. Adds tests for cross-workspace parent rejection and batch-cycle rejection. * chore(knowledge): drop non-TSDoc inline comments from chunks route * fix(webhooks): fail closed when HMAC signing secret is not configured Inbound webhook signature verification failed open for HMAC providers (GitHub, Intercom, Jira, JSM, Confluence, Cal.com, Notion, Greenhouse, Typeform, Fireflies, Circleback): when no signing secret was stored, verifyAuth returned null and the workflow executed on a fully attacker-controlled body. Reject these deliveries with 401 instead, matching the fail-closed Stripe/WhatsApp/Vercel providers. Run provider reachability/verification handshakes (Notion verification_token, Grain/Intercom ping) ahead of auth so the pre-secret setup handshake still completes — those return a canned 200 without executing the workflow, and real event payloads fall through to fail-closed verification. Update the trigger secret-field copy to state the secret is required for deliveries to be accepted (was misleadingly marked optional). * style(files): trim verbose inline comments on delete authorization fix * fix(auth): close account-enumeration oracle on email sign-up The custom before-hook pre-check threw a distinguishing 422/USER_ALREADY_EXISTS for already-registered emails, letting an unauthenticated attacker enumerate accounts — defeating better-auth's own OWASP enumeration protection (active under requireEmailVerification). Remove the pre-check and rely on better-auth's generic duplicate-sign-up response, wiring: - onExistingUserSignUp: notify the real account owner out-of-band, mirroring the privacy-preserving forget-password flow. - customSyntheticUser: include admin (role/banned/banReason/banExpires) and Stripe (stripeCustomerId, billing-gated) user fields so the fake response shape is byte-identical to a real new-user response. Adds an ExistingAccountEmail template + 'existing-account' subject. * style(tools): drop non-TSDoc inline comments from Grafana/Agiloft SSRF tools * chore(api): trim extraneous inline comments in v1 logs/files routes Remove a redundant size annotation and two verbose multi-line materialization comments whose intent is already clear from the code. Load-bearing comments (race-condition and key-translation notes) kept. * fix(billing): exclude table-cell dispatch from admission reservation Table-cell dispatch is row-bounded, async rate-limited, and already surfaces a graceful usage state. Applying the in-flight concurrency reservation there turned its 429 into a hard cell error on a normal >15-concurrent-cell run (only 402 was handled gracefully). Skip the reservation for that surface via a new skipConcurrencyReservation option (the usage-cost cap is still enforced), and tidy the reservation comments to TSDoc. * fix(chat): rate-limit and constant-time password auth for public chats Password-protected public chat (POST /api/chat/[identifier]) had no throttling on the password check and compared with a non-constant-time !==, allowing unlimited brute-force and per-character timing leaks. - Add per-IP rate limiting (10 / 15min) to the password branch of validateChatAuth, mirroring the OTP/SSO endpoints; return 429 with Retry-After. Only explicit unlock attempts consume tokens — message sends carry no password and ride the auth cookie. - Replace password !== decrypted with safeCompare. - Fails open on rate-limiter storage errors; no availability regression. * fix(security): cap JSON request body size and gate public chat endpoint The shared parseJsonBody helper (behind parseRequest, used by nearly every contract route) read request bodies with no size limit, buffering the full body into memory before validation. The unauthenticated public deployed-chat endpoint reached this sink with no admission gate, enabling an anonymous memory-exhaustion DoS. - parseRequest/parseJsonBody now enforce a byte cap via a size-limited stream read (content-length precheck + streamed cap), returning 413. Default is API_MAX_JSON_BODY_BYTES (50 MB), overridable per route via maxBodyBytes. Decoding uses TextDecoder to match request.json() BOM handling. - Public chat POST is wrapped with the admission gate (tryAdmit) and passes an explicit CHAT_MAX_REQUEST_BYTES (20 MB) cap. - Chat body contract gains .max() bounds on input, password, conversationId, file data/name/type, and files array length. - Admin bulk workspace import opts into a higher 100 MB cap to avoid regressing large multi-workflow imports. * fix(chat): rate-limit and constant-time password auth for public chats Password-protected public chat (POST /api/chat/[identifier]) had no throttling on the password check and compared with a non-constant-time !==, allowing unlimited brute-force and per-character timing leaks. - Add per-IP rate limiting (10 / 15min) to the password branch of validateChatAuth, mirroring the OTP/SSO endpoints; return 429 with Retry-After. Only explicit unlock attempts consume tokens — message sends carry no password and ride the auth cookie. - Replace password !== decrypted with safeCompare. - Fails open on rate-limiter storage errors; no availability regression. Reinstates the fix reverted by an intervening commit. * fix(billing): never block a lone execution on usage headroom The admission reservation tapered allowed concurrency by remaining usage headroom. With under one credit of headroom left (but not yet over the cap), floor(headroom / estimate) hit zero and rejected even a single, zero-concurrency execution — stricter than the recorded-usage gate, which would have allowed that last run, and with a misleading "too many concurrent executions" message. Floor the headroom term at 1 so a lone execution is governed only by the cost gate; concurrency above the first slot still tapers with headroom. * refactor(env): document workspace env masking, drop inline comments Extract the workspace-env value masking into a TSDoc-documented maskWorkspaceEnvForViewer helper and remove the redundant inline comments from the GET handler and its test. No behavior change. * refactor(env): convert PUT/DELETE authz comments to TSDoc Move the tiered-authorization rationale for the workspace env upsert and delete handlers into TSDoc blocks and drop the inline comments. No behavior change. * fix(telegram): keep legacy webhooks working via Telegram source-IP fallback The secret-token check rejected every webhook registered before secret_token support, breaking live triggers until re-saved. Fall back to verifying the request originates from Telegram's published webhook IP ranges when no secret is configured, so existing triggers keep firing with no re-save or migration while forged updates from arbitrary hosts are still rejected. Webhooks with a registered secret continue to use strict constant-time token verification. * fix(chat): restore constant-time password auth and IP rate limit A billing commit (ac56525) reverted the public-chat auth hardening as collateral, leaving HEAD with a timing-oracle password comparison (password !== decrypted) and no per-IP brute-force rate limit. Restore safeCompare and the password-attempt rate limiter, and re-add the 429 test. * revert(webhooks): undo trigger auth hardening pending compat plan Reverts the Telegram inbound-token verification (3ed97a4, 41f133a) and the HMAC fail-closed change (5b6cae9). Production data shows ~79 live webhooks have no signing secret configured (63 GitHub, 9 Fireflies, 3 Jira, 2 Circleback, 1 Confluence, 1 Cal.com), so failing closed would 401 them. Restoring fail-open behavior until a backwards-compatible rollout (grandfather existing secretless webhooks / migration) is designed. Other security fixes on this branch are unaffected. * test(chat): make RateLimiter mock a constructable class The arrow-function mockImplementation form was not reliably constructable in the full suite run (`new RateLimiter()` threw "is not a constructor"), though it passed in isolation. Switch to the class-based mock used by the sibling OTP/speech route tests. * fix(billing): release admission slot on pre-execution aborts; cluster-safe release Addresses PR review on the usage-cap admission reservation: - Slot leak: the reservation taken at the end of preprocessing was only released when the LoggingSession finalized. The execute route's pre-execution exits (client cancel, workspace/API-key guards) returned without finalizing a session, leaking the slot until its TTL and wrongly throttling later runs. Release explicitly on those paths; executions that start are still released via session finalization. - Release is now cluster-safe: replaced the Lua script that rebuilt the in-flight key from the pointer value (a key not declared in KEYS, which silently breaks Redis Cluster slot routing) with discrete single-key GETDEL + ZREM commands. * improvement(files): log missing owner metadata distinctly on profile-picture delete deny Per PR review: when a profile-picture delete is denied, distinguish a missing owner record (no userId metadata) from a genuine ownership mismatch so the fail-closed denial is diagnosable. Behavior unchanged — both still deny. * fix(billing): release admission slot when async enqueue fails If queueing the background workflow job throws, no job runs and no LoggingSession finalizes, so the admission slot reserved during preprocessing would leak until its TTL. Release it before returning 500. * fix(api): make body-size caps NaN-safe and raise chat input/attachment limits - DEFAULT_MAX_JSON_BODY_BYTES and CHAT_MAX_REQUEST_BYTES now fall back to hardcoded defaults (50 MB / 220 MB) when the env value is missing or non-numeric, so a misconfig can't silently produce a NaN cap that never rejects. - Raise CHAT_MAX_REQUEST_BYTES default to 220 MB to cover 15 base64 file attachments, and MAX_CHAT_INPUT_CHARS to 1,000,000. - Minor: tidy use-inline-rename onSave type; drop two redundant test comments. * fix(hooks): restore void return in useInlineRename onSave type A prior commit changed onSave's return type from `void | Promise<unknown>` to `undefined | Promise<unknown>`, which broke the build: callbacks that return nothing (table-grid column rename, table header rename) infer a `void` return, which is not assignable to `undefined`. Restore the `void` union so both fire-and-forget and Promise-returning callbacks type-check. * fix(billing,api): release chat reservation slot on early exit; preserve 413 on oversized import - Chat route: preprocessExecution reserves a billing concurrency slot, but the post-preprocess early exits (missing workspaceId, execution-setup failure) returned without releasing it, leaking the slot until TTL and wrongly throttling later runs. Release explicitly on those paths (idempotent), mirroring the workflows execute route. - Admin import route: an oversized JSON body now returns the real 413 from parseJsonBody instead of being remapped to a 400; invalid JSON still 400s. * fix(icons): make Infisical icon black for contrast; regenerate docs The Infisical mark rendered near-white on its yellow block background and was barely visible; switch its fill from currentColor to #000000 (matching the hardcoded-fill pattern of sibling brand icons). Sync the docs icon copy and pick up a stale servicenow doc regeneration. * fix(billing): release reserved slot on execute-route 503 and setup throw After preprocessExecution reserves a billing concurrency slot, the streaming path could exit without releasing it: the 503 return when initializeExecutionStreamMeta fails, and any throw during stream setup (caught by the outer handler, which only returned 500). Both left the slot held until TTL, wrongly throttling unrelated runs. Release on the 503 path and in the outer catch (executionId hoisted so the catch can see it; release is idempotent and a no-op when no slot was reserved). * fix(icons): make Linkup icon black for contrast The Linkup mark rendered with currentColor (near-white on its block background); switch its fill to #000000 for legibility, matching the Infisical fix. Docs icon copy synced via generate-docs. * fix(billing): release reserved slot if inline async job never starts In the inline (single-process) async path, if jobQueue.startJob threw before executeWorkflowJob ran, no LoggingSession finalized and the reserved billing slot was held until TTL. Release it in the fire-and-forget catch (idempotent; a no-op when the job already finalized and released). The queued-worker path and all in-job outcomes already release via the job's LoggingSession finalize.
1 parent 6abcf82 commit 3cedac8

61 files changed

Lines changed: 1702 additions & 295 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

apps/docs/components/icons.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2492,7 +2492,7 @@ export function LinkupIcon(props: SVGProps<SVGSVGElement>) {
24922492
<svg {...props} xmlns='http://www.w3.org/2000/svg' viewBox='0 0 154 107' fill='none'>
24932493
<path
24942494
d='M150.68 72.71C146.61 70.25 137.91 69.54 124.79 70.61C128.99 57.68 133.76 35.39 121.32 25.15C115.89 20.67 107.47 19.04 97.62 20.56C94.68 21.01 91.58 21.74 88.39 22.73C78.87 8.28 66.3 0 53.86 0C39.43 0 26.13 9.34 16.41 26.29C5.68 45.01 0 71.96 0 104.23V104.53L3.6 106.94L3.88 106.83C30.58 95.56 67.58 85.07 100.59 79.4C101.6 87.64 102.12 95.99 102.12 104.24V104.52L105.49 107L105.76 106.91C106.26 106.75 155.16 90.88 153.98 77.59C153.86 76.2 153.18 74.23 150.68 72.71ZM148.41 78.09C148.72 81.54 133.24 91.06 111.84 98.89C115.97 92.1 119.82 84.17 122.78 76.36C135.66 75.14 144.53 75.55 147.79 77.53C148.38 77.88 148.41 78.09 148.41 78.09ZM116.67 77.01C114.08 83.38 110.95 89.63 107.54 95.25C107.33 89.51 106.91 83.88 106.3 78.46C109.92 77.9 113.41 77.41 116.67 77.01ZM117.77 29.5C125.38 35.76 125.78 51.32 118.87 71.18C114.75 71.63 110.28 72.24 105.6 72.98C103.05 55.17 98.28 39.97 91.42 27.75C94.57 26.82 96.97 26.35 98.46 26.11C106.72 24.84 113.58 26.04 117.77 29.5ZM53.86 5.62C65.06 5.62 74.89 12 83.09 24.59C57.7 34.54 30.32 59.41 5.78 94.8C7.43 51.48 23.03 5.62 53.86 5.62M10.19 98.24C40.75 53.93 68.2 36.44 86.07 29.59C92.45 41.24 97.2 56.55 99.84 73.93C70.52 79.03 35.64 88.5 10.19 98.24Z'
2495-
fill='currentColor'
2495+
fill='#000000'
24962496
/>
24972497
</svg>
24982498
)
@@ -4967,7 +4967,7 @@ export function InfisicalIcon(props: SVGProps<SVGSVGElement>) {
49674967
<svg {...props} viewBox='20 25 233 132' xmlns='http://www.w3.org/2000/svg'>
49684968
<path
49694969
d='m191.6 39.4c-20.3 0-37.15 13.21-52.9 30.61-12.99-16.4-29.8-30.61-51.06-30.61-27.74 0-50.44 23.86-50.44 51.33 0 26.68 21.43 51.8 48.98 51.8 20.55 0 37.07-13.86 51.32-31.81 12.69 16.97 29.1 31.41 53.2 31.41 27.13 0 49.85-22.96 49.85-51.4 0-27.12-20.44-51.33-48.95-51.33zm-104.3 77.94c-14.56 0-25.51-12.84-25.51-26.07 0-13.7 10.95-28.29 25.51-28.29 14.93 0 25.71 11.6 37.6 27.34-11.31 15.21-22.23 27.02-37.6 27.02zm104.4 0.25c-15 0-25.28-11.13-37.97-27.37 12.69-16.4 22.01-27.24 37.59-27.24 14.97 0 24.79 13.25 24.79 27.26 0 13-10.17 27.35-24.41 27.35z'
4970-
fill='currentColor'
4970+
fill='#000000'
49714971
/>
49724972
</svg>
49734973
)

apps/sim/app/api/chat/[identifier]/route.ts

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@ import { and, eq, isNull } from 'drizzle-orm'
66
import { type NextRequest, NextResponse } from 'next/server'
77
import { deployedChatPostContract } from '@/lib/api/contracts/chats'
88
import { parseRequest } from '@/lib/api/server'
9+
import { releaseExecutionSlot } from '@/lib/billing/calculations/usage-reservation'
10+
import { admissionRejectedResponse, tryAdmit } from '@/lib/core/admission/gate'
11+
import { env } from '@/lib/core/config/env'
912
import { validateAuthToken } from '@/lib/core/security/deployment'
1013
import { generateRequestId } from '@/lib/core/utils/request'
1114
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
@@ -40,13 +43,21 @@ function toChatConfigResponse(deployment: ChatConfigSource) {
4043
export const dynamic = 'force-dynamic'
4144
export const runtime = 'nodejs'
4245

46+
const CHAT_MAX_REQUEST_BYTES = Number.parseInt(env.CHAT_MAX_REQUEST_BYTES, 10) || 220 * 1024 * 1024
47+
4348
export const POST = withRouteHandler(
4449
async (request: NextRequest, context: { params: Promise<{ identifier: string }> }) => {
4550
const { identifier } = await context.params
4651
const requestId = generateRequestId()
4752

53+
const ticket = tryAdmit()
54+
if (!ticket) {
55+
return admissionRejectedResponse()
56+
}
57+
4858
try {
4959
const parsed = await parseRequest(deployedChatPostContract, request, context, {
60+
maxBodyBytes: CHAT_MAX_REQUEST_BYTES,
5061
validationErrorResponse: (err) => {
5162
const message = err.issues.map((e) => `${e.path.join('.')}: ${e.message}`).join(', ')
5263
return createErrorResponse(`Invalid request body: ${message}`, 400, 'VALIDATION_ERROR')
@@ -125,7 +136,14 @@ export const POST = withRouteHandler(
125136

126137
const authResult = await validateChatAuth(requestId, deployment, request, parsedBody)
127138
if (!authResult.authorized) {
128-
return createErrorResponse(authResult.error || 'Authentication required', 401)
139+
const response = createErrorResponse(
140+
authResult.error || 'Authentication required',
141+
authResult.status || 401
142+
)
143+
if (authResult.status === 429 && authResult.retryAfterMs !== undefined) {
144+
response.headers.set('Retry-After', String(Math.ceil(authResult.retryAfterMs / 1000)))
145+
}
146+
return response
129147
}
130148

131149
const { input, password, email, conversationId, files } = parsedBody
@@ -177,6 +195,9 @@ export const POST = withRouteHandler(
177195
const workspaceId = workflowRecord?.workspaceId
178196
if (!workspaceId) {
179197
logger.error(`[${requestId}] Workflow ${deployment.workflowId} has no workspaceId`)
198+
// preprocessExecution reserved a billing concurrency slot; release it on
199+
// this early exit since no LoggingSession will finalize to free it.
200+
await releaseExecutionSlot(executionId)
180201
return createErrorResponse('Workflow has no associated workspace', 500)
181202
}
182203

@@ -283,11 +304,16 @@ export const POST = withRouteHandler(
283304
return streamResponse
284305
} catch (error: any) {
285306
logger.error(`[${requestId}] Error processing chat request:`, error)
307+
// Setup failed before the workflow stream took over slot release;
308+
// free the reserved billing slot (idempotent if already released).
309+
await releaseExecutionSlot(executionId)
286310
return createErrorResponse(error.message || 'Failed to process request', 500)
287311
}
288312
} catch (error: any) {
289313
logger.error(`[${requestId}] Error processing chat request:`, error)
290314
return createErrorResponse(error.message || 'Failed to process request', 500)
315+
} finally {
316+
ticket.release()
291317
}
292318
}
293319
)

apps/sim/app/api/chat/utils.test.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,21 @@ const {
1919
mockSetDeploymentAuthCookie,
2020
mockIsEmailAllowed,
2121
mockGetSession,
22+
mockCheckRateLimitDirect,
2223
} = vi.hoisted(() => ({
2324
mockMergeSubblockStateWithValues: vi.fn().mockReturnValue({}),
2425
mockMergeSubBlockValues: vi.fn().mockReturnValue({}),
2526
mockValidateAuthToken: vi.fn().mockReturnValue(false),
2627
mockSetDeploymentAuthCookie: vi.fn(),
2728
mockIsEmailAllowed: vi.fn(),
2829
mockGetSession: vi.fn(),
30+
mockCheckRateLimitDirect: vi.fn().mockResolvedValue({ allowed: true }),
31+
}))
32+
33+
vi.mock('@/lib/core/rate-limiter', () => ({
34+
RateLimiter: class {
35+
checkRateLimitDirect = mockCheckRateLimitDirect
36+
},
2937
}))
3038

3139
vi.mock('@/lib/auth', () => ({
@@ -149,6 +157,7 @@ describe('Chat API Utils', () => {
149157
describe('Chat auth validation', () => {
150158
beforeEach(() => {
151159
mockDecryptSecret.mockResolvedValue({ decrypted: 'correct-password' })
160+
mockCheckRateLimitDirect.mockResolvedValue({ allowed: true })
152161
})
153162

154163
it('should allow access to public chats', async () => {
@@ -235,6 +244,32 @@ describe('Chat API Utils', () => {
235244
expect(result.error).toBe('Invalid password')
236245
})
237246

247+
it('should return 429 when the password attempt rate limit is exceeded', async () => {
248+
mockCheckRateLimitDirect.mockResolvedValueOnce({ allowed: false, retryAfterMs: 60_000 })
249+
250+
const deployment = {
251+
id: 'chat-id',
252+
authType: 'password',
253+
password: 'encrypted-password',
254+
}
255+
256+
const mockRequest = {
257+
method: 'POST',
258+
cookies: {
259+
get: vi.fn().mockReturnValue(null),
260+
},
261+
} as any
262+
263+
const result = await validateChatAuth('request-id', deployment, mockRequest, {
264+
password: 'any-guess',
265+
})
266+
267+
expect(result.authorized).toBe(false)
268+
expect(result.status).toBe(429)
269+
expect(result.retryAfterMs).toBe(60_000)
270+
expect(decryptSecret).not.toHaveBeenCalled()
271+
})
272+
238273
it('should request email auth for email-protected chats', async () => {
239274
const deployment = {
240275
id: 'chat-id',

apps/sim/app/api/chat/utils.ts

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,34 @@
11
import { db } from '@sim/db'
22
import { chat, workflow } from '@sim/db/schema'
33
import { createLogger } from '@sim/logger'
4+
import { safeCompare } from '@sim/security/compare'
45
import { authorizeWorkflowByWorkspacePermission } from '@sim/workflow-authz'
56
import { and, eq, isNull } from 'drizzle-orm'
67
import type { NextRequest, NextResponse } from 'next/server'
8+
import type { TokenBucketConfig } from '@/lib/core/rate-limiter'
9+
import { RateLimiter } from '@/lib/core/rate-limiter'
710
import {
811
isEmailAllowed,
912
setDeploymentAuthCookie,
1013
validateAuthToken,
1114
} from '@/lib/core/security/deployment'
1215
import { decryptSecret } from '@/lib/core/security/encryption'
16+
import { getClientIp } from '@/lib/core/utils/request'
1317

1418
const logger = createLogger('ChatAuthUtils')
1519

20+
const rateLimiter = new RateLimiter()
21+
22+
/**
23+
* Throttles unauthenticated password guesses per client IP against a single
24+
* deployment, mirroring the OTP/SSO IP limits.
25+
*/
26+
const PASSWORD_IP_RATE_LIMIT: TokenBucketConfig = {
27+
maxTokens: 10,
28+
refillRate: 10,
29+
refillIntervalMs: 15 * 60_000,
30+
}
31+
1632
export function setChatAuthCookie(
1733
response: NextResponse,
1834
chatId: string,
@@ -88,7 +104,7 @@ export async function validateChatAuth(
88104
deployment: any,
89105
request: NextRequest,
90106
parsedBody?: any
91-
): Promise<{ authorized: boolean; error?: string }> {
107+
): Promise<{ authorized: boolean; error?: string; status?: number; retryAfterMs?: number }> {
92108
const authType = deployment.authType || 'public'
93109

94110
if (authType === 'public') {
@@ -129,8 +145,25 @@ export async function validateChatAuth(
129145
return { authorized: false, error: 'Authentication configuration error' }
130146
}
131147

148+
const ip = getClientIp(request)
149+
const ipRateLimit = await rateLimiter.checkRateLimitDirect(
150+
`chat-password:ip:${deployment.id}:${ip}`,
151+
PASSWORD_IP_RATE_LIMIT
152+
)
153+
if (!ipRateLimit.allowed) {
154+
logger.warn(
155+
`[${requestId}] Password attempt IP rate limit exceeded for chat ${deployment.id} from ${ip}`
156+
)
157+
return {
158+
authorized: false,
159+
error: 'Too many attempts. Please try again later.',
160+
status: 429,
161+
retryAfterMs: ipRateLimit.retryAfterMs ?? PASSWORD_IP_RATE_LIMIT.refillIntervalMs,
162+
}
163+
}
164+
132165
const { decrypted } = await decryptSecret(deployment.password)
133-
if (password !== decrypted) {
166+
if (!safeCompare(password, decrypted)) {
134167
return { authorized: false, error: 'Invalid password' }
135168
}
136169

apps/sim/app/api/files/authorization.test.ts

Lines changed: 64 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,18 @@
1212
import { dbChainMock, dbChainMockFns } from '@sim/testing'
1313
import { beforeEach, describe, expect, it, vi } from 'vitest'
1414

15-
const { mockGetFileMetadataByKey, mockGetUserEntityPermissions } = vi.hoisted(() => ({
16-
mockGetFileMetadataByKey: vi.fn(),
17-
mockGetUserEntityPermissions: vi.fn(),
18-
}))
15+
const { mockGetFileMetadataByKey, mockGetUserEntityPermissions, mockGetFileMetadata } = vi.hoisted(
16+
() => ({
17+
mockGetFileMetadataByKey: vi.fn(),
18+
mockGetUserEntityPermissions: vi.fn(),
19+
mockGetFileMetadata: vi.fn(),
20+
})
21+
)
1922

2023
vi.mock('@sim/db', () => dbChainMock)
2124

2225
vi.mock('@/lib/uploads', () => ({
23-
getFileMetadata: vi.fn(),
26+
getFileMetadata: mockGetFileMetadata,
2427
}))
2528

2629
vi.mock('@/lib/uploads/config', () => ({
@@ -151,3 +154,59 @@ describe('verifyKBFileWriteAccess (binding-only delete authorization)', () => {
151154
await expect(verifyKBFileWriteAccess(CLOUD_KEY, USER_ID)).resolves.toBe(false)
152155
})
153156
})
157+
158+
describe('public-context access (profile-pictures / og-images / workspace-logos)', () => {
159+
beforeEach(() => {
160+
vi.clearAllMocks()
161+
})
162+
163+
function read(cloudKey: string, context: 'profile-pictures' | 'og-images' | 'workspace-logos') {
164+
return verifyFileAccess(cloudKey, USER_ID, undefined, context, false)
165+
}
166+
167+
function write(cloudKey: string, context: 'profile-pictures' | 'og-images' | 'workspace-logos') {
168+
return verifyFileAccess(cloudKey, USER_ID, undefined, context, false, { requireWrite: true })
169+
}
170+
171+
it('grants public reads without any ownership check', async () => {
172+
await expect(read('og-images/banner.png', 'og-images')).resolves.toBe(true)
173+
await expect(read('profile-pictures/123-avatar.png', 'profile-pictures')).resolves.toBe(true)
174+
await expect(read('workspace-logos/123-logo.png', 'workspace-logos')).resolves.toBe(true)
175+
expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
176+
expect(mockGetFileMetadata).not.toHaveBeenCalled()
177+
})
178+
179+
it('denies a cross-tenant delete that names a workspace key under a public context', async () => {
180+
await expect(write('workspace/victim-ws/123-report.pdf', 'og-images')).resolves.toBe(false)
181+
expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
182+
})
183+
184+
it('grants a profile-picture delete only to the owning user', async () => {
185+
mockGetFileMetadata.mockResolvedValue({ userId: USER_ID })
186+
await expect(write('profile-pictures/123-avatar.png', 'profile-pictures')).resolves.toBe(true)
187+
})
188+
189+
it('denies a profile-picture delete for a non-owner', async () => {
190+
mockGetFileMetadata.mockResolvedValue({ userId: 'other-user' })
191+
await expect(write('profile-pictures/123-avatar.png', 'profile-pictures')).resolves.toBe(false)
192+
})
193+
194+
it('grants a workspace-logo delete to write/admin on the owning workspace', async () => {
195+
mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'ws-1' })
196+
mockGetUserEntityPermissions.mockResolvedValue('admin')
197+
await expect(write('workspace-logos/123-logo.png', 'workspace-logos')).resolves.toBe(true)
198+
expect(mockGetUserEntityPermissions).toHaveBeenCalledWith(USER_ID, 'workspace', 'ws-1')
199+
})
200+
201+
it('denies a workspace-logo delete for a non-member of the owning workspace', async () => {
202+
mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'victim-ws' })
203+
mockGetUserEntityPermissions.mockResolvedValue(null)
204+
await expect(write('workspace-logos/123-logo.png', 'workspace-logos')).resolves.toBe(false)
205+
})
206+
207+
it('denies a workspace-logo delete when no ownership binding exists', async () => {
208+
mockGetFileMetadataByKey.mockResolvedValue(null)
209+
await expect(write('workspace-logos/123-logo.png', 'workspace-logos')).resolves.toBe(false)
210+
expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
211+
})
212+
})

0 commit comments

Comments
 (0)