-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
cookie_domain config option for setting cookie on a wider domain #51657
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
cookie_domain config option for setting cookie on a wider domain #51657
Conversation
icewind1991
left a comment
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.
Looks good otherwise
0add860 to
56074a9
Compare
|
I also wondered if / how this option would be enforced for nextcloud apps. I don't know the codebase, so I can't tell if they will automatically pick this option for apps-defined cookies. |
|
Apps should generally not be doing their own cookie logic |
56074a9 to
0f78922
Compare
come-nc
left a comment
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.
Please use typed version of the getter when possble (not possible in base.php as it’s not the same config class in use)
6c904c5 to
f5a89f0
Compare
12c6c07 to
f5a89f0
Compare
f5a89f0 to
2b03b8e
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
Hello. Is there anything missing to this PR before merge ? Thanks. |
Signed-off-by: Samuel Bizien Filippi <[email protected]>
2878964 to
a14cade
Compare
|
Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 |
Summary
Adds a
cookie_domainoption to define to which domain(s) the cookies sent by Nextcloud are valid. By default, it is set to''which is the safe option (i.e. the browser is instructed to send the cookie only for request to the exact same domain that issued it).But when your instance is accessible over 2 domains, for example 'mycloud.mydomain.example' and 'sub.mycloud.mydomain.example', setting
cookie_domainto 'mycloud.mydomain.example' will make the cookie valid formycloud.mydomain.exampleand any subdomain (but not formydomain.example).Documentation : MDN / Cookies / Define where cookies are sent.
TODO
I've updated config.sample.php, but it's not clear where should this functionality should be documented.
Checklist