Skip to content

Commit 1377256

Browse files
j15zicecrasher321claude
authored
fix(chat): show deployment passwords to admins (#6177)
* fix(chat): show deployment passwords to admins * fix(chat): reject whitespace-only passwords * fix(chat): preserve password visibility on regenerate * 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> * fix(chat): correct password-change confirmation gate and stale reveal error Addresses both open Bugbot findings. - shouldConfirmPasswordChange keyed on "a chat exists" rather than "a password exists", so switching a public chat to password protection for the first time asked the admin to confirm changing a password that was never set. It now takes the existing-password signal the component already computes. - A failed reveal left "Failed to load the current password" on screen while the admin typed or generated a replacement, because the mutation only drops its error on the next attempt. Editing or regenerating now resets it. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Vikhyath Mondreti <vikhyath@simstudio.ai> Co-authored-by: Claude <noreply@anthropic.com>
1 parent 797f781 commit 1377256

18 files changed

Lines changed: 874 additions & 36 deletions

File tree

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import {
5+
auditMock,
6+
auditMockFns,
7+
authMockFns,
8+
encryptionMock,
9+
encryptionMockFns,
10+
workflowsApiUtilsMock,
11+
workflowsApiUtilsMockFns,
12+
} from '@sim/testing'
13+
import { NextRequest } from 'next/server'
14+
import { beforeEach, describe, expect, it, vi } from 'vitest'
15+
16+
const { mockCheckChatAccess } = vi.hoisted(() => ({
17+
mockCheckChatAccess: vi.fn(),
18+
}))
19+
20+
const mockCreateErrorResponse = workflowsApiUtilsMockFns.mockCreateErrorResponse
21+
const mockDecryptSecret = encryptionMockFns.mockDecryptSecret
22+
const mockRecordAudit = auditMockFns.mockRecordAudit
23+
24+
vi.mock('@sim/audit', () => auditMock)
25+
vi.mock('@/app/api/workflows/utils', () => workflowsApiUtilsMock)
26+
vi.mock('@/lib/core/security/encryption', () => encryptionMock)
27+
vi.mock('@/app/api/chat/utils', () => ({
28+
checkChatAccess: mockCheckChatAccess,
29+
}))
30+
31+
import { GET } from '@/app/api/chat/manage/[id]/password/route'
32+
33+
const passwordChat = {
34+
id: 'chat-123',
35+
workflowId: 'workflow-123',
36+
identifier: 'test-chat',
37+
title: 'Test Chat',
38+
authType: 'password',
39+
password: 'encrypted-password',
40+
}
41+
42+
function makeRequest() {
43+
return new NextRequest('http://localhost:3000/api/chat/manage/chat-123/password')
44+
}
45+
46+
function callGet() {
47+
return GET(makeRequest(), { params: Promise.resolve({ id: 'chat-123' }) })
48+
}
49+
50+
describe('Chat Password Reveal API Route', () => {
51+
beforeEach(() => {
52+
vi.clearAllMocks()
53+
54+
authMockFns.mockGetSession.mockResolvedValue({
55+
user: { id: 'user-id', name: 'Test User', email: 'user@example.com' },
56+
})
57+
58+
mockCreateErrorResponse.mockImplementation((message, status = 500) => {
59+
return new Response(JSON.stringify({ error: message }), {
60+
status,
61+
headers: { 'Content-Type': 'application/json' },
62+
})
63+
})
64+
65+
mockDecryptSecret.mockResolvedValue({ decrypted: 'super-secret' })
66+
mockCheckChatAccess.mockResolvedValue({
67+
hasAccess: true,
68+
chat: passwordChat,
69+
workspaceId: 'workspace-123',
70+
})
71+
})
72+
73+
it('should return 401 when user is not authenticated', async () => {
74+
authMockFns.mockGetSession.mockResolvedValue(null)
75+
76+
const response = await callGet()
77+
78+
expect(response.status).toBe(401)
79+
const data = await response.json()
80+
expect(data.error).toBe('Unauthorized')
81+
expect(mockDecryptSecret).not.toHaveBeenCalled()
82+
})
83+
84+
it('should return 404 when chat not found or access denied', async () => {
85+
mockCheckChatAccess.mockResolvedValue({ hasAccess: false })
86+
87+
const response = await callGet()
88+
89+
expect(response.status).toBe(404)
90+
const data = await response.json()
91+
expect(data.error).toBe('Chat not found or access denied')
92+
expect(mockCheckChatAccess).toHaveBeenCalledWith('chat-123', 'user-id')
93+
expect(mockDecryptSecret).not.toHaveBeenCalled()
94+
})
95+
96+
it('should return 404 when the chat has no password set', async () => {
97+
mockCheckChatAccess.mockResolvedValue({
98+
hasAccess: true,
99+
chat: { ...passwordChat, authType: 'public', password: null },
100+
workspaceId: 'workspace-123',
101+
})
102+
103+
const response = await callGet()
104+
105+
expect(response.status).toBe(404)
106+
const data = await response.json()
107+
expect(data.error).toBe('This chat does not have a password set')
108+
expect(mockDecryptSecret).not.toHaveBeenCalled()
109+
})
110+
111+
it('should return the decrypted password and record an audit event', async () => {
112+
const response = await callGet()
113+
114+
expect(response.status).toBe(200)
115+
const data = await response.json()
116+
expect(data.password).toBe('super-secret')
117+
expect(mockDecryptSecret).toHaveBeenCalledWith('encrypted-password')
118+
expect(mockRecordAudit).toHaveBeenCalledWith(
119+
expect.objectContaining({
120+
workspaceId: 'workspace-123',
121+
actorId: 'user-id',
122+
action: 'chat.password_viewed',
123+
resourceId: 'chat-123',
124+
})
125+
)
126+
expect(response.headers.get('Cache-Control')).toBe('private, no-store')
127+
})
128+
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+
)
133+
134+
const response = await callGet()
135+
136+
expect(response.status).toBe(500)
137+
const data = await response.json()
138+
expect(data.error).toBe('Failed to reveal chat password')
139+
expect(mockRecordAudit).not.toHaveBeenCalled()
140+
})
141+
})
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
2+
import { createLogger } from '@sim/logger'
3+
import type { NextRequest } from 'next/server'
4+
import { NextResponse } from 'next/server'
5+
import { getChatPasswordContract } from '@/lib/api/contracts/chats'
6+
import { parseRequest } from '@/lib/api/server'
7+
import { getSession } from '@/lib/auth'
8+
import { decryptSecret } from '@/lib/core/security/encryption'
9+
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
10+
import { checkChatAccess } from '@/app/api/chat/utils'
11+
import { createErrorResponse } from '@/app/api/workflows/utils'
12+
13+
export const dynamic = 'force-dynamic'
14+
15+
const logger = createLogger('ChatPasswordAPI')
16+
const PRIVATE_NO_STORE = { 'Cache-Control': 'private, no-store' } as const
17+
18+
/**
19+
* GET endpoint that reveals a chat deployment's current password.
20+
* Restricted to workspace admins (checkChatAccess requires admin permission
21+
* on the workflow's workspace); each reveal is recorded in the audit log.
22+
*/
23+
export const GET = withRouteHandler(
24+
async (request: NextRequest, context: { params: Promise<{ id: string }> }) => {
25+
try {
26+
const session = await getSession()
27+
28+
if (!session) {
29+
return createErrorResponse('Unauthorized', 401)
30+
}
31+
32+
const parsed = await parseRequest(getChatPasswordContract, request, context)
33+
if (!parsed.success) return parsed.response
34+
35+
const { id: chatId } = parsed.data.params
36+
37+
const {
38+
hasAccess,
39+
chat: chatRecord,
40+
workspaceId: chatWorkspaceId,
41+
} = await checkChatAccess(chatId, session.user.id)
42+
43+
if (!hasAccess || !chatRecord) {
44+
return createErrorResponse('Chat not found or access denied', 404)
45+
}
46+
47+
if (chatRecord.authType !== 'password' || !chatRecord.password) {
48+
return createErrorResponse('This chat does not have a password set', 404)
49+
}
50+
51+
const { decrypted } = await decryptSecret(chatRecord.password)
52+
53+
recordAudit({
54+
workspaceId: chatWorkspaceId || null,
55+
actorId: session.user.id,
56+
actorName: session.user.name,
57+
actorEmail: session.user.email,
58+
action: AuditAction.CHAT_PASSWORD_VIEWED,
59+
resourceType: AuditResourceType.CHAT,
60+
resourceId: chatId,
61+
resourceName: chatRecord.title,
62+
description: `Viewed the password for chat deployment "${chatRecord.title}"`,
63+
metadata: {
64+
identifier: chatRecord.identifier,
65+
workflowId: chatRecord.workflowId,
66+
},
67+
request,
68+
})
69+
70+
return NextResponse.json({ password: decrypted }, { headers: PRIVATE_NO_STORE })
71+
} catch (error) {
72+
logger.error('Error revealing chat password:', error)
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)
79+
}
80+
}
81+
)

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -429,6 +429,24 @@ describe('Chat Edit API Route', () => {
429429
expect(data.error).toBe('Password is required when using password protection')
430430
})
431431

432+
it('rejects a whitespace-only replacement password', async () => {
433+
authMockFns.mockGetSession.mockResolvedValue({
434+
user: { id: 'user-id' },
435+
})
436+
437+
const req = new NextRequest('http://localhost:3000/api/chat/manage/chat-123', {
438+
method: 'PATCH',
439+
body: JSON.stringify({ authType: 'password', password: ' ' }),
440+
})
441+
const response = await PATCH(req, { params: Promise.resolve({ id: 'chat-123' }) })
442+
443+
expect(response.status).toBe(400)
444+
const data = await response.json()
445+
expect(data.error).toBe('Password cannot contain only whitespace')
446+
expect(mockCheckChatAccess).not.toHaveBeenCalled()
447+
expect(mockEncryptSecret).not.toHaveBeenCalled()
448+
})
449+
432450
it('should keep the existing password when updating a password-protected chat', async () => {
433451
authMockFns.mockGetSession.mockResolvedValue({
434452
user: { id: 'user-id' },

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

0 commit comments

Comments
 (0)