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

Add a middleware for referrer policy #7346

Merged
merged 3 commits into from Aug 4, 2020

Conversation

davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Jul 31, 2020

This sets a referrer policy with the value of strict-origin-when-cross-origin for the RTD dashboard. Proxito (which serves documentation sites) will use no-referrer-when-downgrade so that API calls from docs sites to readthedocs.org will report the referrer.

Setting the setting SECURE_REFERRER_POLICY=None will turn off the middleware.

I didn't use the existing django-referrer-policy module because this new middleware is setup so that it can just be removed when we upgrade to Django 3. Referrer policy is a built-in feature of Django's SecurityMiddleware (which we use) in Django 3. The django-referrer-policy uses a different setting name than what Django uses in Django3.

@davidfischer davidfischer requested a review from a team Jul 31, 2020
@davidfischer
Copy link
Contributor Author

davidfischer commented Aug 1, 2020

Chrome is going to make strict-origin-when-cross-origin the default in the next few months. Maybe we should use the same for Proxito/API calls and live with the slightly reduced data. It's probably not needed.

@@ -64,6 +64,8 @@ server {
add_header X-RTD-Redirect $rtd_redirect always;
set $cache_tag $upstream_http_cache_tag;
add_header Cache-Tag $cache_tag always;
set $referrer_policy $upstream_http_referrer_policy;
add_header Referrer-Policy $referrer_policy always;
Copy link
Member

@ericholscher ericholscher Aug 4, 2020

Choose a reason for hiding this comment

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

This needs a corresponding ops change as well.

Copy link
Contributor Author

@davidfischer davidfischer Aug 4, 2020

Choose a reason for hiding this comment

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

Yes it does although the critical piece is getting the stricter policy for the dashboard.

Copy link
Contributor Author

@davidfischer davidfischer Aug 4, 2020

Choose a reason for hiding this comment

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

I added a PR for the corresponding ops change.

@@ -11,6 +11,7 @@ class CommunityProxitoSettingsMixin:

ROOT_URLCONF = 'readthedocs.proxito.urls'
USE_SUBDOMAIN = True
SECURE_REFERRER_POLICY = "no-referrer-when-downgrade"
Copy link
Member

@ericholscher ericholscher Aug 4, 2020

Choose a reason for hiding this comment

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

This makes sense 👍

Copy link
Contributor Author

@davidfischer davidfischer Aug 4, 2020

Choose a reason for hiding this comment

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

To be honest, I think strict-origin-when-cross-origin would be fine for docs sites as we don't really use the additional data as far as I'm aware but at least we have an easy upgrade path this way.

@davidfischer davidfischer merged commit f48fb66 into master Aug 4, 2020
2 checks passed
@davidfischer davidfischer deleted the davidfischer/referrer-policy branch Aug 4, 2020
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.

None yet

2 participants