fix: sandboxed iFrames with srcDoc #11426
Conversation
Change iFrame widgets to use sandbox mode if srcDoc is provided Allowed options: `allow-forms allow-modals allow-orientation-lock allow-pointer-lock allow-popups allow-scripts allow-top-navigation-by-user-activation`
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/get-appsmith/appsmith/E9HPKoUc2LewNPmFQ7TpBX9k3A55 |
riodeuno
changed the title
|
Unable to find test scripts. Please add necessary tests to the PR. |
|
Unable to find test scripts. Please add necessary tests to the PR. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
# Conflicts: # app/client/src/widgets/IframeWidget/component/index.tsx
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Unable to find test scripts. Please add necessary tests to the PR. |
|
@riodeuno @mohanarpit, could the two of you take a look at this PR please? I'd like to merge this in soon so we can start working on adding a UI for this setting in Admin Settings. |
| disableIframeWidgetSandbox: process.env | ||
| .APPSMITH_DISABLE_IFRAME_WIDGET_SANDBOX | ||
| ? process.env.APPSMITH_DISABLE_IFRAME_WIDGET_SANDBOX.length > 0 | ||
| : false, |
There was a problem hiding this comment.
Prettier wouldn't be happy until I wrote it like this. Is there a better way to express this, or is this okay?
There was a problem hiding this comment.
This is ok, as we're only making sure that the value is a string or array with length greater than zero.
There was a problem hiding this comment.
Oh yeah, I meant the splitting of process.env.APPSMITH_DISABLE_IFRAME_WIDGET_SANDBOX into two lines, which, honestly looks not very prettier. Sorry, should've been clearer. 🙂
There was a problem hiding this comment.
Opinion: The whole line width hoopla is probably due to side-by-side diffs and old CRT resolutions. With top-down diffs, we can technically increase the number of characters to display in a line by quite a bit. We used to have 80 characters, 120 is today's default, most likely we'll see it increase to 160+ characters in the near future.
There was a problem hiding this comment.
Oh sure, I won't bother to disagree, but even if we want to solve this line-length thingy here, this screenshot looks a lot better than what we have here. Hate that prettier thinks this is, well, prettier.
There was a problem hiding this comment.
Agreed. By virtue of being input agnostic software (handles unknown code purely based on language syntax), it has issues.
|
/ok-to-test sha=3c1126c |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3152188894. |
|
/ok-to-test sha=f2b5f2b |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3157093106. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/3157093106. Click to view performance test results
|
|
/ok-to-test sha=905422c |
|
/ok-to-test sha=905422c |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3173154926. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/3173154926. Click to view performance test results
|
|
/ok-to-test sha=5ae2f3d |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3180681073. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/3180681073. Click to view performance test results
|
riodeuno
added
iFrame
Description
Change iFrame widgets to use sandbox mode if srcDoc is provided
Allowed options:
allow-forms allow-modals allow-orientation-lock allow-pointer-lock allow-popups allow-scripts allow-top-navigation-by-user-activationThe sandbox attribute is configurable with
APPSMITH_DISABLE_IFRAME_WIDGET_SANDBOXenv variable.Fixes #TBD
Type of change
How Has This Been Tested?
Checklist:
Test coverage results 🧪
🟢 Total coverage has increased