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

Implement secure cookies by default #2107

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kdp-cloud
Copy link
Collaborator

  • Use for secure cookies in a production environment
  • Force SSL in production environment

@kdp-cloud kdp-cloud added this to the 1.16.1 milestone Jan 8, 2025
@kdp-cloud kdp-cloud self-assigned this Jan 8, 2025
Copy link
Member

@stuzart stuzart left a comment

Choose a reason for hiding this comment

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

I'm not clear how this will affect running with docker and an nginx / apache front end forwarder. Will need some testing, possible ngnix configuration changes, and maybe an ENV variable to override.
For these reasons I think it might be better left for the main branch and next major version

@kdp-cloud kdp-cloud changed the base branch from seek-1.16 to main January 13, 2025 17:00
@kdp-cloud kdp-cloud modified the milestones: 1.16.1, 1.17.0 Jan 13, 2025
@kdp-cloud
Copy link
Collaborator Author

I'm not clear how this will affect running with docker and an nginx / apache front end forwarder. Will need some testing, possible ngnix configuration changes, and maybe an ENV variable to override. For these reasons I think it might be better left for the main branch and next major version

I changed the branch to main and the milestone to 1.17, but I'm not sure you should override this config in production since this implicates security issues.

@kdp-cloud kdp-cloud requested a review from stuzart January 13, 2025 17:33
@stuzart
Copy link
Member

stuzart commented Jan 15, 2025

I'm not clear how this will affect running with docker and an nginx / apache front end forwarder. Will need some testing, possible ngnix configuration changes, and maybe an ENV variable to override. For these reasons I think it might be better left for the main branch and next major version

I changed the branch to main and the milestone to 1.17, but I'm not sure you should override this config in production since this implicates security issues.

The docker containers always run in production, but are often used through http, e.g for testing, trying out seek, local installation behind a firewall or reverse proxy or load balancer.

I had a good look into this yesterday and some testing, and all is needed is the config.force_ssl, as this also sets the secure cookie https://api.rubyonrails.org/v7.2.2.1/classes/ActionDispatch/SSL.html, which also sets the HSTS (which I found afterwards blocked me from localhost:3000 and was a pain to remove, ending up needing to go to chrome://net-internals/#hsts and I'd be curious how this affects the http->https redirect as it being blocked by my browser )

You may also configure secure cookies separately through the apache or nginx config, which is what we do for https://fairdomhub.org - which may be preferable as it separates the application logic from deployment.

My conclusion is that it would be best to allow force_ssl to be turned on with an environment variable, but off by default, but update our docs and docker-compose file to recommend turning it on if deploying a production instance on the web, if the front end proxy hasn't already been configured instead. We can discuss this more in the meeting today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Coded
Development

Successfully merging this pull request may close these issues.

2 participants