-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
feat(core)!: Set the secure
flag on issued cookies
#8812
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Should we mark this as a breaking change? As it does break the app if someone is hosting it over http.
✅ All Cypress E2E specs passed |
12cd613
to
b1f0546
Compare
secure
flag on issued cookiessecure
flag on issued cookies
Updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
@@ -15,6 +15,7 @@ if (inE2ETests) { | |||
process.env.N8N_LOG_LEVEL = 'silent'; | |||
process.env.N8N_PUBLIC_API_DISABLED = 'true'; | |||
process.env.SKIP_STATISTICS_EVENTS = 'true'; | |||
process.env.N8N_SECURE_COOKIE = 'false'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could set it to true in tests so it corresponds the most common config? Or leave it out so the default value gets tested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we unfortunately can't to do that right now because superagent
doesn't handle secure cookies, which breaks every integration test that updates the cookie in response.
✅ All Cypress E2E specs passed |
Got released with |
Currently n8n does not set
secure
flag on issues cookies. This means that if an instance is reachable over http, it could be vulnerable to MITM attacks.N8N-7246
Review / Merge checklist