diff --git a/apps/sim/app/api/auth/sso/providers/route.ts b/apps/sim/app/api/auth/sso/providers/route.ts index 8428eebc1e1..2f473de4831 100644 --- a/apps/sim/app/api/auth/sso/providers/route.ts +++ b/apps/sim/app/api/auth/sso/providers/route.ts @@ -11,6 +11,21 @@ import { withRouteHandler } from '@/lib/core/utils/with-route-handler' const logger = createLogger('SSOProvidersRoute') +/** Secrets shorter than this reveal too large a fraction of themselves in 4 characters. */ +const MIN_LENGTH_FOR_HINT = 16 + +/** + * Last four characters of a stored client secret, so an admin can tell *which* + * secret is saved rather than only that one exists. Four characters of a + * high-entropy secret is not a meaningful disclosure to an owner or admin, who + * can rotate it anyway — but short secrets are left unhinted, where the same four + * characters would be a large share of the value. + */ +function buildClientSecretHint(clientSecret: unknown): string | null { + if (typeof clientSecret !== 'string' || clientSecret.length < MIN_LENGTH_FOR_HINT) return null + return clientSecret.slice(-4) +} + export const GET = withRouteHandler(async (request: NextRequest) => { try { const session = await getSession() @@ -69,7 +84,9 @@ export const GET = withRouteHandler(async (request: NextRequest) => { if (oidcConfig) { try { const parsed = JSON.parse(oidcConfig) + const hint = buildClientSecretHint(parsed.clientSecret) parsed.clientSecret = REDACTED_MARKER + if (hint) parsed.clientSecretHint = hint oidcConfig = JSON.stringify(parsed) } catch { oidcConfig = null diff --git a/apps/sim/app/api/memory/[id]/route.test.ts b/apps/sim/app/api/memory/[id]/route.test.ts new file mode 100644 index 00000000000..a5179c9a6e4 --- /dev/null +++ b/apps/sim/app/api/memory/[id]/route.test.ts @@ -0,0 +1,91 @@ +/** + * @vitest-environment node + */ + +import { memory } from '@sim/db/schema' +import { + createMockRequest, + hybridAuthMockFns, + queueTableRows, + resetDbChainMock, +} from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { AuthType } from '@/lib/auth/hybrid' +import { + PRIVATE_TOOL_METADATA_REQUEST_HEADER, + PRIVATE_TOOL_METADATA_RESPONSE_HEADER, + RESOLVED_SECRET_PROVENANCE_FIELD, + RESOLVED_SECRET_PROVENANCE_METADATA_V1, +} from '@/lib/execution/private-tool-metadata' + +const { mockCheckWorkspaceAccess } = vi.hoisted(() => ({ + mockCheckWorkspaceAccess: vi.fn(), +})) + +vi.mock('@/lib/workspaces/permissions/utils', () => ({ + checkWorkspaceAccess: mockCheckWorkspaceAccess, +})) + +import { GET } from '@/app/api/memory/[id]/route' + +const WORKSPACE_ID = '11111111-1111-4111-8111-111111111111' +const CONTEXT = { params: Promise.resolve({ id: 'missing-conversation' }) } + +describe('GET /api/memory/[id]', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + hybridAuthMockFns.mockCheckInternalAuth.mockResolvedValue({ + success: true, + userId: 'user-1', + authType: AuthType.INTERNAL_JWT, + }) + mockCheckWorkspaceAccess.mockResolvedValue({ exists: true, hasAccess: true }) + queueTableRows(memory, []) + }) + + it('returns verified exact-empty metadata when a tool lookup has no matching memory', async () => { + const response = await GET( + createMockRequest( + 'GET', + undefined, + { + [PRIVATE_TOOL_METADATA_REQUEST_HEADER]: RESOLVED_SECRET_PROVENANCE_METADATA_V1, + }, + `http://localhost:3000/api/memory/missing-conversation?workspaceId=${WORKSPACE_ID}` + ), + CONTEXT + ) + + expect(response.status).toBe(200) + expect(response.headers.get(PRIVATE_TOOL_METADATA_RESPONSE_HEADER)).toBe( + RESOLVED_SECRET_PROVENANCE_METADATA_V1 + ) + expect(await response.json()).toEqual({ + success: true, + data: null, + [RESOLVED_SECRET_PROVENANCE_FIELD]: { + version: 1, + complete: true, + entries: [], + scope: { userId: 'user-1', workspaceId: WORKSPACE_ID }, + }, + }) + }) + + it('preserves the existing headerless empty response for ordinary API callers', async () => { + const response = await GET( + createMockRequest( + 'GET', + undefined, + {}, + `http://localhost:3000/api/memory/missing-conversation?workspaceId=${WORKSPACE_ID}` + ), + CONTEXT + ) + + expect(response.status).toBe(200) + expect(response.headers.get(PRIVATE_TOOL_METADATA_RESPONSE_HEADER)).toBeNull() + expect(await response.json()).toEqual({ success: true, data: null }) + }) +}) diff --git a/apps/sim/app/api/memory/[id]/route.ts b/apps/sim/app/api/memory/[id]/route.ts index abf574419b7..6178061ec94 100644 --- a/apps/sim/app/api/memory/[id]/route.ts +++ b/apps/sim/app/api/memory/[id]/route.ts @@ -91,7 +91,14 @@ export const GET = withRouteHandler(async (request: NextRequest, context: Memory .limit(1) if (memories.length === 0) { - return NextResponse.json({ success: true, data: null }, { status: 200 }) + return createMemoryResponse({ + request, + authType: accessCheck.authType, + userId: accessCheck.userId, + workspaceId: validatedWorkspaceId, + body: { success: true, data: null }, + memories: [], + }) } const mem = memories[0] diff --git a/apps/sim/app/api/table/row-secret-provenance.test.ts b/apps/sim/app/api/table/row-secret-provenance.test.ts new file mode 100644 index 00000000000..cdbbd29cdda --- /dev/null +++ b/apps/sim/app/api/table/row-secret-provenance.test.ts @@ -0,0 +1,223 @@ +/** + * @vitest-environment node + */ +import { createMockRequest } from '@sim/testing' +import { describe, expect, it } from 'vitest' +import { AuthType } from '@/lib/auth/hybrid' +import { + PRIVATE_SECRET_PROVENANCE_BUNDLE_V1, + PRIVATE_SECRET_PROVENANCE_FIELD, + PRIVATE_SECRET_PROVENANCE_HEADER, +} from '@/lib/execution/private-tool-metadata' +import { rowDataNameToId } from '@/lib/table/column-keys' +import { tableRowSecretProvenanceSelectionKey } from '@/lib/table/secret-provenance-selection' +import type { RowData } from '@/lib/table/types' +import { + createTableWriteProvenanceTargets, + resolveTableWriteSecretProvenance, +} from '@/app/api/table/row-secret-provenance' + +const USER_ID = 'user-1' +const WORKSPACE_ID = 'ws-1' + +/** Mirrors the internal-JWT wire translator: names → ids, unknown names dropped. */ +const ID_BY_NAME = new Map([ + ['email', 'col_email'], + ['company', 'col_company'], +]) + +const translateNames = (data: RowData): RowData => rowDataNameToId(data, ID_BY_NAME) +const translateIdentity = (data: RowData): RowData => data + +function traceProvenance() { + return { + version: 1, + complete: true, + entries: [], + scope: { userId: USER_ID, workspaceId: WORKSPACE_ID }, + } +} + +function bundleRequest(selectionKeys: string[]) { + const payload = { + [PRIVATE_SECRET_PROVENANCE_FIELD]: { + version: 1, + complete: true, + selections: selectionKeys.map((key) => ({ key, provenance: traceProvenance() })), + }, + } + const request = createMockRequest('POST', payload, { + [PRIVATE_SECRET_PROVENANCE_HEADER]: PRIVATE_SECRET_PROVENANCE_BUNDLE_V1, + }) + return { request, payload } +} + +describe('createTableWriteProvenanceTargets', () => { + it('maps column names to their storage ids', () => { + const targets = createTableWriteProvenanceTargets([{ email: 'a@b.c' }], translateNames) + + expect(targets).toEqual([ + { + selectionKey: tableRowSecretProvenanceSelectionKey(0, 'email'), + rowKey: '0', + columnId: 'col_email', + }, + ]) + }) + + it('returns a null column id for a column the wire translator drops', () => { + const targets = createTableWriteProvenanceTargets( + [{ email: 'a@b.c', notAColumn: 'x' }], + translateNames + ) + + expect(targets).toHaveLength(2) + expect(targets[0].columnId).toBe('col_email') + expect(targets[1]).toEqual({ + selectionKey: tableRowSecretProvenanceSelectionKey(0, 'notAColumn'), + rowKey: '0', + columnId: null, + }) + }) + + it('keeps one target per submitted column so bundle selections stay paired', () => { + const targets = createTableWriteProvenanceTargets( + [{ notAColumn: 'x', alsoNotAColumn: 'y' }], + translateNames + ) + + expect(targets.map((target) => target.columnId)).toEqual([null, null]) + }) + + it('passes column ids through for identity (session) translation', () => { + const targets = createTableWriteProvenanceTargets([{ col_email: 'a@b.c' }], translateIdentity) + + expect(targets[0].columnId).toBe('col_email') + }) + + it('keys targets by row index across multiple rows', () => { + const targets = createTableWriteProvenanceTargets( + [{ email: 'a@b.c' }, { company: 'Acme' }], + translateNames + ) + + expect(targets.map((target) => target.rowKey)).toEqual(['0', '1']) + expect(targets[1].selectionKey).toBe(tableRowSecretProvenanceSelectionKey(1, 'company')) + }) +}) + +describe('resolveTableWriteSecretProvenance', () => { + it('records no provenance for a dropped column on an unsupported session write', () => { + const rows = [{ email: 'a@b.c', notAColumn: 'x' }] + const result = resolveTableWriteSecretProvenance({ + request: createMockRequest('POST', { rows }), + payload: { rows }, + authType: AuthType.SESSION, + userId: USER_ID, + workspaceId: WORKSPACE_ID, + targets: createTableWriteProvenanceTargets(rows, translateNames), + rowKeys: ['0'], + }) + + expect(result.success).toBe(true) + if (!result.success) return + expect(Object.keys(result.provenanceByRowKey?.['0'].columns ?? {})).toEqual(['col_email']) + }) + + it('accepts a complete bundle that covers a dropped column', () => { + const rows = [{ email: 'a@b.c', notAColumn: 'x' }] + const { request, payload } = bundleRequest([ + tableRowSecretProvenanceSelectionKey(0, 'email'), + tableRowSecretProvenanceSelectionKey(0, 'notAColumn'), + ]) + + const result = resolveTableWriteSecretProvenance({ + request, + payload, + authType: AuthType.INTERNAL_JWT, + userId: USER_ID, + workspaceId: WORKSPACE_ID, + targets: createTableWriteProvenanceTargets(rows, translateNames), + rowKeys: ['0'], + }) + + expect(result.success).toBe(true) + if (!result.success) return + expect(Object.keys(result.provenanceByRowKey?.['0'].columns ?? {})).toEqual(['col_email']) + }) + + it('stores provenance for a fully translatable bundle', () => { + const rows = [{ email: 'a@b.c', company: 'Acme' }] + const { request, payload } = bundleRequest([ + tableRowSecretProvenanceSelectionKey(0, 'email'), + tableRowSecretProvenanceSelectionKey(0, 'company'), + ]) + + const result = resolveTableWriteSecretProvenance({ + request, + payload, + authType: AuthType.INTERNAL_JWT, + userId: USER_ID, + workspaceId: WORKSPACE_ID, + targets: createTableWriteProvenanceTargets(rows, translateNames), + rowKeys: ['0'], + }) + + expect(result.success).toBe(true) + if (!result.success) return + expect(Object.keys(result.provenanceByRowKey?.['0'].columns ?? {}).sort()).toEqual([ + 'col_company', + 'col_email', + ]) + }) + + it('rejects a bundle whose selection matches no submitted column', () => { + const rows = [{ email: 'a@b.c' }] + const { request, payload } = bundleRequest([tableRowSecretProvenanceSelectionKey(0, 'company')]) + + const result = resolveTableWriteSecretProvenance({ + request, + payload, + authType: AuthType.INTERNAL_JWT, + userId: USER_ID, + workspaceId: WORKSPACE_ID, + targets: createTableWriteProvenanceTargets(rows, translateNames), + rowKeys: ['0'], + }) + + expect(result.success).toBe(false) + }) + + it('rejects a bundle whose selection scope does not match the caller', () => { + const rows = [{ email: 'a@b.c' }] + const payload = { + [PRIVATE_SECRET_PROVENANCE_FIELD]: { + version: 1, + complete: true, + selections: [ + { + key: tableRowSecretProvenanceSelectionKey(0, 'email'), + provenance: { + ...traceProvenance(), + scope: { userId: 'someone-else', workspaceId: WORKSPACE_ID }, + }, + }, + ], + }, + } + + const result = resolveTableWriteSecretProvenance({ + request: createMockRequest('POST', payload, { + [PRIVATE_SECRET_PROVENANCE_HEADER]: PRIVATE_SECRET_PROVENANCE_BUNDLE_V1, + }), + payload, + authType: AuthType.INTERNAL_JWT, + userId: USER_ID, + workspaceId: WORKSPACE_ID, + targets: createTableWriteProvenanceTargets(rows, translateNames), + rowKeys: ['0'], + }) + + expect(result.success).toBe(false) + }) +}) diff --git a/apps/sim/app/api/table/row-secret-provenance.ts b/apps/sim/app/api/table/row-secret-provenance.ts index 50b04854596..1b3a843d975 100644 --- a/apps/sim/app/api/table/row-secret-provenance.ts +++ b/apps/sim/app/api/table/row-secret-provenance.ts @@ -29,10 +29,19 @@ type TableWriteProvenanceResult = interface TableWriteProvenanceTarget { selectionKey: string rowKey: string - columnId: string + /** Storage column id, or `null` when the wire translator drops this column. */ + columnId: string | null } -/** Maps tool-facing column names to the stable storage ids used by the sidecar. */ +/** + * Maps tool-facing column names to the stable storage ids used by the sidecar. + * + * The wire translator drops keys that name no column in the table schema, and the + * write path drops them identically, so such a column is simply never persisted. + * It still gets a target — callers key one provenance selection per column they + * sent, and the completeness check pairs the two — but with a `null` column id so + * no provenance is recorded for a value that was never stored. + */ export function createTableWriteProvenanceTargets( rows: readonly RowData[], translate: (data: RowData) => RowData @@ -40,13 +49,10 @@ export function createTableWriteProvenanceTargets( return rows.flatMap((row, rowIndex) => Object.entries(row).map(([columnKey, value]) => { const translatedKeys = Object.keys(translate({ [columnKey]: value })) - if (translatedKeys.length !== 1) { - throw new Error('Table row secret provenance column translation is invalid') - } return { selectionKey: tableRowSecretProvenanceSelectionKey(rowIndex, columnKey), rowKey: String(rowIndex), - columnId: translatedKeys[0], + columnId: translatedKeys.length === 1 ? translatedKeys[0] : null, } }) ) @@ -81,6 +87,7 @@ export function resolveTableWriteSecretProvenance(options: { provenanceByRowKey[rowKey] = { complete: true, columns: {} } } for (const target of options.targets) { + if (target.columnId === null) continue const row = provenanceByRowKey[target.rowKey] ?? { complete: true, columns: {} } row.columns[target.columnId] = { version: 1, @@ -132,11 +139,14 @@ export function resolveTableWriteSecretProvenance(options: { if ( !target || selection.provenance.scope?.userId !== options.userId || - selection.provenance.scope?.workspaceId !== options.workspaceId || - Object.hasOwn(provenanceByRowKey[target.rowKey].columns, target.columnId) + selection.provenance.scope?.workspaceId !== options.workspaceId ) { return { success: false, response: invalidProvenanceResponse() } } + if (target.columnId === null) continue + if (Object.hasOwn(provenanceByRowKey[target.rowKey].columns, target.columnId)) { + return { success: false, response: invalidProvenanceResponse() } + } provenanceByRowKey[target.rowKey].columns[target.columnId] = selection.provenance } return { success: true, provenanceByRowKey } diff --git a/apps/sim/ee/sso/components/sso-settings.test.tsx b/apps/sim/ee/sso/components/sso-settings.test.tsx index e2c9ed920f9..010d15af70d 100644 --- a/apps/sim/ee/sso/components/sso-settings.test.tsx +++ b/apps/sim/ee/sso/components/sso-settings.test.tsx @@ -20,15 +20,24 @@ vi.mock('@sim/emcn', () => ({ {children} ), + Chip: ({ children, ...props }: { children?: ReactNode }) => ( + + ), ChipCombobox: () =>
, ChipCopyInput: ({ value }: { value?: string }) => , ChipInput: ({ value, onChange, + id, + placeholder, }: { value?: string onChange?: ChangeEventHandler - }) => , + id?: string + placeholder?: string + }) => , ChipSelect: () =>
, ChipTextarea: ({ value, @@ -58,8 +67,11 @@ vi.mock('@/ee/sso/components/verified-domains-section', () => ({ VerifiedDomainsSection: () =>
, })) +// Surface the real Save/Update action so submit paths are reachable from tests. vi.mock('@/components/settings/save-discard-actions', () => ({ - saveDiscardActions: () => [], + saveDiscardActions: ({ saveLabel, onSave }: { saveLabel?: string; onSave?: () => void }) => [ + { text: saveLabel ?? 'Save', onSelect: onSave }, + ], })) vi.mock('@/app/workspace/[workspaceId]/settings/components/settings-empty-state', () => ({ @@ -115,13 +127,26 @@ function provider(organizationId: string) { organizationId, providerType: 'oidc', oidcConfig: JSON.stringify({ + // What the API actually returns: the sentinel plus a display-only hint, + // never the secret itself. clientId: `client-${suffix}`, - clientSecret: `secret-${suffix}`, + clientSecret: '[REDACTED]', + clientSecretHint: '4f2a', scopes: ['openid'], }), } } +function findButton(text: string) { + return Array.from(container.querySelectorAll('button')).find( + (button) => button.textContent === text + ) +} + +function startEditing() { + act(() => findButton('Edit')?.click()) +} + let container: HTMLDivElement let root: Root @@ -137,46 +162,43 @@ beforeAll(() => { afterAll(resetEnvFlagsMock) -describe('SSO organization transitions', () => { - beforeEach(() => { - // The component reads getBaseUrl() during render; make sure the env var is - // present even when the suite runs without a local .env or after another - // test file mutated the environment (auto-restored via unstubEnvs). - vi.stubEnv('NEXT_PUBLIC_APP_URL', 'http://localhost:3000') - ;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true - container = document.createElement('div') - document.body.appendChild(container) - root = createRoot(container) - mockUseSession.mockReturnValue({ data: { user: { id: 'user-1' } } }) - mockUseOrganizationBilling.mockReturnValue({ - data: { data: { subscriptionPlan: 'enterprise' } }, - isLoading: false, - }) - mockUseConfigureSSO.mockReturnValue({ - isPending: false, - mutateAsync: vi.fn(), - }) - mockUseSSOProviders.mockImplementation(({ organizationId }: { organizationId: string }) => ({ - data: { providers: [provider(organizationId)] }, - isLoading: false, - })) +beforeEach(() => { + // The component reads getBaseUrl() during render; make sure the env var is + // present even when the suite runs without a local .env or after another + // test file mutated the environment (auto-restored via unstubEnvs). + vi.stubEnv('NEXT_PUBLIC_APP_URL', 'http://localhost:3000') + ;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + mockUseSession.mockReturnValue({ data: { user: { id: 'user-1' } } }) + mockUseOrganizationBilling.mockReturnValue({ + data: { data: { subscriptionPlan: 'enterprise' } }, + isLoading: false, }) - - afterEach(() => { - act(() => root.unmount()) - container.remove() - vi.clearAllMocks() + mockUseConfigureSSO.mockReturnValue({ + isPending: false, + mutateAsync: vi.fn(), }) + mockUseSSOProviders.mockImplementation(({ organizationId }: { organizationId: string }) => ({ + data: { providers: [provider(organizationId)] }, + isLoading: false, + })) +}) + +afterEach(() => { + act(() => root.unmount()) + container.remove() + vi.clearAllMocks() +}) +describe('SSO organization transitions', () => { it('discards org A edit state before rendering org B settings', () => { renderSso('org-a') expect(container).toHaveTextContent('org-a.example.com') - const editButton = Array.from(container.querySelectorAll('button')).find( - (button) => button.textContent === 'Edit' - ) - expect(editButton).toBeDefined() - act(() => editButton?.click()) + expect(findButton('Edit')).toBeDefined() + startEditing() expect(container.querySelector('input[value="client-a"]')).not.toBeNull() renderSso('org-b') @@ -186,3 +208,129 @@ describe('SSO organization transitions', () => { expect(container.querySelector('input[value="client-a"]')).toBeNull() }) }) + +/** + * The stored client secret never reaches the browser — the API sends a sentinel. + * Three pieces have to agree for an edit to preserve it: hydration must not put the + * sentinel in the form, validation must not demand a value, and submit must send the + * sentinel back. If any one drifts, an admin editing an unrelated field either wipes + * their secret or saves the literal string "[REDACTED]" as one. + */ +describe('SSO client secret preservation', () => { + function secretInput() { + return container.querySelector('#sso-client-secret') + } + + /** Sets the input through the native setter so React's onChange fires. */ + function typeSecret(value: string) { + const input = secretInput() + expect(input).not.toBeNull() + act(() => { + const setter = Object.getOwnPropertyDescriptor( + window.HTMLInputElement.prototype, + 'value' + )?.set + setter?.call(input, value) + input?.dispatchEvent(new Event('input', { bubbles: true })) + }) + } + + it('shows the saved secret as a masked hint rather than the sentinel', () => { + renderSso('org-a') + startEditing() + + expect(container).not.toHaveTextContent('[REDACTED]') + expect(secretInput()?.value).toBe('••••••••••••4f2a') + expect(findButton('Replace')).toBeDefined() + }) + + it('keeps the stored secret when the admin edits without replacing it', async () => { + const mutateAsync = vi.fn().mockResolvedValue({}) + mockUseConfigureSSO.mockReturnValue({ isPending: false, mutateAsync }) + + renderSso('org-a') + startEditing() + await act(async () => { + findButton('Update')?.click() + }) + + expect(mutateAsync).toHaveBeenCalledTimes(1) + expect(mutateAsync.mock.calls[0][0].clientSecret).toBe('[REDACTED]') + }) + + it('sends the new value when the admin replaces the secret', async () => { + const mutateAsync = vi.fn().mockResolvedValue({}) + mockUseConfigureSSO.mockReturnValue({ isPending: false, mutateAsync }) + + renderSso('org-a') + startEditing() + act(() => findButton('Replace')?.click()) + + typeSecret('brand-new-secret') + + await act(async () => { + findButton('Update')?.click() + }) + + expect(mutateAsync).toHaveBeenCalledTimes(1) + expect(mutateAsync.mock.calls[0][0].clientSecret).toBe('brand-new-secret') + }) + + /** + * A whitespace-only value must not reach the server. Validation is skipped only + * while the stored secret is being kept; once Replace is clicked the field is a + * real input, so blank input has to fail rather than overwrite a working secret. + */ + it('refuses to submit a whitespace-only replacement', async () => { + const mutateAsync = vi.fn().mockResolvedValue({}) + mockUseConfigureSSO.mockReturnValue({ isPending: false, mutateAsync }) + + renderSso('org-a') + startEditing() + act(() => findButton('Replace')?.click()) + typeSecret(' ') + + await act(async () => { + findButton('Update')?.click() + }) + + expect(mutateAsync).not.toHaveBeenCalled() + expect(container).toHaveTextContent('Client Secret is required.') + }) + + /** + * Backing out has to revalidate as "keeping the saved secret". Validating against + * the pre-toggle value would leave a required-error stranded on the masked row, + * where there is no longer an input to fix it in. + */ + it('clears a stranded required-error when the replacement is backed out', async () => { + renderSso('org-a') + startEditing() + act(() => findButton('Replace')?.click()) + typeSecret(' ') + await act(async () => { + findButton('Update')?.click() + }) + expect(container).toHaveTextContent('Client Secret is required.') + + act(() => findButton('Keep saved')?.click()) + + expect(container).not.toHaveTextContent('Client Secret is required.') + expect(secretInput()?.value).toBe('••••••••••••4f2a') + }) + + /** + * The label is deliberately not "Cancel": the header already uses that to discard + * the whole edit, and matching it here would make two very different actions + * indistinguishable. + */ + it('restores the masked row and drops the typed value when the replace is backed out', () => { + renderSso('org-a') + startEditing() + act(() => findButton('Replace')?.click()) + act(() => findButton('Keep saved')?.click()) + + expect(secretInput()?.value).toBe('••••••••••••4f2a') + expect(findButton('Replace')).toBeDefined() + }) +}) diff --git a/apps/sim/ee/sso/components/sso-settings.tsx b/apps/sim/ee/sso/components/sso-settings.tsx index cf4f499c05e..536834747a0 100644 --- a/apps/sim/ee/sso/components/sso-settings.tsx +++ b/apps/sim/ee/sso/components/sso-settings.tsx @@ -3,6 +3,7 @@ import { useState } from 'react' import { Button, + Chip, ChipCombobox, ChipCopyInput, ChipInput, @@ -23,6 +24,7 @@ import type { SsoRegistrationBody } from '@/lib/api/contracts/auth' import { useSession } from '@/lib/auth/auth-client' import { isEnterprise } from '@/lib/billing/plan-helpers' import { isBillingEnabled } from '@/lib/core/config/env-flags' +import { REDACTED_MARKER } from '@/lib/core/security/redaction' import { getBaseUrl } from '@/lib/core/utils/urls' import { SettingsEmptyState } from '@/app/workspace/[workspaceId]/settings/components/settings-empty-state' import { SettingsPanel } from '@/app/workspace/[workspaceId]/settings/components/settings-panel' @@ -69,6 +71,115 @@ const SAML_NAMEID_FORMATS = [ const PROVIDER_ID_SUGGESTIONS = SSO_TRUSTED_PROVIDERS.map((id) => ({ label: id, value: id })) +const CLIENT_SECRET_FIELD_ID = 'sso-client-secret' +/** Fixed width, so the mask never leaks how long the stored secret is. */ +const CLIENT_SECRET_MASK = '••••••••••••' + +interface ClientSecretFieldProps { + /** A secret is already saved, so the field opens as a masked fact rather than an input. */ + hasStoredSecret: boolean + /** Last four characters of the saved secret, when the API judged it safe to hint. */ + storedHint: string | null + isReplacing: boolean + onReplace: () => void + onCancelReplace: () => void + value: string + onChange: (value: string) => void + hasError: boolean +} + +/** + * A saved client secret is a fact, not an editable value — the browser never + * receives it. Rendering it as a static masked row with an explicit Replace + * action avoids the "will blank clear it?" ambiguity an empty input invites, and + * keeps a stray keystroke from arming a replacement. + */ +function ClientSecretField({ + hasStoredSecret, + storedHint, + isReplacing, + onReplace, + onCancelReplace, + value, + onChange, + hasError, +}: ClientSecretFieldProps) { + const [isRevealed, setIsRevealed] = useState(false) + + if (hasStoredSecret && !isReplacing) { + return ( +
+ + Replace +
+ ) + } + + return ( +
+ { + e.target.removeAttribute('readOnly') + setIsRevealed(true) + }} + onBlurCapture={() => setIsRevealed(false)} + onChange={(e) => onChange(e.target.value)} + inputClassName={!isRevealed ? '[-webkit-text-security:disc]' : undefined} + error={hasError} + endAdornment={ + // Only offer the reveal once there is something to reveal. + value ? ( + + ) : undefined + } + /> + {/* Not "Cancel" — the header already owns that label for discarding the + whole edit, and these two do very different things. */} + {hasStoredSecret && Keep saved} +
+ ) +} + +/** Reads the display-only hint the API attaches beside the redacted client secret. */ +function readClientSecretHint(oidcConfig?: string): string | null { + if (!oidcConfig) return null + try { + const hint = JSON.parse(oidcConfig).clientSecretHint + return typeof hint === 'string' ? hint : null + } catch { + return null + } +} + const DEFAULT_FORM_DATA = { providerType: 'oidc' as 'oidc' | 'saml', providerId: '', @@ -134,7 +245,6 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { const configureSSOMutation = useConfigureSSO() - const [showClientSecret, setShowClientSecret] = useState(false) const [isEditing, setIsEditing] = useState(false) const [showAdvanced, setShowAdvanced] = useState(false) const [showMapping, setShowMapping] = useState(false) @@ -144,6 +254,19 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { const [errors, setErrors] = useState>(DEFAULT_ERRORS) const [showErrors, setShowErrors] = useState(false) + const [isReplacingClientSecret, setIsReplacingClientSecret] = useState(false) + + /** + * Editing an OIDC provider always means a secret is stored — the contract + * requires one to register, and the API returns only its sentinel, never the + * value. Leaving the field blank therefore means "keep it", not "clear it". + */ + const hasStoredClientSecret = isEditing && existingProvider?.providerType === 'oidc' + /** Last four characters of the saved secret, when the API judged it safe to hint. */ + const storedClientSecretHint = hasStoredClientSecret + ? readClientSecretHint(existingProvider?.oidcConfig) + : null + const hasChanges = (Object.keys(formData) as (keyof typeof formData)[]).some( (k) => formData[k] !== originalFormData[k] ) @@ -208,7 +331,12 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { return out } - const validateAll = (data: typeof formData) => { + /** + * `isReplacingSecret` is a parameter rather than a closure read: callers that + * validate in the same tick as toggling it would otherwise see the previous + * value and leave a stale "required" error on a field that is no longer an input. + */ + const validateAll = (data: typeof formData, isReplacingSecret = isReplacingClientSecret) => { const newErrors: Record = { providerType: [], providerId: validateProviderId(data.providerId), @@ -227,7 +355,13 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { if (providerType === 'oidc') { newErrors.clientId = validateRequired('Client ID', data.clientId) - newErrors.clientSecret = validateRequired('Client Secret', data.clientSecret) + // Skipped only while the stored secret is being kept. Once Replace is + // clicked the field is a real input again, so a blank or whitespace-only + // value has to fail rather than quietly overwrite a working secret. + newErrors.clientSecret = + hasStoredClientSecret && !isReplacingSecret + ? [] + : validateRequired('Client Secret', data.clientSecret) if (!data.scopes || !data.scopes.trim()) { newErrors.scopes = ['Scopes are required for OIDC providers'] } @@ -253,6 +387,7 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { setErrors(DEFAULT_ERRORS) setShowErrors(false) setShowAdvanced(false) + setIsReplacingClientSecret(false) } const handleSubmit = async (e?: React.FormEvent) => { @@ -282,7 +417,14 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { image: OIDC_DEFAULT_MAPPING.image, }, clientId: formData.clientId, - clientSecret: formData.clientSecret, + // Blank on an edit means the admin did not retype it: send the + // sentinel so the server keeps the stored secret. Trimmed because a + // pasted secret often carries a trailing newline, and because a + // whitespace-only value must never be stored as the secret. + clientSecret: + hasStoredClientSecret && !formData.clientSecret.trim() + ? REDACTED_MARKER + : formData.clientSecret.trim(), scopes: formData.scopes.split(',').map((s) => s.trim()), ...(formData.authorizationEndpoint.trim() ? { authorizationEndpoint: formData.authorizationEndpoint.trim() } @@ -324,6 +466,7 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { setShowErrors(false) setIsEditing(false) setShowAdvanced(false) + setIsReplacingClientSecret(false) } catch (err) { const message = getErrorMessage(err, 'Unknown error occurred') toast.error(message) @@ -345,6 +488,18 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { validateAll(next) } + /** + * Backs out of a replacement: drops what was typed and revalidates as "keeping + * the saved secret", so a required-error from a failed submit does not linger on + * a row that is no longer an input. + */ + const handleKeepSavedSecret = () => { + setIsReplacingClientSecret(false) + const next = { ...formData, clientSecret: '' } + setFormData(next) + validateAll(next, false) + } + const isSaml = formData.providerType === 'saml' const mappingDefaults = isSaml ? SAML_DEFAULT_MAPPING : OIDC_DEFAULT_MAPPING const callbackUrl = `${getBaseUrl()}/api/auth/${isSaml ? 'sso/saml2/callback' : 'sso/callback'}/${formData.providerId || existingProvider?.providerId || 'provider-id'}` @@ -373,7 +528,10 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { if (existingProvider.providerType === 'oidc' && existingProvider.oidcConfig) { const config = JSON.parse(existingProvider.oidcConfig) clientId = config.clientId || '' - clientSecret = config.clientSecret || '' + // The API returns the sentinel, never the secret. Showing it verbatim put + // the literal "[REDACTED]" in the field; blanking it lets the placeholder + // say a secret is stored, and submit re-sends the sentinel to keep it. + clientSecret = config.clientSecret === REDACTED_MARKER ? '' : config.clientSecret || '' scopes = config.scopes?.join(',') || 'openid,profile,email' mapping = config.mapping ?? {} authorizationEndpoint = config.authorizationEndpoint || '' @@ -429,6 +587,7 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { setIsEditing(true) setShowErrors(false) setShowAdvanced(false) + setIsReplacingClientSecret(false) setShowMapping(Boolean(snapshot.mapId || snapshot.mapEmail || snapshot.mapName)) } catch (err) { logger.error('Failed to parse provider config', { error: err }) @@ -665,45 +824,25 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { 0 ? errors.clientSecret.join(' ') : undefined } > - setIsReplacingClientSecret(true)} + onCancelReplace={handleKeepSavedSecret} value={formData.clientSecret} - name='sso_client_key' - autoComplete='off' - autoCapitalize='none' - spellCheck={false} - readOnly - onFocus={(e) => { - e.target.removeAttribute('readOnly') - setShowClientSecret(true) - }} - onBlurCapture={() => setShowClientSecret(false)} - onChange={(e) => handleInputChange('clientSecret', e.target.value)} - inputClassName={!showClientSecret ? '[-webkit-text-security:disc]' : undefined} - error={showErrors && errors.clientSecret.length > 0} - endAdornment={ - - } + onChange={(next) => handleInputChange('clientSecret', next)} + hasError={showErrors && errors.clientSecret.length > 0} /> diff --git a/apps/sim/lib/api/contracts/memory.ts b/apps/sim/lib/api/contracts/memory.ts index 34432af0e36..0551959bc59 100644 --- a/apps/sim/lib/api/contracts/memory.ts +++ b/apps/sim/lib/api/contracts/memory.ts @@ -116,7 +116,7 @@ export const getMemoryByIdContract = defineRouteContract({ query: memoryWorkspaceQuerySchema, response: { mode: 'json', - schema: memorySuccessResponseSchema(memoryRecordSchema), + schema: memorySuccessResponseSchema(memoryRecordSchema.nullable()), }, })