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

[3.0.x.x] Remove domain from language and currency cookies #13849

Merged
merged 1 commit into from
Apr 13, 2024

Conversation

ADDCreative
Copy link
Contributor

An invalid domain will now cause a fatal error in PHP 8. Don't use HTTP_HOST as it could be set to anything. Also fixes #7872.

@osworx
Copy link
Contributor

osworx commented Apr 9, 2024

What is the purpose to remove the domain?
Without, the cookie is globally valid, therefore if you have several installations on the same server (like we have on our dev servers) there is no difference betwenn them!

@ADDCreative
Copy link
Contributor Author

What is the purpose to remove the domain? Without, the cookie is globally valid, therefore if you have several installations on the same server (like we have on our dev servers) there is no difference between them!

@osworx That's not correct. If you set the cookie domain it's will actually be valid for that domain and all subdomains. If you leave in empty only and current domain is valid. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#domaindomain-value.

@osworx
Copy link
Contributor

osworx commented Apr 9, 2024

Assume I have x installations - same folder level (no subdomain of domain - basically all are subdomains).
Now, when I set a cookie for the installation A, it's also valid for B, C, D, E, etc.

Which cannot be correct, because when I want to have the language cookie set for A, it should NOT be valid also for B, C, D, E etc. ..
That's not correct, because I want to have A with en-gb, B with de-de, C with fr-fr etc.
With your solution we will have the same as we had with OC 1.4.x 1.5., 2.x which I do not want and cannot really work with.

But I am wrong, correct me ..

@ADDCreative
Copy link
Contributor Author

If you don't pass a domain to setcookie it will still only be valid for the domain that the request was made to. So if the request was to A, the cookie still will not be valid for B, C, D, E etc. However, without this fix it would've been valid for subdomain1.A, subdomain2.A, etc.

With setcookie NOT providing a domain is actually more restrictive than providing one.

@mhcwebdesign
Copy link
Collaborator

@ADDCreative: Question: If I have a website at https://www.example.com , and a demo site at https://www.example.com/demo (in other words: I am not using a sub-domain), how will your proposed change affect this scenario?

@ADDCreative
Copy link
Contributor Author

@mhcwebdesign It wouldn't have any effect in that scenario. It just stops the cookies being leaked to subdomains (or subdomains of subdomains in your example). Currently the code is setting the cookies to be valid for www.example.com and any subdomains of www.example.com (whether you are using them or not). Removing the domain from setcookie actually just sets cookies to be valid to www.example.com and not any of its subdomains. The web browser know the domain it sent the request to and sets that as the domain for the cookie.

The domain has already been remove in 4.0.0.0. Just never got ported to 3.0.x.x.

$option = [
'expires' => time() + 60 * 60 * 24 * 30,
'path' => '/',
'SameSite' => 'Lax'
];
setcookie('currency', $code, $option);

You also get bots trying to manipulate HTTP_HOST, which could now cause a fatal error in PHP 8, so will help stop the PHP error log filling up.

@mhcwebdesign mhcwebdesign merged commit f5dadb8 into opencart:3.0.x.x Apr 13, 2024
6 checks passed
@ADDCreative ADDCreative deleted the patch-11 branch April 16, 2024 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants