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

Allow SameSite flag to be used in cookies #2388

Merged
merged 1 commit into from
Apr 19, 2018

Conversation

sg3s
Copy link

@sg3s sg3s commented Jan 29, 2018

This should resolve #2387 if accepted.

(btw, I think you should use trailing commas for array lists, but of course didn't add those to stay inline with the rest)

@coveralls
Copy link

coveralls commented Jan 29, 2018

Coverage Status

Coverage increased (+0.005%) to 97.078% when pulling fa35755 on sg3s:allow-samesite-in-cookie into 289fd6c on slimphp:3.x.

@sg3s sg3s force-pushed the allow-samesite-in-cookie branch from f00f7fe to 5767db9 Compare January 29, 2018 12:44
@sg3s
Copy link
Author

sg3s commented Jan 29, 2018

I do not know why Travis failed on hhvm, not familiar with either and the errors look vague to me.

If it is something I can fix I would love to hear suggestions.

@akrabat
Copy link
Member

akrabat commented Feb 14, 2018

This looks great. Would you mind raising a PR in https://github.com/slimphp/Slim-Website to document it?

Also, Slim 4.x will use https://github.com/slimphp/Slim-Http, so could you port this PR to that respository too, so we have it for the future?

@akrabat akrabat added this to the 3.9.2 milestone Feb 14, 2018
@akrabat
Copy link
Member

akrabat commented Feb 14, 2018

If you rebase against 3.x and then force push to your branch on your clone, then Travis should pass now.

@sg3s
Copy link
Author

sg3s commented Feb 14, 2018

Thank you for the feedback.

I can rebase & push quite easily, but looking back on my code I see that I have mistaken 'non-strict comparison' for 'case-insensitive comparison'. Which is a dumb mistake. To compare I need to normalize the input properly.

  1. Use strtolower or ucfirst (or something else?) on the input? (the used case doesn't actually matter for setting the cookie flags)
  2. Want a specific test for that case?

Cookies don't seem to be documented in v3 (I'm guessing the class shouldn't be present, it is mentioned in the v3 upgrade guide that it was removed) so where should I put this?

I will cherry-pick the patch to Slim-Http/v4 after figuring out the above.

@akrabat akrabat added the Slim 3 label Feb 18, 2018
@akrabat
Copy link
Member

akrabat commented Feb 18, 2018

  1. Use strtolower or ucfirst (or something else?) on the input? (the used case doesn't actually matter for setting the cookie flags)

Use strtolower() or stripos().

  1. Want a specific test for that case?

Yes.

@akrabat akrabat added the pending response Waiting on a response from the original poster label Feb 25, 2018
@akrabat akrabat modified the milestones: 3.9.2, 3.10.0 Apr 14, 2018
@akrabat
Copy link
Member

akrabat commented Apr 14, 2018

@sg3s are you likely to be able to work on this?

@sg3s sg3s force-pushed the allow-samesite-in-cookie branch from 5767db9 to 95fef86 Compare April 16, 2018 07:33
@sg3s sg3s force-pushed the allow-samesite-in-cookie branch from 95fef86 to fa35755 Compare April 16, 2018 08:05
@sg3s
Copy link
Author

sg3s commented Apr 16, 2018

@akrabat My apologies, I think I've missed the previous comment and I have been quite busy so I wasn't checking up on it either.

Updated, added test & rebased. I think I've done that correctly, feedback is welcome :)

@akrabat
Copy link
Member

akrabat commented Apr 19, 2018

Thanks @sg3s! Fancy putting the same patch into https://github.com/slimphp/Slim-Http ?

@akrabat akrabat merged commit fa35755 into slimphp:3.x Apr 19, 2018
akrabat added a commit that referenced this pull request Apr 19, 2018
@akrabat akrabat removed the pending response Waiting on a response from the original poster label Apr 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not possible to use SameSite in cookies
3 participants