Change 539382 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/core@master] Add optional serialization_type and hmac_key param values to RESTBagOStuff

https://gerrit.wikimedia.org/r/539382

Change 539382 merged by jenkins-bot:
[mediawiki/core@master] Add optional serialization_type and hmac_key param values to RESTBagOStuff

https://gerrit.wikimedia.org/r/539382

@BPirkle can I get you to chip in on https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/OATHAuth/+/538154/ and https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/WebAuthn/+/538155/ please?

Now the core patch is merged (and will be backported to REL1_34), I'm not sure if we should go ahead with the patches above as they exist, or we should make some other changes to them, or do something slightly/completely differently

Whatever happens, I will backport the changes to those extensions to REL1_34 to match

Thanks!

Change 541643 had a related patch set uploaded (by Jforrester; owner: BPirkle):
[mediawiki/core@REL1_34] Add optional serialization_type and hmac_key param values to RESTBagOStuff

https://gerrit.wikimedia.org/r/541643

Change 541643 merged by jenkins-bot:
[mediawiki/core@REL1_34] Add optional serialization_type and hmac_key param values to RESTBagOStuff

https://gerrit.wikimedia.org/r/541643

@Reedy, the OATHAuth and WebAuthn patches look like they're generally on the right track to me and unless that approach proves to be unnecessarily painful, I'd recommend continuing that direction.

This change (the one I'm commenting on, which modified RESTBagOStuff) gave us the flexibility to pick either PHP or JSON serialization for sessions stored in Kask. But that leaves the open question "which are we actually going to use", which was raised in T233537: Document and communicate potentially breaking session storage serialization change.

Which leaves the people working on OATHAuth and WebAuthn in a rough spot, because you need to know that things you store in sessions will work.

The safest approach is to do exactly what those patches are already doing - ensure that you aren't storing proper PHP objects. The alternative is to go ahead and have the discussion about which format we're actually going to use. But I think we're all on various short timelines and resolving that quickly might be challenging.

Is that helpful at all?

I made a comment on one of the patches regarding nesting, and some test code that I played around with locally.

In T233963#5563743, @BPirkle wrote:

@Reedy, the OATHAuth and WebAuthn patches look like they're generally on the right track to me and unless that approach proves to be unnecessarily painful, I'd recommend continuing that direction.

This change (the one I'm commenting on, which modified RESTBagOStuff) gave us the flexibility to pick either PHP or JSON serialization for sessions stored in Kask. But that leaves the open question "which are we actually going to use", which was raised in T233537: Document and communicate potentially breaking session storage serialization change.

Which leaves the people working on OATHAuth and WebAuthn in a rough spot, because you need to know that things you store in sessions will work.

The safest approach is to do exactly what those patches are already doing - ensure that you aren't storing proper PHP objects. The alternative is to go ahead and have the discussion about which format we're actually going to use. But I think we're all on various short timelines and resolving that quickly might be challenging.

Is that helpful at all?

I made a comment on one of the patches regarding nesting, and some test code that I played around with locally.

Thanks, yeah I mostly just wanted a sanity check. Will have a look at and talk to Dejan about the nested stuff if necessary.

Yeah, I think this makes sense. The code itself isn't actually turning things into json (I had that in my head from looking before, but that was definitely a blurred memory), it's mostly turning them into PHP arrays, rather than passing php objects (past behaviour that worked with serialize but doesn't with json_encode), which is where the problems with this stemmed. And as php arrays can be dealt with by both, the two extensions don't really need to care about how MW is encoding them for storage in the sessions (as it should be)

Will test them locally too myself, but I think that's more than enough to fix T233146, and hopefully prevent any future problems from the assumption that the session code would happily accept PHP objects and deal with them

And we've got the documentation and deprecation plan in place now (thanks!) as we shouldn't have to worry about unexpected and un-notified breaking changes happening