Enable CSRF via cookie to header approach #1551

karriebear · 2020-06-08T17:27:25Z

Issue: Fixes #1524

Description:

Add a config option for server.cookieSecret. Secret can be set in config.toml or as an environment variable or CLI argument. If no secret is provided, a random hex is generated (this is not recommended for deployment to multiple replicas).
Enable XSRF on the server.
Update the UploadFile request handler CORs to enable cookies and XSRF token.
Update the FileUploadClient to pass the XSRF token.

Contribution License Agreement

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

lib/streamlit/config.py

lib/streamlit/server/Server.py

lib/streamlit/server/UploadFileRequestHandler.py

lib/tests/streamlit/config_test.py

Amey-D · 2020-06-10T00:35:48Z

lib/tests/streamlit/config_test.py

@@ -28,10 +28,11 @@
 from streamlit import env_util
 from streamlit.ConfigOption import ConfigOption

+os.environ["STREAMLIT_COOKIE_SECRET"] = "chocolatechip"


If we are patching the os.environ dict during ConfigTest.setUp(), why do we need modify the real os.environ dict?

Thanks for the catch. Ideally we would go with patching in ConfigTest.setUp() but unfortunately this conflicts when running all the tests. config.cookieSecret is saved on first call and other tests references this config without the patch, setting the config without using the environment variable. Modifying the os.environ dict here so it can set before any other test will run. Need to clean up ConfigTest.setup()

Maybe this could be moved inside the setUp() function then, given that we try to delete it during tearDown()?

If we move it into setUp(), it looks like it's too late. config.cookieSecret gets set and saved to a random value from another test calling it. by the time config_test runs and sets up the environment variable, we already have a random value set assuming no environment variable exist.

I should actually remove the deleting of the variable into the actual test instead of tearDown()

Ah OK, sounds good, thanks for looking into it!

Co-authored-by: Amey Deshpande <10554902+Amey-D@users.noreply.github.com>

Amey-D · 2020-06-10T18:09:44Z

lib/tests/streamlit/config_test.py

@@ -28,10 +28,11 @@
 from streamlit import env_util
 from streamlit.ConfigOption import ConfigOption

+os.environ["STREAMLIT_COOKIE_SECRET"] = "chocolatechip"


Maybe this could be moved inside the setUp() function then, given that we try to delete it during tearDown()?

Amey-D · 2020-06-10T18:19:04Z

frontend/src/lib/utils.test.ts

@@ -52,3 +52,17 @@ describe("flattenElements", () => {
    )
  })
 })
+
+describe("getCookie", () => {
+  document.cookie = "flavor=chocolatechip"


I missed this part earlier, but it looks like document.cookie is a string containing a semicolon-separated list of all cookies (i.e. key=value pairs) (ref). Given getCookie() uses regex, it might be worthwhile adding testcases when the cookie appears at the first and last locations in the list of all cookies:

document.cookie = "flavor=chocolatechip; type=darkchocolate; size=medium"

document.cookie = "type=darkchocolate; flavor=chocolatechip; size=medium"

and

document.cookie = "type=darkchocolate; size=medium; flavor=chocolatechip"

etc

good point, i'll add more test cases!

Amey-D · 2020-06-10T20:46:35Z

lib/tests/streamlit/config_test.py

@@ -28,10 +28,11 @@
 from streamlit import env_util
 from streamlit.ConfigOption import ConfigOption

+os.environ["STREAMLIT_COOKIE_SECRET"] = "chocolatechip"


Ah OK, sounds good, thanks for looking into it!

* develop: Remove Python 3 reference, add 288 ref (#1582) Use env variable for environment name (#1569) Add diffmazing_cropper.sh to convert diff images from Cypress into snapshots you can commit to Git (#1560) Fix progress bar value (#1573) Updated run_streamlit_remotely.md (#1578) Replace iterrows with itertuples in st.map code (#1562) Pass in server.maxUploadSize to http server max_buffer_size (#1558) Enable CSRF via cookie to header approach (#1551)

* feature/plugins: (CC) Add EventTarget shim (streamlit#1577) Ensure node_modules doesn't land in LocalSourcesWatcher (streamlit#1575) Disable allow-same-origin for security reasons (streamlit#1576) Remove Python 3 reference, add 288 ref (streamlit#1582) Use env variable for environment name (streamlit#1569) Add diffmazing_cropper.sh to convert diff images from Cypress into snapshots you can commit to Git (streamlit#1560) Fix progress bar value (streamlit#1573) Updated run_streamlit_remotely.md (streamlit#1578) Replace iterrows with itertuples in st.map code (streamlit#1562) Pass in server.maxUploadSize to http server max_buffer_size (streamlit#1558) Enable CSRF via cookie to header approach (streamlit#1551)

karriebear requested a review from Amey-D June 8, 2020 17:27

karriebear force-pushed the feature/1524-CSRF-for-file-uploads branch 2 times, most recently from b2e10c6 to c3cc9ad Compare June 8, 2020 17:50

Amey-D reviewed Jun 8, 2020

View reviewed changes

lib/streamlit/config.py Outdated Show resolved Hide resolved

karriebear force-pushed the feature/1524-CSRF-for-file-uploads branch from 2e61ae1 to 6b049bf Compare June 8, 2020 19:09

karriebear marked this pull request as ready for review June 8, 2020 21:23

karriebear requested a review from a team as a code owner June 8, 2020 21:23

Enable CSRF via cookie to header approach

b5afdb5

karriebear force-pushed the feature/1524-CSRF-for-file-uploads branch from 6b049bf to b5afdb5 Compare June 9, 2020 21:21

Amey-D reviewed Jun 10, 2020

View reviewed changes

Update lib/streamlit/server/Server.py

f81b701

Co-authored-by: Amey Deshpande <10554902+Amey-D@users.noreply.github.com>

karriebear force-pushed the feature/1524-CSRF-for-file-uploads branch from 669ba60 to d6b65ca Compare June 10, 2020 16:47

Pep8 spacing + test cleanup

c9e795c

karriebear force-pushed the feature/1524-CSRF-for-file-uploads branch from d6b65ca to c9e795c Compare June 10, 2020 17:47

Amey-D reviewed Jun 10, 2020

View reviewed changes

Add more tests for getCookie + cleanup

f6664e7

karriebear force-pushed the feature/1524-CSRF-for-file-uploads branch from 836d054 to f6664e7 Compare June 10, 2020 19:21

Amey-D approved these changes Jun 10, 2020

View reviewed changes

karriebear merged commit bab76d5 into streamlit:develop Jun 10, 2020

karriebear mentioned this pull request Jun 11, 2020

Create a config option to toggle XSRF in Streamlit server #1565

Closed

karriebear deleted the feature/1524-CSRF-for-file-uploads branch July 20, 2020 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable CSRF via cookie to header approach #1551

Enable CSRF via cookie to header approach #1551

karriebear commented Jun 8, 2020 •

edited

Amey-D Jun 10, 2020

karriebear Jun 10, 2020 •

edited

Amey-D Jun 10, 2020

karriebear Jun 10, 2020 •

edited

Amey-D Jun 10, 2020

Amey-D Jun 10, 2020

Amey-D Jun 10, 2020

karriebear Jun 10, 2020

Amey-D Jun 10, 2020

Enable CSRF via cookie to header approach #1551

Enable CSRF via cookie to header approach #1551

Conversation

karriebear commented Jun 8, 2020 • edited

Amey-D Jun 10, 2020

Choose a reason for hiding this comment

karriebear Jun 10, 2020 • edited

Choose a reason for hiding this comment

Amey-D Jun 10, 2020

Choose a reason for hiding this comment

karriebear Jun 10, 2020 • edited

Choose a reason for hiding this comment

Amey-D Jun 10, 2020

Choose a reason for hiding this comment

Amey-D Jun 10, 2020

Choose a reason for hiding this comment

Amey-D Jun 10, 2020

Choose a reason for hiding this comment

karriebear Jun 10, 2020

Choose a reason for hiding this comment

Amey-D Jun 10, 2020

Choose a reason for hiding this comment

karriebear commented Jun 8, 2020 •

edited

karriebear Jun 10, 2020 •

edited

karriebear Jun 10, 2020 •

edited