Skip to content

Commit e50777f

Browse files
icecrasher321claude
andcommitted
fix(chat): harden password reveal and close deployment lockout paths
Follow-ups from a security review of the password reveal endpoint. The permission model itself was correct — the reveal is gated on workspace admin via the canonical resolver, so derived org-admin access is honored. These address secret handling and validation around it. - Cap set-path passwords at the same 1024 chars the chat login accepts. Neither the input nor the schema bounded length, so a longer password saved fine and then failed the login POST on length before auth ran, locking every visitor out permanently. - Discard the revealed password when the field is hidden. It previously stayed in state and in the input's DOM value with Copy still armed, so the field read as hidden while still handing out the plaintext. - Evict the decrypted password from the mutation cache on unmount, and correct the TSDoc claiming it was never retained — it sat in the MutationCache for the default five minutes after the modal closed. - Validate the password inside performChatDeploy, the writer both callers must use. The copilot deploy_chat tool bypasses the route contract and could still store a whitespace-only or over-long password, or create a password-protected chat with no password at all. - Stop echoing raw decryption errors from the reveal endpoint. - Only persist a new password when the chat ends up password-protected; PATCH { authType: 'email', password } used to re-arm the secret that the auth-type branch had just cleared. Also replaces the hand-rolled copy state with useCopyToClipboard, which fixes an unawaited clipboard write that surfaced as an unhandled rejection and a "Copied" confirmation shown even when the write failed. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent aaccfcf commit e50777f

10 files changed

Lines changed: 272 additions & 31 deletions

File tree

apps/sim/app/api/chat/manage/[id]/password/route.test.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -126,14 +126,16 @@ describe('Chat Password Reveal API Route', () => {
126126
expect(response.headers.get('Cache-Control')).toBe('private, no-store')
127127
})
128128

129-
it('should return 500 when decryption fails', async () => {
130-
mockDecryptSecret.mockRejectedValue(new Error('Decryption failed'))
129+
it('should return 500 without echoing the decryption error', async () => {
130+
mockDecryptSecret.mockRejectedValue(
131+
new Error('Invalid encrypted value format. Expected "iv:encrypted:authTag"')
132+
)
131133

132134
const response = await callGet()
133135

134136
expect(response.status).toBe(500)
135137
const data = await response.json()
136-
expect(data.error).toBe('Decryption failed')
138+
expect(data.error).toBe('Failed to reveal chat password')
137139
expect(mockRecordAudit).not.toHaveBeenCalled()
138140
})
139141
})

apps/sim/app/api/chat/manage/[id]/password/route.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
22
import { createLogger } from '@sim/logger'
3-
import { getErrorMessage } from '@sim/utils/errors'
43
import type { NextRequest } from 'next/server'
54
import { NextResponse } from 'next/server'
65
import { getChatPasswordContract } from '@/lib/api/contracts/chats'
@@ -71,7 +70,12 @@ export const GET = withRouteHandler(
7170
return NextResponse.json({ password: decrypted }, { headers: PRIVATE_NO_STORE })
7271
} catch (error) {
7372
logger.error('Error revealing chat password:', error)
74-
return createErrorResponse(getErrorMessage(error, 'Failed to reveal chat password'), 500)
73+
/**
74+
* Deliberately opaque: the only errors that reach here come from
75+
* decryption, whose messages describe the stored ciphertext's shape.
76+
* The logged error carries the detail for operators.
77+
*/
78+
return createErrorResponse('Failed to reveal chat password', 500)
7579
}
7680
}
7781
)

apps/sim/app/api/chat/manage/[id]/route.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,13 @@ export const PATCH = withRouteHandler(
241241
}
242242
}
243243

244-
if (encryptedPassword) {
244+
/**
245+
* Only store a new password when the chat ends up password-protected.
246+
* Applying it unconditionally re-armed the secret that the branch above
247+
* just cleared, so `PATCH { authType: 'email', password }` persisted an
248+
* encrypted password on an email-gated chat.
249+
*/
250+
if (encryptedPassword && (authType ?? existingChat[0].authType) === 'password') {
245251
updateData.password = encryptedPassword
246252
}
247253

apps/sim/components/ui/generated-password-input.test.tsx

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ import { createRoot, type Root } from 'react-dom/client'
66
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
77
import { GeneratedPasswordInput } from '@/components/ui/generated-password-input'
88

9+
const { mockCopy } = vi.hoisted(() => ({
10+
mockCopy: vi.fn(async () => true),
11+
}))
12+
913
vi.mock('@sim/emcn', () => ({
1014
Button: ({
1115
children,
@@ -34,6 +38,7 @@ vi.mock('@sim/emcn', () => ({
3438
Trigger: ({ children }: { children?: ReactNode }) => children,
3539
Content: () => null,
3640
},
41+
useCopyToClipboard: () => ({ copied: false, copy: mockCopy }),
3742
}))
3843

3944
let container: HTMLDivElement
@@ -43,17 +48,19 @@ interface RenderInputOptions {
4348
fetchCurrentPassword?: () => Promise<string>
4449
onChange?: (value: string) => void
4550
showGenerate?: boolean
51+
value?: string
4652
}
4753

4854
function renderInput({
4955
fetchCurrentPassword,
5056
onChange = vi.fn(),
5157
showGenerate = false,
58+
value = '',
5259
}: RenderInputOptions = {}) {
5360
act(() => {
5461
root.render(
5562
<GeneratedPasswordInput
56-
value=''
63+
value={value}
5764
onChange={onChange}
5865
showGenerate={showGenerate}
5966
fetchCurrentPassword={fetchCurrentPassword}
@@ -99,7 +106,7 @@ describe('GeneratedPasswordInput', () => {
99106
expect(passwordInput()).toHaveAttribute('placeholder', '••••••••')
100107
})
101108

102-
it('fetches the saved password on reveal and reuses it when toggled', async () => {
109+
it('fetches the saved password on reveal', async () => {
103110
const fetchCurrentPassword = vi.fn().mockResolvedValue('saved-secret')
104111

105112
renderInput({ fetchCurrentPassword })
@@ -108,12 +115,37 @@ describe('GeneratedPasswordInput', () => {
108115
expect(fetchCurrentPassword).toHaveBeenCalledOnce()
109116
expect(passwordInput()).toHaveAttribute('type', 'text')
110117
expect(passwordInput()).toHaveValue('saved-secret')
118+
})
119+
120+
it('discards the saved password when hidden and re-fetches on the next reveal', async () => {
121+
const fetchCurrentPassword = vi.fn().mockResolvedValue('saved-secret')
122+
123+
renderInput({ fetchCurrentPassword })
124+
await act(async () => passwordButton('Show password').click())
111125

112126
act(() => passwordButton('Hide password').click())
127+
113128
expect(passwordInput()).toHaveAttribute('type', 'password')
129+
expect(passwordInput()).toHaveValue('')
130+
expect(passwordInput()).toHaveAttribute('placeholder', '••••••••')
131+
expect(passwordButton('Copy password')).toBeDisabled()
114132

115133
await act(async () => passwordButton('Show password').click())
116-
expect(fetchCurrentPassword).toHaveBeenCalledOnce()
134+
135+
expect(fetchCurrentPassword).toHaveBeenCalledTimes(2)
136+
expect(passwordInput()).toHaveValue('saved-secret')
137+
})
138+
139+
it('keeps an edited value when hidden', async () => {
140+
const fetchCurrentPassword = vi.fn().mockResolvedValue('saved-secret')
141+
const onChange = vi.fn()
142+
143+
renderInput({ fetchCurrentPassword, onChange, value: 'typed-secret' })
144+
await act(async () => passwordButton('Show password').click())
145+
act(() => passwordButton('Hide password').click())
146+
147+
expect(fetchCurrentPassword).not.toHaveBeenCalled()
148+
expect(passwordInput()).toHaveValue('typed-secret')
117149
})
118150

119151
it('stays masked when loading the saved password fails', async () => {

apps/sim/components/ui/generated-password-input.tsx

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
'use client'
22

3-
import { useEffect, useState } from 'react'
4-
import { Button, ChipInput, Loader, Tooltip } from '@sim/emcn'
3+
import { useState } from 'react'
4+
import { Button, ChipInput, Loader, Tooltip, useCopyToClipboard } from '@sim/emcn'
55
import { Check, Clipboard, Eye, EyeOff, RefreshCw } from 'lucide-react'
66
import { generatePassword } from '@/lib/core/security/encryption'
77

@@ -41,24 +41,13 @@ export function GeneratedPasswordInput({
4141
fetchCurrentPassword,
4242
}: GeneratedPasswordInputProps) {
4343
const [showPassword, setShowPassword] = useState(false)
44-
const [copySuccess, setCopySuccess] = useState(false)
4544
const [currentPassword, setCurrentPassword] = useState<string | null>(null)
4645
const [isFetchingCurrent, setIsFetchingCurrent] = useState(false)
47-
48-
useEffect(() => {
49-
if (!copySuccess) return
50-
const timer = setTimeout(() => setCopySuccess(false), 2000)
51-
return () => clearTimeout(timer)
52-
}, [copySuccess])
46+
const { copied, copy } = useCopyToClipboard()
5347

5448
const displayValue = currentPassword ?? value
5549
const displayPlaceholder = fetchCurrentPassword && !displayValue ? MASKED_PASSWORD : placeholder
5650

57-
const copyToClipboard = () => {
58-
navigator.clipboard.writeText(displayValue)
59-
setCopySuccess(true)
60-
}
61-
6251
const handleChange = (nextValue: string) => {
6352
setCurrentPassword(null)
6453
onChange(nextValue)
@@ -71,6 +60,14 @@ export function GeneratedPasswordInput({
7160
const toggleShowPassword = async () => {
7261
if (showPassword) {
7362
setShowPassword(false)
63+
/**
64+
* Discard the fetched password instead of masking it. Keeping it would
65+
* leave the plaintext in the input's DOM value and keep Copy armed while
66+
* the field reads as hidden. A later reveal re-fetches, which also keeps
67+
* the audit log at one entry per disclosure. An edited value lives in
68+
* `value` and is deliberately untouched.
69+
*/
70+
setCurrentPassword(null)
7471
return
7572
}
7673

@@ -124,16 +121,16 @@ export function GeneratedPasswordInput({
124121
<Button
125122
type='button'
126123
variant='ghost'
127-
onClick={copyToClipboard}
124+
onClick={() => copy(displayValue)}
128125
disabled={!displayValue || disabled}
129126
aria-label='Copy password'
130127
className='!p-1.5'
131128
>
132-
{copySuccess ? <Check className='size-3' /> : <Clipboard className='size-3' />}
129+
{copied ? <Check className='size-3' /> : <Clipboard className='size-3' />}
133130
</Button>
134131
</Tooltip.Trigger>
135132
<Tooltip.Content>
136-
<span>{copySuccess ? 'Copied' : 'Copy'}</span>
133+
<span>{copied ? 'Copied' : 'Copy'}</span>
137134
</Tooltip.Content>
138135
</Tooltip.Root>
139136
<Tooltip.Root>

apps/sim/hooks/queries/chats.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -385,12 +385,18 @@ interface RevealChatPasswordVariables {
385385
}
386386

387387
/**
388-
* Mutation hook that fetches a chat deployment's current password for
389-
* workspace admins. Modeled as a mutation (despite the GET) so the decrypted
390-
* password is never retained in the query cache.
388+
* Mutation hook that fetches a chat deployment's current password for workspace
389+
* admins. Modeled as a mutation (despite the GET) because revealing a secret is
390+
* an audited, explicitly-triggered action, not cacheable read state.
391+
*
392+
* `gcTime: 0` evicts the decrypted password from the mutation cache as soon as
393+
* the last observer unmounts, rather than letting it sit there for the default
394+
* five minutes after the deploy modal closes. While the modal is open the caller
395+
* holds the plaintext anyway, and it discards it when the field is hidden.
391396
*/
392397
export function useRevealChatPassword() {
393398
return useMutation({
399+
gcTime: 0,
394400
mutationFn: async ({ chatId }: RevealChatPasswordVariables): Promise<string> => {
395401
const result = await requestJson(getChatPasswordContract, { params: { id: chatId } })
396402
return result.password
Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import {
6+
chatDeploymentPasswordSchema,
7+
createChatBodySchema,
8+
deployedChatAuthBodySchema,
9+
deployedChatPostBodySchema,
10+
updateChatBodySchema,
11+
} from '@/lib/api/contracts/chats'
12+
13+
const createBody = {
14+
workflowId: 'wf-1',
15+
identifier: 'my-chat',
16+
title: 'Support',
17+
customizations: { primaryColor: 'var(--brand-hover)', welcomeMessage: 'Hi' },
18+
}
19+
20+
describe('chat deployment password contract', () => {
21+
it('accepts the empty string, which means "keep the stored password"', () => {
22+
expect(chatDeploymentPasswordSchema.safeParse('').success).toBe(true)
23+
})
24+
25+
it('rejects a whitespace-only password', () => {
26+
const result = chatDeploymentPasswordSchema.safeParse(' ')
27+
expect(result.success).toBe(false)
28+
expect(result.error?.issues[0].message).toBe('Password cannot contain only whitespace')
29+
})
30+
31+
it('preserves surrounding whitespace, which login compares byte-exact', () => {
32+
expect(chatDeploymentPasswordSchema.parse(' hunter2 ')).toBe(' hunter2 ')
33+
})
34+
35+
/**
36+
* The security-relevant invariant: a password long enough to save must still
37+
* be short enough to submit. If the set path outgrew the login path, the
38+
* deployment would be permanently unreachable — the login POST would 400 on
39+
* length before authentication ever ran.
40+
*/
41+
it('caps length at the same boundary the deployed-chat login enforces', () => {
42+
const atLimit = 'a'.repeat(1024)
43+
const overLimit = 'a'.repeat(1025)
44+
45+
expect(chatDeploymentPasswordSchema.safeParse(atLimit).success).toBe(true)
46+
expect(chatDeploymentPasswordSchema.safeParse(overLimit).success).toBe(false)
47+
48+
expect(deployedChatAuthBodySchema.safeParse({ password: atLimit }).success).toBe(true)
49+
expect(deployedChatPostBodySchema.safeParse({ password: atLimit }).success).toBe(true)
50+
})
51+
52+
it('applies to both the create and update bodies', () => {
53+
const tooLong = 'a'.repeat(1025)
54+
55+
expect(createChatBodySchema.safeParse({ ...createBody, password: ' ' }).success).toBe(false)
56+
expect(createChatBodySchema.safeParse({ ...createBody, password: tooLong }).success).toBe(false)
57+
expect(createChatBodySchema.safeParse({ ...createBody, password: 'ok' }).success).toBe(true)
58+
59+
expect(updateChatBodySchema.safeParse({ password: ' ' }).success).toBe(false)
60+
expect(updateChatBodySchema.safeParse({ password: tooLong }).success).toBe(false)
61+
expect(updateChatBodySchema.safeParse({ password: '' }).success).toBe(true)
62+
})
63+
})

apps/sim/lib/api/contracts/chats.ts

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,22 @@ import { defineRouteContract } from '@/lib/api/contracts/types'
44
export const chatAuthTypeSchema = z.enum(['public', 'password', 'email', 'sso'])
55
export type ChatAuthType = z.output<typeof chatAuthTypeSchema>
66

7+
/**
8+
* Shared cap for chat deployment passwords. The set path and the deployed-chat
9+
* login path must agree: a password long enough to save but too long to submit
10+
* would lock every visitor out of the deployment permanently.
11+
*/
12+
const MAX_CHAT_PASSWORD_CHARS = 1024
13+
14+
/**
15+
* Password accepted when setting or changing a chat deployment's password. The
16+
* empty string is allowed and means "keep the stored password"; a whitespace-only
17+
* value is rejected because the login form refuses to submit one, which would
18+
* strand the deployment behind an unenterable password.
19+
*/
720
export const chatDeploymentPasswordSchema = z
821
.string()
22+
.max(MAX_CHAT_PASSWORD_CHARS, 'Password is too long')
923
.refine(
1024
(password) => password.length === 0 || password.trim().length > 0,
1125
'Password cannot contain only whitespace'
@@ -127,7 +141,7 @@ export const deployedChatConfigSchema = z.object({
127141
export type DeployedChatConfig = z.output<typeof deployedChatConfigSchema>
128142

129143
export const deployedChatAuthBodySchema = z.object({
130-
password: z.string().max(1024, 'Password is too long').optional(),
144+
password: z.string().max(MAX_CHAT_PASSWORD_CHARS, 'Password is too long').optional(),
131145
email: z.string().email('Invalid email format').optional().or(z.literal('')),
132146
})
133147
export type DeployedChatAuthBody = z.input<typeof deployedChatAuthBodySchema>
@@ -149,7 +163,7 @@ export const deployedChatFileSchema = z.object({
149163

150164
export const deployedChatPostBodySchema = z.object({
151165
input: z.string().max(MAX_CHAT_INPUT_CHARS, 'Input is too long').optional(),
152-
password: z.string().max(1024, 'Password is too long').optional(),
166+
password: z.string().max(MAX_CHAT_PASSWORD_CHARS, 'Password is too long').optional(),
153167
email: z.string().email('Invalid email format').optional().or(z.literal('')),
154168
conversationId: z.string().max(256, 'Conversation ID is too long').optional(),
155169
files: z

0 commit comments

Comments
 (0)