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

Use of SameSite=Strict #16386

Merged
merged 1 commit into from
Nov 1, 2020
Merged

Use of SameSite=Strict #16386

merged 1 commit into from
Nov 1, 2020

Conversation

rajat315315
Copy link
Contributor

Signed-off-by: Rajat Jain rajatjain.ix@gmail.com

Description

Added new cookie parameter SameSite=Strict.

Fixes #16316

Before submitting pull request, please review the following checklist:

  • Make sure you have read our CONTRIBUTING.md document.
  • Make sure you are making a pull request against the correct branch. For example, for bug fixes in a released version use the corresponding QA branch and for new features use the master branch. If you have a doubt, you can ask as a comment in the bug report or on the mailing list.
  • Every commit has proper Signed-off-by line as described in our DCO. This ensures that the work you're submitting is your own creation.
  • Every commit has a descriptive commit message.
  • Every commit is needed on its own, if you have just minor fixes to previous commits, you can squash them.
  • Any new functionality is covered by tests.

@rajat315315 rajat315315 force-pushed the fix-16316 branch 2 times, most recently from 57175b2 to 5f66225 Compare October 6, 2020 11:09
@williamdes williamdes added this to the 5.1.0 milestone Oct 6, 2020
@williamdes williamdes added this to In progress in pull-requests via automation Oct 6, 2020
@williamdes williamdes moved this from In progress to Review in progress in pull-requests Oct 6, 2020
Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

LGTM

pull-requests automation moved this from Review in progress to Reviewer approved Oct 6, 2020
libraries/classes/Config.php Outdated Show resolved Hide resolved
libraries/classes/Config.php Outdated Show resolved Hide resolved
pull-requests automation moved this from Reviewer approved to Review in progress Oct 8, 2020
Copy link
Contributor Author

@rajat315315 rajat315315 left a comment

Choose a reason for hiding this comment

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

@MauricioFauth is it better now?

libraries/classes/Config.php Outdated Show resolved Hide resolved
@rajat315315
Copy link
Contributor Author

@MauricioFauth is it better?

libraries/classes/Config.php Outdated Show resolved Hide resolved
@rajat315315
Copy link
Contributor Author

Just a request..
Can we add hacktoberfest-accepted label to PR if it gets merged?

'domain' => '',
'secure' => $this->isHttps(),
'httponly' => $httponly,
'samesite' => 'Strict',
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really sure if it's a good idea to hard-code the Strict value. Maybe we could use a configuration directive and use Strict by default. What do you think @williamdes?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, this is a better idea.

Copy link
Member

Choose a reason for hiding this comment

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

@rajat315315 Could you do this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@MauricioFauth
Copy link
Member

Can we add hacktoberfest-accepted label to PR if it gets merged?

All pull requests for this repository are valid for Hacktoberfest. Don't need to add the hacktoberfest-accepted label. 😉

pull-requests automation moved this from Review in progress to Reviewer approved Oct 9, 2020
@MauricioFauth
Copy link
Member

@rajat315315 Could you please fix the errors found by phpcs?
https://travis-ci.org/github/phpmyadmin/phpmyadmin/jobs/734257860#L434

@rajat315315
Copy link
Contributor Author

Done.

@rajat315315
Copy link
Contributor Author

So, I have made samesite as a configuration directive and force-pushed.

@williamdes
Copy link
Member

Hi @rajat315315
I appreciate your contribution :)
Could you look at #16406 to add a configuration option?

@rajat315315
Copy link
Contributor Author

Oh, ok.. got it!
Sorry, I mis-understood a bit.

@rajat315315
Copy link
Contributor Author

Is it better?

Copy link
Member

@MauricioFauth MauricioFauth left a comment

Choose a reason for hiding this comment

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

You should add this configuration to the documentation as well.

libraries/config.default.php Outdated Show resolved Hide resolved
pull-requests automation moved this from Reviewer approved to Review in progress Oct 14, 2020
Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

LGTM for now, missing some documentation :)

@rajat315315
Copy link
Contributor Author

Do I need to make changes to doc/config.rst ?

@williamdes
Copy link
Member

Do I need to make changes to doc/config.rst ?

Yes, please

this will solve

You should add this configuration to the documentation as well.

@rajat315315
Copy link
Contributor Author

Done.

@rajat315315 rajat315315 force-pushed the fix-16316 branch 3 times, most recently from 2499bfd to 1add2cc Compare October 15, 2020 06:24
doc/config.rst Outdated Show resolved Hide resolved
Signed-off-by: Rajat Jain <rajatjain.ix@gmail.com>

Update Config.php

Polyfilled

version fixes

Signed-off-by: Rajat Jain <rajatjain.ix@gmail.com>

Update libraries/classes/Config.php

Co-authored-by: Maurício Meneghini Fauth <mauricio@fauth.dev>

phpcs fixes

samesite made as configuration directive

bugfix, sets sameSite as global configuration directive

CodeReviewed

Changed config.rst

IETF RFC link aded

Version added

Trailing whitespace fixed.

RFC hyperlink added

trailing whitespace
Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

💯

@williamdes williamdes self-assigned this Nov 1, 2020
@williamdes williamdes merged commit e7feedd into phpmyadmin:master Nov 1, 2020
pull-requests automation moved this from Review in progress to Done Nov 1, 2020
@williamdes
Copy link
Member

williamdes commented Nov 1, 2020

I improved the documentation with ce6bd5b
(valid values is more used in the documentation)
image

@williamdes williamdes mentioned this pull request Jan 15, 2021
6 tasks
williamdes added a commit that referenced this pull request Jan 21, 2021
Pull-request: #16577
Fixes: #16544
Signed-off-by: William Desportes <williamdes@wdes.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
pull-requests
  
Done
Development

Successfully merging this pull request may close these issues.

Consider use of SameSite=Strict
3 participants