From 1329bc9fda176e70ca7536a5c4ba33eae403d350 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 6 Aug 2026 01:55:14 -0700 Subject: [PATCH 1/7] fix(sso): stop showing the redaction sentinel in the client secret field --- apps/sim/ee/sso/components/sso-settings.tsx | 31 ++++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/apps/sim/ee/sso/components/sso-settings.tsx b/apps/sim/ee/sso/components/sso-settings.tsx index cf4f499c05e..35317c855bd 100644 --- a/apps/sim/ee/sso/components/sso-settings.tsx +++ b/apps/sim/ee/sso/components/sso-settings.tsx @@ -23,6 +23,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' @@ -144,6 +145,13 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { const [errors, setErrors] = useState>(DEFAULT_ERRORS) const [showErrors, setShowErrors] = 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' + const hasChanges = (Object.keys(formData) as (keyof typeof formData)[]).some( (k) => formData[k] !== originalFormData[k] ) @@ -227,7 +235,9 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { if (providerType === 'oidc') { newErrors.clientId = validateRequired('Client ID', data.clientId) - newErrors.clientSecret = validateRequired('Client Secret', data.clientSecret) + newErrors.clientSecret = hasStoredClientSecret + ? [] + : validateRequired('Client Secret', data.clientSecret) if (!data.scopes || !data.scopes.trim()) { newErrors.scopes = ['Scopes are required for OIDC providers'] } @@ -282,7 +292,12 @@ 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. + clientSecret: + hasStoredClientSecret && !formData.clientSecret + ? REDACTED_MARKER + : formData.clientSecret, scopes: formData.scopes.split(',').map((s) => s.trim()), ...(formData.authorizationEndpoint.trim() ? { authorizationEndpoint: formData.authorizationEndpoint.trim() } @@ -373,7 +388,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 || '' @@ -665,6 +683,11 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { 0 ? errors.clientSecret.join(' ') @@ -674,7 +697,7 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { Date: Thu, 6 Aug 2026 01:57:02 -0700 Subject: [PATCH 2/7] fix(sso): hide the reveal toggle when there is nothing to reveal --- apps/sim/ee/sso/components/sso-settings.tsx | 33 +++++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/apps/sim/ee/sso/components/sso-settings.tsx b/apps/sim/ee/sso/components/sso-settings.tsx index 35317c855bd..d99f1475e43 100644 --- a/apps/sim/ee/sso/components/sso-settings.tsx +++ b/apps/sim/ee/sso/components/sso-settings.tsx @@ -713,19 +713,26 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { inputClassName={!showClientSecret ? '[-webkit-text-security:disc]' : undefined} error={showErrors && errors.clientSecret.length > 0} endAdornment={ - + // Only offer the reveal once there is something to reveal. The + // stored secret is never sent to the browser, so on an untouched + // edit the toggle would be a control that visibly does nothing. + formData.clientSecret ? ( + + ) : undefined } /> From cfc8f278aa6462120beb0a4ab0d226b671787dfd Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 6 Aug 2026 02:01:58 -0700 Subject: [PATCH 3/7] feat(sso): show the saved client secret as a masked fact with an explicit Replace action --- apps/sim/app/api/auth/sso/providers/route.ts | 17 +++ apps/sim/ee/sso/components/sso-settings.tsx | 147 +++++++++++++------ 2 files changed, 121 insertions(+), 43 deletions(-) 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.tsx b/apps/sim/ee/sso/components/sso-settings.tsx index d99f1475e43..b06878142d0 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, @@ -70,6 +71,17 @@ const SAML_NAMEID_FORMATS = [ const PROVIDER_ID_SUGGESTIONS = SSO_TRUSTED_PROVIDERS.map((id) => ({ label: id, value: id })) +/** 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: '', @@ -145,12 +157,18 @@ 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] @@ -263,6 +281,7 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { setErrors(DEFAULT_ERRORS) setShowErrors(false) setShowAdvanced(false) + setIsReplacingClientSecret(false) } const handleSubmit = async (e?: React.FormEvent) => { @@ -339,6 +358,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) @@ -447,6 +467,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 }) @@ -684,9 +705,7 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { 0 @@ -694,47 +713,89 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { : undefined } > - { - 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={ - // Only offer the reveal once there is something to reveal. The - // stored secret is never sent to the browser, so on an untouched - // edit the toggle would be a control that visibly does nothing. - formData.clientSecret ? ( - + ) : undefined + } + /> + {hasStoredClientSecret && ( + { + setIsReplacingClientSecret(false) + handleInputChange('clientSecret', '') + }} > - {showClientSecret ? ( - - ) : ( - - )} - - ) : undefined - } - /> + Cancel + + )} + + )}
From f0ca0fde6a00611d1a4efa081ec3bae877530fdb Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 6 Aug 2026 02:04:01 -0700 Subject: [PATCH 4/7] refactor(sso): extract the client secret field and give it its own reveal state --- apps/sim/ee/sso/components/sso-settings.tsx | 194 +++++++++++--------- 1 file changed, 110 insertions(+), 84 deletions(-) diff --git a/apps/sim/ee/sso/components/sso-settings.tsx b/apps/sim/ee/sso/components/sso-settings.tsx index b06878142d0..7b089e1b590 100644 --- a/apps/sim/ee/sso/components/sso-settings.tsx +++ b/apps/sim/ee/sso/components/sso-settings.tsx @@ -71,6 +71,102 @@ 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 + } + /> + {hasStoredSecret && Cancel} +
+ ) +} + /** Reads the display-only hint the API attaches beside the redacted client secret. */ function readClientSecretHint(oidcConfig?: string): string | null { if (!oidcConfig) return null @@ -147,7 +243,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) @@ -704,6 +799,7 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { - {hasStoredClientSecret && !isReplacingClientSecret ? ( - // A saved secret is a fact, not an editable value — the browser - // never receives it. Showing it as a static row with an explicit - // Replace action removes the "is blank going to clear it?" question - // an empty input invites, and stops a stray keystroke from arming - // a replacement. -
- - setIsReplacingClientSecret(true)}>Replace -
- ) : ( -
- { - 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={ - // Only offer the reveal once there is something to reveal. The - // stored secret is never sent to the browser, so on an untouched - // edit the toggle would be a control that visibly does nothing. - formData.clientSecret ? ( - - ) : undefined - } - /> - {hasStoredClientSecret && ( - { - setIsReplacingClientSecret(false) - handleInputChange('clientSecret', '') - }} - > - Cancel - - )} -
- )} + setIsReplacingClientSecret(true)} + onCancelReplace={() => { + setIsReplacingClientSecret(false) + handleInputChange('clientSecret', '') + }} + value={formData.clientSecret} + onChange={(next) => handleInputChange('clientSecret', next)} + hasError={showErrors && errors.clientSecret.length > 0} + />
From 46f5b2350059e4bdb54ee9efa5450e06d804a2a6 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 6 Aug 2026 02:10:34 -0700 Subject: [PATCH 5/7] test(sso): cover client secret preservation, and disambiguate the back-out label --- .../ee/sso/components/sso-settings.test.tsx | 172 ++++++++++++++---- apps/sim/ee/sso/components/sso-settings.tsx | 4 +- 2 files changed, 139 insertions(+), 37 deletions(-) diff --git a/apps/sim/ee/sso/components/sso-settings.test.tsx b/apps/sim/ee/sso/components/sso-settings.test.tsx index e2c9ed920f9..36b0385d7d2 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,81 @@ 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') + } + + 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()) + + const input = secretInput() + expect(input).not.toBeNull() + act(() => { + const setter = Object.getOwnPropertyDescriptor( + window.HTMLInputElement.prototype, + 'value' + )?.set + setter?.call(input, 'brand-new-secret') + input?.dispatchEvent(new Event('input', { bubbles: true })) + }) + + await act(async () => { + findButton('Update')?.click() + }) + + expect(mutateAsync).toHaveBeenCalledTimes(1) + expect(mutateAsync.mock.calls[0][0].clientSecret).toBe('brand-new-secret') + }) + + /** + * 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 7b089e1b590..578484eb278 100644 --- a/apps/sim/ee/sso/components/sso-settings.tsx +++ b/apps/sim/ee/sso/components/sso-settings.tsx @@ -162,7 +162,9 @@ function ClientSecretField({ ) : undefined } /> - {hasStoredSecret && Cancel} + {/* Not "Cancel" — the header already owns that label for discarding the + whole edit, and these two do very different things. */} + {hasStoredSecret && Keep saved}
) } From 091d08a0169dc6c549c4aa6912ca4593f6612c3f Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 6 Aug 2026 02:14:35 -0700 Subject: [PATCH 6/7] fix(sso): reject a blank replacement instead of overwriting the stored secret --- .../ee/sso/components/sso-settings.test.tsx | 47 +++++++++++++++---- apps/sim/ee/sso/components/sso-settings.tsx | 18 ++++--- 2 files changed, 49 insertions(+), 16 deletions(-) diff --git a/apps/sim/ee/sso/components/sso-settings.test.tsx b/apps/sim/ee/sso/components/sso-settings.test.tsx index 36b0385d7d2..98de057d5c5 100644 --- a/apps/sim/ee/sso/components/sso-settings.test.tsx +++ b/apps/sim/ee/sso/components/sso-settings.test.tsx @@ -221,6 +221,20 @@ describe('SSO client secret preservation', () => { 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() @@ -252,16 +266,7 @@ describe('SSO client secret preservation', () => { startEditing() act(() => findButton('Replace')?.click()) - const input = secretInput() - expect(input).not.toBeNull() - act(() => { - const setter = Object.getOwnPropertyDescriptor( - window.HTMLInputElement.prototype, - 'value' - )?.set - setter?.call(input, 'brand-new-secret') - input?.dispatchEvent(new Event('input', { bubbles: true })) - }) + typeSecret('brand-new-secret') await act(async () => { findButton('Update')?.click() @@ -271,6 +276,28 @@ describe('SSO client secret preservation', () => { 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.') + }) + /** * 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 diff --git a/apps/sim/ee/sso/components/sso-settings.tsx b/apps/sim/ee/sso/components/sso-settings.tsx index 578484eb278..62f907c71ab 100644 --- a/apps/sim/ee/sso/components/sso-settings.tsx +++ b/apps/sim/ee/sso/components/sso-settings.tsx @@ -350,9 +350,13 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { if (providerType === 'oidc') { newErrors.clientId = validateRequired('Client ID', data.clientId) - newErrors.clientSecret = hasStoredClientSecret - ? [] - : 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 && !isReplacingClientSecret + ? [] + : validateRequired('Client Secret', data.clientSecret) if (!data.scopes || !data.scopes.trim()) { newErrors.scopes = ['Scopes are required for OIDC providers'] } @@ -409,11 +413,13 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { }, clientId: formData.clientId, // Blank on an edit means the admin did not retype it: send the - // sentinel so the server keeps the stored secret. + // 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 + hasStoredClientSecret && !formData.clientSecret.trim() ? REDACTED_MARKER - : formData.clientSecret, + : formData.clientSecret.trim(), scopes: formData.scopes.split(',').map((s) => s.trim()), ...(formData.authorizationEndpoint.trim() ? { authorizationEndpoint: formData.authorizationEndpoint.trim() } From 707c6754396fbf54d856fcbbf08ae025cc34f968 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 6 Aug 2026 02:25:42 -0700 Subject: [PATCH 7/7] fix(sso): clear the required-error when a secret replacement is backed out --- .../ee/sso/components/sso-settings.test.tsx | 21 +++++++++++++++ apps/sim/ee/sso/components/sso-settings.tsx | 26 ++++++++++++++----- 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/apps/sim/ee/sso/components/sso-settings.test.tsx b/apps/sim/ee/sso/components/sso-settings.test.tsx index 98de057d5c5..010d15af70d 100644 --- a/apps/sim/ee/sso/components/sso-settings.test.tsx +++ b/apps/sim/ee/sso/components/sso-settings.test.tsx @@ -298,6 +298,27 @@ describe('SSO client secret preservation', () => { 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 diff --git a/apps/sim/ee/sso/components/sso-settings.tsx b/apps/sim/ee/sso/components/sso-settings.tsx index 62f907c71ab..536834747a0 100644 --- a/apps/sim/ee/sso/components/sso-settings.tsx +++ b/apps/sim/ee/sso/components/sso-settings.tsx @@ -331,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), @@ -354,7 +359,7 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { // 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 && !isReplacingClientSecret + hasStoredClientSecret && !isReplacingSecret ? [] : validateRequired('Client Secret', data.clientSecret) if (!data.scopes || !data.scopes.trim()) { @@ -483,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'}` @@ -822,10 +839,7 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) { storedHint={storedClientSecretHint} isReplacing={isReplacingClientSecret} onReplace={() => setIsReplacingClientSecret(true)} - onCancelReplace={() => { - setIsReplacingClientSecret(false) - handleInputChange('clientSecret', '') - }} + onCancelReplace={handleKeepSavedSecret} value={formData.clientSecret} onChange={(next) => handleInputChange('clientSecret', next)} hasError={showErrors && errors.clientSecret.length > 0}