Skip to content

Replace changed by expires_at in session handling#9990

Merged
pabzm merged 1 commit intomasterfrom
replace-changed-by-expires_at_in_session
Dec 4, 2025
Merged

Replace changed by expires_at in session handling#9990
pabzm merged 1 commit intomasterfrom
replace-changed-by-expires_at_in_session

Conversation

@pabzm
Copy link
Member

@pabzm pabzm commented Sep 28, 2025

This prepares using extended session lifetimes configurable per session

@pabzm pabzm requested a review from alecpl September 28, 2025 13:32
@pabzm pabzm force-pushed the replace-changed-by-expires_at_in_session branch from 8493ba0 to f214fe7 Compare September 29, 2025 10:15
@pabzm
Copy link
Member Author

pabzm commented Sep 29, 2025

I rebased the branch onto the lastest of the "master" branch to fix the CI runs.

@pabzm
Copy link
Member Author

pabzm commented Sep 30, 2025

We should think about changing set_auth_cookie() to not use time-based slots but something else, because its anti-session-hijacking function is not really powerful anymore if it changes only every 18 hours.

@pabzm pabzm added this to the 1.7-rc milestone Oct 1, 2025
@alecpl
Copy link
Member

alecpl commented Oct 5, 2025

Some of my comments in #9991 apply here.

@pabzm
Copy link
Member Author

pabzm commented Oct 22, 2025

@alecpl I fixed the version number in the "initial.sql"-files, and setting the expiry time in rcube_session_php. The only other of your comments in the other PR that I understand would apply to this PR is this on, of which I don't understand why you consider the change wrong.
Are these the only issues you see with the changes I proposed in this PR?

@pabzm pabzm force-pushed the replace-changed-by-expires_at_in_session branch from 91dba0c to b804142 Compare October 22, 2025 14:43
@pabzm
Copy link
Member Author

pabzm commented Oct 22, 2025

I rebased onto the latest of the "master" branch.

@johndoh
Copy link
Contributor

johndoh commented Oct 22, 2025

This might be a bit picky but I think update files should also include something like

ALTER TABLE `session` RENAME INDEX `changed_index` TO `expires_at_index`; 

I think the name of the index is not that important but I think the schema should be the same when created by the initial file or through updates.

The change to sqlite.initial.sql is missing.

@pabzm
Copy link
Member Author

pabzm commented Oct 23, 2025

@johndoh Thank you for the review! I agree that renaming the index is a good idea and added it to the SQL migration files.

The change to sqlite.initial.sql is missing.

What do you mean by that? I don't see a missing change in there that the other initial-files have.

(I also fixed the roundcube-version number to match the date-string of the corresponding SQL migration files.)

@johndoh
Copy link
Contributor

johndoh commented Oct 23, 2025

@pabzm my apologies. I somehow missed that sqlite.initial.sql was in the diff. I mistakenly thought only the update file was there not the initial one.

The alter table statement for sqlite should be

ALTER TABLE `session` RENAME INDEX `ix_session_changed` TO `ix_session_expires_at`;

i think, i have not tested it, just corrected the index names.

and for postgres

ALTER TABLE `session` RENAME INDEX `session_changed_idx` TO `session_expires_at_idx`;

i think

@pabzm
Copy link
Member Author

pabzm commented Oct 28, 2025

@johndoh Thank you for that hint! I didn't expect a different index name in the differente databases.

@pabzm
Copy link
Member Author

pabzm commented Oct 28, 2025

If y'all would be fine with merging please let me know so I can cleanup the commit history before someone hits the button!

@pabzm pabzm requested a review from johndoh October 28, 2025 16:48
@pabzm pabzm force-pushed the replace-changed-by-expires_at_in_session branch from e395ede to c34092b Compare November 4, 2025 09:07
@pabzm
Copy link
Member Author

pabzm commented Nov 4, 2025

I squashed all the commits into one, didn't change anything further.

@pabzm
Copy link
Member Author

pabzm commented Nov 4, 2025

@alecpl Please have another look, I'd like to merge this soon.

@pabzm pabzm requested a review from alecpl November 10, 2025 14:48
@pabzm
Copy link
Member Author

pabzm commented Nov 24, 2025

@alecpl This is the last code change planned for v1.7, could you please have another look? Or let me know when/how we can go forward with this? I requested a new review from you 2 weeks ago, is there some blocker?

@teoberi
Copy link

teoberi commented Nov 25, 2025

I'm using 1.7 RC2 on the test server with PHP 8.5.0 and waiting for the final version for the production server.

@pabzm pabzm requested a review from alecpl December 3, 2025 16:08
Copy link
Member

@alecpl alecpl left a comment

Choose a reason for hiding this comment

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

Looks good. Rebase on master to make CI green before merge, please

This prepares using extended session lifetimes configurable per session
@pabzm pabzm force-pushed the replace-changed-by-expires_at_in_session branch from 358d08d to d676576 Compare December 4, 2025 13:34
@pabzm pabzm merged commit 202daa6 into master Dec 4, 2025
23 checks passed
@pabzm pabzm deleted the replace-changed-by-expires_at_in_session branch December 4, 2025 13:47
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.

4 participants