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

Set session name to prevent collision when multiple applications are hosted on same domain #198

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

BlackbitDevs
Copy link
Contributor

@BlackbitDevs BlackbitDevs commented Aug 1, 2024

Imagine you run a Magento app on http://example.org and Pimcore on http://pim.example.org. Then you will have 2 cookies with name PHPSESSID (if session.name in php.ini is the same for both):

Name Value Domain Path
PHPSESSID 5a9b08750387d9e11c738a2947d93e38 .example.org /
PHPSESSID irqnjh5p96gp2i8iu743ulm32p pim.example.org /

First one is from Magento, second from Pimcore.
When trying to log in at http://example.org/admin you will get a 403 Forbidden error.

The reason why Magento sets the cookie for .example.org instead of example.org probably is to also support subdomains.
But of course Magento is only an example - the problem applies to all applications which run on same domain.

When we set the session name in Pimcore as proposed in this PR, such collisions will not happen.

@mattamon
Copy link
Contributor

@BlackbitDevs
As I understood it, Magento also does not care of this right?
Shouldn't this be more like a hint in the docs if you want to run multiple applications that you can change it, instead of setting it directly? I know it is nice to prevent it in the first place, but not sure if this more of a infrastructure topic than application layer topic.

@brusch WDYT?

@mattamon mattamon self-assigned this Aug 16, 2024
@BlackbitDevs
Copy link
Contributor Author

BlackbitDevs commented Sep 2, 2024

As I understood it, Magento also does not care of this right?

Not sure. The client asked us first ;-)
But it is not limited to Magento but happens to all applications which run on same domain (for example we also had this problem with ISPconfig).

Of course we can simply note it in the docs but then every project which runs multiple applications on same domain will get this error, then blames Pimcore and then (hopefully) finds the solution in the docs. And on the other hand we can simply set the session name and everything will just work. It is a matter of defensive programming / enhancing developer experience.

@mattamon
Copy link
Contributor

As I understood it, Magento also does not care of this right?

Not sure. The client asked us first ;-) But it is not limited to Magento but happens to all applications which run on same domain (for example we also had this problem with ISPconfig).

Of course we can simply note it in the docs but then every project which runs multiple applications on same domain will get this error, then blames Pimcore and then (hopefully) finds the solution in the docs. And on the other hand we can simply set the session name and everything will just work. It is a matter of defensive programming / enhancing developer experience.

@brusch what is your opinion on this?

@mattamon
Copy link
Contributor

mattamon commented Oct 4, 2024

@fashxp what's your opinion about this?

@fashxp
Copy link
Member

fashxp commented Oct 4, 2024

That's a good question, but I don’t really have a strong opinion on it.

It probably doesn’t hurt, but why don’t other applications do it either? For example, Magento doesn’t, and Symfony doesn’t seem to recommend it.

Could it be that dev-ops tools, monitoring systems, or firewalls might even get confused (or need additional configuration) by custom session names?

The thing is: I’ve never encountered this as an issue so far. And running two different applications on the same domain seems like an edge case to me, where you'd need to know what you're doing anyway. So additional configuration in such cases is fine I would say.

Any other opinions in the community?

@jdreesen
Copy link
Contributor

jdreesen commented Oct 4, 2024

I also think that this is an edge case and that we don't need to change the default setting.

@mattamon
Copy link
Contributor

mattamon commented Oct 7, 2024

@BlackbitDevs maybe we could adapt this PR, commented out this config and add a comment at the top to explain why and when you would need this. We should also then add this in the docs in the core as a hint as I suggested.

Maybe in the https://github.com/pimcore/pimcore/blob/11.x/doc/21_Deployment/03_Configuration_Environments.md here for the enviroment or somewhere in the https://github.com/pimcore/pimcore/tree/11.x/doc/23_Installation_and_Upgrade/03_System_Setup_and_Hosting Hosting part?

@BlackbitDevs
Copy link
Contributor Author

@mattamon Have commented the config out and described what it does.

Will create another PR for updating https://github.com/pimcore/pimcore/tree/11.x/doc/23_Installation_and_Upgrade/03_System_Setup_and_Hosting

Copy link
Contributor

@mattamon mattamon left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

@mattamon mattamon merged commit eede019 into pimcore:2024.x Oct 8, 2024
3 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants