Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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")}
/>
)}

Expand Down
16 changes: 15 additions & 1 deletion apps/web/modules/auth/lib/personal-email-domains.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
];
11 changes: 11 additions & 0 deletions apps/web/modules/core/rate-limit/rate-limit-configs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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" },
Expand Down
1 change: 1 addition & 0 deletions apps/web/modules/core/rate-limit/rate-limit-configs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
5 changes: 3 additions & 2 deletions apps/web/modules/email/components/preview-email-template.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import {
getSecondaryButtonStyle,
importantStyle,
normalizeRichTextSpacing,
suppressNestedListMarkers,
} from "../lib/preview-email-template-styles";
import { getNPSOptionColor, getRatingNumberOptionColor } from "../lib/utils";

Expand Down Expand Up @@ -159,9 +160,9 @@ function PreviewElementHeader({
return (
<ElementHeader
className={className}
headline={normalizeRichTextSpacing(headline)}
headline={suppressNestedListMarkers(normalizeRichTextSpacing(headline))}
style={getLightModeTextStyle(styleTokens)}
subheader={subheader ? normalizeRichTextSpacing(subheader) : undefined}
subheader={subheader ? suppressNestedListMarkers(normalizeRichTextSpacing(subheader)) : undefined}
subheaderStyle={{
...getForcedColorStyle(styleTokens.elementDescriptionColor),
fontSize: styleTokens.elementDescriptionFontSize,
Expand Down
64 changes: 64 additions & 0 deletions apps/web/modules/email/lib/preview-email-template-styles.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import { describe, expect, test } from "vitest";
import {
NESTED_LIST_ITEM_CLASS,
NESTED_LIST_ITEM_MARKER_STYLE,
} from "@/modules/ui/components/editor/lib/example-theme";
import { suppressNestedListMarkers } from "./preview-email-template-styles";

const marker = `style="${NESTED_LIST_ITEM_MARKER_STYLE}"`;

describe("suppressNestedListMarkers", () => {
test.each([
[
"a nested-list wrapper li",
`<ul><li class="${NESTED_LIST_ITEM_CLASS}"><ul><li>child</li></ul></li></ul>`,
`<ul><li class="${NESTED_LIST_ITEM_CLASS}" ${marker}><ul><li>child</li></ul></li></ul>`,
],
[
"the nested class among multiple classes, whatever the attribute order",
`<ol><li value="2" class="fb-editor-listitem ${NESTED_LIST_ITEM_CLASS}"><ol></ol></li></ol>`,
`<ol><li value="2" class="fb-editor-listitem ${NESTED_LIST_ITEM_CLASS}" ${marker}><ol></ol></li></ol>`,
],
[
"a wrapper li without dropping its ordered-list value",
`<ol><li class="${NESTED_LIST_ITEM_CLASS}" value="3"><ol></ol></li></ol>`,
`<ol><li class="${NESTED_LIST_ITEM_CLASS}" value="3" ${marker}><ol></ol></li></ol>`,
],
[
"a wrapper li by appending to its existing style instead of replacing it",
`<ul><li class="${NESTED_LIST_ITEM_CLASS}" style="text-align: center"><ul></ul></li></ul>`,
`<ul><li class="${NESTED_LIST_ITEM_CLASS}" style="text-align: center;${NESTED_LIST_ITEM_MARKER_STYLE}"><ul></ul></li></ul>`,
],
[
"a wrapper li with single-quoted attributes",
`<ul><li class='${NESTED_LIST_ITEM_CLASS}'><ul></ul></li></ul>`,
`<ul><li class='${NESTED_LIST_ITEM_CLASS}' ${marker}><ul></ul></li></ul>`,
],
])("suppresses the marker on %s", (_case, html, expected) => {
expect(suppressNestedListMarkers(html)).toBe(expected);
});

test.each([
[
"list items without the nested class",
'<ul><li class="fb-editor-listitem" value="1">one</li><li>two</li></ul>',
],
[
"class names that merely contain the nested class as a substring",
`<ul><li class="${NESTED_LIST_ITEM_CLASS}-custom">item</li></ul>`,
],
["html without list items", "<p>hello <strong>world</strong></p>"],
])("leaves %s untouched", (_case, html) => {
expect(suppressNestedListMarkers(html)).toBe(html);
});

test("is idempotent when applied twice", () => {
const html =
`<ul><li class="${NESTED_LIST_ITEM_CLASS}"><ul></ul></li>` +
`<li class="${NESTED_LIST_ITEM_CLASS}" style="text-align: right"><ol></ol></li></ul>`;

const once = suppressNestedListMarkers(html);

expect(suppressNestedListMarkers(once)).toBe(once);
});
});
39 changes: 39 additions & 0 deletions apps/web/modules/email/lib/preview-email-template-styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -73,8 +77,10 @@ const EMAIL_PREVIEW_ACCENT_COLORS = {
} as const;

const RICH_TEXT_PARAGRAPH_TAG_REGEX = /<p\b([^>]*)>/gi;
const RICH_TEXT_LIST_ITEM_TAG_REGEX = /<li\b([^>]*)>/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`;

Expand All @@ -95,6 +101,39 @@ export const normalizeRichTextSpacing = (html: string): string =>
return `<p${attributes} style="margin:0">`;
});

/**
* Lexical wraps a nested list in a structural <li> (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 `<li${attributes.replaceAll(
RICH_TEXT_STYLE_ATTRIBUTE_REPLACE_REGEX,
(_styleAttribute, quote: string, styleValue: string) => {
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 `<li${attributes} style="${NESTED_LIST_ITEM_MARKER_STYLE}">`;
});

const getPreviewDimension = (value: PreviewStyleValue, fallback: PreviewStyleValue): string => {
const resolvedValue = value ?? fallback;

Expand Down
21 changes: 21 additions & 0 deletions apps/web/modules/integrations/webhooks/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
});

Expand Down Expand Up @@ -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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ export const AddWebhookModal = ({
const testEndpointActionResult = await testEndpointAction({
url: testEndpointInput,
secret: webhookSecret,
workspaceId,
});

if (!testEndpointActionResult?.data) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
57 changes: 57 additions & 0 deletions apps/web/modules/storage/service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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 = {
Expand Down
Loading
Loading