Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Bug fix for incorrect SameSite=none behaviour on some platforms. #386

Merged
merged 11 commits into from
Feb 6, 2020

Conversation

jedimdan
Copy link
Contributor

@jedimdan jedimdan commented Feb 4, 2020

A bug was discovered from PR #382 in ShopSession.php's new getBrowserDetails() and getPlatformDetails() method that causes the app to not return SameSite=none on some platforms. This is likely caused by the fact that some platform's version numbers do not have a minor number.

The proposed fix is to rely on Jenssegers\Agent's built-in functions that can generate a float version number from any user-agent.

Reference: #382 (comment)_

Other fixes include:

  • A mistake in the unit-test, where a compatible UCBrowser user-agent wound up in the incompatible list (confusion probably caused because both "Chrome" and "UCBrowser" is mentioned in the user-agent).
  • Flipping the logic from a whitelist to a blacklist, since SameSite=None is expected to be a norm on more browsers moving forward (Refer to this comment)
  • Updated Travis Laravel version constraint to properly support Laravel's new Semantic Versioning

…ents. Instead of manually generating the major and minor numbers of a version, to rely on the Agent library's built in tools.
… now using Semver. previously, it would only receive bug-fixes to 6.0, but now minor releases are in 6.* as well.
@jedimdan
Copy link
Contributor Author

jedimdan commented Feb 4, 2020

@mferrario @muhammadasfar Could you guys share some user agents that you've seen fail on your app so we can add it to the tests to confirm that this fixes the issues you're seeing?

@jedimdan
Copy link
Contributor Author

jedimdan commented Feb 4, 2020

@darrynten Would like your feedback on this PR

@coveralls
Copy link

coveralls commented Feb 4, 2020

Coverage Status

Coverage decreased (-0.002%) to 99.773% when pulling 8e4bb7c on jedimdan:bugfix-samesite-compatibility into 2f6f1d2 on ohmybrew:master.

@jedimdan
Copy link
Contributor Author

jedimdan commented Feb 4, 2020

@darrynten also right now the logic is a whitelist of compatible browsers rather than a blacklist of incompatible browsers. Based on the Chrome team's writeup, they suggest the blacklist as all other browsers should not have an issue with the new cookie flag. We also do expect more browsers to follow suit in the future, so having a blacklist might make more sense since there won't be a need to keep updating the code every time a new browser vendor adopts the same requirement.

@muhammadasfar
Copy link

muhammadasfar commented Feb 4, 2020

@jedimdan

I am using chrome 80 Beta and user agents is

Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.78 Safari/537.36

Domain: https://myprojectname.test

In my case this function return is Undefined offset: 1 as show explanation in screenshot
samesite-none-issue-src

And now I am trying this version release on my live app domain for confirmation.

@mferrario
Copy link

mferrario commented Feb 4, 2020

@mferrario @muhammadasfar Could you guys share some user agents that you've seen fail on your app so we can add it to the tests to confirm that this fixes the issues you're seeing?

@jedimdan here are two examples:

My Code:
`
$agent = new Agent();
$browser = $agent->browser();
$version = $agent->version($browser);
$platform = $agent->platform();
$platform_version = $agent->version($platform);
$robot = $agent->robot();

// print
print "Visitor: ".$_SERVER['SERVER_NAME']." | $browser - $version | $platform - $platform_version | Robot: $robot";
`
// Actual values
Visitor: 52.0.4.244 | Mozilla - | - | Robot: Zgrab
Visitor: secure-stage.jerseywatch.net | Mozilla - | - | Robot: Nimbostratus-Bot

@jedimdan
Copy link
Contributor Author

jedimdan commented Feb 4, 2020

@mferrario Could you try printing $_SERVER['HTTP_USER_AGENT'] instead? As the error we are trying to fix has to do with deciphering the actual user agent string.

src/ShopifyApp/Services/ShopSession.php Show resolved Hide resolved
src/ShopifyApp/Services/ShopSession.php Show resolved Hide resolved
tests/Services/ShopSessionTest.php Show resolved Hide resolved
tests/Services/ShopSessionTest.php Show resolved Hide resolved
@jedimdan
Copy link
Contributor Author

jedimdan commented Feb 4, 2020

@darrynten by the way, apologies for not spotting and highlighting some of these in your original PR.

@gnikyt gnikyt added fix-in-progress In progress bug Bug with the code labels Feb 4, 2020
@gnikyt gnikyt self-assigned this Feb 4, 2020
@gnikyt
Copy link
Owner

gnikyt commented Feb 4, 2020

Feel free to tag me once you feel its out of draft, and I'll gladly push a patch release.

@mferrario
Copy link

@mferrario Could you try printing $_SERVER['HTTP_USER_AGENT'] instead? As the error we are trying to fix has to do with deciphering the actual user agent string.

Here are some new examples:

//PRINT
print "Visitor: ".$_SERVER['SERVER_NAME']." :: ".$_SERVER['HTTP_USER_AGENT']." | $browser - $version | $platform - $platform_version | Robot: $robot";

// OUTPUT
Visitor: 52.0.4.244 :: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 | Firefox - 52.0 | Linux - | Robot:
Visitor: 54.152.130.178 :: Mozilla/5.0 zgrab/0.x | Mozilla - | - | Robot: Zgrab
Visitor: www.doingaworldofgood.org :: Mozilla/5.0 (compatible; Baiduspider/2.0; +http://www.baidu.com/search/spider.html) | Mozilla - | - | Robot: Baiduspider
Visitor: 54.152.130.178 :: Mozilla/5.0 NetSeen/1.0 | Mozilla - | - | Robot:
Visitor: stage.jerseywatch.com :: Mozilla/5.0 (compatible; Nimbostratus-Bot/v1.3.2; http://cloudsystemnetworks.com) | Mozilla - | - | Robot: Nimbostratus-Bot
Visitor: example.com :: AWS Security Scanner | - | - | Robot: AWS Security Scanner
Visitor: [::ffff:a9fe:a9fe] :: AWS Security Scanner | - | - | Robot: AWS Security Scanner
Visitor: 169.254.169.254 :: AWS Security Scanner | - | - | Robot: AWS Security Scanner
Visitor: 52.0.4.244 :: checks | - | - | Robot:
Visitor: 54.152.130.178 :: Mozilla/5.0 NetSeen/1.0 | Mozilla - | - | Robot:
Visitor: 169.254.169.254 :: AWS Security Scanner | - | - | Robot: AWS Security Scanner
Visitor: [::ffff:a9fe:a9fe] :: AWS Security Scanner | - | - | Robot: AWS Security Scanner

@jedimdan
Copy link
Contributor Author

jedimdan commented Feb 4, 2020

@darrynten One more thing I wanted to ask: with the change to relying on the Agent class' built-in float versions, I assume the try-catch is no longer necessary? Also, let me know if my answers to your comments are acceptable.

@darrynten
Copy link
Contributor

@jedimdan yes, it seems the try/catch will not be needed anymore

@jedimdan jedimdan marked this pull request as ready for review February 5, 2020 12:44
@jedimdan
Copy link
Contributor Author

jedimdan commented Feb 5, 2020

Thanks, @ohmybrew, this PR is ready. Thanks @darrynten for the review!

@gnikyt
Copy link
Owner

gnikyt commented Feb 5, 2020 via email

@gnikyt gnikyt merged commit dfd8b6b into gnikyt:master Feb 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Bug with the code fix-in-progress In progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants