fix(client): show wildcard frame-ancestors value as allow-everywhere in embed settings#42013
fix(client): show wildcard frame-ancestors value as allow-everywhere in embed settings#42013wyattwalter wants to merge 7 commits into
Conversation
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.
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".
|
/build-deploy-preview skip-tests=true |
|
/ci-test-limit |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughEmbed settings now distinguish bare ChangesEmbed frame-ancestor handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AdminSettings
participant FrameAncestorsTagInput
participant FrameAncestorUtils
participant SettingsPersistence
AdminSettings->>FrameAncestorsTagInput: render limited URL input
FrameAncestorsTagInput->>FrameAncestorUtils: remove wildcard or disable chips
FrameAncestorUtils-->>FrameAncestorsTagInput: cleaned value and validation status
FrameAncestorsTagInput-->>AdminSettings: normalized value and message
AdminSettings->>SettingsPersistence: sanitize allowed frame ancestors
SettingsPersistence-->>AdminSettings: persist limited value or remove key
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/29595080282. |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29595080322. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/client/src/ce/pages/AdminSettings/config/configuration.tsx (1)
126-147: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not return stale allow-all sources through the limit branch.
The new guard only skips
localStorage.setItem; it still returnssourcesat Line 147. A pre-existing stored value such as'self' *is therefore restored when an admin selects “Limit embedding,” and is saved as an allow-all CSP policy. Return an empty list for allow-all sources and remove the stale storage entry.Proposed fix
const sources = isUndefined(value.additionalData) ? localStorage.getItem("ALLOWED_FRAME_ANCESTORS") ?? "" : value.additionalData.replaceAll(",", " "); + const containsAllowAllSource = containsAllowAllFrameAncestor(sources); - if (!containsAllowAllFrameAncestor(sources) && sources !== "'none'") { + if (!containsAllowAllSource && sources !== "'none'") { localStorage.setItem("ALLOWED_FRAME_ANCESTORS", sources); + } else if (containsAllowAllSource) { + localStorage.removeItem("ALLOWED_FRAME_ANCESTORS"); } @@ } else { - return sources; + return containsAllowAllSource ? "" : sources; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/client/src/ce/pages/AdminSettings/config/configuration.tsx` around lines 126 - 147, Update the limit-embedding branch around containsAllowAllFrameAncestor so allow-all sources return an empty value instead of stale sources such as "'self' *". Remove the existing ALLOWED_FRAME_ANCESTORS localStorage entry when the sources contain a bare "*" or otherwise represent allow-all, while preserving normal persistence and return behavior for valid limited sources and the explicit everywhere/disabled options.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/client/src/ce/pages/AdminSettings/config/configuration.tsx`:
- Around line 126-147: Update the limit-embedding branch around
containsAllowAllFrameAncestor so allow-all sources return an empty value instead
of stale sources such as "'self' *". Remove the existing ALLOWED_FRAME_ANCESTORS
localStorage entry when the sources contain a bare "*" or otherwise represent
allow-all, while preserving normal persistence and return behavior for valid
limited sources and the explicit everywhere/disabled options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b96874b-e7ba-4bd5-95ce-773bd1f172f3
📒 Files selected for processing (5)
app/client/src/ce/constants/messages.tsapp/client/src/ce/pages/AdminSettings/config/configuration.tsxapp/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsxapp/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.tsapp/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts
|
Deploy-Preview-URL: https://ce-42013.dp.appsmith.com |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/29595080282. |
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.
|
Addressed in 499cf3e: the parse step now strips any stale bare |
|
/ci-test-limit |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/29603355190. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29603483834. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/client/src/ce/pages/AdminSettings/config/configuration.tsx`:
- Around line 130-141: Normalize the result of stripAllowAllFrameAncestorTokens
in the configuration flow so "'none'" becomes an empty limited-source value
before both localStorage handling and the LIMIT-mode return at the relevant
configuration function. Preserve host wildcards, remove non-persistable values
from storage, and add a regression test covering "* 'none'" to ensure LIMIT mode
does not return the disable-everywhere sentinel.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90ae3841-76d7-41c0-bc1f-f38c2a90858e
📒 Files selected for processing (3)
app/client/src/ce/pages/AdminSettings/config/configuration.tsxapp/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.tsapp/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts
|
Deploy-Preview-URL: https://ce-42013.dp.appsmith.com |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/29603355190. |
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'".
|
Addressed in 37c2e1b: added |
|
/ci-test-limit |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/29605071086. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/29605071086. |
|
Deploy-Preview-URL: https://ce-42013.dp.appsmith.com |
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.
|
/ci-test-limit |
|
/build-deploy-preview skip-tests=true |
|
@coderabbitai review |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/29616248467. |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29616251069. |
✅ Action performedReview finished.
|
|
Deploy-Preview-URL: https://ce-42013.dp.appsmith.com |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/29616248467. |
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.
|
/ci-test-limit |
|
/build-deploy-preview skip-tests=true |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29618272838. |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/29618273203. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx`:
- Around line 60-62: Update the disable-token utility used by
removeDisableFrameAncestorChips to canonicalize quoted CSP keywords
case-insensitively, so "'NONE'" is recognized and rejected like the bare none
token. In
app/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsx lines
60-62, ensure this utility is used for the sanitizer path; in
app/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.ts lines
261-328, add "'NONE'" coverage for the predicate, contains, removal, and
sanitization behaviors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a764fbe-8348-4f13-86d1-30044b92f62b
📒 Files selected for processing (5)
app/client/src/ce/constants/messages.tsapp/client/src/ce/pages/AdminSettings/config/configuration.test.tsxapp/client/src/pages/Applications/EmbedSnippet/FrameAncestorsTagInput.tsxapp/client/src/pages/Applications/EmbedSnippet/Utils/utils.test.tsapp/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/src/ce/constants/messages.ts
- app/client/src/ce/pages/AdminSettings/config/configuration.test.tsx
- app/client/src/pages/Applications/EmbedSnippet/Utils/utils.ts
|
Deploy-Preview-URL: https://ce-42013.dp.appsmith.com |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/29618273203. |
…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.
|
Addressed in the latest commit: |
|
/ci-test-limit |
|
/build-deploy-preview skip-tests=true |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29619614478. |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/29619615013. |
|
Deploy-Preview-URL: https://ce-42013.dp.appsmith.com |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/29619615013. |
What & why
The Admin → Configuration embed setting picks which radio to show for the stored
APPSMITH_ALLOWED_FRAME_ANCESTORSvalue. Previously only the exact string*mapped to "Allow embedding everywhere"; any other value fell through to "Limit embedding to certain URLs" and rendered its tokens as chips.In a CSP
frame-ancestorspolicy a bare*token matches every origin and overrides the other sources, so a value like the shipped default'self' *was functionally 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 while the UI reassured them it was restricted.Changes
formatEmbedSettingsnow treats any value containing a standalone*token (whitespace-split) as allow-everywhere, so an existing'self' *shows the truthful "Allow embedding everywhere" radio instead of a contradictory "Limit" state. A host wildcard likehttps://*.example.comonly matches subdomains of a specific host and is still treated as a limit-list entry.*chip is rejected with an inline message steering the admin to the "Allow embedding everywhere" radio. The check is whitespace-aware per chip, so a pasted value that arrives as a single chip (e.g.'self' *or* https://a.com) is caught too. Legitimate host wildcards such ashttps://*.example.comare preserved.parselocalStorage guard uses the same bare-*check, so an allow-all value is never remembered as a limit list.Tests
Unit tests for
formatEmbedSettingscover: exact*,'self' *and* 'self'→ allow-everywhere;'none'→ disable;'self'and'self' https://a.com→ limit;https://*.example.com→ limit (host wildcard preserved); empty → limit/empty. Additional tests cover the input guard, including pasted single-chip'self' *and* https://a.com.Scope
Client-only change. No server, Caddy, or
docker.env.shchanges.Issue
https://linear.app/appsmith/issue/APP-15353
Summary by CodeRabbit
frame-ancestorswith dedicated formatting and validation, including clearer guidance for bare*andnone/disable cases.*token anywhere as “Allow embedding everywhere”.self/nonekeyword formatting for safer, consistent saving.Warning
Tests have not run on the HEAD 19fb2b1 yet
Fri, 17 Jul 2026 23:03:04 UTC