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

Don't call session_start() when PHP session is still or already open. #31286

Conversation

rotdrop
Copy link
Contributor

@rotdrop rotdrop commented Feb 20, 2022

Signed-off-by: Claus-Justus Heine himself@claus-justus-heine.de

See #31248

@solracsf solracsf added the 3. to review Waiting for reviews label Feb 21, 2022
@solracsf solracsf added this to the Nextcloud 24 milestone Feb 21, 2022
@szaimen szaimen requested review from a team, ArtificialOwl, skjnldsv and vanpertsch and removed request for a team February 21, 2022 09:21
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
@blizzz blizzz mentioned this pull request Apr 7, 2022
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Makes sense 👍

@blizzz blizzz mentioned this pull request Apr 13, 2022
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
@PVince81 PVince81 requested review from nickvergessen and come-nc and removed request for vanpertsch June 10, 2022 14:41
@PVince81
Copy link
Member

#28695 was an older alternative, not sure which is best

@come-nc
Copy link
Contributor

come-nc commented Jun 13, 2022

#28695 was an older alternative, not sure which is best

No idea 😕

@juliusknorr
Copy link
Member

I've been trying to figure out why we actually need the session_start when clearing and generating a new session id, but couldn't find a good reasoning from the git history nor php documentation so I would consider even dropping that call. But hard to tell if that has any side effects.

So either the start_session is superfluous and can be omitted -- at least in current versions of php -- or there is missing something like a session_destroy() (but then regenerate_id() could be omitted?) or a session_write_close() which would write the (now empty) session file to disk.

@rotdrop Did you have any reasoning to go with the session_write_close instead of dropping the start_session?

@nickvergessen
Copy link
Member

session_write_close should basically break @UseSession annotation?

@juliusknorr
Copy link
Member

session_write_close should basically break @usesession annotation?

No as the session_start would reopen it for writing.

This was referenced Aug 12, 2022
@blizzz blizzz mentioned this pull request Aug 24, 2022
@solracsf
Copy link
Member

solracsf commented Aug 25, 2022

#28695 was an older alternative, not sure which is best

Both perform a similar goal, this one seems more clean.

@blizzz blizzz mentioned this pull request Aug 30, 2022
@come-nc
Copy link
Contributor

come-nc commented Sep 1, 2022

/rebase

This was referenced Sep 6, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
@solracsf
Copy link
Member

Is this still a thing after 9e1d431?
Cc @juliushaertl

This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@juliusknorr
Copy link
Member

Is this still a thing after 9e1d431?
Cc @juliushaertl

Yes, that linked commit only reduced one common case where a session would be opened for writing. This might still occur later in the code path.

@juliusknorr
Copy link
Member

/rebase

@juliusknorr
Copy link
Member

@rotdrop Can you maybe rebase your PR so we trigger CI again and can get this merged?

@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
Signed-off-by: Claus-Justus Heine <himself@claus-justus-heine.de>
@szaimen szaimen enabled auto-merge April 17, 2023 14:23
@szaimen szaimen force-pushed the bugfix/dont-start-session-when-session-is-already-open branch from ffdb4d2 to 45ec432 Compare April 17, 2023 14:24
@szaimen szaimen merged commit d87cc20 into nextcloud:master Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SessionInternal::clear() erroneously calls start_session() when session is still open
9 participants