diff --git a/apps/web/app/(app)/workspaces/[workspaceId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx b/apps/web/app/(app)/workspaces/[workspaceId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx index 67cb506e0298..f5ae2fb218d5 100644 --- a/apps/web/app/(app)/workspaces/[workspaceId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx +++ b/apps/web/app/(app)/workspaces/[workspaceId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx @@ -305,10 +305,13 @@ export const SurveyAnalysisCTA = ({ open={isCautionDialogOpen} setOpen={setIsCautionDialogOpen} isLoading={loading} - primaryButtonAction={() => duplicateSurveyAndRoute(survey.id)} - primaryButtonText={t("workspace.surveys.edit.caution_edit_duplicate")} - secondaryButtonAction={() => router.push(`/workspaces/${workspace?.id}/surveys/${survey.id}/edit`)} - secondaryButtonText={t("common.edit")} + primaryButtonAction={async () => { + setIsCautionDialogOpen(false); + router.push(`/workspaces/${workspace?.id}/surveys/${survey.id}/edit`); + }} + primaryButtonText={t("common.edit")} + secondaryButtonAction={() => duplicateSurveyAndRoute(survey.id)} + secondaryButtonText={t("workspace.surveys.edit.caution_edit_duplicate")} /> )} diff --git a/apps/web/modules/auth/lib/personal-email-domains.ts b/apps/web/modules/auth/lib/personal-email-domains.ts index cfe4af14e924..a363cab073c0 100644 --- a/apps/web/modules/auth/lib/personal-email-domains.ts +++ b/apps/web/modules/auth/lib/personal-email-domains.ts @@ -19,21 +19,28 @@ export const PERSONAL_EMAIL_DOMAINS: readonly string[] = [ // Microsoft "outlook.com", "hotmail.com", + "hotmail.fr", "live.com", "msn.com", // Yahoo / AOL "yahoo.com", "yahoo.co.uk", "ymail.com", + "rocketmail.com", "aol.com", // Apple "icloud.com", "me.com", "mac.com", - // Privacy-focused + // Privacy-focused / relay "proton.me", "protonmail.com", "pm.me", + "passmail.com", + "mailbox.org", + "posteo.de", + "mozmail.com", + "duck.com", // Other mainstream / international "gmx.com", "gmx.net", @@ -42,6 +49,13 @@ export const PERSONAL_EMAIL_DOMAINS: readonly string[] = [ "zoho.com", "mail.com", "qq.com", + "foxmail.com", "163.com", "126.com", + "yeah.net", + "emailn.de", + "sfr.fr", + "ukr.net", + // Common typo of gmail.com + "gmaill.com", ]; diff --git a/apps/web/modules/core/rate-limit/rate-limit-configs.test.ts b/apps/web/modules/core/rate-limit/rate-limit-configs.test.ts index 0ca958f92575..9e0cc8103b32 100644 --- a/apps/web/modules/core/rate-limit/rate-limit-configs.test.ts +++ b/apps/web/modules/core/rate-limit/rate-limit-configs.test.ts @@ -97,10 +97,20 @@ describe("rateLimitConfigs", () => { "isSurveyResponsePresent", "validateSurveyPin", "licenseRecheck", + "unsplash", "inviteMember", "bulkInviteMembers", "generateExampleResponses", ]); + + // Exact values, not just presence: this quota is the only thing bounding one account from + // exhausting the instance-wide UNSPLASH_ACCESS_KEY, so a loosened interval, allowance or + // namespace is a security regression and should fail here rather than in production. + expect(rateLimitConfigs.actions.unsplash).toEqual({ + interval: 60, + allowedPerInterval: 30, + namespace: "action:unsplash", + }); }); test("should have all storage configurations", () => { @@ -171,6 +181,7 @@ describe("rateLimitConfigs", () => { { config: rateLimitConfigs.api.clientEnvironment, identifier: "environment-id" }, { config: rateLimitConfigs.actions.emailUpdate, identifier: "user-profile" }, { config: rateLimitConfigs.actions.accountDeletion, identifier: "user-account-delete" }, + { config: rateLimitConfigs.actions.unsplash, identifier: "user-unsplash" }, { config: rateLimitConfigs.storage.upload, identifier: "storage-upload" }, { config: rateLimitConfigs.storage.uploadPerWorkspace, identifier: "storage-upload-workspace" }, { config: rateLimitConfigs.storage.delete, identifier: "storage-delete" }, diff --git a/apps/web/modules/core/rate-limit/rate-limit-configs.ts b/apps/web/modules/core/rate-limit/rate-limit-configs.ts index 44e7e70f4e7b..74cfd28ff616 100644 --- a/apps/web/modules/core/rate-limit/rate-limit-configs.ts +++ b/apps/web/modules/core/rate-limit/rate-limit-configs.ts @@ -48,6 +48,7 @@ export const rateLimitConfigs = { namespace: "action:validate-survey-pin", }, // 10 per minute — prevents brute-force PIN guessing licenseRecheck: { interval: 60, allowedPerInterval: 5, namespace: "action:license-recheck" }, // 5 per minute + unsplash: { interval: 60, allowedPerInterval: 30, namespace: "action:unsplash" }, // 30 per minute per user — bounds one account exhausting the instance-wide UNSPLASH_ACCESS_KEY quota inviteMember: { interval: 3600 * 24, allowedPerInterval: 20, namespace: "action:invite-member" }, // 20 per day — bounds invite-spam abuse bulkInviteMembers: { interval: 3600 * 24, diff --git a/apps/web/modules/email/components/preview-email-template.tsx b/apps/web/modules/email/components/preview-email-template.tsx index 1fb5d1c73ea6..4c9a25255759 100644 --- a/apps/web/modules/email/components/preview-email-template.tsx +++ b/apps/web/modules/email/components/preview-email-template.tsx @@ -57,6 +57,7 @@ import { getSecondaryButtonStyle, importantStyle, normalizeRichTextSpacing, + suppressNestedListMarkers, } from "../lib/preview-email-template-styles"; import { getNPSOptionColor, getRatingNumberOptionColor } from "../lib/utils"; @@ -159,9 +160,9 @@ function PreviewElementHeader({ return ( { + test.each([ + [ + "a nested-list wrapper li", + ``, + ``, + ], + [ + "the nested class among multiple classes, whatever the attribute order", + `
    `, + `
      `, + ], + [ + "a wrapper li without dropping its ordered-list value", + `
        `, + `
          `, + ], + [ + "a wrapper li by appending to its existing style instead of replacing it", + ``, + ``, + ], + [ + "a wrapper li with single-quoted attributes", + ``, + ``, + ], + ])("suppresses the marker on %s", (_case, html, expected) => { + expect(suppressNestedListMarkers(html)).toBe(expected); + }); + + test.each([ + [ + "list items without the nested class", + '', + ], + [ + "class names that merely contain the nested class as a substring", + ``, + ], + ["html without list items", "

          hello world

          "], + ])("leaves %s untouched", (_case, html) => { + expect(suppressNestedListMarkers(html)).toBe(html); + }); + + test("is idempotent when applied twice", () => { + const html = + ``; + + const once = suppressNestedListMarkers(html); + + expect(suppressNestedListMarkers(once)).toBe(once); + }); +}); diff --git a/apps/web/modules/email/lib/preview-email-template-styles.ts b/apps/web/modules/email/lib/preview-email-template-styles.ts index 3453207c65e1..31af7aa7805a 100644 --- a/apps/web/modules/email/lib/preview-email-template-styles.ts +++ b/apps/web/modules/email/lib/preview-email-template-styles.ts @@ -2,6 +2,10 @@ import type { CSSProperties } from "react"; import type { TSurveyStyling } from "@formbricks/types/surveys/types"; import { COLOR_DEFAULTS, STYLE_DEFAULTS } from "@/lib/styling/constants"; import { isLight, mixColor } from "@/lib/utils/colors"; +import { + NESTED_LIST_ITEM_CLASS, + NESTED_LIST_ITEM_MARKER_STYLE, +} from "@/modules/ui/components/editor/lib/example-theme"; export interface PreviewEmailStyleTokens { accentBackgroundColor: string; @@ -73,8 +77,10 @@ const EMAIL_PREVIEW_ACCENT_COLORS = { } as const; const RICH_TEXT_PARAGRAPH_TAG_REGEX = /]*)>/gi; +const RICH_TEXT_LIST_ITEM_TAG_REGEX = /]*)>/gi; const RICH_TEXT_STYLE_ATTRIBUTE_REGEX = /\sstyle=(["'])(.*?)\1/i; const RICH_TEXT_STYLE_ATTRIBUTE_REPLACE_REGEX = /\sstyle=(["'])(.*?)\1/gi; +const RICH_TEXT_CLASS_ATTRIBUTE_REGEX = /\sclass=(["'])(.*?)\1/i; export const importantStyle = (value: string): string => `${value} !important`; @@ -95,6 +101,39 @@ export const normalizeRichTextSpacing = (html: string): string => return ``; }); +/** + * Lexical wraps a nested list in a structural
        2. (NESTED_LIST_ITEM_CLASS) that must not show its + * own bullet/number. Email clients ignore the app stylesheets, so the suppression is inlined per + * list item (best effort: legacy Outlook's Word engine ignores list-style-type). + */ +export const suppressNestedListMarkers = (html: string): string => + html.replaceAll(RICH_TEXT_LIST_ITEM_TAG_REGEX, (tag: string, attributes: string = "") => { + const classMatch = RICH_TEXT_CLASS_ATTRIBUTE_REGEX.exec(attributes); + const classNames = classMatch ? classMatch[2].split(/\s+/) : []; + if (!classNames.includes(NESTED_LIST_ITEM_CLASS)) { + return tag; + } + + if (RICH_TEXT_STYLE_ATTRIBUTE_REGEX.test(attributes)) { + return ` { + const trimmedStyle = styleValue.trim(); + if (trimmedStyle.includes(NESTED_LIST_ITEM_MARKER_STYLE)) { + return ` style=${quote}${styleValue}${quote}`; + } + const styleWithMarker = trimmedStyle + ? `${trimmedStyle};${NESTED_LIST_ITEM_MARKER_STYLE}` + : NESTED_LIST_ITEM_MARKER_STYLE; + + return ` style=${quote}${styleWithMarker}${quote}`; + } + )}>`; + } + + return ``; + }); + const getPreviewDimension = (value: PreviewStyleValue, fallback: PreviewStyleValue): string => { const resolvedValue = value ?? fallback; diff --git a/apps/web/modules/integrations/webhooks/actions.ts b/apps/web/modules/integrations/webhooks/actions.ts index 986aaeafea4e..7b88a89c5e6d 100644 --- a/apps/web/modules/integrations/webhooks/actions.ts +++ b/apps/web/modules/integrations/webhooks/actions.ts @@ -141,6 +141,10 @@ export const updateWebhookAction = authenticatedActionClient.inputSchema(ZUpdate const ZTestEndpointAction = z.object({ url: z.string(), webhookId: ZId.optional(), + // Required so the not-yet-created-webhook path has something to authorize against; without it this + // action did no authorization at all and any logged-in user — including a billing-only member with no + // product access — could make the server POST a signed test payload to any URL they named. + workspaceId: ZId, secret: z.string().optional(), }); @@ -173,6 +177,23 @@ export const testEndpointAction = authenticatedActionClient secret = webhookResult.data.secret ?? undefined; } else { + // No webhook yet: authorize against the workspace the webhook is being created in. + await checkAuthorizationUpdated({ + userId: ctx.user.id, + organizationId: await getOrganizationIdFromWorkspaceId(parsedInput.workspaceId), + access: [ + { + type: "organization", + roles: ["owner", "manager"], + }, + { + type: "workspaceTeam", + minPermission: "readWrite", + workspaceId: parsedInput.workspaceId, + }, + ], + }); + // New webhook, use the provided secret or generate a new one secret = parsedInput.secret ?? generateWebhookSecret(); } diff --git a/apps/web/modules/integrations/webhooks/components/add-webhook-modal.tsx b/apps/web/modules/integrations/webhooks/components/add-webhook-modal.tsx index 5ad1e764d48c..c3a63771601e 100644 --- a/apps/web/modules/integrations/webhooks/components/add-webhook-modal.tsx +++ b/apps/web/modules/integrations/webhooks/components/add-webhook-modal.tsx @@ -75,6 +75,7 @@ export const AddWebhookModal = ({ const testEndpointActionResult = await testEndpointAction({ url: testEndpointInput, secret: webhookSecret, + workspaceId, }); if (!testEndpointActionResult?.data) { diff --git a/apps/web/modules/integrations/webhooks/components/webhook-settings-tab.tsx b/apps/web/modules/integrations/webhooks/components/webhook-settings-tab.tsx index a93c81b25c08..b523665210fb 100644 --- a/apps/web/modules/integrations/webhooks/components/webhook-settings-tab.tsx +++ b/apps/web/modules/integrations/webhooks/components/webhook-settings-tab.tsx @@ -76,6 +76,7 @@ export const WebhookSettingsTab = ({ const testEndpointActionResult = await testEndpointAction({ url: testEndpointInput, webhookId: webhook.id, + workspaceId: webhook.workspaceId, }); if (!testEndpointActionResult?.data?.success) { const errorMessage = getFormattedErrorMessage(testEndpointActionResult); diff --git a/apps/web/modules/storage/service.test.ts b/apps/web/modules/storage/service.test.ts index ae0a9948a1d0..a3687344d054 100644 --- a/apps/web/modules/storage/service.test.ts +++ b/apps/web/modules/storage/service.test.ts @@ -405,6 +405,29 @@ describe("storage service", () => { expect(result).toEqual(mockError); expect(deleteFileFromS3).toHaveBeenCalledTimes(1); }); + + // Regression: the key is `${id}/${accessType}/${fileName}`, so a `..` segment would delete + // objects outside the workspace prefix the caller was authorized against. + test.each([ + "../../ws-victim/private/secret.pdf", + "sub/../../../ws-victim/private/secret.pdf", + "./file.jpg", + ])("should reject traversal in the file name: %s", async (fileName) => { + const result = await deleteFile("ws-456", "public" as TAccessType, fileName); + + expect(result.ok).toBe(false); + expect(deleteFileFromS3).not.toHaveBeenCalled(); + }); + + test("should still allow nested file paths without dot segments", async () => { + const mockSuccess = { ok: true, data: undefined } as MockedDeleteFileReturn; + vi.mocked(deleteFileFromS3).mockResolvedValue(mockSuccess); + + const result = await deleteFile("ws-456", "public" as TAccessType, "survey-1/q-2/file.jpg"); + + expect(result).toEqual(mockSuccess); + expect(deleteFileFromS3).toHaveBeenCalledWith("ws-456/public/survey-1/q-2/file.jpg"); + }); }); describe("deleteFilesByWorkspaceId", () => { @@ -449,6 +472,40 @@ describe("storage service", () => { }); describe("getFileStreamForDownload", () => { + // Regression: `fileName` comes from the `[...filePath]` route param and the key is + // `${id}/${accessType}/${fileName}`, so a `..` segment let a caller name an object outside their + // own workspace prefix — reachable with no auth at all through the `public/` access type, which + // skips authorizePrivateDownload. The `%252e`/`%2e` cases cover the extra decodeURIComponent the + // handler applies on top of Next's own param decoding. + test.each([ + "../../ws-victim/private/secret.pdf", + "sub/../../../ws-victim/private/secret.pdf", + "%2e%2e/%2e%2e/ws-victim/private/secret.pdf", + "./secret.pdf", + ])("should reject traversal in the file name: %s", async (fileName) => { + const result = await getFileStreamForDownload(fileName, "ws-attacker", "public" as TAccessType); + + expect(result.ok).toBe(false); + expect(getFileStream).not.toHaveBeenCalled(); + }); + + test("should still allow nested file paths without dot segments", async () => { + const mockStreamResult = { + ok: true, + data: { body: new ReadableStream(), contentType: "image/jpeg", contentLength: 1 }, + } as MockedFileStreamReturn; + vi.mocked(getFileStream).mockResolvedValue(mockStreamResult); + + const result = await getFileStreamForDownload( + "survey-1/q-2/file.jpg", + "ws-456", + "public" as TAccessType + ); + + expect(result.ok).toBe(true); + expect(getFileStream).toHaveBeenCalledWith("ws-456/public/survey-1/q-2/file.jpg"); + }); + test("should return file stream for public file", async () => { const mockStream = new ReadableStream(); const mockStreamResult = { diff --git a/apps/web/modules/storage/service.ts b/apps/web/modules/storage/service.ts index 201fa7cd4c49..eaffbf07ec9e 100644 --- a/apps/web/modules/storage/service.ts +++ b/apps/web/modules/storage/service.ts @@ -15,6 +15,21 @@ import { sanitizeFileName } from "./utils"; const SAFE_FILE_PATH_SEGMENT = /^[A-Za-z0-9_-]+$/; +/** + * Rejects relative path segments in a caller-supplied storage file name. + * + * Download/delete keys are built as `${id}/${accessType}/${fileName}`, and `fileName` arrives from the + * `[...filePath]` route param, so `..` segments let a caller step out of their own + * `workspaceId/accessType/` prefix and name another tenant's object — including a `private/` one via + * the unauthenticated `public/` route. Object stores treat keys opaquely and SigV4 signs the + * unnormalized path, so this does not resolve on a stock S3/MinIO backend; but whether one tenant can + * name another's file must not depend on how the storage backend or any proxy in front of it happens + * to treat dot segments. `fileName` legitimately contains `/` (upload nests it under + * `filePathSegments`), so only the segments themselves are constrained. + */ +const hasTraversalSegment = (fileName: string): boolean => + fileName.split("/").some((segment) => segment === "." || segment === ".."); + export const getSignedUrlForUpload = async ( fileName: string, workspaceId: string, @@ -89,6 +104,13 @@ export const getFileStreamForDownload = async ( ): Promise> => { try { const fileNameDecoded = decodeURIComponent(fileName); + + // Checked after the decode: the route param is already URL-decoded once, so a `%252e%252e` + // traversal only becomes `..` here. + if (hasTraversalSegment(fileName) || hasTraversalSegment(fileNameDecoded)) { + return err({ code: StorageErrorCode.InvalidInput }); + } + const primaryKey = `${primaryId}/${accessType}/${fileNameDecoded}`; const streamResult = await getFileStream(primaryKey); @@ -116,6 +138,12 @@ export const deleteFile = async ( fileName: string, fallbackId?: string ) => { + // Same reasoning as the download path: a `..` segment would let an authorized caller delete objects + // outside the workspace prefix they were authorized against. + if (hasTraversalSegment(fileName)) { + return err({ code: StorageErrorCode.InvalidInput }); + } + const result = await deleteFileFromS3(`${primaryId}/${accessType}/${fileName}`); if (!result.ok && result.error.code === StorageErrorCode.FileNotFoundError && fallbackId) { diff --git a/apps/web/modules/survey/components/edit-public-survey-alert-dialog/index.tsx b/apps/web/modules/survey/components/edit-public-survey-alert-dialog/index.tsx index ff6ccf2a6dc0..c34e575104b5 100644 --- a/apps/web/modules/survey/components/edit-public-survey-alert-dialog/index.tsx +++ b/apps/web/modules/survey/components/edit-public-survey-alert-dialog/index.tsx @@ -1,3 +1,4 @@ +import { useState } from "react"; import { useTranslation } from "react-i18next"; import { Button } from "@/modules/ui/components/button"; import { @@ -10,6 +11,9 @@ import { DialogTitle, } from "@/modules/ui/components/dialog"; +/** Stable identity for each footer button, so state doesn't hinge on translated labels. */ +type ActionId = "primary" | "secondary" | "close"; + interface EditPublicSurveyAlertDialogProps { open: boolean; setOpen: (open: boolean) => void; @@ -30,31 +34,34 @@ export const EditPublicSurveyAlertDialog = ({ secondaryButtonText, }: EditPublicSurveyAlertDialogProps) => { const { t } = useTranslation(); + // Track which action is running so the spinner shows on the clicked button, + // regardless of whether the async action is the primary or secondary one. + const [pendingAction, setPendingAction] = useState(null); const actions = [] as Array<{ + id: ActionId; label?: string; onClick: () => void | Promise; - disabled?: boolean; - loading?: boolean; variant: React.ComponentProps["variant"]; }>; if (secondaryButtonAction) { actions.push({ + id: "secondary", label: secondaryButtonText, onClick: secondaryButtonAction, - disabled: isLoading, variant: "secondary", }); } if (primaryButtonAction) { actions.push({ + id: "primary", label: primaryButtonText, onClick: primaryButtonAction, - loading: isLoading, variant: "default", }); } if (actions.length === 0) { actions.push({ + id: "close", label: secondaryButtonText ?? t("common.close"), onClick: () => setOpen(false), variant: "default", @@ -78,8 +85,16 @@ export const EditPublicSurveyAlertDialog = ({ - {actions.map(({ label, onClick, loading, variant, disabled }) => ( - ))} diff --git a/apps/web/modules/survey/editor/actions.ts b/apps/web/modules/survey/editor/actions.ts index a2aac7b6e106..7d1793e8a810 100644 --- a/apps/web/modules/survey/editor/actions.ts +++ b/apps/web/modules/survey/editor/actions.ts @@ -13,13 +13,15 @@ import { UNSPLASH_ALLOWED_DOMAINS, } from "@/lib/constants"; import { capturePostHogEvent } from "@/lib/posthog"; -import { actionClient, authenticatedActionClient } from "@/lib/utils/action-client"; +import { authenticatedActionClient } from "@/lib/utils/action-client"; import { checkAuthorizationUpdated } from "@/lib/utils/action-client/action-client-middleware"; import { getOrganizationIdFromSurveyId, getOrganizationIdFromWorkspaceId, getWorkspaceIdFromSurveyId, } from "@/lib/utils/helper"; +import { applyRateLimit } from "@/modules/core/rate-limit/helpers"; +import { rateLimitConfigs } from "@/modules/core/rate-limit/rate-limit-configs"; import { withAuditLogging } from "@/modules/ee/audit-logs/lib/handler"; import { createActionClass } from "@/modules/survey/editor/lib/action-class"; import { checkExternalUrlsPermission } from "@/modules/survey/editor/lib/check-external-urls-permission"; @@ -399,9 +401,16 @@ const ZGetImagesFromUnsplashAction = z.object({ page: z.number().optional(), }); -export const getImagesFromUnsplashAction = actionClient +// Authenticated: these spend the instance's UNSPLASH_ACCESS_KEY quota on the caller's behalf, and +// plain `actionClient` left them reachable by anyone on the internet. Both are only ever called +// from the survey editor, which already requires a session. +export const getImagesFromUnsplashAction = authenticatedActionClient .inputSchema(ZGetImagesFromUnsplashAction) - .action(async ({ parsedInput }) => { + .action(async ({ parsedInput, ctx }) => { + // Per-user: neither action carries a workspace or survey id, so a session is the only thing to + // scope against, and the quota being spent belongs to the whole instance. + await applyRateLimit(rateLimitConfigs.actions.unsplash, ctx.user.id); + if (!UNSPLASH_ACCESS_KEY) { throw new Error("Unsplash access key is not set"); } @@ -461,9 +470,14 @@ const ZTriggerDownloadUnsplashImageAction = z.object({ downloadUrl: z.url(), }); -export const triggerDownloadUnsplashImageAction = actionClient +// Authenticated: these spend the instance's UNSPLASH_ACCESS_KEY quota on the caller's behalf, and +// plain `actionClient` left them reachable by anyone on the internet. Both are only ever called +// from the survey editor, which already requires a session. +export const triggerDownloadUnsplashImageAction = authenticatedActionClient .inputSchema(ZTriggerDownloadUnsplashImageAction) - .action(async ({ parsedInput }) => { + .action(async ({ parsedInput, ctx }) => { + await applyRateLimit(rateLimitConfigs.actions.unsplash, ctx.user.id); + if (!isValidUnsplashUrl(parsedInput.downloadUrl)) { throw new Error("Invalid Unsplash URL"); } diff --git a/apps/web/modules/ui/components/editor/components/editor.tsx b/apps/web/modules/ui/components/editor/components/editor.tsx index 99ca5047c56f..604dd4c9d4d3 100644 --- a/apps/web/modules/ui/components/editor/components/editor.tsx +++ b/apps/web/modules/ui/components/editor/components/editor.tsx @@ -24,6 +24,7 @@ import "../styles-editor.css"; import { PlaygroundAutoLinkPlugin as AutoLinkPlugin } from "./auto-link-plugin"; import { EditorContentChecker } from "./editor-content-checker"; import { LinkEditor } from "./link-editor"; +import { ListTabIndentationPlugin } from "./list-tab-indentation-plugin"; import { RecallNode } from "./recall-node"; import { RecallPlugin } from "./recall-plugin"; import { ToolbarPlugin } from "./toolbar-plugin"; @@ -138,6 +139,7 @@ export const Editor = (props: TextEditorProps) => { ErrorBoundary={LexicalErrorBoundary} /> + {props.isExternalUrlsAllowed && } {props.isExternalUrlsAllowed && } {props.autoFocus && } diff --git a/apps/web/modules/ui/components/editor/components/list-tab-indentation-plugin.tsx b/apps/web/modules/ui/components/editor/components/list-tab-indentation-plugin.tsx new file mode 100644 index 000000000000..3585bad88fdc --- /dev/null +++ b/apps/web/modules/ui/components/editor/components/list-tab-indentation-plugin.tsx @@ -0,0 +1,69 @@ +import { $isListItemNode } from "@lexical/list"; +import { useLexicalComposerContext } from "@lexical/react/LexicalComposerContext"; +import { $findMatchingParent, $handleIndentAndOutdent, mergeRegister } from "@lexical/utils"; +import { + $getSelection, + $isRangeSelection, + COMMAND_PRIORITY_CRITICAL, + COMMAND_PRIORITY_EDITOR, + INDENT_CONTENT_COMMAND, + KEY_TAB_COMMAND, + OUTDENT_CONTENT_COMMAND, +} from "lexical"; +import { useEffect } from "react"; + +// Same cap as Lexical's playground (maxIndent 7): items can nest up to indent level 6. +const MAX_INDENT = 7; + +/** + * Scoped alternative to Lexical's TabIndentationPlugin: Tab indents and Shift+Tab outdents only + * while the selection is inside list items, so Tab keeps moving focus everywhere else and the + * editor does not trap keyboard users. + */ +export const ListTabIndentationPlugin = () => { + const [editor] = useLexicalComposerContext(); + + useEffect(() => { + return mergeRegister( + editor.registerCommand( + KEY_TAB_COMMAND, + (event) => { + const selection = $getSelection(); + if (!$isRangeSelection(selection)) { + return false; + } + const anchorListItem = $findMatchingParent(selection.anchor.getNode(), $isListItemNode); + const focusListItem = $findMatchingParent(selection.focus.getNode(), $isListItemNode); + if (!anchorListItem || !focusListItem) { + return false; + } + event.preventDefault(); + return editor.dispatchCommand( + event.shiftKey ? OUTDENT_CONTENT_COMMAND : INDENT_CONTENT_COMMAND, + undefined + ); + }, + COMMAND_PRIORITY_EDITOR + ), + // Caps indentation before @lexical/rich-text's uncapped INDENT_CONTENT_COMMAND handler runs. + editor.registerCommand( + INDENT_CONTENT_COMMAND, + () => { + const selection = $getSelection(); + if (!$isRangeSelection(selection)) { + return false; + } + return $handleIndentAndOutdent((block) => { + const newIndent = block.getIndent() + 1; + if (newIndent < MAX_INDENT) { + block.setIndent(newIndent); + } + }); + }, + COMMAND_PRIORITY_CRITICAL + ) + ); + }, [editor]); + + return null; +}; diff --git a/apps/web/modules/ui/components/editor/components/toolbar-plugin.tsx b/apps/web/modules/ui/components/editor/components/toolbar-plugin.tsx index 56b0ae1c464e..5dc71e1e634d 100644 --- a/apps/web/modules/ui/components/editor/components/toolbar-plugin.tsx +++ b/apps/web/modules/ui/components/editor/components/toolbar-plugin.tsx @@ -99,6 +99,7 @@ const ToolbarButton = ({ icon: Icon, active, onClick, tooltipText, disabled }: T variant="ghost" size="icon" type="button" + aria-label={tooltipText} tabIndex={-1} onClick={onClick} disabled={disabled} diff --git a/apps/web/modules/ui/components/editor/lib/example-theme.test.ts b/apps/web/modules/ui/components/editor/lib/example-theme.test.ts index 104931f03834..d8ed7d25a775 100644 --- a/apps/web/modules/ui/components/editor/lib/example-theme.test.ts +++ b/apps/web/modules/ui/components/editor/lib/example-theme.test.ts @@ -1,5 +1,7 @@ +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; import { describe, expect, test } from "vitest"; -import { exampleTheme } from "./example-theme"; +import { NESTED_LIST_ITEM_CLASS, NESTED_LIST_ITEM_MARKER_STYLE, exampleTheme } from "./example-theme"; describe("exampleTheme", () => { test("contains all required theme properties", () => { @@ -23,7 +25,19 @@ describe("exampleTheme", () => { expect(exampleTheme.list).toHaveProperty("ol"); expect(exampleTheme.list).toHaveProperty("ul"); expect(exampleTheme.list).toHaveProperty("listitem"); - expect(exampleTheme.list.nested).toHaveProperty("listitem"); + expect(exampleTheme.list.nested.listitem).toBe(NESTED_LIST_ITEM_CLASS); + }); + + // The stylesheet cannot import the constants, and it is what hides the marker on the structural + //
        3. in the editor, the survey runtime and the studio preview alike. Renaming the theme class + // without renaming the selector would silently bring the stray bullet back on every surface. + test("the shared stylesheet suppresses the marker for the theme's nested list class", () => { + const stylesheet = readFileSync( + fileURLToPath(new URL("../styles-editor-frontend.css", import.meta.url)), + "utf8" + ).replaceAll(/\s+/g, ""); + + expect(stylesheet).toContain(`.${NESTED_LIST_ITEM_CLASS}{${NESTED_LIST_ITEM_MARKER_STYLE}`); }); test("contains text formatting styles", () => { diff --git a/apps/web/modules/ui/components/editor/lib/example-theme.ts b/apps/web/modules/ui/components/editor/lib/example-theme.ts index 018f95988bcb..0cdb76fa4521 100644 --- a/apps/web/modules/ui/components/editor/lib/example-theme.ts +++ b/apps/web/modules/ui/components/editor/lib/example-theme.ts @@ -1,3 +1,13 @@ +/** + * Lexical wraps a nested list in a structural
        4. that carries no text of its own. Its class and + * the declaration that hides its marker are declared here once and consumed by + * `suppressNestedListMarkers` (email preview) and, by reference, by the two stylesheets that style + * the same markup: `styles-editor-frontend.css` and `packages/surveys/src/styles/global.css`. + * `example-theme.test.ts` fails if a rename leaves one of those stylesheets behind. + */ +export const NESTED_LIST_ITEM_CLASS = "fb-editor-nested-listitem"; +export const NESTED_LIST_ITEM_MARKER_STYLE = "list-style-type:none"; + export const exampleTheme = { rtl: "fb-editor-rtl", ltr: "fb-editor-ltr", @@ -9,7 +19,7 @@ export const exampleTheme = { }, list: { nested: { - listitem: "fb-editor-nested-listitem", + listitem: NESTED_LIST_ITEM_CLASS, }, ol: "fb-editor-list-ol", ul: "fb-editor-list-ul", diff --git a/apps/web/modules/ui/components/editor/styles-editor-frontend.css b/apps/web/modules/ui/components/editor/styles-editor-frontend.css index 8531c887c010..ab84c734ac9b 100644 --- a/apps/web/modules/ui/components/editor/styles-editor-frontend.css +++ b/apps/web/modules/ui/components/editor/styles-editor-frontend.css @@ -39,22 +39,47 @@ margin-bottom: 20px !important; } +/* + * One list model for every surface: the survey runtime injects this stylesheet as well + * (packages/surveys/src/lib/styles.ts), so a list looks the same in the editor, in the preview and + * for the respondent. Every level — the first one included — advances by exactly the list's own + * 1.5em padding; the items themselves add no horizontal offset. + */ +.fb-editor-list-ul, +.fb-editor-list-ol { + list-style-position: outside !important; + margin-block: 0 12px !important; + padding-inline-start: 1.5em !important; +} + .fb-editor-list-ul { - margin-bottom: 12px !important; + list-style-type: disc !important; } .fb-editor-list-ol { - margin-bottom: 12px !important; + list-style-type: decimal !important; } .fb-editor-listitem { - margin: 0px 32px !important; + margin: 0 !important; } +/* + * Lexical wraps a nested list in a structural
        5. that carries no text of its own, so it must not + * show a marker. Source of truth for the class: lib/example-theme.ts -> list.nested.listitem. + */ .fb-editor-nested-listitem { list-style-type: none !important; } +/* Only the outermost list keeps the trailing gap; nesting adds no vertical space. */ +.fb-editor-list-ul .fb-editor-list-ul, +.fb-editor-list-ul .fb-editor-list-ol, +.fb-editor-list-ol .fb-editor-list-ul, +.fb-editor-list-ol .fb-editor-list-ol { + margin-block: 0 !important; +} + .fb-editor-rtl { text-align: right !important; } diff --git a/apps/web/modules/ui/components/editor/styles-editor.css b/apps/web/modules/ui/components/editor/styles-editor.css index 07cac0002461..a0d430d0403a 100644 --- a/apps/web/modules/ui/components/editor/styles-editor.css +++ b/apps/web/modules/ui/components/editor/styles-editor.css @@ -3,18 +3,10 @@ font-size: 14px; } -.editor li { - padding-left: 1.28571429em; - text-indent: -1.28571429em; -} - -.editor ul { - list-style: disc inside; -} - -.editor ol { - list-style: decimal inside; -} +/* + * List rendering lives in styles-editor-frontend.css, which the survey runtime injects too, so the + * editor and the respondent view stay in sync. Editor-only list rules would re-introduce the drift. + */ .editor-container { border-radius: 6px; diff --git a/apps/web/playwright/survey-editor-rich-text-tabs.spec.ts b/apps/web/playwright/survey-editor-rich-text-tabs.spec.ts new file mode 100644 index 000000000000..0ad022dbffbf --- /dev/null +++ b/apps/web/playwright/survey-editor-rich-text-tabs.spec.ts @@ -0,0 +1,133 @@ +import { type Page, expect } from "@playwright/test"; +import { type Fixtures, test } from "./lib/fixtures"; +import { createSurveyFromScratch } from "./utils/helper"; + +/** + * Tab behavior of the survey editor's rich text editor (ENG-1747). + * + * Regression: Tab always moved focus out of the Lexical editor, so nested lists + * could not be created. The fix (ListTabIndentationPlugin) makes Tab indent and + * Shift+Tab outdent only while the caret is inside a list item. Outside list + * items Tab must keep its default behavior and move focus out of the editor — + * a deliberate a11y requirement (no keyboard trap). + * + * Both scenarios use the welcome card panel, which renders two rich text + * editors: "Note*" (headline) and "Welcome message" (description). + */ + +const openWelcomeCardPanel = async (page: Page): Promise => { + await expect(page.locator("#welcome-toggle")).toBeVisible(); + await page.getByText("Welcome Card").click(); + await page.locator("#welcome-toggle").check(); + await expect(page.locator('label:has-text("Welcome message")')).toBeVisible(); + + // Enabling the card remounts the live preview's card, which focuses its first + // control (the welcome card's "Next" button) once it re-renders. Wait for that + // one-time focus steal to complete, otherwise it lands mid-typing and both + // drops characters and breaks focus assertions. Stacked off-screen preview + // cards render dummy buttons with tabindex="-1"; the real one is tabbable. + const previewNext = page + .locator("#formbricks-survey-container") + .getByRole("button", { name: "Next", exact: true }) + .and(page.locator('[tabindex="0"]')); + await expect(previewNext).toBeFocused({ timeout: 15000 }); +}; + +/** + * Same label -> container walk as fillRichTextEditor (utils/helper.ts); the + * container also holds the editor's toolbar, whose icon buttons expose their + * translated tooltip text as the accessible name ("Bulleted list" is + * workspace.surveys.edit.bulleted_list in en-US.json). + */ +const editorField = (page: Page, labelText: string) => { + const container = page.locator(`label:has-text("${labelText}")`).locator("..").locator(".."); + return { + input: container.locator(".editor-input").first(), + bulletListButton: container.getByRole("button", { name: "Bulleted list", exact: true }), + }; +}; + +const gotoFreshSurveyEditorWelcomeCard = async (page: Page, users: Fixtures["users"]): Promise => { + const user = await users.create(); + await user.login(); + await page.waitForURL(/\/workspaces\/[^/]+\/surveys/); + await createSurveyFromScratch(page); + await openWelcomeCardPanel(page); +}; + +const clearEditor = async (input: ReturnType["input"]): Promise => { + await input.click(); + await input.press("ControlOrMeta+a"); + await input.press("Backspace"); +}; + +/** Deepest chain of nested lists in the editor: 1 for a flat list, 2 after one Tab, and so on. */ +const listNestingDepth = async (input: ReturnType["input"]): Promise => + input.evaluate((editor) => { + const depthOf = (item: Element): number => { + let depth = 0; + for (let node = item.parentElement; node && node !== editor; node = node.parentElement) { + if (node.tagName === "UL" || node.tagName === "OL") depth++; + } + return depth; + }; + + return Math.max(0, ...Array.from(editor.querySelectorAll("li"), depthOf)); + }); + +test.describe("Survey editor rich text editor Tab behavior", () => { + test("Tab nests a bullet list item and Shift+Tab flattens it back", async ({ page, users }) => { + await gotoFreshSurveyEditorWelcomeCard(page, users); + const { input, bulletListButton } = editorField(page, "Welcome message"); + + // Turn the cleared editor's block into a bullet list via the toolbar + // (the markdown "- " shortcut is not registered in this editor). + await clearEditor(input); + await bulletListButton.click(); + await expect(input.locator("ul > li")).toHaveCount(1); + + // Two sibling items; the caret ends up at the end of the second item. + await input.pressSequentially("First item", { delay: 50 }); + await input.press("Enter"); + await input.pressSequentially("Second item", { delay: 50 }); + await expect(input.locator("ul > li")).toHaveCount(2); + await expect(input.locator("li ul li")).toHaveCount(0); + + // Tab keeps focus in the editor and nests the second item (li > ul > li). + await input.press("Tab"); + await expect(input).toBeFocused(); + await expect(input.locator("li ul li")).toHaveText("Second item"); + await expect(input.locator("li.fb-editor-nested-listitem")).toBeVisible(); + + // Shift+Tab returns it to a single-level list. + await input.press("Shift+Tab"); + await expect(input).toBeFocused(); + await expect(input.locator("li ul li")).toHaveCount(0); + await expect(input.locator("ul > li")).toHaveText(["First item", "Second item"]); + + // Each further Tab nests one more level until MAX_INDENT (7 levels). Past the ceiling the + // plugin still swallows the key, so focus stays in the editor instead of escaping mid-list. + for (let press = 0; press < 10; press++) { + await input.press("Tab"); + } + await expect(input).toBeFocused(); + await expect.poll(() => listNestingDepth(input)).toBe(7); + }); + + test("Tab in plain text moves focus out of the editor (no keyboard trap)", async ({ page, users }) => { + await gotoFreshSurveyEditorWelcomeCard(page, users); + const { input } = editorField(page, "Note*"); + + // Caret sits in a plain paragraph, not a list item. + await clearEditor(input); + await input.pressSequentially("Plain text", { delay: 50 }); + await expect(input.locator("li")).toHaveCount(0); + await expect(input).toBeFocused(); + + // Tab must not be swallowed by the editor: focus leaves this contenteditable. + // (It may legitimately land on the page's next tab stop, e.g. another field.) + await input.press("Tab"); + await expect(input).not.toBeFocused(); + await expect.poll(() => input.evaluate((el) => el.contains(document.activeElement))).toBe(false); + }); +}); diff --git a/packages/surveys/src/lib/use-focus-trap.test.tsx b/packages/surveys/src/lib/use-focus-trap.test.tsx index b3da45ce13c9..27681142ac90 100644 --- a/packages/surveys/src/lib/use-focus-trap.test.tsx +++ b/packages/surveys/src/lib/use-focus-trap.test.tsx @@ -1,5 +1,6 @@ import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/preact"; import { type ComponentChildren } from "preact"; +import { useEffect, useRef } from "preact/hooks"; import { afterEach, describe, expect, test, vi } from "vitest"; import { useFocusTrap } from "./use-focus-trap"; @@ -43,6 +44,64 @@ const FocusTrapUnmountFixture = ({ ); +// Mimics a host page that runs its own focus manager (another focus trap, a modal from the embedding +// app) and pushes focus back out every time it lands inside the survey. +const CompetingFocusManagerFixture = ({ + onCompetingRedirect, + reactAsynchronously = false, +}: { + onCompetingRedirect: () => void; + reactAsynchronously?: boolean; +}) => { + const focusTrapRef = useFocusTrap({ enabled: true }); + const outsideButtonRef = useRef(null); + const onCompetingRedirectRef = useRef(onCompetingRedirect); + + useEffect(() => { + onCompetingRedirectRef.current = onCompetingRedirect; + }, [onCompetingRedirect]); + + useEffect(() => { + const takeFocusOut = () => { + const outsideButton = outsideButtonRef.current; + if (!outsideButton) return; + + onCompetingRedirectRef.current(); + outsideButton.focus(); + }; + + const handleFocusIn = (event: FocusEvent) => { + const container = focusTrapRef.current; + const target = event.target as HTMLElement | null; + + if (!container || !target) return; + if (!container.contains(target)) return; + + if (reactAsynchronously) { + setTimeout(takeFocusOut, 0); + return; + } + + takeFocusOut(); + }; + + document.addEventListener("focusin", handleFocusIn); + return () => { + document.removeEventListener("focusin", handleFocusIn); + }; + }, [focusTrapRef, reactAsynchronously]); + + return ( + <> + + +
          + +
          + + ); +}; + describe("useFocusTrap", () => { afterEach(() => { cleanup(); @@ -251,6 +310,65 @@ describe("useFocusTrap", () => { }); }); + test("gives up instead of recursing when the page fights it for focus", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const onCompetingRedirect = vi.fn(); + + render(); + + const trappedButton = screen.getByRole("button", { name: "Survey action" }); + const hostPageButton = screen.getByRole("button", { name: "Host page button" }); + const trapContainer = trappedButton.parentElement as HTMLElement; + + await waitFor(() => { + expect(document.activeElement).toBe(trappedButton); + }); + + // Every round the trap pulls focus back in and the competing manager takes it straight out again. + // Before the guards this recursed until "Maximum call stack size exceeded". + for (let round = 0; round < 5; round++) { + hostPageButton.focus(); + await Promise.resolve(); + } + + expect(onCompetingRedirect.mock.calls.length).toBeLessThan(20); + expect(warnSpy).toHaveBeenCalled(); + expect(trapContainer.contains(document.activeElement)).toBe(false); + + warnSpy.mockRestore(); + }); + + test("stops the focus tug-of-war when the page fights back asynchronously", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const onCompetingRedirect = vi.fn(); + + render(); + + const trappedButton = screen.getByRole("button", { name: "Survey action" }); + const hostPageButton = screen.getByRole("button", { name: "Host page button" }); + + await waitFor(() => { + expect(document.activeElement).toBe(trappedButton); + }); + + // An async competing manager cannot overflow the stack, but it can keep the two sides swapping + // focus forever. The trap has to run out of redirect budget and back off. + hostPageButton.focus(); + + await waitFor(() => { + expect(warnSpy).toHaveBeenCalled(); + }); + + const redirectsOnBackoff = onCompetingRedirect.mock.calls.length; + await new Promise((resolve) => { + setTimeout(resolve, 50); + }); + + expect(onCompetingRedirect.mock.calls.length).toBe(redirectsOnBackoff); + + warnSpy.mockRestore(); + }); + test("does not move focus when inactive", async () => { render( diff --git a/packages/surveys/src/lib/use-focus-trap.ts b/packages/surveys/src/lib/use-focus-trap.ts index d61f2a440215..a88d81cc5215 100644 --- a/packages/surveys/src/lib/use-focus-trap.ts +++ b/packages/surveys/src/lib/use-focus-trap.ts @@ -7,6 +7,16 @@ type UseFocusTrapOptions = { onEscapeKeyDown?: () => void; }; +// A host page can run its own focus manager (another focus trap, a modal from the embedding app, a +// second embedded survey bundle). When it pulls focus out every time this trap pulls it back in, the +// two ping-pong focus synchronously and blow the call stack. Two guards stop that: the trap ignores +// focus events caused by its own redirects, and it gives up after a few redirects the page undoes. +const REDIRECT_WINDOW_MS = 250; +const MAX_REDIRECTS_PER_WINDOW = 10; +const MAX_FAILED_REDIRECTS = 3; + +const getTimestamp = () => (typeof performance === "undefined" ? Date.now() : performance.now()); + // focus trap behavior adapted from Radix UI FocusScope (MIT) for this Preact runtime. const focusScopesStack = (() => { let stack: FocusScope[] = []; @@ -170,6 +180,57 @@ export const useFocusTrap = ({ focus(lastFocusedElement ?? firstFocusableElement ?? container, { select: true }); }; + let isRedirectingFocus = false; + let hasYieldedToPage = false; + let redirectWindowStart = getTimestamp(); + let redirectsInWindow = 0; + let failedRedirects = 0; + + const yieldToPage = () => { + hasYieldedToPage = true; + console.warn("Formbricks: focus trap turned off, another focus handler on the page keeps taking focus"); + }; + + const hasRedirectBudget = () => { + const timestamp = getTimestamp(); + if (timestamp - redirectWindowStart > REDIRECT_WINDOW_MS) { + redirectWindowStart = timestamp; + redirectsInWindow = 0; + } + + redirectsInWindow += 1; + return redirectsInWindow <= MAX_REDIRECTS_PER_WINDOW; + }; + + // Pull focus back into the survey. Never re-entrant, so a competing focus manager that reacts to + // our own focus move cannot recurse through the handlers below. + const redirectFocusIntoContainer = () => { + if (isRedirectingFocus || hasYieldedToPage) return; + + if (!hasRedirectBudget()) { + yieldToPage(); + return; + } + + isRedirectingFocus = true; + try { + focusLastElementInsideContainer(); + } finally { + isRedirectingFocus = false; + } + + if (container.contains(document.activeElement)) { + failedRedirects = 0; + return; + } + + // Something moved focus straight back out. Stop fighting for it after a few tries. + failedRedirects += 1; + if (failedRedirects >= MAX_FAILED_REDIRECTS) { + yieldToPage(); + } + }; + const handleFocusIn = (event: FocusEvent) => { if (focusScope.paused) return; @@ -179,7 +240,7 @@ export const useFocusTrap = ({ return; } - focusLastElementInsideContainer(); + redirectFocusIntoContainer(); }; const handleFocusOut = (event: FocusEvent) => { @@ -187,14 +248,14 @@ export const useFocusTrap = ({ const relatedTarget = event.relatedTarget as HTMLElement | null; if (relatedTarget && !container.contains(relatedTarget)) { - focusLastElementInsideContainer(); + redirectFocusIntoContainer(); return; } if (relatedTarget === null) { setTimeout(() => { if (!isUnmounting && !container.contains(document.activeElement)) { - focusLastElementInsideContainer(); + redirectFocusIntoContainer(); } }, 0); } @@ -202,7 +263,7 @@ export const useFocusTrap = ({ const handleMutations = () => { if (!container.contains(document.activeElement)) { - focusLastElementInsideContainer(); + redirectFocusIntoContainer(); } }; diff --git a/packages/surveys/src/styles/global.css b/packages/surveys/src/styles/global.css index ace2dadb61c7..3439c4326fa5 100644 --- a/packages/surveys/src/styles/global.css +++ b/packages/surveys/src/styles/global.css @@ -85,7 +85,7 @@ #fbjs [data-variant="description"] ol { list-style-position: outside; margin: 0.5em 0; - padding-left: 1.5em; + padding-inline-start: 1.5em; } #fbjs .htmlbody ul, #fbjs [data-variant="headline"] ul, @@ -102,6 +102,14 @@ #fbjs [data-variant="description"] li { margin: 0.25em 0; } +/* + * The rules above are the fallback for list markup without the editor's theme classes. Rich text + * authored in the app carries them, and its geometry (1.5em per level, no item margins, no marker + * on the .fb-editor-nested-listitem wrapper Lexical puts around a nested list) comes from the + * editor stylesheet that lib/styles.ts injects right after this file — one definition shared with + * the editor itself. Source of truth for those class names: + * apps/web/modules/ui/components/editor/lib/example-theme.ts + */ /* without this, it wont override the color */ #fbjs p.editor-paragraph {