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

Server error v Z39.50 #5

Open
radiuscz opened this issue Sep 9, 2016 · 7 comments
Open

Server error v Z39.50 #5

radiuscz opened this issue Sep 9, 2016 · 7 comments
Assignees

Comments

@radiuscz
Copy link

radiuscz commented Sep 9, 2016

cat /var/log/koha/ctrebova/plack-error.log | grep Breeding

Undefined subroutine &C4::Breeding::GetZ3950BatchId called at /usr/share/koha/lib/C4/Breeding.pm line 429.

V České Třebové aplikován hotfix přidáním prefixu C4::Breeding::, ale proč to tak musí být, když je knihovna Breeding importovaná správně?

@black23
Copy link

black23 commented Sep 9, 2016

Zlobí to kvůli úpravě na $7. @jirislav neví, čím by to mohlo být?

xmorave2 pushed a commit that referenced this issue Oct 11, 2016
…cannot be null

Error:
DBIx::Class::Storage::DBI::_dbh_execute(): Column 'checkprevcheckout'
cannot be null at C4/Members.pm line 697

Test Plan:
1) Attempt to import a patron via csv
2) Note the error
3) Apply this patch
4) Repeat the import
5) No error!

NOTE: Given that all the other tests ran (comment #2), except
      those in comment #3, I resorted to following the test plan
      above using the attachment provided in comment #5. I
      believe the issues in comment #3 constitute other bugs
      which need fixing and are unrelated this bug. Applying the
      patch does resolve the error triggered, and the code is
      good.

Signed-off-by: Mark Tompsett <[email protected]>
Signed-off-by: Marcel de Rooy <[email protected]>

Signed-off-by: Kyle M Hall <[email protected]>
xmorave2 pushed a commit that referenced this issue Oct 11, 2016
…e splitting in catalogue/detail.tt

In koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt,
fix line splitted TT directives and sentence splitting leading
to translatability problems. (See first comment).

To test:
- Apply patch
- Go to detail pages of biblios with waiting holds
- Verify that messages in column 'Status' are OK
- Examine code in patch to make sure that the simplification in logic
  makes sense and that no TT directive is splitted
- Bonus test:
  - Go to folder misc/translator. Run perl translate create xx-XX
  - Verify that monster mentioned in first comment no longer exists
    (in po/xx-XX-staff-prog.po)

Signed-off-by: Hector Castro <[email protected]>
Works as advertised

Amended for wording (comment #5) 2016-07-26 mv

Signed-off-by: Brendan Gallagher <[email protected]>
xmorave2 pushed a commit that referenced this issue Oct 11, 2016
…cannot be null

Error:
DBIx::Class::Storage::DBI::_dbh_execute(): Column 'checkprevcheckout'
cannot be null at C4/Members.pm line 697

Test Plan:
1) Attempt to import a patron via csv
2) Note the error
3) Apply this patch
4) Repeat the import
5) No error!

NOTE: Given that all the other tests ran (comment #2), except
      those in comment #3, I resorted to following the test plan
      above using the attachment provided in comment #5. I
      believe the issues in comment #3 constitute other bugs
      which need fixing and are unrelated this bug. Applying the
      patch does resolve the error triggered, and the code is
      good.

Signed-off-by: Mark Tompsett <[email protected]>
Signed-off-by: Marcel de Rooy <[email protected]>

Signed-off-by: Kyle M Hall <[email protected]>
xmorave2 pushed a commit that referenced this issue Oct 11, 2016
…e splitting in catalogue/detail.tt

In koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt,
fix line splitted TT directives and sentence splitting leading
to translatability problems. (See first comment).

To test:
- Apply patch
- Go to detail pages of biblios with waiting holds
- Verify that messages in column 'Status' are OK
- Examine code in patch to make sure that the simplification in logic
  makes sense and that no TT directive is splitted
- Bonus test:
  - Go to folder misc/translator. Run perl translate create xx-XX
  - Verify that monster mentioned in first comment no longer exists
    (in po/xx-XX-staff-prog.po)

Signed-off-by: Hector Castro <[email protected]>
Works as advertised

Amended for wording (comment #5) 2016-07-26 mv

Signed-off-by: Brendan Gallagher <[email protected]>
xmorave2 pushed a commit that referenced this issue Aug 9, 2017
This noise is from a failure. This patch expands the delete
to 952$c for the ACQ framework as per comment #5.

TEST PLAN
---------

insert into marc_subfield_structure (tagfield,tagsubfield,liblibrarian, libopac, repeatable, mandatory, kohafield,tab,authorised_value,authtypecode,value_builder,isurl,hidden,frameworkcode,seealso,link,defaultvalue,maxlength) values (952,'c','Shelving location','Shelving location',0,0,'items.location',10,'LOC','','',0,0,'ACQ','','',null,9999);
-- this makes sure you have a pre-existing 952$c ACQ record.

prove t/db_dependent/AuthorisedValues.t
-- should have ugly message like in comment #0
apply patch
prove t/db_dependent/AuthorisedValues.t
-- should be green
run koha qa test tools

Signed-off-by: Lee Jamison <[email protected]>

Signed-off-by: Marcel de Rooy <[email protected]>

Signed-off-by: Jonathan Druart <[email protected]>
(cherry picked from commit 441f6fa)
Signed-off-by: Fridolin Somers <[email protected]>
(cherry picked from commit 97498a6)
Signed-off-by: Katrin Fischer <[email protected]>
xmorave2 pushed a commit that referenced this issue Aug 9, 2017
This noise is from a failure. This patch expands the delete
to 952$c for the ACQ framework as per comment #5.

TEST PLAN
---------

insert into marc_subfield_structure (tagfield,tagsubfield,liblibrarian, libopac, repeatable, mandatory, kohafield,tab,authorised_value,authtypecode,value_builder,isurl,hidden,frameworkcode,seealso,link,defaultvalue,maxlength) values (952,'c','Shelving location','Shelving location',0,0,'items.location',10,'LOC','','',0,0,'ACQ','','',null,9999);
-- this makes sure you have a pre-existing 952$c ACQ record.

prove t/db_dependent/AuthorisedValues.t
-- should have ugly message like in comment #0
apply patch
prove t/db_dependent/AuthorisedValues.t
-- should be green
run koha qa test tools

Signed-off-by: Lee Jamison <[email protected]>

Signed-off-by: Marcel de Rooy <[email protected]>

Signed-off-by: Jonathan Druart <[email protected]>
(cherry picked from commit 441f6fa)
Signed-off-by: Fridolin Somers <[email protected]>
xmorave2 pushed a commit that referenced this issue Aug 9, 2017
This noise is from a failure. This patch expands the delete
to 952$c for the ACQ framework as per comment #5.

TEST PLAN
---------

insert into marc_subfield_structure (tagfield,tagsubfield,liblibrarian, libopac, repeatable, mandatory, kohafield,tab,authorised_value,authtypecode,value_builder,isurl,hidden,frameworkcode,seealso,link,defaultvalue,maxlength) values (952,'c','Shelving location','Shelving location',0,0,'items.location',10,'LOC','','',0,0,'ACQ','','',null,9999);
-- this makes sure you have a pre-existing 952$c ACQ record.

prove t/db_dependent/AuthorisedValues.t
-- should have ugly message like in comment #0
apply patch
prove t/db_dependent/AuthorisedValues.t
-- should be green
run koha qa test tools

Signed-off-by: Lee Jamison <[email protected]>

Signed-off-by: Marcel de Rooy <[email protected]>

Signed-off-by: Jonathan Druart <[email protected]>
xmorave2 pushed a commit that referenced this issue Sep 20, 2017
Translation tool shows toe following for ncertainprice.tt
0; url=[% scriptname %]?booksellerid=[% booksellerid %]

This patch fixes it.

To test:
- Apply patch
- Verify that code change makes sense
- Verify that Home > Acquisitions > [vendor] > Uncertain prices for [vendor]
  works as before

- Additional test (for a langunage 'aa-AA')
  perl translate create aa-AA
  verify that line 41 no longer appears in aa-AA-staff-prog.po

Amended to switch from BLOCK to a template variable, see comment #5

Signed-off-by: Owen Leonard <[email protected]>

Signed-off-by: Jonathan Druart <[email protected]>
xmorave2 pushed a commit that referenced this issue Sep 22, 2017
Translation tool shows toe following for ncertainprice.tt
0; url=[% scriptname %]?booksellerid=[% booksellerid %]

This patch fixes it.

To test:
- Apply patch
- Verify that code change makes sense
- Verify that Home > Acquisitions > [vendor] > Uncertain prices for [vendor]
  works as before

- Additional test (for a langunage 'aa-AA')
  perl translate create aa-AA
  verify that line 41 no longer appears in aa-AA-staff-prog.po

Amended to switch from BLOCK to a template variable, see comment #5

Signed-off-by: Owen Leonard <[email protected]>

Signed-off-by: Jonathan Druart <[email protected]>
(cherry picked from commit 2ee8280)
Signed-off-by: Fridolin Somers <[email protected]>
(cherry picked from commit 4d42774)
Signed-off-by: Katrin Fischer <[email protected]>
xmorave2 pushed a commit that referenced this issue Sep 22, 2017
Translation tool shows toe following for ncertainprice.tt
0; url=[% scriptname %]?booksellerid=[% booksellerid %]

This patch fixes it.

To test:
- Apply patch
- Verify that code change makes sense
- Verify that Home > Acquisitions > [vendor] > Uncertain prices for [vendor]
  works as before

- Additional test (for a langunage 'aa-AA')
  perl translate create aa-AA
  verify that line 41 no longer appears in aa-AA-staff-prog.po

Amended to switch from BLOCK to a template variable, see comment #5

Signed-off-by: Owen Leonard <[email protected]>

Signed-off-by: Jonathan Druart <[email protected]>
(cherry picked from commit 2ee8280)
Signed-off-by: Fridolin Somers <[email protected]>
(cherry picked from commit 4d42774)
Signed-off-by: Katrin Fischer <[email protected]>
xmorave2 pushed a commit that referenced this issue Sep 22, 2017
Translation tool shows toe following for ncertainprice.tt
0; url=[% scriptname %]?booksellerid=[% booksellerid %]

This patch fixes it.

To test:
- Apply patch
- Verify that code change makes sense
- Verify that Home > Acquisitions > [vendor] > Uncertain prices for [vendor]
  works as before

- Additional test (for a langunage 'aa-AA')
  perl translate create aa-AA
  verify that line 41 no longer appears in aa-AA-staff-prog.po

Amended to switch from BLOCK to a template variable, see comment #5

Signed-off-by: Owen Leonard <[email protected]>

Signed-off-by: Jonathan Druart <[email protected]>
(cherry picked from commit 2ee8280)
Signed-off-by: Fridolin Somers <[email protected]>
xmorave2 pushed a commit that referenced this issue Nov 2, 2017
…ublic' syspref combination.

TEST PLAN
----------
1/ configure a working 'GoogleOpenIDConnect' account

See comment #5 which also links back to
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16892#c3

2/ set 'OpacPublic' (under OPAC) to 'Disabled' and
   'GoogleOpenIDConnect' (under Administration) to 'Yes'.

3/ log in user successfully via google-auth, observe redirect to
   opac-user.pl (bad)

4/ apply patch
   -- on kohadevbox remember to restart all! Plack is unforgiving. :)

5/ log in user successfully via google-auth, observe expected
   redirect to opac-main.pl (good)

While I would normally suggest running koha qa test tools, because
this file doesn't end in .pl, it doesn't get picked up by them.

6/ perlcritic -4 opac/svc/auth/googleopenidconnect
   -- notice this is a level better than required. :)

This also eyeballs easily well.

Signed-off-by: Mark Tompsett <[email protected]>

Signed-off-by: Kyle M Hall <[email protected]>

Signed-off-by: Jonathan Druart <[email protected]>
@phavel
Copy link

phavel commented Feb 19, 2018

Podobný problém (Server error při hledání v Z39.50):

Undefined subroutine &C4::Breeding::GetZ3950BatchId called at /usr/share/koha/lib/C4/Breeding.pm line 279.

Dočasně vyřešeno přidáním prefixu C4::ImportBatch::, viz https://lists.katipo.co.nz/public/koha/2016-November/046595.html

@xmorave2
Copy link

@phavel V které konkrétní verzi? Opravím to pro příští vydání balíčků

@phavel
Copy link

phavel commented Feb 19, 2018

Verze 16.11.11.000 (CZ balíček)

@radiuscz
Copy link
Author

Pepo, myslím, že tohle je obecný problém, který máme napříč všemi verzemi. Workaround zafunguje, jádro problému jsme zatím neodhalili. Kdyby to bylo v balíčku, bude fajn. Takhle to musíme stále dokola ručně opravovat po každé aktualizaci.

@black23
Copy link

black23 commented Feb 19, 2018

To jsou ty speciality, co nám vymyslela ta národní knihovna :-(

@xmorave2
Copy link

Definitivní řešení bude refactoring na objekty. Zajímavé je, že u mě se to neprojevuje na tomhle řádku, ale projevují se jiné (teď již opravené) no nic, udělám opravu i tohohle.

xmorave2 pushed a commit that referenced this issue Jun 20, 2018
Until Perl 5.26, the current directory is added to @inc when running a
Perl script [1]. Having the current directory in @inc means it can be
tried to be traversed when performing a lib lookup. Since version 5.18,
Perl dies when it finds an unreadable directory (permissions) in @inc
that needs to be traversed. This behaviour won't change because Perl
devs consider it an enhancement to security. [2]

Because of this, we need to make sure our scripts are ran **from** a
directory in which they have read permissions.

Ths patch adds a --chdir option switch to the **koha-foreach** wrapper
script, that makes the inner shells/scripts to be ran within the Koha
instance's user home directory.

The change is trivial and should be QAed easily. I tested this on a prod
server:

- Create a /tmp/test.pl file containing:

use Modern::Perl;

use Cwd;
my $dir = getcwd;

warn $dir;

1;

A) then create a cronjob entry to run it using koha-foreach:
(in /etc/cron.d/test):
1/* * * * * root koha-foreach perl /tmp/test.pl
- Once I noticed the cronjob ran, I used mutt to read the emails in the
root user.
=> FAIL:
...
Subject: Cron <root@koha> koha-foreach --enabled perl /tmp/test.pl

"/root"
"/root"
"/root"
"/root"
"/root"
...

B) I then used the patched koha-foreach with different results:
=> SUCCESS:
...
Subject: Cron <root@koha> /root/koha-foreach --chdir --enabled perl /tmp/test.pl

"/var/lib/koha/acaderc"
"/var/lib/koha/agro"
"/var/lib/koha/anc"
"/var/lib/koha/arico"
"/var/lib/koha/artes"
...

So this patch's approach works. But...

C) master's koha-foreach seems to work just the same... I think it is
because of my previous attempt to fix this by using sudo in koha-shell.
So I think environmental conditions affect the behaviour (which shell is
configured for cron, sudo configuration, etc).

====

In conclusion, I think we should go ahead with this patch as it will solve
peoples issues, and it is a right solution (option #5 on the list) to
this Perl behaviour change. It doesn't cover other commands, but
followup patches could do.

I avoided /tmp as it is writable by any user... so it is an easy path
for both exploiting by replacing some lib, and also because the
existence of an unreadable dir that the interpreter could try to
traverse (unreadable /tmp/Authen or /tmp/Koha will trigger the same
error, and I assume people know what they are putting on the instance's
dir, at least it will be easier to track).

A followup patch takes care of making the cronjobs use --chdir when
calling koha-foreach

[1] https://lists.debian.org/debian-devel-announce/2016/08/msg00013.html
[2] https://rt.perl.org/Public/Bug/Display.html?id=123795

Signed-off-by: Kyle M Hall <[email protected]>

Signed-off-by: Marcel de Rooy <[email protected]>

Signed-off-by: Nick Clemens <[email protected]>
(cherry picked from commit 3db7e1a)
Signed-off-by: Fridolin Somers <[email protected]>
xmorave2 pushed a commit that referenced this issue Jun 20, 2018
Until Perl 5.26, the current directory is added to @inc when running a
Perl script [1]. Having the current directory in @inc means it can be
tried to be traversed when performing a lib lookup. Since version 5.18,
Perl dies when it finds an unreadable directory (permissions) in @inc
that needs to be traversed. This behaviour won't change because Perl
devs consider it an enhancement to security. [2]

Because of this, we need to make sure our scripts are ran **from** a
directory in which they have read permissions.

Ths patch adds a --chdir option switch to the **koha-foreach** wrapper
script, that makes the inner shells/scripts to be ran within the Koha
instance's user home directory.

The change is trivial and should be QAed easily. I tested this on a prod
server:

- Create a /tmp/test.pl file containing:

use Modern::Perl;

use Cwd;
my $dir = getcwd;

warn $dir;

1;

A) then create a cronjob entry to run it using koha-foreach:
(in /etc/cron.d/test):
1/* * * * * root koha-foreach perl /tmp/test.pl
- Once I noticed the cronjob ran, I used mutt to read the emails in the
root user.
=> FAIL:
...
Subject: Cron <root@koha> koha-foreach --enabled perl /tmp/test.pl

"/root"
"/root"
"/root"
"/root"
"/root"
...

B) I then used the patched koha-foreach with different results:
=> SUCCESS:
...
Subject: Cron <root@koha> /root/koha-foreach --chdir --enabled perl /tmp/test.pl

"/var/lib/koha/acaderc"
"/var/lib/koha/agro"
"/var/lib/koha/anc"
"/var/lib/koha/arico"
"/var/lib/koha/artes"
...

So this patch's approach works. But...

C) master's koha-foreach seems to work just the same... I think it is
because of my previous attempt to fix this by using sudo in koha-shell.
So I think environmental conditions affect the behaviour (which shell is
configured for cron, sudo configuration, etc).

====

In conclusion, I think we should go ahead with this patch as it will solve
peoples issues, and it is a right solution (option #5 on the list) to
this Perl behaviour change. It doesn't cover other commands, but
followup patches could do.

I avoided /tmp as it is writable by any user... so it is an easy path
for both exploiting by replacing some lib, and also because the
existence of an unreadable dir that the interpreter could try to
traverse (unreadable /tmp/Authen or /tmp/Koha will trigger the same
error, and I assume people know what they are putting on the instance's
dir, at least it will be easier to track).

A followup patch takes care of making the cronjobs use --chdir when
calling koha-foreach

[1] https://lists.debian.org/debian-devel-announce/2016/08/msg00013.html
[2] https://rt.perl.org/Public/Bug/Display.html?id=123795

Signed-off-by: Kyle M Hall <[email protected]>

Signed-off-by: Marcel de Rooy <[email protected]>

Signed-off-by: Nick Clemens <[email protected]>
xmorave2 pushed a commit that referenced this issue Jan 30, 2019
Correct the two issues I pointed out.

Signed-off-by: Owen Leonard <[email protected]>

Signed-off-by: Jonathan Druart <[email protected]>

Signed-off-by: Nick Clemens <[email protected]>
(cherry picked from commit 77871b4)
Signed-off-by: Martin Renvoize <[email protected]>
(cherry picked from commit 49158f3)
Signed-off-by: Fridolin Somers <[email protected]>
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

No branches or pull requests

5 participants