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/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} />