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

Remove public cache-controls (fixes settings override, and perhaps shouldn't be there anyway) #81

Merged
merged 5 commits into from
Apr 17, 2023

Conversation

weshinsley
Copy link
Contributor

This makes sure HSTS turns on by copying add_header to places where it otherwise would not get inherited.

See https://mrc-ide.myjetbrains.com/youtrack/issue/VIMC-7074
and https://www.nginx.com/blog/http-strict-transport-security-hsts-and-nginx/

@weshinsley weshinsley requested review from richfitz and hillalex March 30, 2023 19:06
Copy link
Member

@richfitz richfitz left a comment

Choose a reason for hiding this comment

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

Suggest reducing duplication here by using the include command: http://nginx.org/en/docs/ngx_core_module.html#include

@weshinsley
Copy link
Contributor Author

weshinsley commented Mar 31, 2023

Yes - we could do that - or perhaps even easier:

Could add_header Cache-Control "public"; be moved to the top level, since it is common to all three locations which cause the cancelling of global settings. Would it be problematic if that were also inherited in the other locations?

@weshinsley
Copy link
Contributor Author

Tested on prod2 - the HSTS header seems correctly set, and everything appears to be working.

@weshinsley weshinsley changed the title Copy headers to loc sections Rmove public cache-controls (fixes settings override, and perhaps shouldn't be there anyway) Apr 9, 2023
@weshinsley weshinsley changed the title Rmove public cache-controls (fixes settings override, and perhaps shouldn't be there anyway) Remove public cache-controls (fixes settings override, and perhaps shouldn't be there anyway) Apr 9, 2023
@weshinsley weshinsley requested a review from richfitz April 17, 2023 10:35
@richfitz richfitz merged commit 2a82ae4 into master Apr 17, 2023
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