Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New toolbar cookie + Secure session/csrf cookies #1387

Merged
merged 15 commits into from
Aug 11, 2020

Conversation

mariusandra
Copy link
Collaborator

Changes

This PR does two things.

First,

The django documentation says the following:

If a browser connects initially via HTTP, which is the default for most browsers, it is possible for existing cookies to be leaked. For this reason, you should set your SESSION_COOKIE_SECURE and CSRF_COOKIE_SECURE settings to True.

This PR does that. These settings now default to True and revert to False if running under DEBUG/TEST or if the env var DISABLE_SECURE_COOKIES is set.

This PR will be breaking if you're running posthog under regular HTTP in production (no nginx ssl proxy, no nothing). Then you need to set the DISABLE_SECURE_COOKIES flag.

This should be documented if the PR is merged.

Second,

This PR sets a new cross-domain cookie, phtoolbar, which is available to the /decide endpoint. It uses it to determine that the user is logged in and returns true for isAuthenticated... causing the toolbar to appear and ask for authorization.

This is a lot safer than setting the session cookie to be cross domain, as in case there's an error with this cookie (strong security in the browser, etc), nothing bad will happen. You'll just need to click "Launch Toolbar" manually.

Caveats: I'm not sure if I should add tests for this. Probably, if so, it's a WIP. I just ran out of time now.

Checklist

  • All querysets/queries filter by Team (if this PR affects any querysets/queries)
  • Backend tests (if this PR affects the backend)
  • Cypress E2E tests (if this PR affects the front and/or backend)

@timgl timgl temporarily deployed to posthog-secure-and-tool-i0ek4p August 7, 2020 14:25 Inactive
@mariusandra
Copy link
Collaborator Author

Tests are failing, setting to WIP/draft

@mariusandra mariusandra marked this pull request as draft August 7, 2020 14:55
@mariusandra mariusandra temporarily deployed to posthog-secure-and-tool-i0ek4p August 10, 2020 08:43 Inactive
@mariusandra mariusandra temporarily deployed to posthog-secure-and-tool-i0ek4p August 10, 2020 08:45 Inactive
@mariusandra mariusandra temporarily deployed to posthog-secure-and-tool-i0ek4p August 10, 2020 09:47 Inactive
@mariusandra
Copy link
Collaborator Author

This is waiting behind a new posthog-js release, which is waiting for a review on PostHog/posthog-js#71 .

@mariusandra mariusandra mentioned this pull request Aug 10, 2020
@mariusandra mariusandra temporarily deployed to posthog-secure-and-tool-i0ek4p August 10, 2020 13:08 Inactive
@mariusandra mariusandra temporarily deployed to posthog-secure-and-tool-i0ek4p August 10, 2020 13:20 Inactive
@mariusandra mariusandra temporarily deployed to posthog-secure-and-tool-i0ek4p August 10, 2020 13:51 Inactive
@mariusandra mariusandra temporarily deployed to posthog-secure-and-tool-i0ek4p August 10, 2020 14:13 Inactive
@mariusandra
Copy link
Collaborator Author

Small change from the text above. Instead of the DISABLE_* env, there's now a variable SECURE_COOKIES that should either be unset (defaults to False in DEBUG/TEST and True otherwise) or then explicitly set to True/False depending if the app is running behind https or not. This adds the Secure flag to all cookies, bringing us in line with what the django docs and Chrome's errors suggest we do.

This might cause trouble for people upgrading who have some special configs... and this new variable should be well documented.


Some issue still with cypress tests...?

@mariusandra
Copy link
Collaborator Author

This might be outside the scope for this PR, but after giving it a though, I'd actually nuke DISABLE_SECURE_SSL_REDIRECT, always set SECURE_SSL_REDIRECT to False and let users manage their own SSL.

This setting makes sense only for users who direct both their HTTP and HTTPS traffic directly to their app. How many people do that? What's the risk here vs benefit of an easier setup with fewer compilcated env variables?

@timgl
Copy link
Collaborator

timgl commented Aug 10, 2020

The big one is Heroku, where if you don't set it you allow both http and https access.

@mariusandra mariusandra temporarily deployed to posthog-secure-and-tool-i0ek4p August 10, 2020 20:17 Inactive
@mariusandra mariusandra temporarily deployed to posthog-secure-and-tool-i0ek4p August 10, 2020 20:48 Inactive
@mariusandra mariusandra temporarily deployed to posthog-secure-and-tool-i0ek4p August 10, 2020 20:54 Inactive
@mariusandra mariusandra temporarily deployed to posthog-secure-and-tool-i0ek4p August 10, 2020 21:07 Inactive
@mariusandra mariusandra temporarily deployed to posthog-secure-and-tool-i0ek4p August 10, 2020 21:13 Inactive
@mariusandra mariusandra temporarily deployed to posthog-secure-and-tool-i0ek4p August 10, 2020 21:27 Inactive
@mariusandra
Copy link
Collaborator Author

  • I added a test for the toolbar cookie middleware
  • Cypress tests pass again.
  • This PR also now contains cypress-terminal-report, which gives detailed logs for cypress runs if they fail. It helped debug my issue (bad host on XHR) and seems like a practical thing to have around:

image

Regarding heroku... oh well. I think then it could make sense to eventually flip the SSL redirect toggle, so it must be explicitly enabled. There are bigger fish to fry now.

@mariusandra mariusandra marked this pull request as ready for review August 10, 2020 21:34
@mariusandra mariusandra requested a review from timgl August 10, 2020 21:34
@timgl timgl merged commit 52cdd7e into master Aug 11, 2020
@timgl timgl deleted the secure-and-toolbar-cookies branch August 11, 2020 08:52
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.

2 participants