https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #1 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Why does it need to be listed in the cpanfile? Is it because it is needed so that it is included in the debian control file? If I run ./debian/list-deps it adds libyaml-libyaml-perl but is it by random chance (I don't know)? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #3 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Why don't we use CGI::Session::Serialize::default instead? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #4 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to Jonathan Druart from comment #3)

Why don't we use CGI::Session::Serialize::default instead?

It was switched to yaml because of a bug in the default serializer: session management: use YAML::Syck for serialization (a858e8a8b895640f2). Could check if that diacritic bug is still valid. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #6 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I've tried as well and I didn't face any encoding issues. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #8 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to Jonathan Druart from comment #7)

(In reply to Joonas Kylmälä from comment #5)

...

I tried to use čollčá as the library name and the default serializer and the bug reported a decade a code doesn't happen anymore, so I think we could use the default serializer. Just have to come up with a upgrade plan. At least the sessions table need to be emptied but do we have update scripts for emptying memcached and temp files where the sesssions might also be stored?

Why that?

Maybe it is not needed after all, I thought it would somehow try to parse all the sessions and possibly break, e.g. maybe if a malicous user tries to use the session id for the old YAML session could it break something or allow unauthorized access? Also now I found an encoding issue with the default serializer, if you use branchname and branchcode "ÄÄÄ~ÄãÃ" it breaks and shows question marks in intranet: [WARN] (in cleanup) CGI::Session::Driver::mysql::store(): DBI Exception: DBD::mysql::db do failed: Incorrect string value: '\xC4\xC4\xC4~\xC4\xE3...' for column `koha_kohadev`.`sessions`.`a_session` at row 1 [for Statement "INSERT INTO sessions (id, a_session) VALUES(?, ?) ON DUPLICATE KEY UPDATE a_session = ?"] at /usr/share/perl5/CGI/Session.pm line 251 This works with the YAML serializer. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #10 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to Jonathan Druart from comment #9)

Indeed, I only tried with the SessionStorage=tmp file

I managed to fix this problem if I remove the decode in set_userenv, but need to find side-effects.

I was also investigating if this happens because of some wrong DB connection initialization in Koha::Database.pm code. Nothing interesting found yet. I got the idea from this discussion: https://stackoverflow.com/questions/10957238/incorrect-string-value-when-try... -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #12 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Everything seems to work correctly with that patch. Tested all the 3 combinaisons of SessionStorage and the following branchcode/branchname: "CPL", "ÄÄÄ~ÄãÃ", "✔️ ❤️ ★" -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #14 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Joonas Kylmälä from comment #13)

Thanks for the code Joubu. I have not checked the solution yet but if you think it is right, could you please do similar changes for C4/InstallAuth.pm?

and remove CGI::Session::Serializer::yaml from cpanfile ? :) The patch works great for me -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |22824 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22824 [Bug 22824] Replace YAML::Syck with YAML::XS -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120835|0 |1 is obsolete| | --- Comment #15 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 120836 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120836&action=edit Bug 28317: Use the default CGI::Session serializer We remove YAML::Syck on bug 22824 and YAML on 27673, to use YAML::XS. However we need one of them for CGI::Session::Serialize::yaml It's preferable to change the serializer and use the default one instead of writing one based on YAML::XS (or patch the existing ::yaml that does not seem maintained). There was an encoding bug reported on the default serializer (see commit a858e8a8b895640f2) but we fail to recreate it. Test plan: Create 3 libraries with branchcode=branchname: "CPL", "ÄÄÄ~ÄãÃ" and "✔️ ❤️ ★" Use the 3 options of SessionStorage and switch from one logged in library to another. Confirm that everything is working correctly (ie. no ending issue in the library name at the top-right corner) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120836|0 |1 is obsolete| | --- Comment #16 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 120837 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120837&action=edit Bug 28317: Use the default CGI::Session serializer We remove YAML::Syck on bug 22824 and YAML on 27673, to use YAML::XS. However we need one of them for CGI::Session::Serialize::yaml It's preferable to change the serializer and use the default one instead of writing one based on YAML::XS (or patch the existing ::yaml that does not seem maintained). There was an encoding bug reported on the default serializer (see commit a858e8a8b895640f2) but we fail to recreate it. Test plan: Create 3 libraries with branchcode=branchname: "CPL", "ÄÄÄ~ÄãÃ" and "✔️ ❤️ ★" Use the 3 options of SessionStorage and switch from one logged in library to another. Confirm that everything is working correctly (ie. no ending issue in the library name at the top-right corner) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120837|0 |1 is obsolete| | --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 120839 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120839&action=edit Bug 28317: Use the default CGI::Session serializer We remove YAML::Syck on bug 22824 and YAML on 27673, to use YAML::XS. However we need one of them for CGI::Session::Serialize::yaml It's preferable to change the serializer and use the default one instead of writing one based on YAML::XS (or patch the existing ::yaml that does not seem maintained). There was an encoding bug reported on the default serializer (see commit a858e8a8b895640f2) but we fail to recreate it. Test plan: Create 3 libraries with branchcode=branchname: "CPL", "ÄÄÄ~ÄãÃ" and "✔️ ❤️ ★" Use the 3 options of SessionStorage and switch from one logged in library to another. Confirm that everything is working correctly (ie. no ending issue in the library name at the top-right corner) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #19 from Julian Maurice <julian.maurice@biblibre.com> --- Followed the test plan + tested the install process after making sure I had no YAML or YAML::Syck installed. No problem detected -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #21 from David Cook <dcook@prosentient.com.au> --- (In reply to Julian Maurice from comment #0)

Recently we replaced every use of YAML::Syck or YAML by YAML::XS, and removed YAML::Syck and YAML from cpanfile. But we use CGI::Session::Serialize::yaml which does not work with YAML::XS and need either YAML::Syck or YAML

So we shoud either: 1) Reintroduce one of YAML or YAML::Syck into cpanfile, or 2) Use a different serializer, or 3) Implement in Koha a serializer that use YAML::XS (should be easy, look at https://metacpan.org/release/CGI-Session-Serialize-yaml/source/lib/CGI/ Session/Serialize/yaml.pm)

Which one do you prefer ?

We could fork CGI::Session::Serialize::yaml and make a CGI::Session::Serialize::Koha. We could also contribute a patch or raise an enhancement request to CGI::Session::Serialize::yaml, so that it can use YAML::XS. It looks like the module got a new maintainer last year https://rt.cpan.org/Public/Bug/Display.html?id=131715 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #23 from David Cook <dcook@prosentient.com.au> --- I've opened a couple issues at https://rt.cpan.org/Dist/Display.html?Name=CGI-Session-Serialize-yaml regarding documentation and module options. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #24 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to David Cook from comment #20)

I have third-party apps that can use Koha session data by parsing the YAML. It looks like I'll have to think of something much more complicated to deal with the change in serialization. It's not very complicated, you just have to use `CGI::Session::Serialize::default->thaw($session_data)`

We could fork CGI::Session::Serialize::yaml and make a CGI::Session::Serialize::Koha.

Ahhh CGI::Session::Serialize::yaml also has an undocumented feature. If you look at the source code, you'll see that you can set $CGI::Session::Serialize::yaml::Flavour to whatever YAML module you want that can use Dump/Load methods. (Note that you'd have to preload it yourself with 'use' or 'require'.)

So... it looks like CGI::Session::Serialize::yaml can work with YAML::XS.

Personally, it would be awesome if we could retain the use of CGI::Session::Serialize::yaml. If we really want to stick with YAML, I'd prefer Koha to have its own implementation rather than relying on an undocumented "feature" (from my point of view that is not a feature, this is just messing with a module's internal variables)

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #26 from David Cook <dcook@prosentient.com.au> --- (In reply to Julian Maurice from comment #24)

(In reply to David Cook from comment #20)

...

I have third-party apps that can use Koha session data by parsing the YAML. It looks like I'll have to think of something much more complicated to deal with the change in serialization. It's not very complicated, you just have to use `CGI::Session::Serialize::default->thaw($session_data)`

My third-party apps neither use Perl nor have the capacity to use Perl, so that's not an option :p.

...

We could fork CGI::Session::Serialize::yaml and make a CGI::Session::Serialize::Koha.

Ahhh CGI::Session::Serialize::yaml also has an undocumented feature. If you look at the source code, you'll see that you can set $CGI::Session::Serialize::yaml::Flavour to whatever YAML module you want that can use Dump/Load methods. (Note that you'd have to preload it yourself with 'use' or 'require'.)

So... it looks like CGI::Session::Serialize::yaml can work with YAML::XS.

Personally, it would be awesome if we could retain the use of CGI::Session::Serialize::yaml. If we really want to stick with YAML, I'd prefer Koha to have its own implementation rather than relying on an undocumented "feature" (from my point of view that is not a feature, this is just messing with a module's internal variables)

It's not an internal variable. It's a package variable. That's why they have scoped it that way. Based on my conversations with the CGI::Session::Serialize::yaml maintainer, we'll probably add a class method for explicitly choosing the YAML implementation. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Comment #28 is|1 |0 private| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #30 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Kyle M Hall from comment #29)

(In reply to David Cook from comment #27)

...

(In reply to Jonathan Druart from comment #25)

...

David, thanks for the investigation.

All what you suggest is more work than what we have now, and I won't have more time to dedicate to this, we are close to the release. We should open a new bug report to reintroduce yaml using YAML::XS, later. Unless someone else has time to do it before the end of the week.

Surely the least amount of work would be to just keep the YAML::Syck dependency for CGI::Session::Serialize::yaml for the time being, and then change it to YAML::XS later?

If we change it to the default serializer now, I think it would much harder to switch back to YAML later.

Wouldn't it make more sense for your to add an API endpoint for outputting deserialized session info? That way you aren't messing with Koha's internals, which are subject to change.

+1 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #32 from David Cook <dcook@prosentient.com.au> --- (In reply to Kyle M Hall from comment #29)

Wouldn't it make more sense for your to add an API endpoint for outputting deserialized session info? That way you aren't messing with Koha's internals, which are subject to change.

I shouldn't have said third-party apps, as they're not really third-party. They're just non-Koha apps served off the same domain. Regarding your question, yes and no. 1. No: Firstly, everything is subject to change. I've seen API integrations break when the API endpoint gets updated. In this case, I can see Koha changes coming, and the code can get updated. Plus, the sessions are fairly stable overall (when serialized as YAML at least). It's not a drama. Secondly, at the moment I can use the session without any local customizations to Koha. 2. Yes: If I were to use an API endpoint, it would require customization, and I probably wouldn't create an endpoint outputting session data. Instead, I'd probably make multiple API endpoints to get the specific data that I need (like effective user permissions). In this case, I have access to the CGISESSID cookie, so I have the session ID, and Koha is working kind of like a SSO system. If I were to use an API, I would need an endpoint that could convert a session ID to a borrowernumber. But then Koha doesn't have any authorizations/permissions that would really be appropriate for that kind of administrative process. And I would consider it an administrative process. As such, I wouldn't love the idea of it being part of Koha's public Internet-accessible API. I would want to lock down that API endpoint within a separate Koha Admin API which could be limited to specific IP addresses or a local network. Koha also doesn't have a concept of "system users" that are reserved just for API user (and which can't be deleted by library staff). So... while using an API might make sense... it would be a tonne of work. Much more work than just reading the session from the database. -- At the end of the day, I imagine we'll switch the serializer to default, and then I'll have to think up some kind of hacked together API solution. Or scrap the other apps. Koha really needs an administrator level above superlibrarian, or some other management interface. But it all takes considerable work. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #34 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #33)

(In reply to Julian Maurice from comment #24)

...

If we really want to stick with YAML, I'd prefer Koha to have its own implementation rather than relying on an undocumented "feature" (from my point of view that is not a feature, this is just messing with a module's internal variables)

(In reply to Jonathan Druart from comment #28)

...

Then write an alternative patch and test it please.

Ok. I figured Julian had already disagreed with my proposal, but I can do it anyway.

Ah, alas, due to the Debian packaging of https://packages.debian.org/buster/libcgi-session-serialize-yaml-perl, you cannot remove libyaml-perl and libyaml-syck-perl without removing libcgi-session-serialize-yaml-perl. I'm still not sure why we have to remove YAML or YAML::Syck. I understand that it would be annoying to have one of the modules around just for handling the session, but it would be nice to keep the session serialized as YAML. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #36 from David Cook <dcook@prosentient.com.au> --- Interestingly, CGI::Session::Serialize::default writes out a Perl data structure and then uses the equivalent of eval() to inject it back into Perl: https://metacpan.org/release/CGI-Session/source/lib/CGI/Session/Serialize/de.... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #38 from David Cook <dcook@prosentient.com.au> --- If we're set on removing the YAML and YAML::Syck dependencies, then I say we move this back to "Passed QA". While in theory we could use YAML::XS with CGI::Session::Serialize::yaml, the Debian packaging of it means we would have to keep either YAML or YAML::Syck installed on the system, even if it was unused. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_21_05_candidate | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 --- Comment #41 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- On Koha-testing-docker I get randomly the following error when trying to select a new branch on intranet (/cgi-bin/koha/circ/set-library.pl): CGI::Session::Driver::mysql::store(): DBI Exception: DBD::mysql::db do failed: Incorrect string value: '\xE4iskap...' for column `koha_kohadev`.`sessions`.`a_session` at row 1 [for Statement "INSERT INTO sessions (id, a_session) VALUES(?, ?) ON DUPLICATE KEY UPDATE a_session = ?"] at /usr/share/perl5/CGI/Session.pm line 251 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 The branch I try to select has the following name: "Alkuperäiskappaleet, ei lainata". -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28317 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28489 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28489 [Bug 28489] CGI::Session is incorrectly serialized to DB in production env / when strict_sql_modes = 0 -- You are receiving this mail because: You are watching all bug changes.