From a7725ae860522aee2f43d39b645ed6ffbf25b4dc Mon Sep 17 00:00:00 2001 From: Wyatt Walter Date: Fri, 17 Jul 2026 15:29:48 +0000 Subject: [PATCH 1/7] fix(client): show bare "*" frame-ancestors value as allow-everywhere The Admin > Configuration embed setting decides which radio to show for a stored APPSMITH_ALLOWED_FRAME_ANCESTORS value. Previously only the exact string "*" mapped to "Allow embedding everywhere"; any other value fell to "Limit embedding to certain URLs" and rendered its tokens as chips. In a CSP frame-ancestors policy a bare "*" token matches every origin and overrides the other sources, so a value like "'self' *" is effectively allow-everywhere while the UI claimed embedding was limited. An admin could add "*" to the limit list and silently reopen the instance to all origins. - Display: formatEmbedSettings now treats any value containing a standalone "*" token (split on whitespace) as allow-everywhere, so "'self' *" shows the truthful radio. A host wildcard like "https://*.example.com" is still a limit-list entry. - Input: the limit-URLs field now rejects a bare "*" chip with an inline message steering the admin to "Allow embedding everywhere"; host wildcards are not blocked. - Round-trip: the parse localStorage guard uses the same bare-"*" check so an allow-all value is never remembered as a limit list. Adds unit tests for formatEmbedSettings covering the wildcard cases. --- app/client/src/ce/constants/messages.ts | 2 + .../AdminSettings/config/configuration.tsx | 15 ++- .../EmbedSnippet/FrameAncestorsTagInput.tsx | 71 ++++++++++++++ .../EmbedSnippet/Utils/utils.test.ts | 96 +++++++++++++++++++ .../Applications/EmbedSnippet/Utils/utils.ts | 21 +++- 5 files changed, 199 insertions(+), 6 deletions(-) create mode 100644 app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx create mode 100644 app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts diff --git a/app/client/src/ce/constants/messages.ts b/app/client/src/ce/constants/messages.ts index 93533ed3e330..e2383e10467f 100644 --- a/app/client/src/ce/constants/messages.ts +++ b/app/client/src/ce/constants/messages.ts @@ -2093,6 +2093,8 @@ export const IN_APP_EMBED_SETTING = { copied: () => "Copied", limitEmbeddingLabel: () => "Embedding restricted", limitEmbeddingTooltip: () => "This app can be embedded in approved URLs only", + limitEmbeddingBareWildcardError: () => + 'A bare "*" allows embedding on every domain. Select "Allow embedding everywhere" instead, or enter specific URLs.', disableEmbeddingLabel: () => "Embedding disabled", disableEmbeddingTooltip: () => "This app cannot be embedded anywhere on the internet", diff --git a/app/client/src/ce/pages/AdminSettings/config/configuration.tsx b/app/client/src/ce/pages/AdminSettings/config/configuration.tsx index bbab80ec5a2c..e530a0ef2b0e 100644 --- a/app/client/src/ce/pages/AdminSettings/config/configuration.tsx +++ b/app/client/src/ce/pages/AdminSettings/config/configuration.tsx @@ -9,11 +9,14 @@ import { SettingSubtype, SettingTypes, } from "ee/pages/AdminSettings/config/types"; -import { TagInput } from "@appsmith/ads-old"; import localStorage from "utils/localStorage"; import isUndefined from "lodash/isUndefined"; import { AppsmithFrameAncestorsSetting } from "pages/Applications/EmbedSnippet/Constants/constants"; -import { formatEmbedSettings } from "pages/Applications/EmbedSnippet/Utils/utils"; +import { + containsAllowAllFrameAncestor, + formatEmbedSettings, +} from "pages/Applications/EmbedSnippet/Utils/utils"; +import FrameAncestorsTagInput from "pages/Applications/EmbedSnippet/FrameAncestorsTagInput"; import { isAirgapped } from "ee/utils/airgapHelpers"; import { APPSMITH_BASE_URL_SETUP_DOC } from "constants/ThirdPartyConstants"; @@ -105,7 +108,7 @@ export const APPSMITH_ALLOWED_FRAME_ANCESTORS_SETTING: Setting = { label: "Limit embedding to certain URLs", value: AppsmithFrameAncestorsSetting.LIMIT_EMBEDDING, nodeLabel: "You can add one or more URLs", - node: , + node: , nodeInputPath: "input", nodeParentClass: "tag-input", }, @@ -125,8 +128,10 @@ export const APPSMITH_ALLOWED_FRAME_ANCESTORS_SETTING: Setting = { : value.additionalData.replaceAll(",", " "); // If they are one of the other options we don't store it in storage since it will - // set in the env variable on save - if (sources !== "*" && sources !== "'none'") { + // set in the env variable on save. A value containing a bare "*" is + // effectively allow-everywhere (see containsAllowAllFrameAncestor), so we + // never persist it as a remembered limit list. + if (!containsAllowAllFrameAncestor(sources) && sources !== "'none'") { localStorage.setItem("ALLOWED_FRAME_ANCESTORS", sources); } diff --git a/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx b/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx new file mode 100644 index 000000000000..4720b64554fa --- /dev/null +++ b/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx @@ -0,0 +1,71 @@ +import React, { useCallback, useMemo, useState } from "react"; +import styled from "styled-components"; +import { TagInput } from "@appsmith/ads-old"; +import { Text } from "@appsmith/ads"; +import { createMessage, IN_APP_EMBED_SETTING } from "ee/constants/messages"; +import { isAllowAllFrameAncestorToken } from "./Utils/utils"; + +const ErrorText = styled(Text)` + display: block; + margin-top: 4px; + color: var(--ads-v2-color-fg-error); +`; + +interface FrameAncestorsTagInputProps { + // Injected by the radio field via nodeInputPath. Values are comma-separated. + input?: { + value?: string; + onChange?: (value: string) => void; + }; + placeholder: string; + type: string; +} + +// Wraps the shared TagInput for the "Limit embedding to certain URLs" list and +// rejects a bare "*" entry. A bare "*" is not a URL: in a CSP frame-ancestors +// policy it re-opens the instance to every origin, contradicting the "limit" +// intent, so we drop it and steer the admin to the "Allow embedding everywhere" +// radio. Host wildcards like "https://*.example.com" are left untouched. +function FrameAncestorsTagInput(props: FrameAncestorsTagInputProps) { + const { input = {}, ...rest } = props; + const [error, setError] = useState(""); + const { onChange, value } = input; + + const handleChange = useCallback( + (nextValue: string) => { + const tokens = nextValue ? nextValue.split(",") : []; + const accepted = tokens.filter( + (token) => !isAllowAllFrameAncestorToken(token), + ); + + if (accepted.length !== tokens.length) { + setError( + createMessage(IN_APP_EMBED_SETTING.limitEmbeddingBareWildcardError), + ); + } else { + setError(""); + } + + onChange?.(accepted.join(",")); + }, + [onChange], + ); + + const tagInputProps = useMemo( + () => ({ value, onChange: handleChange }), + [value, handleChange], + ); + + return ( + <> + + {error && ( + + {error} + + )} + + ); +} + +export default FrameAncestorsTagInput; diff --git a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts new file mode 100644 index 000000000000..0ba51478f908 --- /dev/null +++ b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts @@ -0,0 +1,96 @@ +import { + containsAllowAllFrameAncestor, + formatEmbedSettings, + isAllowAllFrameAncestorToken, +} from "./utils"; +import { AppsmithFrameAncestorsSetting } from "../Constants/constants"; + +describe("isAllowAllFrameAncestorToken", () => { + it("matches a bare *", () => { + expect(isAllowAllFrameAncestorToken("*")).toBe(true); + expect(isAllowAllFrameAncestorToken(" * ")).toBe(true); + }); + + it("does not match host wildcards or other sources", () => { + expect(isAllowAllFrameAncestorToken("https://*.example.com")).toBe(false); + expect(isAllowAllFrameAncestorToken("'self'")).toBe(false); + expect(isAllowAllFrameAncestorToken("")).toBe(false); + }); +}); + +describe("containsAllowAllFrameAncestor", () => { + it("detects a standalone * anywhere in the list", () => { + expect(containsAllowAllFrameAncestor("*")).toBe(true); + expect(containsAllowAllFrameAncestor("'self' *")).toBe(true); + expect(containsAllowAllFrameAncestor("* 'self'")).toBe(true); + }); + + it("does not treat host wildcards as allow-everywhere", () => { + expect(containsAllowAllFrameAncestor("https://*.example.com")).toBe(false); + expect(containsAllowAllFrameAncestor("'self' https://*.example.com")).toBe( + false, + ); + }); + + it("is false for other sources and empty values", () => { + expect(containsAllowAllFrameAncestor("'self'")).toBe(false); + expect(containsAllowAllFrameAncestor("'none'")).toBe(false); + expect(containsAllowAllFrameAncestor("")).toBe(false); + expect(containsAllowAllFrameAncestor(" ")).toBe(false); + }); +}); + +describe("formatEmbedSettings", () => { + it("maps an exact * to allow-embedding-everywhere", () => { + expect(formatEmbedSettings("*")).toEqual({ + value: AppsmithFrameAncestorsSetting.ALLOW_EMBEDDING_EVERYWHERE, + }); + }); + + it("maps a value containing a bare * to allow-embedding-everywhere", () => { + // "*" overrides every other source in a CSP frame-ancestors policy, so the + // effective policy is allow-everywhere - show the truthful radio. + expect(formatEmbedSettings("'self' *")).toEqual({ + value: AppsmithFrameAncestorsSetting.ALLOW_EMBEDDING_EVERYWHERE, + }); + expect(formatEmbedSettings("* 'self'")).toEqual({ + value: AppsmithFrameAncestorsSetting.ALLOW_EMBEDDING_EVERYWHERE, + }); + }); + + it("maps 'none' to disable-embedding-everywhere", () => { + expect(formatEmbedSettings("'none'")).toEqual({ + value: AppsmithFrameAncestorsSetting.DISABLE_EMBEDDING_EVERYWHERE, + }); + }); + + it("maps a single source to limit-embedding", () => { + expect(formatEmbedSettings("'self'")).toEqual({ + value: AppsmithFrameAncestorsSetting.LIMIT_EMBEDDING, + additionalData: "'self'", + }); + }); + + it("maps multiple sources to limit-embedding with comma-separated chips", () => { + expect(formatEmbedSettings("'self' https://a.com")).toEqual({ + value: AppsmithFrameAncestorsSetting.LIMIT_EMBEDDING, + additionalData: "'self',https://a.com", + }); + }); + + it("keeps a host wildcard as a limit-embedding entry", () => { + // "https://*.example.com" only matches subdomains of example.com; it is a + // legitimate limit-list source and must NOT be read as allow-everywhere. + expect(formatEmbedSettings("https://*.example.com")).toEqual({ + value: AppsmithFrameAncestorsSetting.LIMIT_EMBEDDING, + additionalData: "https://*.example.com", + }); + }); + + it("maps an empty value to an empty limit-embedding list", () => { + expect(formatEmbedSettings("")).toEqual({ + value: AppsmithFrameAncestorsSetting.LIMIT_EMBEDDING, + additionalData: "", + }); + }); +}); diff --git a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts index 32cf74a06d88..c4d0074b8f2e 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts +++ b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts @@ -1,7 +1,26 @@ import { AppsmithFrameAncestorsSetting } from "../Constants/constants"; +// A bare "*" token is a wildcard source in a CSP frame-ancestors policy. When +// present it matches every origin and overrides every other source in the list, +// so the effective policy is "allow embedding everywhere" regardless of the +// other tokens. This is different from a host wildcard like +// "https://*.example.com", which only matches subdomains of a specific host and +// is a legitimate limit-list entry. +export const isAllowAllFrameAncestorToken = (token: string): boolean => + token.trim() === "*"; + +// True when the stored frame-ancestors value contains a standalone "*" token. +// Split on whitespace so we match a bare "*" but not a "*" embedded in a host +// pattern such as "https://*.example.com". +export const containsAllowAllFrameAncestor = (value: string): boolean => + value.trim().length > 0 && + value.trim().split(/\s+/).some(isAllowAllFrameAncestorToken); + export const formatEmbedSettings = (value: string) => { - if (value === "*") { + // A value containing a bare "*" is effectively allow-everywhere, even when it + // also lists other sources (e.g. "'self' *"). Show the truthful radio rather + // than a contradictory "limit" state. + if (containsAllowAllFrameAncestor(value)) { return { value: AppsmithFrameAncestorsSetting.ALLOW_EMBEDDING_EVERYWHERE, }; From 3c829d04064503ebfe4dc2301380aa5beb81c2ae Mon Sep 17 00:00:00 2001 From: Wyatt Walter Date: Fri, 17 Jul 2026 15:42:19 +0000 Subject: [PATCH 2/7] fix(client): reject bare "*" pasted as a single frame-ancestors chip The limit-URLs guard split the TagInput value only on commas and matched each token against the exact string "*". The shared TagInput commits a pasted value as one comma-chip, so a pasted "'self' *" or "* https://a.com" arrived as a single token: the exact-match check missed the embedded "*" and it slipped into the limit list, reopening embedding to every origin. Reject any comma-chip whose whitespace-separated tokens include a bare "*" (reusing containsAllowAllFrameAncestor via the new removeAllowAllFrameAncestorChips helper). Host wildcards like "https://*.example.com" are still accepted. Adds unit tests for the paste path, including single-chip "'self' *" and "* https://a.com". --- .../EmbedSnippet/FrameAncestorsTagInput.tsx | 22 ++++----- .../EmbedSnippet/Utils/utils.test.ts | 45 +++++++++++++++++++ .../Applications/EmbedSnippet/Utils/utils.ts | 20 +++++++++ 3 files changed, 76 insertions(+), 11 deletions(-) diff --git a/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx b/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx index 4720b64554fa..81eca9d31003 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx +++ b/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx @@ -3,7 +3,7 @@ import styled from "styled-components"; import { TagInput } from "@appsmith/ads-old"; import { Text } from "@appsmith/ads"; import { createMessage, IN_APP_EMBED_SETTING } from "ee/constants/messages"; -import { isAllowAllFrameAncestorToken } from "./Utils/utils"; +import { removeAllowAllFrameAncestorChips } from "./Utils/utils"; const ErrorText = styled(Text)` display: block; @@ -22,10 +22,12 @@ interface FrameAncestorsTagInputProps { } // Wraps the shared TagInput for the "Limit embedding to certain URLs" list and -// rejects a bare "*" entry. A bare "*" is not a URL: in a CSP frame-ancestors -// policy it re-opens the instance to every origin, contradicting the "limit" -// intent, so we drop it and steer the admin to the "Allow embedding everywhere" -// radio. Host wildcards like "https://*.example.com" are left untouched. +// rejects any chip containing a bare "*". A bare "*" is not a URL: in a CSP +// frame-ancestors policy it re-opens the instance to every origin, contradicting +// the "limit" intent, so we drop it and steer the admin to the "Allow embedding +// everywhere" radio. The check is whitespace-aware because a pasted value can +// arrive as a single chip (e.g. "'self' *"). Host wildcards like +// "https://*.example.com" are left untouched. function FrameAncestorsTagInput(props: FrameAncestorsTagInputProps) { const { input = {}, ...rest } = props; const [error, setError] = useState(""); @@ -33,12 +35,10 @@ function FrameAncestorsTagInput(props: FrameAncestorsTagInputProps) { const handleChange = useCallback( (nextValue: string) => { - const tokens = nextValue ? nextValue.split(",") : []; - const accepted = tokens.filter( - (token) => !isAllowAllFrameAncestorToken(token), - ); + const { removed, value: cleaned } = + removeAllowAllFrameAncestorChips(nextValue); - if (accepted.length !== tokens.length) { + if (removed) { setError( createMessage(IN_APP_EMBED_SETTING.limitEmbeddingBareWildcardError), ); @@ -46,7 +46,7 @@ function FrameAncestorsTagInput(props: FrameAncestorsTagInputProps) { setError(""); } - onChange?.(accepted.join(",")); + onChange?.(cleaned); }, [onChange], ); diff --git a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts index 0ba51478f908..b1f2fb04cc52 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts +++ b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts @@ -2,6 +2,7 @@ import { containsAllowAllFrameAncestor, formatEmbedSettings, isAllowAllFrameAncestorToken, + removeAllowAllFrameAncestorChips, } from "./utils"; import { AppsmithFrameAncestorsSetting } from "../Constants/constants"; @@ -94,3 +95,47 @@ describe("formatEmbedSettings", () => { }); }); }); + +describe("removeAllowAllFrameAncestorChips", () => { + it("drops a bare * committed as its own chip", () => { + expect(removeAllowAllFrameAncestorChips("*")).toEqual({ + value: "", + removed: true, + }); + expect(removeAllowAllFrameAncestorChips("'self',*")).toEqual({ + value: "'self'", + removed: true, + }); + }); + + it("drops a pasted single chip that contains a bare *", () => { + // A pasted value has no comma separator, so the whole "'self' *" arrives as + // one chip. The guard must still catch the bare "*" inside it. + expect(removeAllowAllFrameAncestorChips("'self' *")).toEqual({ + value: "", + removed: true, + }); + expect(removeAllowAllFrameAncestorChips("* https://a.com")).toEqual({ + value: "", + removed: true, + }); + }); + + it("keeps host wildcards and regular URLs", () => { + expect(removeAllowAllFrameAncestorChips("https://*.example.com")).toEqual({ + value: "https://*.example.com", + removed: false, + }); + expect(removeAllowAllFrameAncestorChips("'self',https://a.com")).toEqual({ + value: "'self',https://a.com", + removed: false, + }); + }); + + it("returns an empty result for an empty value", () => { + expect(removeAllowAllFrameAncestorChips("")).toEqual({ + value: "", + removed: false, + }); + }); +}); diff --git a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts index c4d0074b8f2e..defcd01a31d7 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts +++ b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts @@ -16,6 +16,26 @@ export const containsAllowAllFrameAncestor = (value: string): boolean => value.trim().length > 0 && value.trim().split(/\s+/).some(isAllowAllFrameAncestorToken); +// Filter the comma-separated limit list emitted by the TagInput, dropping any +// chip that contains a bare "*". Pasted content can arrive as a single chip +// holding whitespace-separated tokens (e.g. "'self' *"), so each chip is checked +// with the whitespace-aware helper rather than a strict token match - otherwise +// a bare "*" inside a pasted chip would slip into the list and silently reopen +// embedding to every origin. A host wildcard like "https://*.example.com" is +// kept. `removed` reports whether anything was dropped so the caller can surface +// an inline message. +export const removeAllowAllFrameAncestorChips = ( + value: string, +): { value: string; removed: boolean } => { + const chips = value ? value.split(",") : []; + const accepted = chips.filter((chip) => !containsAllowAllFrameAncestor(chip)); + + return { + value: accepted.join(","), + removed: accepted.length !== chips.length, + }; +}; + export const formatEmbedSettings = (value: string) => { // A value containing a bare "*" is effectively allow-everywhere, even when it // also lists other sources (e.g. "'self' *"). Show the truthful radio rather From 499cf3e5ff424d5bba65f872f721815097669325 Mon Sep 17 00:00:00 2001 From: Wyatt Walter Date: Fri, 17 Jul 2026 18:17:02 +0000 Subject: [PATCH 3/7] fix(client): strip stale bare "*" from limited frame-ancestors on save Addresses a CodeRabbit finding: the parse step returned the limited-embedding sources verbatim, so a stale "*" left in the ALLOWED_FRAME_ANCESTORS localStorage entry (from before allow-all detection existed) could round-trip back into the stored value and silently reopen embedding to every origin. Sanitize the limited list with the new stripAllowAllFrameAncestorTokens helper before returning it, and clear any stale allow-all entry from localStorage. Host wildcards like "https://*.example.com" are preserved. Adds unit tests for the helper. --- .../AdminSettings/config/configuration.tsx | 22 ++++++++++------- .../EmbedSnippet/Utils/utils.test.ts | 24 +++++++++++++++++++ .../Applications/EmbedSnippet/Utils/utils.ts | 12 ++++++++++ 3 files changed, 50 insertions(+), 8 deletions(-) diff --git a/app/client/src/ce/pages/AdminSettings/config/configuration.tsx b/app/client/src/ce/pages/AdminSettings/config/configuration.tsx index e530a0ef2b0e..cd29329c059a 100644 --- a/app/client/src/ce/pages/AdminSettings/config/configuration.tsx +++ b/app/client/src/ce/pages/AdminSettings/config/configuration.tsx @@ -13,8 +13,8 @@ import localStorage from "utils/localStorage"; import isUndefined from "lodash/isUndefined"; import { AppsmithFrameAncestorsSetting } from "pages/Applications/EmbedSnippet/Constants/constants"; import { - containsAllowAllFrameAncestor, formatEmbedSettings, + stripAllowAllFrameAncestorTokens, } from "pages/Applications/EmbedSnippet/Utils/utils"; import FrameAncestorsTagInput from "pages/Applications/EmbedSnippet/FrameAncestorsTagInput"; import { isAirgapped } from "ee/utils/airgapHelpers"; @@ -127,12 +127,18 @@ export const APPSMITH_ALLOWED_FRAME_ANCESTORS_SETTING: Setting = { ? localStorage.getItem("ALLOWED_FRAME_ANCESTORS") ?? "" : value.additionalData.replaceAll(",", " "); - // If they are one of the other options we don't store it in storage since it will - // set in the env variable on save. A value containing a bare "*" is - // effectively allow-everywhere (see containsAllowAllFrameAncestor), so we - // never persist it as a remembered limit list. - if (!containsAllowAllFrameAncestor(sources) && sources !== "'none'") { - localStorage.setItem("ALLOWED_FRAME_ANCESTORS", sources); + // Strip any bare "*" from the limited list. A stale localStorage value from + // before allow-all detection existed could still hold a "*", which would + // otherwise round-trip back into the stored value and silently reopen + // embedding to every origin. Host wildcards are preserved. + const limitedSources = stripAllowAllFrameAncestorTokens(sources); + + // Remember only real limited sources; never persist an allow-all/disabled + // value, and clear any stale allow-all entry left in storage. + if (limitedSources && limitedSources !== "'none'") { + localStorage.setItem("ALLOWED_FRAME_ANCESTORS", limitedSources); + } else { + localStorage.removeItem("ALLOWED_FRAME_ANCESTORS"); } if ( @@ -144,7 +150,7 @@ export const APPSMITH_ALLOWED_FRAME_ANCESTORS_SETTING: Setting = { ) { return "'none'"; } else { - return sources; + return limitedSources; } }, validate: (value: string) => { diff --git a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts index b1f2fb04cc52..be987c28650e 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts +++ b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts @@ -3,6 +3,7 @@ import { formatEmbedSettings, isAllowAllFrameAncestorToken, removeAllowAllFrameAncestorChips, + stripAllowAllFrameAncestorTokens, } from "./utils"; import { AppsmithFrameAncestorsSetting } from "../Constants/constants"; @@ -139,3 +140,26 @@ describe("removeAllowAllFrameAncestorChips", () => { }); }); }); + +describe("stripAllowAllFrameAncestorTokens", () => { + it("removes bare * tokens while keeping other sources", () => { + expect(stripAllowAllFrameAncestorTokens("'self' *")).toBe("'self'"); + expect(stripAllowAllFrameAncestorTokens("* 'self' https://a.com")).toBe( + "'self' https://a.com", + ); + }); + + it("returns an empty string when only a bare * is present", () => { + expect(stripAllowAllFrameAncestorTokens("*")).toBe(""); + expect(stripAllowAllFrameAncestorTokens("")).toBe(""); + }); + + it("preserves host wildcards and normal sources unchanged", () => { + expect(stripAllowAllFrameAncestorTokens("https://*.example.com")).toBe( + "https://*.example.com", + ); + expect(stripAllowAllFrameAncestorTokens("'self' https://a.com")).toBe( + "'self' https://a.com", + ); + }); +}); diff --git a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts index defcd01a31d7..469c63fe99d1 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts +++ b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts @@ -36,6 +36,18 @@ export const removeAllowAllFrameAncestorChips = ( }; }; +// Remove any bare "*" tokens from a whitespace-separated frame-ancestors value, +// keeping legitimate sources (including host wildcards like +// "https://*.example.com"). Used to sanitize a limited-embedding list so a stale +// allow-all "*" - e.g. one left in localStorage before allow-all detection +// existed - can never round-trip back into the stored value. +export const stripAllowAllFrameAncestorTokens = (value: string): string => + value + .trim() + .split(/\s+/) + .filter((token) => token.length > 0 && !isAllowAllFrameAncestorToken(token)) + .join(" "); + export const formatEmbedSettings = (value: string) => { // A value containing a bare "*" is effectively allow-everywhere, even when it // also lists other sources (e.g. "'self' *"). Show the truthful radio rather From 37c2e1b73ca160f798c391e4233e1df10dd6b855 Mon Sep 17 00:00:00 2001 From: Wyatt Walter Date: Fri, 17 Jul 2026 18:46:07 +0000 Subject: [PATCH 4/7] fix(client): normalize disable sentinel out of limited frame-ancestors Addresses a CodeRabbit finding: stripping a bare "*" from a value like "* 'none'" left the disable-everywhere sentinel "'none'", which the LIMIT branch would then emit - silently switching the instance to "Disable embedding" instead of a limit list. Add sanitizeLimitedFrameAncestors, which strips bare "*" tokens and normalizes a resulting "'none'" to empty, and use it in the parse step. Host wildcards are preserved. Adds a regression test covering "* 'none'". --- .../AdminSettings/config/configuration.tsx | 17 +++++++------ .../EmbedSnippet/Utils/utils.test.ts | 25 +++++++++++++++++++ .../Applications/EmbedSnippet/Utils/utils.ts | 11 ++++++++ 3 files changed, 45 insertions(+), 8 deletions(-) diff --git a/app/client/src/ce/pages/AdminSettings/config/configuration.tsx b/app/client/src/ce/pages/AdminSettings/config/configuration.tsx index cd29329c059a..d8b9b8f309fe 100644 --- a/app/client/src/ce/pages/AdminSettings/config/configuration.tsx +++ b/app/client/src/ce/pages/AdminSettings/config/configuration.tsx @@ -14,7 +14,7 @@ import isUndefined from "lodash/isUndefined"; import { AppsmithFrameAncestorsSetting } from "pages/Applications/EmbedSnippet/Constants/constants"; import { formatEmbedSettings, - stripAllowAllFrameAncestorTokens, + sanitizeLimitedFrameAncestors, } from "pages/Applications/EmbedSnippet/Utils/utils"; import FrameAncestorsTagInput from "pages/Applications/EmbedSnippet/FrameAncestorsTagInput"; import { isAirgapped } from "ee/utils/airgapHelpers"; @@ -127,15 +127,16 @@ export const APPSMITH_ALLOWED_FRAME_ANCESTORS_SETTING: Setting = { ? localStorage.getItem("ALLOWED_FRAME_ANCESTORS") ?? "" : value.additionalData.replaceAll(",", " "); - // Strip any bare "*" from the limited list. A stale localStorage value from - // before allow-all detection existed could still hold a "*", which would - // otherwise round-trip back into the stored value and silently reopen - // embedding to every origin. Host wildcards are preserved. - const limitedSources = stripAllowAllFrameAncestorTokens(sources); + // Sanitize the limited list: drop any bare "*" and normalize the disable + // sentinel "'none'" to empty. A stale localStorage value from before allow-all + // detection existed could still hold a "*" (or "* 'none'"), which would + // otherwise round-trip into the stored value and silently reopen embedding or + // emit the disable sentinel from LIMIT mode. Host wildcards are preserved. + const limitedSources = sanitizeLimitedFrameAncestors(sources); // Remember only real limited sources; never persist an allow-all/disabled - // value, and clear any stale allow-all entry left in storage. - if (limitedSources && limitedSources !== "'none'") { + // value, and clear any stale entry left in storage. + if (limitedSources) { localStorage.setItem("ALLOWED_FRAME_ANCESTORS", limitedSources); } else { localStorage.removeItem("ALLOWED_FRAME_ANCESTORS"); diff --git a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts index be987c28650e..3044d091f055 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts +++ b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts @@ -3,6 +3,7 @@ import { formatEmbedSettings, isAllowAllFrameAncestorToken, removeAllowAllFrameAncestorChips, + sanitizeLimitedFrameAncestors, stripAllowAllFrameAncestorTokens, } from "./utils"; import { AppsmithFrameAncestorsSetting } from "../Constants/constants"; @@ -163,3 +164,27 @@ describe("stripAllowAllFrameAncestorTokens", () => { ); }); }); + +describe("sanitizeLimitedFrameAncestors", () => { + it("normalizes the disable sentinel to empty so LIMIT mode never emits it", () => { + // Stripping "*" from "* 'none'" would leave "'none'"; LIMIT mode must not + // return the disable-everywhere sentinel. + expect(sanitizeLimitedFrameAncestors("* 'none'")).toBe(""); + expect(sanitizeLimitedFrameAncestors("'none'")).toBe(""); + }); + + it("drops bare * tokens like the underlying strip", () => { + expect(sanitizeLimitedFrameAncestors("*")).toBe(""); + expect(sanitizeLimitedFrameAncestors("* 'self'")).toBe("'self'"); + expect(sanitizeLimitedFrameAncestors("")).toBe(""); + }); + + it("preserves host wildcards and normal sources", () => { + expect(sanitizeLimitedFrameAncestors("https://*.example.com")).toBe( + "https://*.example.com", + ); + expect(sanitizeLimitedFrameAncestors("'self' https://a.com")).toBe( + "'self' https://a.com", + ); + }); +}); diff --git a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts index 469c63fe99d1..2a3ca6940d1c 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts +++ b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts @@ -48,6 +48,17 @@ export const stripAllowAllFrameAncestorTokens = (value: string): string => .filter((token) => token.length > 0 && !isAllowAllFrameAncestorToken(token)) .join(" "); +// Sanitize a whitespace-separated value for use as a limited-embedding list: +// drop bare "*" tokens and normalize the disable-everywhere sentinel "'none'" to +// empty, since neither is a valid limit-list source. Stripping "*" from a value +// like "* 'none'" would otherwise leave the "'none'" sentinel, which LIMIT mode +// must never emit. Host wildcards such as "https://*.example.com" are preserved. +export const sanitizeLimitedFrameAncestors = (value: string): string => { + const stripped = stripAllowAllFrameAncestorTokens(value); + + return stripped === "'none'" ? "" : stripped; +}; + export const formatEmbedSettings = (value: string) => { // A value containing a bare "*" is effectively allow-everywhere, even when it // also lists other sources (e.g. "'self' *"). Show the truthful radio rather From 716ef7463e7758ffe318648d5687d3a3f9defc97 Mon Sep 17 00:00:00 2001 From: Wyatt Walter Date: Fri, 17 Jul 2026 21:54:40 +0000 Subject: [PATCH 5/7] fix(client): guard cold-load crash and quote self/none in embed settings Two issues found while manual-testing the embed settings on the deploy preview. 1. Hard-refresh / direct load of /settings/configuration crashed to the error boundary. On a cold bootstrap the admin-settings store is not yet hydrated, so redux-form calls the frame-ancestors field's format() with undefined. The allow-all detection this PR added (containsAllowAllFrameAncestor) called value.trim() unconditionally and threw "Cannot read properties of undefined (reading 'trim')". The original formatEmbedSettings tolerated undefined, so this was a regression. Make the helpers null-safe so format(undefined) behaves like an empty limit list again. In-app navigation was unaffected because the store is already populated by then. 2. The limit-list input persisted a bare unquoted "self"/"none". In a CSP frame-ancestors policy those keywords must be quoted; a bare "self" is parsed as a hostname and silently breaks the policy. Normalize bare "self"/"none" to the quoted "'self'"/"'none'" form both in the tag input (immediate feedback) and in the parse/save path (the guarantee), so the stored value is never raw. Host/scheme sources and wildcards stay unquoted. Adds unit tests for the null-safe helpers and keyword normalization, plus an integration test that renders the real frame-ancestors radio with an unhydrated value (reproducing the crash) and asserts the stored value never contains a bare self/none. --- .../config/configuration.test.tsx | 91 +++++++++++++++++++ .../EmbedSnippet/FrameAncestorsTagInput.tsx | 23 ++++- .../EmbedSnippet/Utils/utils.test.ts | 76 ++++++++++++++++ .../Applications/EmbedSnippet/Utils/utils.ts | 52 ++++++++--- 4 files changed, 228 insertions(+), 14 deletions(-) create mode 100644 app/client/src/ce/pages/AdminSettings/config/configuration.test.tsx diff --git a/app/client/src/ce/pages/AdminSettings/config/configuration.test.tsx b/app/client/src/ce/pages/AdminSettings/config/configuration.test.tsx new file mode 100644 index 000000000000..4fe4d284f7da --- /dev/null +++ b/app/client/src/ce/pages/AdminSettings/config/configuration.test.tsx @@ -0,0 +1,91 @@ +import { render } from "test/testUtils"; +import React from "react"; +import Radio from "pages/AdminSettings/FormGroup/Radio"; +import { SETTINGS_FORM_NAME } from "ee/constants/forms"; +import { reduxForm } from "redux-form"; +import { AppsmithFrameAncestorsSetting } from "pages/Applications/EmbedSnippet/Constants/constants"; +import { APPSMITH_ALLOWED_FRAME_ANCESTORS_SETTING } from "./configuration"; + +// Render the real frame-ancestors radio setting with the given form value. +// Passing `undefined` simulates a cold bootstrap of /settings/configuration, +// where the admin-settings store is not yet hydrated and redux-form calls the +// field's format() with undefined. +function renderFrameAncestorsRadio(value: string | undefined) { + function FrameAncestorsRadio() { + return ; + } + + // TODO: Fix this the next time the file is edited + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const Parent = reduxForm({ + validate: () => ({}), + form: SETTINGS_FORM_NAME, + touchOnBlur: true, + })(FrameAncestorsRadio); + + return render(, { + initialState: { + form: { + [SETTINGS_FORM_NAME]: { + values: + value === undefined + ? {} + : { [APPSMITH_ALLOWED_FRAME_ANCESTORS_SETTING.id]: value }, + }, + }, + }, + }); +} + +describe("APPSMITH_ALLOWED_FRAME_ANCESTORS_SETTING radio", () => { + it("renders without crashing when the setting is not yet hydrated (cold bootstrap)", () => { + // Regression: on a hard refresh of /settings/configuration the value is + // undefined, so format() must tolerate it instead of throwing and taking the + // whole settings page to the error boundary. + expect(() => renderFrameAncestorsRadio(undefined)).not.toThrow(); + expect(document.querySelectorAll("input[type=radio]").length).toBe(3); + }); + + it("selects Allow embedding everywhere for a value containing a bare *", () => { + renderFrameAncestorsRadio("'self' *"); + + const radios = + document.querySelectorAll("input[type=radio]"); + + // Options render in config order: allow / limit / disable. + expect(radios[0].value).toBe( + AppsmithFrameAncestorsSetting.ALLOW_EMBEDDING_EVERYWHERE, + ); + expect(radios[0].checked).toBe(true); + }); +}); + +describe("APPSMITH_ALLOWED_FRAME_ANCESTORS_SETTING parse (stored value)", () => { + // The parse step is what actually writes the env value on save. + const parse = APPSMITH_ALLOWED_FRAME_ANCESTORS_SETTING.parse as (v: { + value: string; + additionalData?: string; + }) => string; + + it("quotes a bare self typed in the limit list so it is never persisted raw", () => { + expect( + parse({ + value: AppsmithFrameAncestorsSetting.LIMIT_EMBEDDING, + additionalData: "self,https://a.com", + }), + ).toBe("'self' https://a.com"); + }); + + it("stores the quoted keywords for the allow/disable options", () => { + expect( + parse({ + value: AppsmithFrameAncestorsSetting.ALLOW_EMBEDDING_EVERYWHERE, + }), + ).toBe("*"); + expect( + parse({ + value: AppsmithFrameAncestorsSetting.DISABLE_EMBEDDING_EVERYWHERE, + }), + ).toBe("'none'"); + }); +}); diff --git a/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx b/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx index 81eca9d31003..9c7b208cd3b8 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx +++ b/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx @@ -3,7 +3,26 @@ import styled from "styled-components"; import { TagInput } from "@appsmith/ads-old"; import { Text } from "@appsmith/ads"; import { createMessage, IN_APP_EMBED_SETTING } from "ee/constants/messages"; -import { removeAllowAllFrameAncestorChips } from "./Utils/utils"; +import { + normalizeFrameAncestorToken, + removeAllowAllFrameAncestorChips, +} from "./Utils/utils"; + +// Quote bare "self"/"none" keywords in each comma-separated chip so the chip the +// user sees matches what is stored. Chips can hold whitespace-separated tokens +// (pasted values), so normalize per token. +const normalizeChips = (value: string): string => + value + .split(",") + .filter(Boolean) + .map((chip) => + chip + .split(/\s+/) + .filter(Boolean) + .map(normalizeFrameAncestorToken) + .join(" "), + ) + .join(","); const ErrorText = styled(Text)` display: block; @@ -46,7 +65,7 @@ function FrameAncestorsTagInput(props: FrameAncestorsTagInputProps) { setError(""); } - onChange?.(cleaned); + onChange?.(normalizeChips(cleaned)); }, [onChange], ); diff --git a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts index 3044d091f055..3652c05dcd57 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts +++ b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts @@ -2,6 +2,7 @@ import { containsAllowAllFrameAncestor, formatEmbedSettings, isAllowAllFrameAncestorToken, + normalizeFrameAncestorToken, removeAllowAllFrameAncestorChips, sanitizeLimitedFrameAncestors, stripAllowAllFrameAncestorTokens, @@ -19,6 +20,13 @@ describe("isAllowAllFrameAncestorToken", () => { expect(isAllowAllFrameAncestorToken("'self'")).toBe(false); expect(isAllowAllFrameAncestorToken("")).toBe(false); }); + + it("tolerates a null/undefined token", () => { + expect(isAllowAllFrameAncestorToken(undefined as unknown as string)).toBe( + false, + ); + expect(isAllowAllFrameAncestorToken(null as unknown as string)).toBe(false); + }); }); describe("containsAllowAllFrameAncestor", () => { @@ -41,6 +49,15 @@ describe("containsAllowAllFrameAncestor", () => { expect(containsAllowAllFrameAncestor("")).toBe(false); expect(containsAllowAllFrameAncestor(" ")).toBe(false); }); + + it("tolerates a null/undefined value without throwing", () => { + expect(containsAllowAllFrameAncestor(undefined as unknown as string)).toBe( + false, + ); + expect(containsAllowAllFrameAncestor(null as unknown as string)).toBe( + false, + ); + }); }); describe("formatEmbedSettings", () => { @@ -96,6 +113,20 @@ describe("formatEmbedSettings", () => { additionalData: "", }); }); + + it("tolerates an unhydrated (undefined/null) value without throwing", () => { + // On a cold load of /settings/configuration the admin-settings store is not + // yet populated, so redux-form calls format() with undefined. This must not + // crash the settings page - it should behave like an empty limit list. + expect(formatEmbedSettings(undefined as unknown as string)).toEqual({ + value: AppsmithFrameAncestorsSetting.LIMIT_EMBEDDING, + additionalData: "", + }); + expect(formatEmbedSettings(null as unknown as string)).toEqual({ + value: AppsmithFrameAncestorsSetting.LIMIT_EMBEDDING, + additionalData: "", + }); + }); }); describe("removeAllowAllFrameAncestorChips", () => { @@ -155,6 +186,15 @@ describe("stripAllowAllFrameAncestorTokens", () => { expect(stripAllowAllFrameAncestorTokens("")).toBe(""); }); + it("tolerates a null/undefined value", () => { + expect( + stripAllowAllFrameAncestorTokens(undefined as unknown as string), + ).toBe(""); + expect(stripAllowAllFrameAncestorTokens(null as unknown as string)).toBe( + "", + ); + }); + it("preserves host wildcards and normal sources unchanged", () => { expect(stripAllowAllFrameAncestorTokens("https://*.example.com")).toBe( "https://*.example.com", @@ -187,4 +227,40 @@ describe("sanitizeLimitedFrameAncestors", () => { "'self' https://a.com", ); }); + + it("quotes bare self/none keywords so they are never persisted raw", () => { + // A bare "self" is parsed as a hostname and breaks the CSP policy, so it must + // be stored as the quoted "'self'" keyword. + expect(sanitizeLimitedFrameAncestors("self")).toBe("'self'"); + expect(sanitizeLimitedFrameAncestors("self https://a.com")).toBe( + "'self' https://a.com", + ); + expect(sanitizeLimitedFrameAncestors("SELF *")).toBe("'self'"); + // A lone bare "none" normalizes to the disable sentinel, which LIMIT mode + // drops to empty rather than persisting. + expect(sanitizeLimitedFrameAncestors("none")).toBe(""); + }); +}); + +describe("normalizeFrameAncestorToken", () => { + it("quotes the bare self/none keywords (case-insensitive)", () => { + expect(normalizeFrameAncestorToken("self")).toBe("'self'"); + expect(normalizeFrameAncestorToken(" SELF ")).toBe("'self'"); + expect(normalizeFrameAncestorToken("none")).toBe("'none'"); + expect(normalizeFrameAncestorToken("None")).toBe("'none'"); + }); + + it("leaves already-quoted keywords unchanged (idempotent)", () => { + expect(normalizeFrameAncestorToken("'self'")).toBe("'self'"); + expect(normalizeFrameAncestorToken("'none'")).toBe("'none'"); + }); + + it("leaves host/scheme sources and wildcards unquoted", () => { + expect(normalizeFrameAncestorToken("https://a.com")).toBe("https://a.com"); + expect(normalizeFrameAncestorToken("https://*.example.com")).toBe( + "https://*.example.com", + ); + expect(normalizeFrameAncestorToken("*")).toBe("*"); + expect(normalizeFrameAncestorToken("")).toBe(""); + }); }); diff --git a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts index 2a3ca6940d1c..d79d19331f78 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts +++ b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts @@ -7,14 +7,21 @@ import { AppsmithFrameAncestorsSetting } from "../Constants/constants"; // "https://*.example.com", which only matches subdomains of a specific host and // is a legitimate limit-list entry. export const isAllowAllFrameAncestorToken = (token: string): boolean => - token.trim() === "*"; + (token ?? "").trim() === "*"; // True when the stored frame-ancestors value contains a standalone "*" token. // Split on whitespace so we match a bare "*" but not a "*" embedded in a host -// pattern such as "https://*.example.com". -export const containsAllowAllFrameAncestor = (value: string): boolean => - value.trim().length > 0 && - value.trim().split(/\s+/).some(isAllowAllFrameAncestorToken); +// pattern such as "https://*.example.com". Tolerates a null/undefined value: +// on a cold load of /settings/configuration the admin-settings store is not yet +// hydrated, so this runs before the value exists. +export const containsAllowAllFrameAncestor = (value: string): boolean => { + const trimmed = (value ?? "").trim(); + + return ( + trimmed.length > 0 && + trimmed.split(/\s+/).some(isAllowAllFrameAncestorToken) + ); +}; // Filter the comma-separated limit list emitted by the TagInput, dropping any // chip that contains a bare "*". Pasted content can arrive as a single chip @@ -42,21 +49,42 @@ export const removeAllowAllFrameAncestorChips = ( // allow-all "*" - e.g. one left in localStorage before allow-all detection // existed - can never round-trip back into the stored value. export const stripAllowAllFrameAncestorTokens = (value: string): string => - value + (value ?? "") .trim() .split(/\s+/) .filter((token) => token.length > 0 && !isAllowAllFrameAncestorToken(token)) .join(" "); +// Quote the CSP keyword sources. In a frame-ancestors policy "'self'" and +// "'none'" must be single-quoted; a bare "self"/"none" is parsed by the browser +// as a hostname and silently breaks the policy. Normalize them to the quoted +// form (case-insensitive). Host/scheme sources and wildcards +// ("https://x.com", "*.example.com", "*") must stay unquoted and are left as-is. +export const normalizeFrameAncestorToken = (token: string): string => { + const trimmed = (token ?? "").trim(); + const lower = trimmed.toLowerCase(); + + if (lower === "self") return "'self'"; + + if (lower === "none") return "'none'"; + + return trimmed; +}; + // Sanitize a whitespace-separated value for use as a limited-embedding list: -// drop bare "*" tokens and normalize the disable-everywhere sentinel "'none'" to -// empty, since neither is a valid limit-list source. Stripping "*" from a value -// like "* 'none'" would otherwise leave the "'none'" sentinel, which LIMIT mode -// must never emit. Host wildcards such as "https://*.example.com" are preserved. +// drop bare "*" tokens, quote bare "self"/"none" keywords, and normalize the +// disable-everywhere sentinel "'none'" to empty (neither "*" nor "'none'" is a +// valid limit-list source). Stripping "*" from a value like "* 'none'" would +// otherwise leave the "'none'" sentinel, which LIMIT mode must never emit. Host +// wildcards such as "https://*.example.com" are preserved. export const sanitizeLimitedFrameAncestors = (value: string): string => { - const stripped = stripAllowAllFrameAncestorTokens(value); + const normalized = stripAllowAllFrameAncestorTokens(value) + .split(/\s+/) + .filter((token) => token.length > 0) + .map(normalizeFrameAncestorToken) + .join(" "); - return stripped === "'none'" ? "" : stripped; + return normalized === "'none'" ? "" : normalized; }; export const formatEmbedSettings = (value: string) => { From 1befe84184d1b2578fb122ac9b581df1f744494c Mon Sep 17 00:00:00 2001 From: Wyatt Walter Date: Fri, 17 Jul 2026 22:33:42 +0000 Subject: [PATCH 6/7] fix(client): never persist 'none' alongside allow-list frame-ancestors Follow-up to the embed-settings fix. In a CSP frame-ancestors policy "'none'" is an exclusive source: alongside other sources the others are ignored. A LIMIT-mode value like "none https://trusted.example" normalized to "'none' https://trusted.example" and was persisted verbatim, so the UI said "Limit embedding to certain URLs" while the saved policy actually disabled embedding entirely - the same silent-contradiction footgun this PR set out to fix. The tag-input paste path had the same problem. Mirror the existing bare-"*" handling (which the review confirmed is correct): - Save path: sanitizeLimitedFrameAncestors now strips every normalized "'none'" token from the list (not just the exact singleton), so "none https://a.com" saves as "https://a.com" and "'none'" can never combine with other sources. - Tag input: reject a chip containing a "none"/"'none'" keyword with an inline message steering the admin to the "Disable embedding everywhere" radio, exactly as a bare "*" is rejected toward "Allow embedding everywhere". Adds unit tests for the disable-keyword helpers, the paste/reject path, and the save-path strip (including "none https://a.com"), plus a parse-level test. The cold-bootstrap null-safety and bare-self/"*" behavior are unchanged and stay green. --- app/client/src/ce/constants/messages.ts | 2 + .../config/configuration.test.tsx | 11 +++ .../EmbedSnippet/FrameAncestorsTagInput.tsx | 26 ++++-- .../EmbedSnippet/Utils/utils.test.ts | 85 +++++++++++++++++++ .../Applications/EmbedSnippet/Utils/utils.ts | 51 ++++++++--- 5 files changed, 156 insertions(+), 19 deletions(-) diff --git a/app/client/src/ce/constants/messages.ts b/app/client/src/ce/constants/messages.ts index e2383e10467f..3a36ee3e489d 100644 --- a/app/client/src/ce/constants/messages.ts +++ b/app/client/src/ce/constants/messages.ts @@ -2095,6 +2095,8 @@ export const IN_APP_EMBED_SETTING = { limitEmbeddingTooltip: () => "This app can be embedded in approved URLs only", limitEmbeddingBareWildcardError: () => 'A bare "*" allows embedding on every domain. Select "Allow embedding everywhere" instead, or enter specific URLs.', + limitEmbeddingDisableKeywordError: () => + '"none" disables embedding everywhere and cannot be combined with URLs. Select "Disable embedding everywhere" instead, or enter specific URLs.', disableEmbeddingLabel: () => "Embedding disabled", disableEmbeddingTooltip: () => "This app cannot be embedded anywhere on the internet", diff --git a/app/client/src/ce/pages/AdminSettings/config/configuration.test.tsx b/app/client/src/ce/pages/AdminSettings/config/configuration.test.tsx index 4fe4d284f7da..c75a7fe787bd 100644 --- a/app/client/src/ce/pages/AdminSettings/config/configuration.test.tsx +++ b/app/client/src/ce/pages/AdminSettings/config/configuration.test.tsx @@ -76,6 +76,17 @@ describe("APPSMITH_ALLOWED_FRAME_ANCESTORS_SETTING parse (stored value)", () => ).toBe("'self' https://a.com"); }); + it("never combines 'none' with allow-list sources when saving", () => { + // "'none'" is exclusive in CSP; persisting "'none' https://a.com" would + // silently disable embedding despite the "limit" selection. + expect( + parse({ + value: AppsmithFrameAncestorsSetting.LIMIT_EMBEDDING, + additionalData: "none,https://a.com", + }), + ).toBe("https://a.com"); + }); + it("stores the quoted keywords for the allow/disable options", () => { expect( parse({ diff --git a/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx b/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx index 9c7b208cd3b8..2d6f5da9a9ef 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx +++ b/app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx @@ -6,6 +6,7 @@ import { createMessage, IN_APP_EMBED_SETTING } from "ee/constants/messages"; import { normalizeFrameAncestorToken, removeAllowAllFrameAncestorChips, + removeDisableFrameAncestorChips, } from "./Utils/utils"; // Quote bare "self"/"none" keywords in each comma-separated chip so the chip the @@ -41,11 +42,12 @@ interface FrameAncestorsTagInputProps { } // Wraps the shared TagInput for the "Limit embedding to certain URLs" list and -// rejects any chip containing a bare "*". A bare "*" is not a URL: in a CSP -// frame-ancestors policy it re-opens the instance to every origin, contradicting -// the "limit" intent, so we drop it and steer the admin to the "Allow embedding -// everywhere" radio. The check is whitespace-aware because a pasted value can -// arrive as a single chip (e.g. "'self' *"). Host wildcards like +// rejects any chip containing a keyword source that contradicts the "limit" +// intent: a bare "*" (belongs to "Allow embedding everywhere") or a +// "none"/"'none'" (belongs to "Disable embedding everywhere" - in CSP "'none'" +// is exclusive and would silently disable embedding if combined with URLs). Both +// checks are whitespace-aware because a pasted value can arrive as a single chip +// (e.g. "'self' *" or "none https://a.com"). Host wildcards like // "https://*.example.com" are left untouched. function FrameAncestorsTagInput(props: FrameAncestorsTagInputProps) { const { input = {}, ...rest } = props; @@ -54,18 +56,24 @@ function FrameAncestorsTagInput(props: FrameAncestorsTagInputProps) { const handleChange = useCallback( (nextValue: string) => { - const { removed, value: cleaned } = - removeAllowAllFrameAncestorChips(nextValue); + const withoutWildcard = removeAllowAllFrameAncestorChips(nextValue); + const withoutDisable = removeDisableFrameAncestorChips( + withoutWildcard.value, + ); - if (removed) { + if (withoutWildcard.removed) { setError( createMessage(IN_APP_EMBED_SETTING.limitEmbeddingBareWildcardError), ); + } else if (withoutDisable.removed) { + setError( + createMessage(IN_APP_EMBED_SETTING.limitEmbeddingDisableKeywordError), + ); } else { setError(""); } - onChange?.(normalizeChips(cleaned)); + onChange?.(normalizeChips(withoutDisable.value)); }, [onChange], ); diff --git a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts index 3652c05dcd57..c52fdf4c112a 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts +++ b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts @@ -1,9 +1,12 @@ import { containsAllowAllFrameAncestor, + containsDisableFrameAncestor, formatEmbedSettings, isAllowAllFrameAncestorToken, + isDisableFrameAncestorToken, normalizeFrameAncestorToken, removeAllowAllFrameAncestorChips, + removeDisableFrameAncestorChips, sanitizeLimitedFrameAncestors, stripAllowAllFrameAncestorTokens, } from "./utils"; @@ -240,6 +243,88 @@ describe("sanitizeLimitedFrameAncestors", () => { // drops to empty rather than persisting. expect(sanitizeLimitedFrameAncestors("none")).toBe(""); }); + + it("strips a none/'none' token combined with allow-list sources", () => { + // In CSP "'none'" is exclusive, so "none https://a.com" must never persist as + // "'none' https://a.com" (which would silently disable embedding). The + // "'none'" is stripped so the allow-list saves as intended. + expect(sanitizeLimitedFrameAncestors("none https://a.com")).toBe( + "https://a.com", + ); + expect(sanitizeLimitedFrameAncestors("'none' https://a.com 'self'")).toBe( + "https://a.com 'self'", + ); + expect(sanitizeLimitedFrameAncestors("* 'none'")).toBe(""); + }); +}); + +describe("isDisableFrameAncestorToken", () => { + it("matches bare and quoted none (case-insensitive)", () => { + expect(isDisableFrameAncestorToken("none")).toBe(true); + expect(isDisableFrameAncestorToken("NONE")).toBe(true); + expect(isDisableFrameAncestorToken("'none'")).toBe(true); + }); + + it("does not match other sources", () => { + expect(isDisableFrameAncestorToken("'self'")).toBe(false); + expect(isDisableFrameAncestorToken("https://a.com")).toBe(false); + expect(isDisableFrameAncestorToken("*")).toBe(false); + expect(isDisableFrameAncestorToken("")).toBe(false); + }); +}); + +describe("containsDisableFrameAncestor", () => { + it("detects a none/'none' token anywhere in the list", () => { + expect(containsDisableFrameAncestor("none")).toBe(true); + expect(containsDisableFrameAncestor("none https://a.com")).toBe(true); + expect(containsDisableFrameAncestor("'self' 'none'")).toBe(true); + }); + + it("is false for lists without a disable keyword", () => { + expect(containsDisableFrameAncestor("'self' https://a.com")).toBe(false); + expect(containsDisableFrameAncestor("https://*.example.com")).toBe(false); + expect(containsDisableFrameAncestor("")).toBe(false); + expect(containsDisableFrameAncestor(undefined as unknown as string)).toBe( + false, + ); + }); +}); + +describe("removeDisableFrameAncestorChips", () => { + it("drops a none chip committed on its own", () => { + expect(removeDisableFrameAncestorChips("none")).toEqual({ + value: "", + removed: true, + }); + expect(removeDisableFrameAncestorChips("'self',none")).toEqual({ + value: "'self'", + removed: true, + }); + }); + + it("drops a pasted single chip that contains a disable keyword", () => { + // Pasting "none https://a.com" arrives as one chip; it must be rejected so + // "'none'" is never combined with allow-list sources. + expect(removeDisableFrameAncestorChips("none https://a.com")).toEqual({ + value: "", + removed: true, + }); + expect(removeDisableFrameAncestorChips("'none' https://a.com")).toEqual({ + value: "", + removed: true, + }); + }); + + it("keeps allow-list sources untouched", () => { + expect(removeDisableFrameAncestorChips("'self',https://a.com")).toEqual({ + value: "'self',https://a.com", + removed: false, + }); + expect(removeDisableFrameAncestorChips("")).toEqual({ + value: "", + removed: false, + }); + }); }); describe("normalizeFrameAncestorToken", () => { diff --git a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts index d79d19331f78..ec69a29891bc 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts +++ b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts @@ -71,22 +71,53 @@ export const normalizeFrameAncestorToken = (token: string): string => { return trimmed; }; +// True when a single token is the disable-everywhere keyword ("none"/"'none'", +// case-insensitive). +export const isDisableFrameAncestorToken = (token: string): boolean => + normalizeFrameAncestorToken(token) === "'none'"; + +// True when a value contains a disable keyword token. In a CSP frame-ancestors +// policy "'none'" is exclusive: alongside other sources it disables embedding +// entirely, so it must never sit in a limit list. +export const containsDisableFrameAncestor = (value: string): boolean => { + const trimmed = (value ?? "").trim(); + + return ( + trimmed.length > 0 && trimmed.split(/\s+/).some(isDisableFrameAncestorToken) + ); +}; + +// Filter the comma-separated limit list emitted by the TagInput, dropping any +// chip that contains a disable keyword ("none"/"'none'"). Mirrors +// removeAllowAllFrameAncestorChips; the caller steers the admin to the "Disable +// embedding everywhere" radio. `removed` reports whether anything was dropped. +export const removeDisableFrameAncestorChips = ( + value: string, +): { value: string; removed: boolean } => { + const chips = value ? value.split(",") : []; + const accepted = chips.filter((chip) => !containsDisableFrameAncestor(chip)); + + return { + value: accepted.join(","), + removed: accepted.length !== chips.length, + }; +}; + // Sanitize a whitespace-separated value for use as a limited-embedding list: -// drop bare "*" tokens, quote bare "self"/"none" keywords, and normalize the -// disable-everywhere sentinel "'none'" to empty (neither "*" nor "'none'" is a -// valid limit-list source). Stripping "*" from a value like "* 'none'" would -// otherwise leave the "'none'" sentinel, which LIMIT mode must never emit. Host -// wildcards such as "https://*.example.com" are preserved. -export const sanitizeLimitedFrameAncestors = (value: string): string => { - const normalized = stripAllowAllFrameAncestorTokens(value) +// drop bare "*" tokens, quote bare "self"/"none" keywords, and strip every +// "'none'" token. Neither "*" nor "'none'" is a valid limit-list source, and +// because "'none'" is exclusive in CSP a value like "none https://a.com" must +// not persist as "'none' https://a.com" (which would silently disable embedding); +// the "'none'" is removed so the allow-list saves as intended. Host wildcards +// such as "https://*.example.com" are preserved. +export const sanitizeLimitedFrameAncestors = (value: string): string => + stripAllowAllFrameAncestorTokens(value) .split(/\s+/) .filter((token) => token.length > 0) .map(normalizeFrameAncestorToken) + .filter((token) => token !== "'none'") .join(" "); - return normalized === "'none'" ? "" : normalized; -}; - export const formatEmbedSettings = (value: string) => { // A value containing a bare "*" is effectively allow-everywhere, even when it // also lists other sources (e.g. "'self' *"). Show the truthful radio rather From 19fb2b16e77ad92a58200670aea14dd2dfafa0aa Mon Sep 17 00:00:00 2001 From: Wyatt Walter Date: Fri, 17 Jul 2026 23:02:34 +0000 Subject: [PATCH 7/7] fix(client): canonicalize quoted uppercase self/none frame-ancestors keywords CodeRabbit follow-up: CSP keywords are case-insensitive and users may type them already quoted, but normalizeFrameAncestorToken only matched the bare lowercase forms. A quoted-uppercase "'NONE'" was therefore not recognized as the disable keyword, so "'NONE' https://a.com" could slip past the reject/strip and persist - the same exclusive-'none' footgun. Match the quoted forms too ("'self'"/"'none'" case-insensitively) so every disable variant is canonicalized and stripped/rejected. Extends the existing tests with "'NONE'"/"'SELF'" coverage across the predicate, contains, chip-removal, and sanitizer paths. --- .../Applications/EmbedSnippet/Utils/utils.test.ts | 14 +++++++++++++- .../pages/Applications/EmbedSnippet/Utils/utils.ts | 6 ++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts index c52fdf4c112a..965a8167a112 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts +++ b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts @@ -255,6 +255,10 @@ describe("sanitizeLimitedFrameAncestors", () => { "https://a.com 'self'", ); expect(sanitizeLimitedFrameAncestors("* 'none'")).toBe(""); + // Case-insensitive quoted keyword must be stripped too. + expect(sanitizeLimitedFrameAncestors("'NONE' https://a.com")).toBe( + "https://a.com", + ); }); }); @@ -263,6 +267,7 @@ describe("isDisableFrameAncestorToken", () => { expect(isDisableFrameAncestorToken("none")).toBe(true); expect(isDisableFrameAncestorToken("NONE")).toBe(true); expect(isDisableFrameAncestorToken("'none'")).toBe(true); + expect(isDisableFrameAncestorToken("'NONE'")).toBe(true); }); it("does not match other sources", () => { @@ -313,6 +318,11 @@ describe("removeDisableFrameAncestorChips", () => { value: "", removed: true, }); + // Quoted-uppercase keyword is still a case-insensitive CSP "'none'". + expect(removeDisableFrameAncestorChips("'NONE' https://a.com")).toEqual({ + value: "", + removed: true, + }); }); it("keeps allow-list sources untouched", () => { @@ -335,9 +345,11 @@ describe("normalizeFrameAncestorToken", () => { expect(normalizeFrameAncestorToken("None")).toBe("'none'"); }); - it("leaves already-quoted keywords unchanged (idempotent)", () => { + it("canonicalizes already-quoted keywords, including uppercase", () => { expect(normalizeFrameAncestorToken("'self'")).toBe("'self'"); expect(normalizeFrameAncestorToken("'none'")).toBe("'none'"); + expect(normalizeFrameAncestorToken("'SELF'")).toBe("'self'"); + expect(normalizeFrameAncestorToken("'NONE'")).toBe("'none'"); }); it("leaves host/scheme sources and wildcards unquoted", () => { diff --git a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts index ec69a29891bc..399507e43f69 100644 --- a/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts +++ b/app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts @@ -62,11 +62,13 @@ export const stripAllowAllFrameAncestorTokens = (value: string): string => // ("https://x.com", "*.example.com", "*") must stay unquoted and are left as-is. export const normalizeFrameAncestorToken = (token: string): string => { const trimmed = (token ?? "").trim(); + // CSP keywords are case-insensitive, and users may type them already quoted, so + // canonicalize both the bare and quoted forms (e.g. "NONE" and "'NONE'"). const lower = trimmed.toLowerCase(); - if (lower === "self") return "'self'"; + if (lower === "self" || lower === "'self'") return "'self'"; - if (lower === "none") return "'none'"; + if (lower === "none" || lower === "'none'") return "'none'"; return trimmed; };