Skip to content

Conversation

@F1nnM
Copy link
Contributor

@F1nnM F1nnM commented Mar 31, 2021

Issue: Closes #3040
Description: Added "allow-downloads" to the sandbox attributes. This enables components to provide a download button for example for data or images.

This enables components to provide a download button for example for data or images.
@F1nnM F1nnM requested a review from a team March 31, 2021 20:31
Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

Hey @F1nnM, the code change here looks good to me, but I'll need to double-check that we're okay with allowing this in case it was intentionally omitted for security reasons.

There's a team meeting tomorrow that I'll bring this up in, so I should be able to get back to you on this afterwards.

@F1nnM
Copy link
Contributor Author

F1nnM commented Mar 31, 2021

There is the following comment in this file:

  // From MDN:
  // "When the embedded document has the same origin as the embedding page, it is
  // strongly discouraged to use both allow-scripts and allow-same-origin, as
  // that lets the embedded document remove the sandbox attribute — making it no
  // more secure than not using the sandbox attribute at all."
  //
  // As of December 2020, we've turned the allow-same-origin flag *on* despite
  // the fact that it basically un-sandboxes us - this was a product decision
  // after lots of back and forth: ultimately, it un-blocks a number of use-cases
  // without making Streamlit Components any less secure than they actually were,
  // since we don't sandbox a Component's Python code.

So, it seems to me like if there was a malicious component it could add that tag itself.

@vdonato
Copy link
Collaborator

vdonato commented Apr 2, 2021

@F1nnM to give you an update on this, the consensus was that we're most likely okay with allowing components to provide download buttons, but we may take some more time to run it by people with a bit more security expertise. Sorry if this is moving slowly for a tiny change, but we do want to be sure that we're extra-careful with any security concerns.

I'll go ahead and add my +1 since our processes require two approvals for community contributions, so final confirmation that we're okay adding this can come from the second reviewer.

@kmcgrady kmcgrady merged commit 73457ac into streamlit:develop Apr 4, 2021
@kmcgrady
Copy link
Collaborator

kmcgrady commented Apr 4, 2021

Looks great to me! Merging it!

@F1nnM
Copy link
Contributor Author

F1nnM commented Apr 4, 2021

Awesome. No worries that it took a little longer, I very much understand that security is an important concern. 4 days, including the easter weekend is still faster than I expected ^^
Thank you!

tconkling added a commit that referenced this pull request Apr 5, 2021
* develop:
  Added "allow-downloads" to the sandbox attributes (#3053)
  Small fix for `make pylint` command (#3062)
  Set genericColors properly and make theme defs more consistent (#3051)
  Downgrade st.warning in local_sources_watcher to a log (#3050)
  Extend docstring of st.image (#3055)
  Move docs.streamlit.io to Segment (#3005)
  Fix bokeh example in docs (`legend` arg) (#2907)
  Remove incorrect markdown table styling (#3038)
  document getattr
  Show the beta_warning only a single time per object
  object_beta_warning: handle every __ magic method
  object_beta_warning: handle the subscript operator
  beta_util_test
  Move secrets out of beta
  [Security] Upgrade y18n to 4.0.1 or later (#3041)
  Fix gap not working on Safari (#3042)
  formatting
  call _repr_html_ when available
tconkling added a commit to tconkling/streamlit that referenced this pull request Apr 5, 2021
* st_form:
  Added "allow-downloads" to the sandbox attributes (streamlit#3053)
  Small fix for `make pylint` command (streamlit#3062)
  Set genericColors properly and make theme defs more consistent (streamlit#3051)
  Downgrade st.warning in local_sources_watcher to a log (streamlit#3050)
  Extend docstring of st.image (streamlit#3055)
  Move docs.streamlit.io to Segment (streamlit#3005)
  Fix bokeh example in docs (`legend` arg) (streamlit#2907)
  Remove incorrect markdown table styling (streamlit#3038)
  document getattr
  Show the beta_warning only a single time per object
  object_beta_warning: handle every __ magic method
  object_beta_warning: handle the subscript operator
  beta_util_test
  Move secrets out of beta
  [Security] Upgrade y18n to 4.0.1 or later (streamlit#3041)
  Fix gap not working on Safari (streamlit#3042)
  formatting
  call _repr_html_ when available
tconkling added a commit to tconkling/streamlit that referenced this pull request Apr 5, 2021
# By Tim Conkling (7) and others
# Via GitHub (3) and Tim Conkling (1)
* st_form:
  FormsManager (streamlit#3046)
  Added "allow-downloads" to the sandbox attributes (streamlit#3053)
  Small fix for `make pylint` command (streamlit#3062)
  Set genericColors properly and make theme defs more consistent (streamlit#3051)
  Downgrade st.warning in local_sources_watcher to a log (streamlit#3050)
  Extend docstring of st.image (streamlit#3055)
  Move docs.streamlit.io to Segment (streamlit#3005)
  Fix bokeh example in docs (`legend` arg) (streamlit#2907)
  Remove incorrect markdown table styling (streamlit#3038)
  document getattr
  Show the beta_warning only a single time per object
  object_beta_warning: handle every __ magic method
  object_beta_warning: handle the subscript operator
  beta_util_test
  Move secrets out of beta
  [Security] Upgrade y18n to 4.0.1 or later (streamlit#3041)
  Fix gap not working on Safari (streamlit#3042)
  formatting
  call _repr_html_ when available

# Conflicts:
#	frontend/src/components/core/Block/Block.tsx
#	frontend/src/components/widgets/Form/Form.tsx
#	frontend/src/components/widgets/Form/FormSubmitButton.tsx
#	frontend/src/components/widgets/Form/FormsData.test.ts
#	frontend/src/components/widgets/Form/FormsManager.ts
kmcgrady pushed a commit that referenced this pull request Apr 6, 2021
* call _repr_html_ when available

* formatting

* Changes on the Git Deploy Button Frontend

* Update Cypress screenshot

* Small fix for `make pylint` command (#3062)

* make 'pylint' actually raise a non-zero exit

* run black so that 'make pylint' passes once more

* Added "allow-downloads" to the sandbox attributes (#3053)

* Added "allow-downloads" to the sandbox attributes

This enables components to provide a download button for example for data or images.

* Removed spaces

* Fixed Typo

* Trigger Build

* Fix pylint errors

* Specify pylint version

* Update sphinx

* See if pylint was red herring

* Is this a red herring also?

* Addressed PR comments

* Update snapshots

* Used separate variable

* Update snapshot

Co-authored-by: Jon Roes <[email protected]>
Co-authored-by: Jon Roes <[email protected]>
Co-authored-by: Emiliano Rosso <[email protected]>
Co-authored-by: Simon Biggs <[email protected]>
Co-authored-by: Finn <[email protected]>
Co-authored-by: Randy Zwitch <[email protected]>
Co-authored-by: Marisa Smith <[email protected]>
tconkling added a commit that referenced this pull request Apr 13, 2021
* develop: (165 commits)
  Add s4a message to close active modals (#2893)
  Add fail_on_warning to build options
  Docutils 0.16
  Put back Sphinx to 3.0.3
  Add import urllib to streamlit hello (#2969) (#3106)
  Update Num_Input to correct for Type Errors (#3074)
  Upgrade bokehjs to 2.2.2, and run `npx browserslist@latest --update-db` (#3090)
  remove extra star in docs (#3086)
  Remove flake8 linting tool (#3085)
  Makefile cleanup (#3083)
  Merge anchor headers feature branch into develop (#2983)
  Fix image galleries (#3044)
  Is this a red herring also?
  See if pylint was red herring
  Update sphinx
  Specify pylint version
  Fix pylint errors
  Added "allow-downloads" to the sandbox attributes (#3053)
  Small fix for `make pylint` command (#3062)
  Set genericColors properly and make theme defs more consistent (#3051)
  ...
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.

Can't trigger downloads from inside components

3 participants