feat: disable cookie access under restricted sandboxes#1080
feat: disable cookie access under restricted sandboxes#1080Marc-Andre-Rivet merged 9 commits intodevfrom
Conversation
josegonzalez
requested review from
Marc-Andre-Rivet and
alexcjohnson
as code owners
|
Not sure how to fix the |
May be worth |
josegonzalez
force-pushed
the
iframe-sandbox-support
branch
from
91638aa to
127b020
Compare
|
heh: |
josegonzalez
force-pushed
the
iframe-sandbox-support
branch
from
127b020 to
260792c
Compare
|
@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... |
The most robust thing I can think of is to make a container html file (in an |
josegonzalez
force-pushed
the
iframe-sandbox-support
branch
4 times, most recently
from
5dbb6af to
6d9660c
Compare
|
@alexcjohnson once tests pass (I rebased everything together, they previously passed) this should be good for a re-review/merge :) |
| document.cookie = | ||
| `${constants.OAUTH_COOKIE_NAME}=; ` + | ||
| 'expires=Thu, 01 Jan 1970 00:00:01 GMT;'; | ||
| } catch (e) {} |
There was a problem hiding this comment.
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
|
|
||
| dash_duo.driver.get("data:text/html;charset=utf-8," + html_content) | ||
|
|
||
| assert not dash_duo.get_logs() |
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
josegonzalez
force-pushed
the
iframe-sandbox-support
branch
from
0e5a5f6 to
d6ad43c
Compare
|
|
||
| dash_duo.driver.get("data:text/html;charset=utf-8," + html_content) | ||
|
|
||
| assert len(dash_duo.get_logs()) == 2 |
There was a problem hiding this comment.
@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.
* 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>
* 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>
3 participants
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 tasksnot neededoptionals
CHANGELOG.md