Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add iframeSandbox config #157

Merged
merged 12 commits into from
May 31, 2020
Merged

Conversation

parshap
Copy link
Contributor

@parshap parshap commented May 21, 2020

Hi! We’re looking at integrating Playroom on our design system style guide at NerdWallet. Given Playroom’s inherent ability to execute arbitrary code, one of our security engineers had some recommendations:

  1. Move the site to its own domain where cookies for other NerdWallet applications will not be available
  2. Use the sandbox attribute in Playroom’s iframe to sandbox arbitrary code execution

This PR is about the second item and implements the sandbox attribute in the <Iframe> component driven by playroom.config.js config. Credit for the implementation also goes to @mhensley-nw.

This security concern was also raised in #125.

Unbreaking Webpack HMR

Like #125 mentions, using sandbox=”allow-scripts” breaks Webpack hot module replacement in development. This is because Webpack expects an Origin header and sandbox makes requests omit Origin. This is fixed by using webpack-dev-server’s disableHostCheck option, which makes Webpack not expect the Origin header. More information in this Webpack issue: webpack/webpack-dev-server#1604.

Configuration

This is an opt-in feature using the iframeSandbox key in playroom.config.js. A value of true defaults to ”allow-scripts” or a custom string can be used. We think ”allow-scripts” might be a sane default, so you may consider making that the default in the future.

@parshap parshap requested a review from a team as a code owner May 21, 2020 23:57
README.md Outdated
@@ -79,6 +80,8 @@ export { Button } from '../Button'; // Re-exporting a named export
// etc...
```

The `iframeSandbox` option can be used to control the `sandbox` attribute on Playroom's iframe. A value of `true` will set `sandbox="allow-scripts"` or a custom string value can be used.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to document this feature somewhere, but let me know if this is too distracting to be upfront in the readme and you'd like to move it elsewhere.

cypress/projects/basic/playroom.config.js Outdated Show resolved Hide resolved
@@ -12,6 +12,18 @@ interface IframeProps extends AllHTMLAttributes<HTMLIFrameElement> {
intersectionRootRef: MutableRefObject<Element | null>;
}

const playroomConfig = (window.__playroomConfig__ = __PLAYROOM_GLOBAL__CONFIG__);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if I should read the config in this component or if the value should be read elsewhere and passed as a prop. I tried to follow existing patterns, which looked like it's ok for components to read config.

Using sandbox breaks Cypress assertions because it restricts iframe
access.
@parshap parshap changed the title Add iframeSandbox config to control <iframe sandbox> attribute feat: Add iframeSandbox config May 22, 2020
@mattcompiles
Copy link
Contributor

Hey Mate, Thanks for the PR. Controlling the sandbox attribute seems reasonable. However, I'm not sure about the iframeSandbox: true behaviour. I think it's not clear how secure the iframe now is. Maybe it'd be cleaner to just leave the string forward behaviour and let users decide what value they pass?

Probably also worth mentioning that at least allow-scripts is required for playroom to work?

@parshap
Copy link
Contributor Author

parshap commented May 30, 2020

Hey @mattcompiles. I think you're right that a default of allow-scripts may not be right for all use cases and doesn't make clear what's happening. I made your suggested change of requiring an explicit string value and added a note about needing allow-scripts in the readme. Thanks for the input!

Copy link
Contributor

@mattcompiles mattcompiles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR 👏

@mattcompiles mattcompiles merged commit 482bafc into seek-oss:master May 31, 2020
@parshap parshap deleted the iframe-sandbox branch June 1, 2020 17:45
@penx
Copy link

penx commented Nov 25, 2021

@parshap @mattcompiles it would be great to note these security concerns/recommendations in the readme, especially the advice to put Playroom on to its own domain. From my understanding, hosting Playroom on a github.io subdomain would be secure (as far as it couldn't read any sensitive cookies/localstorage), but I expect people may want to deploy Playroom in other ways and may not be immediately aware of these considerations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants