From 465bdbd12b8f608753907fc960dd24ed4b9f0abe Mon Sep 17 00:00:00 2001 From: Waleed Date: Sun, 23 Aug 2026 04:16:12 -0700 Subject: [PATCH 01/32] fix(settings): keep billing header stable (#7010) --- .claude/rules/sim-settings-pages.md | 8 +- .../settings/[section]/settings.tsx | 1 - .../components/billing/billing.test.tsx | 124 +++++++++++++----- .../settings/components/billing/billing.tsx | 31 +++-- .../settings/settings-header-shell.test.tsx | 10 +- .../components/settings/settings-panel.tsx | 19 ++- 6 files changed, 137 insertions(+), 56 deletions(-) diff --git a/.claude/rules/sim-settings-pages.md b/.claude/rules/sim-settings-pages.md index 2cff0545957..b65deabafe3 100644 --- a/.claude/rules/sim-settings-pages.md +++ b/.claude/rules/sim-settings-pages.md @@ -13,8 +13,9 @@ The Next.js `settings/[section]/layout.tsx` owns all settings page chrome via `SettingsHeaderShell` — a fixed header bar (a left back chip + right-aligned action chips), a scroll region, and a centered `max-w-[48rem]` content column led by a **title + description from navigation metadata**. The chrome stays mounted -across section navigation (it never re-renders or re-lays-out). Each section -renders through the **`SettingsPanel`** registrar +across section navigation. Its routed title and description are available before +the section body resolves. Each section renders through the **`SettingsPanel`** +registrar (`@/app/workspace/[workspaceId]/settings/components/settings-panel`), which feeds the shell its header data and renders only the section body. Sections supply **data**, never chrome. @@ -82,6 +83,9 @@ return ( `children` instead and omit the prop. - `title?` / `description?` — overrides for the nav-driven defaults. **Only** for a detail sub-view that needs a different heading; normal pages never pass these. + A top-level page's header identity must remain stable while its data loads: + never replace navigation metadata with client-fetched copy after first paint. + Put data-dependent context in the page body instead. - `scrollContainerRef?: React.Ref` — forwards a ref to the scroll region (e.g. programmatic scroll-to-bottom). diff --git a/apps/sim/app/workspace/[workspaceId]/settings/[section]/settings.tsx b/apps/sim/app/workspace/[workspaceId]/settings/[section]/settings.tsx index 2eebb5a5b80..edb01ca9214 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/[section]/settings.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/[section]/settings.tsx @@ -177,7 +177,6 @@ export function SettingsPage({ section }: SettingsPageProps) { )} diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/billing/billing.test.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/billing/billing.test.tsx index a270afc4070..4b75e1440d0 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/billing/billing.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/billing/billing.test.tsx @@ -174,6 +174,14 @@ vi.mock('@/app/workspace/[workspaceId]/settings/components/settings-panel', () = ), })) +vi.mock('@/app/workspace/[workspaceId]/settings/components/settings-empty-state', () => ({ + SettingsEmptyState: ({ children, tone }: { children: ReactNode; tone?: 'muted' | 'error' }) => ( +
+ {children} +
+ ), +})) + vi.mock( '@/app/workspace/[workspaceId]/settings/components/settings-section/settings-section', () => ({ @@ -269,13 +277,7 @@ describe('Billing payer scope', () => { it('uses the target organization DTO for annual, canceled, credit, cap, and link state', async () => { await act(async () => { - root.render( - - ) + root.render() }) expect(mockUseSubscriptionData).toHaveBeenCalledWith( @@ -290,9 +292,7 @@ describe('Billing payer scope', () => { container.querySelector('a[href="/workspace/organization-workspace/upgrade"]')?.textContent ).toBe('Explore organization plans') expect(container.textContent).toContain('Organization Max for Teams plan') - expect(container.textContent).toContain( - 'Target organization’s subscription governs Production.' - ) + expect(container.querySelector('main > p')).toBeNull() expect(container.textContent).toContain('billed annually') expect(container.textContent).toContain('Access until') expect(container.textContent).toContain('Subscription canceled') @@ -316,19 +316,45 @@ describe('Billing payer scope', () => { it('uses a guaranteed personal payer workspace for account upgrades', async () => { await act(async () => { - root.render() + root.render() }) expect( container.querySelector('a[href="/workspace/personal-workspace/upgrade"]')?.textContent ).toBe('Explore personal plans') expect(container.textContent).toContain('Personal Pro plan') - expect(container.textContent).toContain( - 'Your personal subscription governs Personal workspace.' - ) }) - it('does not show a governing subscription description for a free personal workspace', async () => { + it('does not override the route-owned header while billing transitions from loading to success', async () => { + mockPersonalQuery.current = { + data: undefined, + error: null, + isLoading: true, + refetch: vi.fn(), + } + + await act(async () => { + root.render() + }) + + expect(container.innerHTML).toBe('') + + mockPersonalQuery.current = { + data: { success: true, context: 'user', data: PERSONAL_DATA }, + error: null, + isLoading: false, + refetch: vi.fn(), + } + + await act(async () => { + root.render() + }) + + expect(container.textContent).toContain('Personal Pro plan') + expect(container.querySelector('main > p')).toBeNull() + }) + + it('does not add a dynamic header description for a free personal workspace', async () => { mockPersonalQuery.current = { data: { success: true, @@ -340,7 +366,7 @@ describe('Billing payer scope', () => { } await act(async () => { - root.render() + root.render() }) expect(container.textContent).toContain('Personal Free plan') @@ -368,13 +394,7 @@ describe('Billing payer scope', () => { } await act(async () => { - root.render( - - ) + root.render() }) expect(container.textContent).toContain('Organization Free plan') @@ -398,13 +418,7 @@ describe('Billing payer scope', () => { } await act(async () => { - root.render( - - ) + root.render() }) expect(container.textContent).toContain('Organization Max for Teams plan ended') @@ -415,4 +429,54 @@ describe('Billing payer scope', () => { container.querySelector('a[href="/workspace/organization-workspace/upgrade"]')?.textContent ).toBe('Explore organization plans') }) + + it('renders the canonical error state when the active billing query fails', async () => { + mockPersonalQuery.current = { + data: undefined, + error: new Error('Billing temporarily unavailable'), + isLoading: false, + refetch: vi.fn(), + } + + await act(async () => { + root.render() + }) + + const errorState = container.querySelector('[data-testid="settings-empty-state"]') + expect(errorState).toHaveAttribute('data-tone', 'error') + expect(errorState?.textContent).toBe('Billing temporarily unavailable') + }) + + it('keeps cached billing content visible when a background refresh fails', async () => { + mockPersonalQuery.current = { + data: { success: true, context: 'user', data: PERSONAL_DATA }, + error: new Error('Background refresh failed'), + isLoading: false, + refetch: vi.fn(), + } + + await act(async () => { + root.render() + }) + + expect(container.textContent).toContain('Personal Pro plan') + expect(container.querySelector('[data-testid="settings-empty-state"]')).toBeNull() + }) + + it('renders the canonical fallback error when billing completes without data', async () => { + mockOrganizationQuery.current = { + data: undefined, + error: null, + isLoading: false, + refetch: vi.fn(), + } + + await act(async () => { + root.render() + }) + + const errorState = container.querySelector('[data-testid="settings-empty-state"]') + expect(errorState).toHaveAttribute('data-tone', 'error') + expect(errorState?.textContent).toBe('Failed to load billing information') + }) }) diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/billing/billing.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/billing/billing.tsx index 31febdeaecc..0cc86a7be47 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/billing/billing.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/billing/billing.tsx @@ -46,6 +46,7 @@ import { getBaseUrl } from '@/lib/core/utils/urls' import { CreditUsageSection } from '@/app/workspace/[workspaceId]/settings/components/billing/components/credit-usage-section/credit-usage-section' import { UsageLimitField } from '@/app/workspace/[workspaceId]/settings/components/billing/components/usage-limit-field/usage-limit-field' import { getSubscriptionPermissions } from '@/app/workspace/[workspaceId]/settings/components/billing/subscription-permissions' +import { SettingsEmptyState } from '@/app/workspace/[workspaceId]/settings/components/settings-empty-state' import { SettingsPanel } from '@/app/workspace/[workspaceId]/settings/components/settings-panel' import { RESOURCE_ROW_ARROW_CLASSES } from '@/app/workspace/[workspaceId]/settings/components/settings-resource-row' import { SettingsSection } from '@/app/workspace/[workspaceId]/settings/components/settings-section/settings-section' @@ -103,20 +104,15 @@ interface BillingProps { scope: 'account' | 'organization' organizationId?: string creditUsageHref?: string - governingWorkspaceName?: string } -export function Billing({ - scope, - organizationId, - creditUsageHref, - governingWorkspaceName, -}: BillingProps) { +export function Billing({ scope, organizationId, creditUsageHref }: BillingProps) { const router = useRouter() const isOrganizationScope = scope === 'organization' const { data: subscriptionData, + error: subscriptionError, isLoading: isSubscriptionLoading, refetch: refetchSubscription, } = useSubscriptionData({ @@ -127,6 +123,7 @@ export function Billing({ const { data: organizationBillingData, + error: organizationBillingError, isLoading: isOrgBillingLoading, refetch: refetchOrganizationBilling, } = useOrganizationBilling(billingOrganizationId || '', { enabled: isOrganizationScope }) @@ -157,6 +154,7 @@ export function Billing({ ? (organizationBilling?.subscriptionStatus ?? 'inactive') : (subscriptionData?.data?.status ?? 'inactive') const isLoading = isOrganizationScope ? isOrgBillingLoading : isSubscriptionLoading + const billingError = isOrganizationScope ? organizationBillingError : subscriptionError const subscription = { isFree: isFree(plan), @@ -403,7 +401,15 @@ export function Billing({ } if (isLoading) return null - if (isOrganizationScope ? !organizationBilling : !subscriptionData?.data) return null + if (isOrganizationScope ? !organizationBilling : !subscriptionData?.data) { + return ( + + + {getErrorMessage(billingError, 'Failed to load billing information')} + + + ) + } const planName = getDisplayPlanName(subscription.plan) const billingInterval = isOrganizationScope @@ -458,16 +464,9 @@ export function Billing({ const explorePlansLabel = isOrganizationScope ? 'Explore organization plans' : 'Explore personal plans' - const subscriptionOwner = isOrganizationScope - ? `${organizationBilling?.organizationName ?? 'The organization'}’s subscription` - : 'Your personal subscription' - const settingsDescription = - governingWorkspaceName && subscription.isPaid - ? `${subscriptionOwner} governs ${governingWorkspaceName}.` - : undefined return ( - +
diff --git a/apps/sim/components/settings/settings-header-shell.test.tsx b/apps/sim/components/settings/settings-header-shell.test.tsx index 40cd2a33bde..e2d588159ff 100644 --- a/apps/sim/components/settings/settings-header-shell.test.tsx +++ b/apps/sim/components/settings/settings-header-shell.test.tsx @@ -41,7 +41,7 @@ function renderHeader(actions: SettingsAction[]) { root.render( - +
@@ -152,7 +152,11 @@ describe('SettingsHeaderShell static meta', () => { it('yields to a body that registers its own header', () => { renderWithMeta( - +
) @@ -192,7 +196,7 @@ describe('SettingsHeaderShell static meta', () => { it('falls back to the meta title when the body unmounts mid-navigation', () => { renderWithMeta( - +
) diff --git a/apps/sim/components/settings/settings-panel.tsx b/apps/sim/components/settings/settings-panel.tsx index 5a688d142a3..da919ad88be 100644 --- a/apps/sim/components/settings/settings-panel.tsx +++ b/apps/sim/components/settings/settings-panel.tsx @@ -38,17 +38,28 @@ export function SettingsSectionProvider({ ) } -interface SettingsPanelProps { +interface SettingsPanelBaseProps { children?: ReactNode actions?: SettingsAction[] - back?: SettingsBackAction search?: SettingsHeaderSearch - title?: string - description?: string docsLink?: string scrollContainerRef?: Ref } +type SettingsPanelProps = SettingsPanelBaseProps & + ( + | { + back: SettingsBackAction + title?: string + description?: string + } + | { + back?: undefined + title?: never + description?: never + } + ) + export function SettingsPanel({ children, actions, From f3867694b84042e8dfc522dbb3df382dbea2aab8 Mon Sep 17 00:00:00 2001 From: Waleed Date: Sun, 23 Aug 2026 10:31:42 -0700 Subject: [PATCH 02/32] fix(files): allowlist the schemes a markdown link may target (#7012) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `normalizeLinkHref` rejected only `file` for a `scheme://` target, so any other scheme was returned unchanged. `scheme://` is well-formed for every scheme, so the check let through spellings that are not navigable targets at all. - Keep a scheme only when it is http(s), ftp(s), mailto, or tel; drop the rest - Leave an existing link alone when a committed target normalizes away, rather than unsetting it — the editor seeds that field with the current href, so committing an untouched one previously removed the link Detection is unchanged for relative, anchor, protocol-relative, and bare-domain targets. A document's stored markdown is untouched: normalization runs on the render and edit paths, never on parse or serialize, so a target that is refused still round-trips verbatim. --- .../rich-markdown-editor/markdown-fidelity.ts | 21 ++++--- .../menus/link-editing.test.ts | 46 ++++++++++++++ .../menus/link-editing.tsx | 12 +++- .../rich-markdown-editor/round-trip.test.ts | 62 +++++++++++++++++++ 4 files changed, 128 insertions(+), 13 deletions(-) create mode 100644 apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.test.ts diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity.ts index 4470187fefa..fd46d579527 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity.ts @@ -76,21 +76,24 @@ export function applyFrontmatter(frontmatter: string, body: string): string { return frontmatter + body } -/** A leading `scheme://` URL (network protocol). */ -const SCHEME_URL = /^([a-z][a-z0-9+.-]*):\/\//i /** A leading `scheme:` token (per the URL grammar). */ const HAS_SCHEME = /^[a-z][a-z0-9+.-]*:/i /** A bare `host:port` (digits after the colon) — looks scheme-like but is really a domain. */ const HOST_PORT = /^[a-z0-9.-]+:\d+(?:[/?#]|$)/i +/** + * The only schemes a document link may target — an allowlist, because `scheme://` is well-formed for + * every scheme: rejecting just the ones known to be dangerous leaves the next one through, and + * `javascript://…` is a valid URL whose `//` run is merely a comment. + */ +const SAFE_SCHEME = /^(?:(?:https?|ftps?):\/\/|(?:mailto|tel):)/i + /** * Normalize a user-entered link target: prefix a bare domain with `https://` so it doesn't resolve * as an in-app relative URL, while leaving already-qualified, relative (`./other.md`, `../doc.md`), and - * protocol-relative URLs intact. Dangerous schemes are rejected outright rather than trusted or mangled: - * any `scheme:` without `//` other than `mailto:`/`tel:` (so `javascript:`, `data:`, `vbscript:`, - * `blob:`, …), and `file://` (local file access). Other network `scheme://` URLs (`http(s)`, `ftp`, …) - * pass through. A bare `host:port` (digits after the colon) is a domain, not a scheme, so it still gets - * the `https://` prefix. + * protocol-relative URLs intact. A scheme is kept only when {@link SAFE_SCHEME} matches; every other + * one is dropped to `''`, which callers render as inert text rather than a link. A bare `host:port` + * (digits after the colon) is a domain, not a scheme, so it still gets the `https://` prefix. */ export function normalizeLinkHref(href: string): string { const trimmed = href.trim() @@ -99,9 +102,7 @@ export function normalizeLinkHref(href: string): string { if (trimmed.startsWith('//')) return `https:${trimmed}` if (trimmed.startsWith('/')) return trimmed if (trimmed.startsWith('./') || trimmed.startsWith('../')) return trimmed - if (/^(?:mailto|tel):/i.test(trimmed)) return trimmed - const schemed = trimmed.match(SCHEME_URL) - if (schemed) return /^file$/i.test(schemed[1]) ? '' : trimmed + if (SAFE_SCHEME.test(trimmed)) return trimmed if (HAS_SCHEME.test(trimmed) && !HOST_PORT.test(trimmed)) return '' return `https://${trimmed}` } diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.test.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.test.ts new file mode 100644 index 00000000000..e66652f8e54 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.test.ts @@ -0,0 +1,46 @@ +import type { ChainedCommands } from '@tiptap/core' +import { describe, expect, it, vi } from 'vitest' +import { applyLink } from './link-editing' + +function chainSpy() { + const calls: string[] = [] + const chain = { + extendMarkRange: vi.fn(() => chain), + setLink: vi.fn(({ href }: { href: string }) => { + calls.push(`setLink:${href}`) + return chain + }), + unsetLink: vi.fn(() => { + calls.push('unsetLink') + return chain + }), + run: vi.fn(() => true), + } + return { chain: chain as unknown as ChainedCommands, calls } +} + +describe('applyLink', () => { + it('sets a link for a target that survives normalization', () => { + const { chain, calls } = chainSpy() + applyLink(chain, ' sim.ai ') + expect(calls).toEqual(['setLink:https://sim.ai']) + }) + + it('removes the link when the field is cleared', () => { + const { chain, calls } = chainSpy() + applyLink(chain, ' ') + expect(calls).toEqual(['unsetLink']) + }) + + /** + * The field is seeded with the raw href, so committing one untouched must not be read as "remove". + * Dropping an unsafe target is a refusal to link, not an instruction to delete what is already there. + */ + it('leaves the existing link untouched when the target normalizes away', () => { + for (const target of ['javascript://%0aalert(1)', 'customproto://host/path']) { + const { chain, calls } = chainSpy() + applyLink(chain, target) + expect(calls).toEqual([]) + } + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.tsx b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.tsx index f294e88a950..8cbd0dc7b6d 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.tsx +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.tsx @@ -4,11 +4,17 @@ import { normalizeLinkHref } from '../markdown-fidelity' /** * Applies a link to the chain's current selection: normalizes `rawHref`, expands to the full link - * mark, and sets it — or removes the link when the href is empty/unsafe. The caller supplies a chain - * already focused with the target selection (the captured bubble-menu range / the hovered link range). + * mark, and sets it. Clearing the field removes the link; a target that survives normalization + * replaces it. A target that normalizes away is neither set nor removed — the editor seeds this field + * with the raw href, so committing an untouched one would otherwise delete a link the user only + * opened, and dropping an unsafe target is not the same instruction as "remove this link". The + * caller supplies a chain already focused with the target selection (the captured bubble-menu range / + * the hovered link range). */ export function applyLink(chain: ChainedCommands, rawHref: string): void { - const href = normalizeLinkHref(rawHref.trim()) + const trimmed = rawHref.trim() + const href = normalizeLinkHref(trimmed) + if (!href && trimmed) return chain.extendMarkRange('link') if (href) chain.setLink({ href }) else chain.unsetLink() diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/round-trip.test.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/round-trip.test.ts index c8745e5e861..961fd1d86df 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/round-trip.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/round-trip.test.ts @@ -5,6 +5,7 @@ * be idempotent (a second pass changes nothing) so autosave never churns. Mirrors the exact * pipeline the editor uses: split frontmatter out, serialize the body, re-attach + clean up. */ +import type { JSONContent } from '@tiptap/core' import { Editor } from '@tiptap/core' import { afterEach, describe, expect, it } from 'vitest' import { createMarkdownContentExtensions } from './extensions' @@ -14,6 +15,7 @@ import { postProcessSerializedMarkdown, splitFrontmatter, } from './markdown-fidelity' +import { parseMarkdownToDoc } from './markdown-parse' let editor: Editor | null = null @@ -126,6 +128,66 @@ describe('markdown-fidelity utils', () => { expect(normalizeLinkHref('blob:https://x.com/uuid')).toBe('') expect(normalizeLinkHref('vbscript:msgbox(1)')).toBe('') expect(normalizeLinkHref('localhost:3000/path')).toBe('https://localhost:3000/path') + // Adding `//` doesn't make a scheme safe, and an unknown scheme is dropped rather than trusted — + // the allowlist is the whole rule. + expect(normalizeLinkHref('javascript://%0aalert(1)')).toBe('') + expect(normalizeLinkHref('customproto://host/path')).toBe('') + }) + + /** + * The property that matters, stated over the spellings a browser collapses before it resolves a + * scheme: whatever comes back must not be executable. Padding and interior tabs/newlines are the + * usual way a blocked scheme is smuggled past a matcher that only reads the literal text. + */ + it('never returns a target that resolves to an executable scheme', () => { + const tab = String.fromCharCode(9) + const lf = String.fromCharCode(10) + const nbsp = String.fromCharCode(160) + const inputs = [ + 'javascript://%0aalert(1)', + 'javascript:alert(1)', + 'JAVASCRIPT://x', + ' javascript:alert(1) ', + `${nbsp}javascript:alert(1)`, + `java${tab}script://alert(1)`, + `java${lf}script:alert(1)`, + 'data://text/html,