Please explain why this change is necessary. Basically, why does the CMS need to continue relying on its own method that was created to generate a string of a random length when the engine provides those capabilities out of the box (hence what the framework uses). Is there some sort of allowed character difference? What does it matter what specific function calls are used to generate a “secure” 32 character string?

You’re saying this doesn’t actually fix the issue it is intended to fix, so why propose it as a change request in the first place?

You’re saying this doesn’t actually fix the issue it is intended to fix, so why propose it as a change request in the first place?

It does not completly fix the issue that is what i mean by partly.

Basically, why does the CMS need to continue relying on its own method that was created to generate a string of a random length when the engine provides those capabilities out of the box (hence what the framework uses). Is there some sort of allowed character difference? What does it matter what specific function calls are used to generate a “secure” 32 character string?

It is used by the formToken and 4.x needs to have a way to accept the 3.x form token on upgrade. Right now the session is completly lost on upgrade and this issue here does not come up right now but we need to keep B/C here with the 3.x form token else we break the update path from 3.x to 4.0.

Or do you have a better idea how to solve the B/C issue for upgrades?

I don’t know what the issue with sessions on upgrade actually is but I fail to see why this change fixes anything. An automated testing case would really help here as you’re getting into a highly technical behavior in a very specialized portion of the code, and this is the type of change that mandates a regression test to make sure things do not randomly break in the future (essentially demonstrating there is a failure to validate a token generated with the 3.x custom “secure string” generator that prohibits being able to use the native PHP API).

So basically, what is actually causing the session failure. Is it the token, or is it something else within either the Session or Application APIs (as those would be prime candidates for a change causing your issue). And if it is the token, why does changing the logic used to generate it “fix” the problem when the end result is a string that allows a similar character set.

I don’t know what the issue with sessions on upgrade actually is

It seems that for some reason we get a new session name and for that reason get logged out of the system at the upgrade time.

An automated testing case would really help here as you’re getting into a highly technical behavior in a very specialized portion of the code, and this is the type of change that mandates a regression test to make sure things do not randomly break in the future (essentially demonstrating there is a failure to validate a token generated with the 3.x custom “secure string” generator that prohibits being able to use the native PHP API).

This should take care of this test when the other issue is fixed too #28427

So basically, what is actually causing the session failure. Is it the token, or is it something else within either the Session or Application APIs (as those would be prime candidates for a change causing your issue). And if it is the token, why does changing the logic used to generate it “fix” the problem when the end result is a string that allows a similar character set.

Well the token is part of the issue is fixed here and the Session thing is the seccond part I have not found the patch for yet

The main issue with the new token method is that the new code generates the token based on a differt algo / with different code that result into an invalid token on upgrades. (The new token is longer than 32 chars for example)

I would be more happy to keep the frameworks method but i dont't see a way to make sure the token from the 3.x app gets correctly validated with the 4.x code.

The only solution i could think of would be to do a hack to the 3.x token generation for the final token and there we already use the new 4.x code so the 4.x code can than successfully check the token and does not fail.

Would that be an acceptable hack of the upgrade process where we than have to check wether we upgrade to 4.x and than use a different token method?

I don’t know what the issue with sessions on upgrade actually is

It seems that for some reason we get a new session name and for that reason get logged out of the system at the upgrade time.

The fixing the token generation has nothing to do with things.

The main issue with the new token method is that the new code generates the token based on a differt algo / with different code that result into an invalid token on upgrades.

A regression test proving that would actually be very helpful, both for right now and for 5 years later if someone were to propose changing things. Otherwise right now this is just change for the sake of change in hoping that it actually fixes something.

(The new token is longer than 32 chars for example)

I would consider this a bug upstream and have fixed it by removing the customizable length parameter of the session token (because this really doesn't need a parameter and one can subclass the Session class if they want a different length, or someone can add support to the options array) and accounting for how bin2hex() generates a string twice the length of what was expected (bin2hex(random_bytes(32)) results in a 64 character string, bin2hex(random_bytes(16)) will generate the expected 32 character string). There are now also basic tests covering the token portion of the Session class as that was previously uncovered in any way.

The above changes should negate the need for this pull request, and you are left back at your original issue of a change in the session name.

The fixing the token generation has nothing to do with things.

Yes.

I would consider this a bug upstream and have fixed it by removing the customizable length parameter of the session token (because this really doesn't need a parameter and one can subclass the Session class if they want a different length, or someone can add support to the options array) and accounting for how bin2hex() generates a string twice the length of what was expected (bin2hex(random_bytes(32)) results in a 64 character string, bin2hex(random_bytes(16)) will generate the expected 32 character string). There are now also basic tests covering the token portion of the Session class as that was previously uncovered in any way.

Just to be sure this commit still has bin2hex(random_bytes(32)) that would still result into in a 64 character string right?
joomla-framework/session@3dc6fd4

To me the main issue between the framework and the CMS code is not the length it is more how we generate the token and that we use completly different ways to generate it:

This is what the UserHelper does in 3.x: https://github.com/joomla/joomla-cms/blob/staging/libraries/src/User/UserHelper.php#L667-L687

This is what the framework does shipped with 4.x:
https://github.com/joomla-framework/session/blob/2.0-dev/src/Session.php#L635

That is a different code and can never generate a token that matches each other right or is it just me that sees an issue that we use different code to generate the token?

And what do you think about that hack proposed above for the new token code to be used on the last step of the upgrade process to 4.x?

Just to be sure this commit still has bin2hex(random_bytes(32)) that would still result into in a 64 character string right?
joomla-framework/session@3dc6fd4

It was the second commit I linked, joomla-framework/session@620f406 that fixed the bin2hex() behavior (the one you linked was removing the configurable length; separate commits for separate changes).

This is what the framework does shipped with 4.x:
https://github.com/joomla-framework/session/blob/3dc6fd4ad94e0cb12496a5364d02f4e8f6ef0ba4/src/Session.php#L629

Then a full composer update has not been run because the current 2.0 branch has the right changes. joomla-framework/session@3dc6fd4...2.0-dev

Then a full composer update has not been run because the current 2.0 branch has the right changes. joomla-framework/session@3dc6fd4...2.0-dev

Ok just updated the link to: https://github.com/joomla-framework/session/blob/2.0-dev/src/Session.php#L635 my point about using totally different code to generate the token still stands.

To me the main issue between the framework and the CMS code is not the length it is more how we generate the token and that we use completly different ways to generate it:

Please provide a reproducible test scenario that demonstrates that the manner in which the tokens generated having changed is what is causing this token related issue. Said test scenario would serve as an automated regression test ensuring that any changes in the Session API regarding token generation do not cause issues like this one in the future. Otherwise, if your issue is purely the length of the generated token then that has been fixed.

Joomla\Session\Session::hasToken() does not attempt to generate a token to compare against. Source:

https://github.com/joomla-framework/session/blob/7259b163f5f61c597cff5aab05b924d8ecad9635/src/Session.php#L156-L166

It checks the internal "session.token" key to determine if the token supplied to the method matches the one that has been stored to the session. It is a strict string comparison. That method is therefore blissfully ignorant if the token is generated through UserHelper::genRandomPassword(), bin2hex(random_bytes()), or even other types of random string generators.

Well the only thing I can say right now is that when i was debuggin the mention session issue I ran over the changed code mention here when this is not an issue. You know that session stuff far better than me. So I'm happy to close here. Thanks.