fix(files): share upsert validation returns 400 not 500; disabling always succeeds

Author: TheodoreSpeaks

apps/sim/app/api/workspaces/[id]/files/[fileId]/share/route.test.ts

@@ -17,10 +17,19 @@ vi.mock('@/lib/uploads/contexts/workspace', () => ({
   getWorkspaceFile: mockGetWorkspaceFile,
 }))
 
-vi.mock('@/lib/public-shares/share-manager', () => ({
-  getShareForResource: mockGetShareForResource,
-  upsertFileShare: mockUpsertFileShare,
-}))
+vi.mock('@/lib/public-shares/share-manager', () => {
+  class ShareValidationError extends Error {
+    constructor(message: string) {
+      super(message)
+      this.name = 'ShareValidationError'
+    }
+  }
+  return {
+    getShareForResource: mockGetShareForResource,
+    upsertFileShare: mockUpsertFileShare,
+    ShareValidationError,
+  }
+})
 
 vi.mock('@/ee/access-control/utils/permission-check', () => {
   class PublicFileSharingNotAllowedError extends Error {
@@ -38,6 +47,7 @@ vi.mock('@sim/audit', () => auditMock)
 const WS = '7727ef3f-8cf6-4686-b063-2bb006a10785'
 const FILE_ID = 'wf_abc'
 
+import { ShareValidationError } from '@/lib/public-shares/share-manager'
 import { GET, PUT } from '@/app/api/workspaces/[id]/files/[fileId]/share/route'
 
 const params = (id = WS, fileId = FILE_ID) => ({ params: Promise.resolve({ id, fileId }) })
@@ -102,6 +112,15 @@ describe('share route', () => {
       expect(mockUpsertFileShare).not.toHaveBeenCalled()
     })
 
+    it('maps a ShareValidationError to 400, not 500', async () => {
+      mockUpsertFileShare.mockRejectedValueOnce(
+        new ShareValidationError('Password is required for password-protected shares')
+      )
+      const res = await PUT(putRequest({ isActive: true, authType: 'password' }), params())
+      expect(res.status).toBe(400)
+      expect((await res.json()).error).toBe('Password is required for password-protected shares')
+    })
+
     it('returns 404 when the file is not in the workspace', async () => {
       mockGetWorkspaceFile.mockResolvedValueOnce(null)
       const res = await PUT(putRequest({ isActive: true }), params())

apps/sim/app/api/workspaces/[id]/files/[fileId]/share/route.ts

@@ -7,7 +7,11 @@ import { parseRequest } from '@/lib/api/server'
 import { getSession } from '@/lib/auth'
 import { generateRequestId } from '@/lib/core/utils/request'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
-import { getShareForResource, upsertFileShare } from '@/lib/public-shares/share-manager'
+import {
+  getShareForResource,
+  ShareValidationError,
+  upsertFileShare,
+} from '@/lib/public-shares/share-manager'
 import { getWorkspaceFile } from '@/lib/uploads/contexts/workspace'
 import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
 import {
@@ -138,6 +142,9 @@ export const PUT = withRouteHandler(
 
       return NextResponse.json({ share })
     } catch (error) {
+      if (error instanceof ShareValidationError) {
+        return NextResponse.json({ error: error.message }, { status: 400 })
+      }
       logger.error(`[${requestId}] Error updating file share:`, error)
       return NextResponse.json(
         { error: getErrorMessage(error, 'Failed to update share') },

apps/sim/lib/public-shares/share-manager.ts

@@ -14,6 +14,14 @@ import { getBaseUrl } from '@/lib/core/utils/urls'
 
 const logger = createLogger('PublicShareManager')
 
+/** Thrown when share auth config is invalid (e.g. enabling a password share with no password). Maps to a 400. */
+export class ShareValidationError extends Error {
+  constructor(message: string) {
+    super(message)
+    this.name = 'ShareValidationError'
+  }
+}
+
 type ShareResourceType = z.infer<typeof shareResourceTypeSchema>
 
 type PublicShareRow = typeof publicShare.$inferSelect
@@ -94,10 +102,11 @@ interface UpsertFileShareInput {
  * a fresh unguessable token; subsequent calls flip `isActive`/`authType` and keep
  * the token stable (so an existing link resolves again after re-enable).
  *
- * Auth handling (fail-fast): `password` requires a plaintext `password` unless the
- * share already has one (encrypted at rest via {@link encryptSecret}); `email`/`sso`
- * require a non-empty `allowedEmails` (provided or already stored); any other type
- * clears both the password and the allow-list.
+ * Auth validation only applies when **enabling** (`isActive: true`): `password`
+ * requires a plaintext `password` unless one is already stored (encrypted via
+ * {@link encryptSecret}); `email`/`sso` require a non-empty `allowedEmails`.
+ * Disabling (going Private) always succeeds and preserves the stored config so a
+ * later re-enable restores it. Validation failures throw {@link ShareValidationError}.
  */
 export async function upsertFileShare({
   workspaceId,
@@ -117,23 +126,35 @@ export async function upsertFileShare({
 
   const finalAuthType: ShareAuthType =
     authType ?? (existing?.authType as ShareAuthType | undefined) ?? 'public'
-
-  let finalPassword: string | null = null
-  let finalAllowedEmails: string[] = []
-  if (finalAuthType === 'password') {
-    if (password) {
-      finalPassword = (await encryptSecret(password)).encrypted
-    } else if (existing?.password) {
-      finalPassword = existing.password
+  const existingAllowedEmails = Array.isArray(existing?.allowedEmails)
+    ? (existing.allowedEmails as string[])
+    : []
+
+  // Disabling preserves the stored config (and skips validation) so turning
+  // sharing off always succeeds; only enabling validates the chosen auth mode.
+  let finalPassword: string | null = existing?.password ?? null
+  let finalAllowedEmails: string[] = existingAllowedEmails
+  if (isActive) {
+    if (finalAuthType === 'password') {
+      if (password) {
+        finalPassword = (await encryptSecret(password)).encrypted
+      } else if (existing?.password) {
+        finalPassword = existing.password
+      } else {
+        throw new ShareValidationError('Password is required for password-protected shares')
+      }
+      finalAllowedEmails = []
+    } else if (finalAuthType === 'email' || finalAuthType === 'sso') {
+      finalAllowedEmails = allowedEmails ?? existingAllowedEmails
+      if (finalAllowedEmails.length === 0) {
+        throw new ShareValidationError(
+          'At least one allowed email is required for email/SSO shares'
+        )
+      }
+      finalPassword = null
     } else {
-      throw new Error('Password is required for password-protected shares')
-    }
-  } else if (finalAuthType === 'email' || finalAuthType === 'sso') {
-    finalAllowedEmails =
-      allowedEmails ??
-      (Array.isArray(existing?.allowedEmails) ? (existing.allowedEmails as string[]) : [])
-    if (finalAllowedEmails.length === 0) {
-      throw new Error('At least one allowed email is required for email/SSO shares')
+      finalPassword = null
+      finalAllowedEmails = []
     }
   }