feat: disable cookie access under restricted sandboxes by josegonzalez · Pull Request #1080 · plotly/dash

josegonzalez · 2020-01-11T00:52:43Z

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality.

This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Contributor Checklist

~~I have broken down my PR scope into the following TODO tasks~~ not needed
I have run the tests locally and they passed. (refer to testing section in contributing)
I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

I have added entry in the CHANGELOG.md

josegonzalez · 2020-01-11T00:57:34Z

Not sure how to fix the Empty block statement error...

alexcjohnson · 2020-01-11T01:31:02Z

Not sure how to fix the Empty block statement error...

May be worth console.log-ing the error anyway, could be useful to sandbox users for debugging. That would fill the block :)

josegonzalez · 2020-01-13T17:12:51Z

heh:

Unexpected console statement no-console

josegonzalez · 2020-01-13T17:54:31Z

@alexcjohnson I've ignored the prettier issue in code for now since there isn't a great way to handle it otherwise.

Do you have any thoughts on how I should write a test for the code? The change itself was manually tested and confirmed working within a large-ish dash app, so I'm fairly confident it works, but I'm not sure how to write tests for the change...

alexcjohnson · 2020-01-13T18:14:14Z

how I should write a test for the code?

The most robust thing I can think of is to make a container html file (in an assets/ folder next to the test file so dash will serve it) with the actual dash app in a sandboxed iframe - then go interact with the app in the iframe - I've never done that but it looks manageable http://allselenium.info/handling-iframes-using-selenium-webdriver/

josegonzalez · 2020-01-16T19:46:00Z

@alexcjohnson once tests pass (I rebased everything together, they previously passed) this should be good for a re-review/merge :)

Marc-Andre-Rivet · 2020-01-21T20:28:20Z

+ document.cookie =
+ `${constants.OAUTH_COOKIE_NAME}=; ` +
+ 'expires=Thu, 01 Jan 1970 00:00:01 GMT;';
+ } catch (e) {}


Between an eslint exception allowing error swallowing and an eslint exception for a console.warn entry, I'd rather have the warning. Dash will be operating with limited capabilities and the information should be exposed. Maybe later we can approve it with #1088

Marc-Andre-Rivet · 2020-01-22T20:20:42Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert not dash_duo.get_logs()


@josegonzalez I suppose that test failure was predictable.. we now log a warning.. and the test picks it up! You'll want to update assert not dash_duo.get_logs() accordingly

Yeah, the other failures i was talking about previously were eslint errors, but i got them covered.

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality. This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Marc-Andre-Rivet · 2020-01-24T23:48:59Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert len(dash_duo.get_logs()) == 2


@josegonzalez The ==2 part is brittle but ok for now I think -- if you could open an issue to improve this part of the test later, I'm fine as-is. What I would like so see here is some validation that the Dash app actually loaded and works as expected. Maybe select the #btn, click it, and make sure at least one of the divs got updated by the callback.

Marc-Andre-Rivet

💃

* Add standard dash interface for location change (plotly#1094) * feat: disable cookie access under restricted sandboxes (plotly#1080) * IE11 compatibility (plotly#1106) * Add `inheritAsyncDecorator` for wrapper components (plotly#1109) * bump version, loosen version requirements * dcc>=1.7.0, 1.7.1 was JS only * stricten versions * stricten table version Co-authored-by: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com> Co-authored-by: Jose Diaz-Gonzalez <email@josediazgonzalez.com>

josegonzalez · 2020-01-11T00:52:43Z

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality.

This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Contributor Checklist

~~I have broken down my PR scope into the following TODO tasks~~ not needed
I have run the tests locally and they passed. (refer to testing section in contributing)
I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

I have added entry in the CHANGELOG.md

josegonzalez · 2020-01-11T00:57:34Z

Not sure how to fix the Empty block statement error...

alexcjohnson · 2020-01-11T01:31:02Z

Not sure how to fix the Empty block statement error...

May be worth console.log-ing the error anyway, could be useful to sandbox users for debugging. That would fill the block :)

josegonzalez · 2020-01-13T17:12:51Z

heh:

Unexpected console statement no-console

josegonzalez · 2020-01-13T17:54:31Z

@alexcjohnson I've ignored the prettier issue in code for now since there isn't a great way to handle it otherwise.

Do you have any thoughts on how I should write a test for the code? The change itself was manually tested and confirmed working within a large-ish dash app, so I'm fairly confident it works, but I'm not sure how to write tests for the change...

alexcjohnson · 2020-01-13T18:14:14Z

how I should write a test for the code?

The most robust thing I can think of is to make a container html file (in an assets/ folder next to the test file so dash will serve it) with the actual dash app in a sandboxed iframe - then go interact with the app in the iframe - I've never done that but it looks manageable http://allselenium.info/handling-iframes-using-selenium-webdriver/

josegonzalez · 2020-01-16T19:46:00Z

@alexcjohnson once tests pass (I rebased everything together, they previously passed) this should be good for a re-review/merge :)

Marc-Andre-Rivet · 2020-01-21T20:28:20Z

+ document.cookie =
+ `${constants.OAUTH_COOKIE_NAME}=; ` +
+ 'expires=Thu, 01 Jan 1970 00:00:01 GMT;';
+ } catch (e) {}


Between an eslint exception allowing error swallowing and an eslint exception for a console.warn entry, I'd rather have the warning. Dash will be operating with limited capabilities and the information should be exposed. Maybe later we can approve it with #1088

Marc-Andre-Rivet · 2020-01-22T20:20:42Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert not dash_duo.get_logs()


@josegonzalez I suppose that test failure was predictable.. we now log a warning.. and the test picks it up! You'll want to update assert not dash_duo.get_logs() accordingly

Yeah, the other failures i was talking about previously were eslint errors, but i got them covered.

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality. This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Marc-Andre-Rivet · 2020-01-24T23:48:59Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert len(dash_duo.get_logs()) == 2


@josegonzalez The ==2 part is brittle but ok for now I think -- if you could open an issue to improve this part of the test later, I'm fine as-is. What I would like so see here is some validation that the Dash app actually loaded and works as expected. Maybe select the #btn, click it, and make sure at least one of the divs got updated by the callback.

Marc-Andre-Rivet

💃

* Add standard dash interface for location change (plotly#1094) * feat: disable cookie access under restricted sandboxes (plotly#1080) * IE11 compatibility (plotly#1106) * Add `inheritAsyncDecorator` for wrapper components (plotly#1109) * bump version, loosen version requirements * dcc>=1.7.0, 1.7.1 was JS only * stricten versions * stricten table version Co-authored-by: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com> Co-authored-by: Jose Diaz-Gonzalez <email@josediazgonzalez.com>

josegonzalez · 2020-01-11T00:52:43Z

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality.

This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Contributor Checklist

~~I have broken down my PR scope into the following TODO tasks~~ not needed
I have run the tests locally and they passed. (refer to testing section in contributing)
I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

I have added entry in the CHANGELOG.md

josegonzalez · 2020-01-11T00:57:34Z

Not sure how to fix the Empty block statement error...

alexcjohnson · 2020-01-11T01:31:02Z

Not sure how to fix the Empty block statement error...

May be worth console.log-ing the error anyway, could be useful to sandbox users for debugging. That would fill the block :)

josegonzalez · 2020-01-13T17:12:51Z

heh:

Unexpected console statement no-console

josegonzalez · 2020-01-13T17:54:31Z

@alexcjohnson I've ignored the prettier issue in code for now since there isn't a great way to handle it otherwise.

Do you have any thoughts on how I should write a test for the code? The change itself was manually tested and confirmed working within a large-ish dash app, so I'm fairly confident it works, but I'm not sure how to write tests for the change...

alexcjohnson · 2020-01-13T18:14:14Z

how I should write a test for the code?

The most robust thing I can think of is to make a container html file (in an assets/ folder next to the test file so dash will serve it) with the actual dash app in a sandboxed iframe - then go interact with the app in the iframe - I've never done that but it looks manageable http://allselenium.info/handling-iframes-using-selenium-webdriver/

josegonzalez · 2020-01-16T19:46:00Z

@alexcjohnson once tests pass (I rebased everything together, they previously passed) this should be good for a re-review/merge :)

Marc-Andre-Rivet · 2020-01-21T20:28:20Z

+ document.cookie =
+ `${constants.OAUTH_COOKIE_NAME}=; ` +
+ 'expires=Thu, 01 Jan 1970 00:00:01 GMT;';
+ } catch (e) {}


Between an eslint exception allowing error swallowing and an eslint exception for a console.warn entry, I'd rather have the warning. Dash will be operating with limited capabilities and the information should be exposed. Maybe later we can approve it with #1088

Marc-Andre-Rivet · 2020-01-22T20:20:42Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert not dash_duo.get_logs()


@josegonzalez I suppose that test failure was predictable.. we now log a warning.. and the test picks it up! You'll want to update assert not dash_duo.get_logs() accordingly

Yeah, the other failures i was talking about previously were eslint errors, but i got them covered.

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality. This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Marc-Andre-Rivet · 2020-01-24T23:48:59Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert len(dash_duo.get_logs()) == 2


@josegonzalez The ==2 part is brittle but ok for now I think -- if you could open an issue to improve this part of the test later, I'm fine as-is. What I would like so see here is some validation that the Dash app actually loaded and works as expected. Maybe select the #btn, click it, and make sure at least one of the divs got updated by the callback.

Marc-Andre-Rivet

💃

* Add standard dash interface for location change (plotly#1094) * feat: disable cookie access under restricted sandboxes (plotly#1080) * IE11 compatibility (plotly#1106) * Add `inheritAsyncDecorator` for wrapper components (plotly#1109) * bump version, loosen version requirements * dcc>=1.7.0, 1.7.1 was JS only * stricten versions * stricten table version Co-authored-by: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com> Co-authored-by: Jose Diaz-Gonzalez <email@josediazgonzalez.com>

josegonzalez · 2020-01-11T00:52:43Z

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality.

This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Contributor Checklist

~~I have broken down my PR scope into the following TODO tasks~~ not needed
I have run the tests locally and they passed. (refer to testing section in contributing)
I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

I have added entry in the CHANGELOG.md

josegonzalez · 2020-01-11T00:57:34Z

Not sure how to fix the Empty block statement error...

alexcjohnson · 2020-01-11T01:31:02Z

Not sure how to fix the Empty block statement error...

May be worth console.log-ing the error anyway, could be useful to sandbox users for debugging. That would fill the block :)

josegonzalez · 2020-01-13T17:12:51Z

heh:

Unexpected console statement no-console

josegonzalez · 2020-01-13T17:54:31Z

@alexcjohnson I've ignored the prettier issue in code for now since there isn't a great way to handle it otherwise.

Do you have any thoughts on how I should write a test for the code? The change itself was manually tested and confirmed working within a large-ish dash app, so I'm fairly confident it works, but I'm not sure how to write tests for the change...

alexcjohnson · 2020-01-13T18:14:14Z

how I should write a test for the code?

The most robust thing I can think of is to make a container html file (in an assets/ folder next to the test file so dash will serve it) with the actual dash app in a sandboxed iframe - then go interact with the app in the iframe - I've never done that but it looks manageable http://allselenium.info/handling-iframes-using-selenium-webdriver/

josegonzalez · 2020-01-16T19:46:00Z

@alexcjohnson once tests pass (I rebased everything together, they previously passed) this should be good for a re-review/merge :)

Marc-Andre-Rivet · 2020-01-21T20:28:20Z

+ document.cookie =
+ `${constants.OAUTH_COOKIE_NAME}=; ` +
+ 'expires=Thu, 01 Jan 1970 00:00:01 GMT;';
+ } catch (e) {}


Between an eslint exception allowing error swallowing and an eslint exception for a console.warn entry, I'd rather have the warning. Dash will be operating with limited capabilities and the information should be exposed. Maybe later we can approve it with #1088

Marc-Andre-Rivet · 2020-01-22T20:20:42Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert not dash_duo.get_logs()


@josegonzalez I suppose that test failure was predictable.. we now log a warning.. and the test picks it up! You'll want to update assert not dash_duo.get_logs() accordingly

Yeah, the other failures i was talking about previously were eslint errors, but i got them covered.

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality. This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Marc-Andre-Rivet · 2020-01-24T23:48:59Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert len(dash_duo.get_logs()) == 2


@josegonzalez The ==2 part is brittle but ok for now I think -- if you could open an issue to improve this part of the test later, I'm fine as-is. What I would like so see here is some validation that the Dash app actually loaded and works as expected. Maybe select the #btn, click it, and make sure at least one of the divs got updated by the callback.

Marc-Andre-Rivet

💃

* Add standard dash interface for location change (plotly#1094) * feat: disable cookie access under restricted sandboxes (plotly#1080) * IE11 compatibility (plotly#1106) * Add `inheritAsyncDecorator` for wrapper components (plotly#1109) * bump version, loosen version requirements * dcc>=1.7.0, 1.7.1 was JS only * stricten versions * stricten table version Co-authored-by: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com> Co-authored-by: Jose Diaz-Gonzalez <email@josediazgonzalez.com>

josegonzalez · 2020-01-11T00:52:43Z

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality.

This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Contributor Checklist

~~I have broken down my PR scope into the following TODO tasks~~ not needed
I have run the tests locally and they passed. (refer to testing section in contributing)
I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

I have added entry in the CHANGELOG.md

josegonzalez · 2020-01-11T00:57:34Z

Not sure how to fix the Empty block statement error...

alexcjohnson · 2020-01-11T01:31:02Z

Not sure how to fix the Empty block statement error...

May be worth console.log-ing the error anyway, could be useful to sandbox users for debugging. That would fill the block :)

josegonzalez · 2020-01-13T17:12:51Z

heh:

Unexpected console statement no-console

josegonzalez · 2020-01-13T17:54:31Z

@alexcjohnson I've ignored the prettier issue in code for now since there isn't a great way to handle it otherwise.

Do you have any thoughts on how I should write a test for the code? The change itself was manually tested and confirmed working within a large-ish dash app, so I'm fairly confident it works, but I'm not sure how to write tests for the change...

alexcjohnson · 2020-01-13T18:14:14Z

how I should write a test for the code?

The most robust thing I can think of is to make a container html file (in an assets/ folder next to the test file so dash will serve it) with the actual dash app in a sandboxed iframe - then go interact with the app in the iframe - I've never done that but it looks manageable http://allselenium.info/handling-iframes-using-selenium-webdriver/

josegonzalez · 2020-01-16T19:46:00Z

@alexcjohnson once tests pass (I rebased everything together, they previously passed) this should be good for a re-review/merge :)

Marc-Andre-Rivet · 2020-01-21T20:28:20Z

+ document.cookie =
+ `${constants.OAUTH_COOKIE_NAME}=; ` +
+ 'expires=Thu, 01 Jan 1970 00:00:01 GMT;';
+ } catch (e) {}


Between an eslint exception allowing error swallowing and an eslint exception for a console.warn entry, I'd rather have the warning. Dash will be operating with limited capabilities and the information should be exposed. Maybe later we can approve it with #1088

Marc-Andre-Rivet · 2020-01-22T20:20:42Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert not dash_duo.get_logs()


@josegonzalez I suppose that test failure was predictable.. we now log a warning.. and the test picks it up! You'll want to update assert not dash_duo.get_logs() accordingly

Yeah, the other failures i was talking about previously were eslint errors, but i got them covered.

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality. This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Marc-Andre-Rivet · 2020-01-24T23:48:59Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert len(dash_duo.get_logs()) == 2


@josegonzalez The ==2 part is brittle but ok for now I think -- if you could open an issue to improve this part of the test later, I'm fine as-is. What I would like so see here is some validation that the Dash app actually loaded and works as expected. Maybe select the #btn, click it, and make sure at least one of the divs got updated by the callback.

Marc-Andre-Rivet

💃

* Add standard dash interface for location change (plotly#1094) * feat: disable cookie access under restricted sandboxes (plotly#1080) * IE11 compatibility (plotly#1106) * Add `inheritAsyncDecorator` for wrapper components (plotly#1109) * bump version, loosen version requirements * dcc>=1.7.0, 1.7.1 was JS only * stricten versions * stricten table version Co-authored-by: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com> Co-authored-by: Jose Diaz-Gonzalez <email@josediazgonzalez.com>

josegonzalez · 2020-01-11T00:52:43Z

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality.

This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Contributor Checklist

~~I have broken down my PR scope into the following TODO tasks~~ not needed
I have run the tests locally and they passed. (refer to testing section in contributing)
I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

I have added entry in the CHANGELOG.md

josegonzalez · 2020-01-11T00:57:34Z

Not sure how to fix the Empty block statement error...

alexcjohnson · 2020-01-11T01:31:02Z

Not sure how to fix the Empty block statement error...

May be worth console.log-ing the error anyway, could be useful to sandbox users for debugging. That would fill the block :)

josegonzalez · 2020-01-13T17:12:51Z

heh:

Unexpected console statement no-console

josegonzalez · 2020-01-13T17:54:31Z

@alexcjohnson I've ignored the prettier issue in code for now since there isn't a great way to handle it otherwise.

Do you have any thoughts on how I should write a test for the code? The change itself was manually tested and confirmed working within a large-ish dash app, so I'm fairly confident it works, but I'm not sure how to write tests for the change...

alexcjohnson · 2020-01-13T18:14:14Z

how I should write a test for the code?

The most robust thing I can think of is to make a container html file (in an assets/ folder next to the test file so dash will serve it) with the actual dash app in a sandboxed iframe - then go interact with the app in the iframe - I've never done that but it looks manageable http://allselenium.info/handling-iframes-using-selenium-webdriver/

josegonzalez · 2020-01-16T19:46:00Z

@alexcjohnson once tests pass (I rebased everything together, they previously passed) this should be good for a re-review/merge :)

Marc-Andre-Rivet · 2020-01-21T20:28:20Z

+ document.cookie =
+ `${constants.OAUTH_COOKIE_NAME}=; ` +
+ 'expires=Thu, 01 Jan 1970 00:00:01 GMT;';
+ } catch (e) {}


Between an eslint exception allowing error swallowing and an eslint exception for a console.warn entry, I'd rather have the warning. Dash will be operating with limited capabilities and the information should be exposed. Maybe later we can approve it with #1088

Marc-Andre-Rivet · 2020-01-22T20:20:42Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert not dash_duo.get_logs()


@josegonzalez I suppose that test failure was predictable.. we now log a warning.. and the test picks it up! You'll want to update assert not dash_duo.get_logs() accordingly

Yeah, the other failures i was talking about previously were eslint errors, but i got them covered.

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality. This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Marc-Andre-Rivet · 2020-01-24T23:48:59Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert len(dash_duo.get_logs()) == 2


@josegonzalez The ==2 part is brittle but ok for now I think -- if you could open an issue to improve this part of the test later, I'm fine as-is. What I would like so see here is some validation that the Dash app actually loaded and works as expected. Maybe select the #btn, click it, and make sure at least one of the divs got updated by the callback.

Marc-Andre-Rivet

💃

* Add standard dash interface for location change (plotly#1094) * feat: disable cookie access under restricted sandboxes (plotly#1080) * IE11 compatibility (plotly#1106) * Add `inheritAsyncDecorator` for wrapper components (plotly#1109) * bump version, loosen version requirements * dcc>=1.7.0, 1.7.1 was JS only * stricten versions * stricten table version Co-authored-by: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com> Co-authored-by: Jose Diaz-Gonzalez <email@josediazgonzalez.com>

josegonzalez · 2020-01-11T00:52:43Z

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality.

This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Contributor Checklist

~~I have broken down my PR scope into the following TODO tasks~~ not needed
I have run the tests locally and they passed. (refer to testing section in contributing)
I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

I have added entry in the CHANGELOG.md

josegonzalez · 2020-01-11T00:57:34Z

Not sure how to fix the Empty block statement error...

alexcjohnson · 2020-01-11T01:31:02Z

Not sure how to fix the Empty block statement error...

May be worth console.log-ing the error anyway, could be useful to sandbox users for debugging. That would fill the block :)

josegonzalez · 2020-01-13T17:12:51Z

heh:

Unexpected console statement no-console

josegonzalez · 2020-01-13T17:54:31Z

@alexcjohnson I've ignored the prettier issue in code for now since there isn't a great way to handle it otherwise.

Do you have any thoughts on how I should write a test for the code? The change itself was manually tested and confirmed working within a large-ish dash app, so I'm fairly confident it works, but I'm not sure how to write tests for the change...

alexcjohnson · 2020-01-13T18:14:14Z

how I should write a test for the code?

The most robust thing I can think of is to make a container html file (in an assets/ folder next to the test file so dash will serve it) with the actual dash app in a sandboxed iframe - then go interact with the app in the iframe - I've never done that but it looks manageable http://allselenium.info/handling-iframes-using-selenium-webdriver/

josegonzalez · 2020-01-16T19:46:00Z

@alexcjohnson once tests pass (I rebased everything together, they previously passed) this should be good for a re-review/merge :)

Marc-Andre-Rivet · 2020-01-21T20:28:20Z

+ document.cookie =
+ `${constants.OAUTH_COOKIE_NAME}=; ` +
+ 'expires=Thu, 01 Jan 1970 00:00:01 GMT;';
+ } catch (e) {}


Between an eslint exception allowing error swallowing and an eslint exception for a console.warn entry, I'd rather have the warning. Dash will be operating with limited capabilities and the information should be exposed. Maybe later we can approve it with #1088

Marc-Andre-Rivet · 2020-01-22T20:20:42Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert not dash_duo.get_logs()


@josegonzalez I suppose that test failure was predictable.. we now log a warning.. and the test picks it up! You'll want to update assert not dash_duo.get_logs() accordingly

Yeah, the other failures i was talking about previously were eslint errors, but i got them covered.

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality. This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Marc-Andre-Rivet · 2020-01-24T23:48:59Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert len(dash_duo.get_logs()) == 2


@josegonzalez The ==2 part is brittle but ok for now I think -- if you could open an issue to improve this part of the test later, I'm fine as-is. What I would like so see here is some validation that the Dash app actually loaded and works as expected. Maybe select the #btn, click it, and make sure at least one of the divs got updated by the callback.

Marc-Andre-Rivet

💃

* Add standard dash interface for location change (plotly#1094) * feat: disable cookie access under restricted sandboxes (plotly#1080) * IE11 compatibility (plotly#1106) * Add `inheritAsyncDecorator` for wrapper components (plotly#1109) * bump version, loosen version requirements * dcc>=1.7.0, 1.7.1 was JS only * stricten versions * stricten table version Co-authored-by: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com> Co-authored-by: Jose Diaz-Gonzalez <email@josediazgonzalez.com>

josegonzalez · 2020-01-11T00:52:43Z

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality.

This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Contributor Checklist

~~I have broken down my PR scope into the following TODO tasks~~ not needed
I have run the tests locally and they passed. (refer to testing section in contributing)
I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

I have added entry in the CHANGELOG.md

josegonzalez · 2020-01-11T00:57:34Z

Not sure how to fix the Empty block statement error...

alexcjohnson · 2020-01-11T01:31:02Z

Not sure how to fix the Empty block statement error...

May be worth console.log-ing the error anyway, could be useful to sandbox users for debugging. That would fill the block :)

josegonzalez · 2020-01-13T17:12:51Z

heh:

Unexpected console statement no-console

josegonzalez · 2020-01-13T17:54:31Z

@alexcjohnson I've ignored the prettier issue in code for now since there isn't a great way to handle it otherwise.

Do you have any thoughts on how I should write a test for the code? The change itself was manually tested and confirmed working within a large-ish dash app, so I'm fairly confident it works, but I'm not sure how to write tests for the change...

alexcjohnson · 2020-01-13T18:14:14Z

how I should write a test for the code?

The most robust thing I can think of is to make a container html file (in an assets/ folder next to the test file so dash will serve it) with the actual dash app in a sandboxed iframe - then go interact with the app in the iframe - I've never done that but it looks manageable http://allselenium.info/handling-iframes-using-selenium-webdriver/

josegonzalez · 2020-01-16T19:46:00Z

@alexcjohnson once tests pass (I rebased everything together, they previously passed) this should be good for a re-review/merge :)

Marc-Andre-Rivet · 2020-01-21T20:28:20Z

+ document.cookie =
+ `${constants.OAUTH_COOKIE_NAME}=; ` +
+ 'expires=Thu, 01 Jan 1970 00:00:01 GMT;';
+ } catch (e) {}


Between an eslint exception allowing error swallowing and an eslint exception for a console.warn entry, I'd rather have the warning. Dash will be operating with limited capabilities and the information should be exposed. Maybe later we can approve it with #1088

Marc-Andre-Rivet · 2020-01-22T20:20:42Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert not dash_duo.get_logs()


@josegonzalez I suppose that test failure was predictable.. we now log a warning.. and the test picks it up! You'll want to update assert not dash_duo.get_logs() accordingly

Yeah, the other failures i was talking about previously were eslint errors, but i got them covered.

When dash is embedded into an iframe with a sandbox attribute that only has allow-scripts, cookie access is disabled and dash fails to load. As such, we need to restrict our cookie usage by disabling functionality. This patch removes the disabled functionality in a graceful manner, allowing dash to load in very restricted iframes.

Marc-Andre-Rivet · 2020-01-24T23:48:59Z

+
+ dash_duo.driver.get("data:text/html;charset=utf-8," + html_content)
+
+ assert len(dash_duo.get_logs()) == 2


@josegonzalez The ==2 part is brittle but ok for now I think -- if you could open an issue to improve this part of the test later, I'm fine as-is. What I would like so see here is some validation that the Dash app actually loaded and works as expected. Maybe select the #btn, click it, and make sure at least one of the divs got updated by the callback.

Marc-Andre-Rivet

💃

* Add standard dash interface for location change (plotly#1094) * feat: disable cookie access under restricted sandboxes (plotly#1080) * IE11 compatibility (plotly#1106) * Add `inheritAsyncDecorator` for wrapper components (plotly#1109) * bump version, loosen version requirements * dcc>=1.7.0, 1.7.1 was JS only * stricten versions * stricten table version Co-authored-by: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com> Co-authored-by: Jose Diaz-Gonzalez <email@josediazgonzalez.com>

josegonzalez requested review from Marc-Andre-Rivet and alexcjohnson as code owners January 11, 2020 00:52

josegonzalez requested a review from shammamah-zz January 11, 2020 00:53

josegonzalez force-pushed the iframe-sandbox-support branch from 91638aa to 127b020Compare January 13, 2020 15:21

josegonzalez force-pushed the iframe-sandbox-support branch from 127b020 to 260792cCompare January 13, 2020 17:14

josegonzalez force-pushed the iframe-sandbox-support branch 4 times, most recently from 5dbb6af to 6d9660cCompare January 16, 2020 19:45

Marc-Andre-Rivet added this to the Dash v1.9 milestone Jan 17, 2020

Marc-Andre-Rivet added the Stage: In Review label Jan 17, 2020

Marc-Andre-Rivet reviewed Jan 21, 2020
View reviewed changes

Comment threaddash-renderer/src/actions/index.js

Marc-Andre-Rivet reviewed Jan 22, 2020
View reviewed changes

Comment threadrelease Outdated

josegonzalez force-pushed the iframe-sandbox-support branch from 0e5a5f6 to d6ad43cCompare January 22, 2020 22:32

josegonzalez added 2 commits January 24, 2020 16:54

tests: assert that the error length is 1
f936df1

tests: 2 log lines are output in the restricted iframe test
bab71de

Marc-Andre-Rivet reviewed Jan 24, 2020
View reviewed changes

Marc-André Rivet added 2 commits January 28, 2020 11:52

add app interaction
0609adf

there should be warnings
d28c961

josegonzalez mentioned this pull request Jan 28, 2020
Add correct test for warnings logged in restricted iframes #1100
Open

Marc-Andre-Rivet approved these changes Jan 28, 2020
View reviewed changes

Marc-Andre-Rivetand others added 4 commits January 28, 2020 20:04

warn once
74c9a97

changelog
b87ad7b

Merge branch 'dev' into iframe-sandbox-support
b4e7824

undo bad copypasta
126b7a1

Marc-Andre-Rivet merged commit ca58cc4 into devJan 29, 2020

Marc-Andre-Rivet deleted the iframe-sandbox-support branch January 29, 2020 21:03

alexcjohnson mentioned this pull request Dec 17, 2020
Loosen testing requirements & improve testing #1506
Merged
2 tasks