Skip to content

Conversation

@mayagbarnes
Copy link
Collaborator

@mayagbarnes mayagbarnes commented Sep 30, 2022

📚 Context

Integrating the expression interpreter with vega-embed to avoid unsafe-eval permission for CSP

  • What kind of change does this PR introduce?
    • Refactoring

🧠 Description of Changes

  • Passing ast: true and expression interpreter to vega-embed

🧪 Testing Done

  • Manual

🌐 References


Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@mayagbarnes mayagbarnes changed the title DRAFT: Add vega-interpreter to Vega Lite Charts Add vega-interpreter to Vega Lite Charts Oct 8, 2022
@mayagbarnes
Copy link
Collaborator Author

Not entirely sure how I ought to write tests for this...
I only manual tested with <meta http-equiv="Content-Security-Policy" content="script-src 'self'" /> in the index.html file

@mayagbarnes mayagbarnes marked this pull request as ready for review October 8, 2022 21:01
@lukasmasuch
Copy link
Collaborator

lukasmasuch commented Oct 10, 2022

Not entirely sure how I ought to write tests for this...

Yep, that sounds pretty complicated. This might require some research, but I think we cannot test this through our unit tests (jest, pytest) since there isn't any browser involved. So, we either have to test it via e2e tests by adding it somehow to the index.html (injecting it via javascript would not work) or we have to add the CSP header as default to Streamlit. Maybe adding the CSP header to Streamlit core is even requested by the SIS/Security team, not sure.

One super hacky way I can think of would be the following: inject the header into the index.html file (see this for an example) from the python e2e script. This might also require to force one rerun (e.g. via experimental_rerun) to have the cypress browser load the modified file. And we might need to clean the index.html file up again for the following tests.

Another cleaner solution might be that we add an optional config option (e.g. server.enableCSPProtection) that - if activated - adds the CSP to the HTTP headers of the webserver (similar to how it is done here). Maybe that is even a valid feature to officially add to our config options.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Do you know if this is actually required? I think it is used as default if ast is set to true:

https://github.com/vega/vega-embed/blob/88fb1224cbb2e51644849a20f1e7ba629629bab9/src/embed.ts#L378

But there is most likely no harm in explicitly specifying this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed it looks like its passed as the default, just thought it would be more straightforward to understand if it was explicitly passed to expr:

Copy link
Collaborator

@lukasmasuch lukasmasuch left a comment

Choose a reason for hiding this comment

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

Overall looks good 👍 We somehow need to figure out if there is a good way to test this. Maybe we can bring this up as a discussion topic in the next standup to discuss some of the options.

Copy link
Collaborator

@kmcgrady kmcgrady left a comment

Choose a reason for hiding this comment

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

Vega is BSD 3-Clause so I approve this dependency change

@mayagbarnes mayagbarnes merged commit 9e9c4bc into develop Oct 11, 2022
@mayagbarnes mayagbarnes deleted the vega-interpreter branch October 11, 2022 18:41
willhuang1997 added a commit to willhuang1997/streamlit that referenced this pull request Nov 29, 2022
willhuang1997 added a commit to willhuang1997/streamlit that referenced this pull request Nov 29, 2022
willhuang1997 added a commit to willhuang1997/streamlit that referenced this pull request Nov 29, 2022
willhuang1997 added a commit to willhuang1997/streamlit that referenced this pull request Nov 29, 2022
willhuang1997 added a commit to willhuang1997/streamlit that referenced this pull request Nov 29, 2022
willhuang1997 added a commit to willhuang1997/streamlit that referenced this pull request Nov 29, 2022
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.

5 participants