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 warning about the cookie configuration #7255

Closed
wants to merge 1 commit into from

Conversation

mikeSimonson
Copy link
Contributor

@mikeSimonson mikeSimonson commented Aug 29, 2017

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926


This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Aug 29, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@laf
Copy link
Member

laf commented Aug 29, 2017

What's the reason for this? We actually setting as part of the authentication cookies. It's not used in install.php though.

@mikeSimonson
Copy link
Contributor Author

If the httponly setting is set to on in the php configuration the ajax request made in the step of the install where the database is updated brakes.

@murrant
Copy link
Member

murrant commented Aug 30, 2017

@mikeSimonson Interesting, could setting that explicitly in install.php fix the issue too? or maybe stop passing the variables from php to javascript.

@laf
Copy link
Member

laf commented Sep 7, 2017

@mikeSimonson Any comment?

@mikeSimonson
Copy link
Contributor Author

@murrant Maybe it could, I am not really sure that you can taught.

@laf
Copy link
Member

laf commented Sep 7, 2017

@mikeSimonson We set it in https://github.com/librenms/librenms/blob/master/html/includes/authenticate.inc.php#L7 already so seems possible to toggle it on / off in php. Not sure if that overwrites what the user sets in php.ini but it's worth a test.

@laf
Copy link
Member

laf commented Sep 14, 2017

@mikeSimonson bump

@laf
Copy link
Member

laf commented Sep 27, 2017

@mikeSimonson Feel free to comment on this still and we can re-open.

@laf laf closed this Sep 27, 2017
@lock
Copy link

lock bot commented May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants