Bug 38810 - SIP account level system preference overrides not properly cleared between requests
Summary: SIP account level system preference overrides not properly cleared between re...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Kyle M Hall (khall)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords: release-notes-needed
Depends on: 20954
Blocks:
  Show dependency treegraph
 
Reported: 2025-01-02 14:31 UTC by Kyle M Hall (khall)
Modified: 2025-04-17 12:45 UTC (History)
4 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00,24.11.04
Circulation function:


Attachments
Bug 38810 - SIP account level system preference overrides not properly cleared between requests (1.87 KB, patch)
2025-01-02 14:37 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38810 - SIP account level system preference overrides not properly cleared between requests (2.92 KB, patch)
2025-01-02 16:40 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38810 - SIP account level system preference overrides not properly cleared between requests (2.96 KB, patch)
2025-01-06 15:26 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 38810 - SIP account level system preference overrides not properly cleared between requests (3.02 KB, patch)
2025-01-07 15:40 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall (khall) 2025-01-02 14:31:23 UTC
Account level syspref overrides in SIP will "leak" between SIP requests. 

Basically an account level system preference override will may be set by a given request, but those overrides will never be cleared for the life of that SIP process thus "contaminating" that process until it reaches end of life".

This is because the code
    # Clear overrides from previous message handling first
    foreach my $key ( keys %ENV ) {
        delete $ENV{$key} if index($key, 'OVERRIDE_SYSPREF_') > 0;
    }
is checking if the env key contains the substring OVERRIDE_SYSPREF_ at an index greater than 0. The problem is that for all overrides the subtring *starts at 0* since it's always the first part of the string. If the substring is not part of the string index will return -1. TLDR we have an "off by one" error. We need to check that the return value is non-negative instead of positive.
Comment 1 Kyle M Hall (khall) 2025-01-02 14:34:05 UTC
I will amend my solution. Since the string will always start with OVERRIDE_SYSPREF_ we just need to verify if the return value is 0, not non-negative.
Comment 2 Kyle M Hall (khall) 2025-01-02 14:37:59 UTC
Created attachment 176056 [details] [review]
Bug 38810 - SIP account level system preference overrides not properly cleared between requests

Account level syspref overrides in SIP will "leak" between SIP requests.

Basically an account level system preference override will may be set by a given request, but those overrides will never be cleared for the life of that SIP process thus "contaminating" that process until it reaches end of life".

This is because the code
    # Clear overrides from previous message handling first
    foreach my $key ( keys %ENV ) {
        delete $ENV{$key} if index($key, 'OVERRIDE_SYSPREF_') > 0;
    }
is checking if the env key contains the substring OVERRIDE_SYSPREF_ at an index greater than 0. The problem is that for all overrides the subtring *starts at 0* since it's always the first part of the string. If the substring is not part of the string index will return -1. TLDR we have an "off by one" error. We need to check that the return value is zero instead of any positive value.
Comment 3 Kyle M Hall (khall) 2025-01-02 16:40:58 UTC
Created attachment 176096 [details] [review]
Bug 38810 - SIP account level system preference overrides not properly cleared between requests

Account level syspref overrides in SIP will "leak" between SIP requests.

Basically an account level system preference override will may be set by a given request, but those overrides will never be cleared for the life of that SIP process thus "contaminating" that process until it reaches end of life".

This is because the code
    # Clear overrides from previous message handling first
    foreach my $key ( keys %ENV ) {
        delete $ENV{$key} if index($key, 'OVERRIDE_SYSPREF_') > 0;
    }
is checking if the env key contains the substring OVERRIDE_SYSPREF_ at an index greater than 0. The problem is that for all overrides the subtring *starts at 0* since it's always the first part of the string. If the substring is not part of the string index will return -1. TLDR we have an "off by one" error. We need to check that the return value is zero instead of any positive value.

1) Set your SIP server params so that only one SIP process should run,
  <server-params
    min_servers='1'
    min_spare_servers='0'
    max_spare_servers='0'
    max_servers='0'
    custom_tcp_keepalive='0'
    custom_tcp_keepalive_time='7200'
    custom_tcp_keepalive_intvl='75'
  />
2) Set noissuescharge to $5
3) Create two SIP accounts
4) Set an account level override for noissuescharge to $500
5) Create a patron that owes $10 in fines
6) Run a patron information request on the "normal" SIP account for the
   patron, the patron flags should show the patron being not able to
   check out items
7) Run a patron information request of the "overridden" SIp account for
   the patron, the patron flags should show the patron being allowed to
   check out items
8) Run a second patron information request on the "normal" SIP account
   for the patron, the patron flags should show the patron being able to
   check out even though they should not be able to check out
9) Apply this patch
10) Restart all the things!
11) Repeast steps 6-8
12) The flags should be correct!
Comment 4 David Nind 2025-01-03 18:04:27 UTC
Could you provide an example request for step 6? 

SIP is still a bit of a mystery to me 8-)
Comment 5 Kyle M Hall (khall) 2025-01-06 12:08:12 UTC
(In reply to David Nind from comment #4)
> Could you provide an example request for step 6? 
> 
> SIP is still a bit of a mystery to me 8-)

You can use the SIP cli tester:
misc/sip_cli_emulator.pl --address 127.0.0.1 --port 6001 --sip_user koha --sip_pass koha --location MPL --terminator CR --message patron_informatio --patron $CARDNUMBER
Comment 6 Jake Deery 2025-01-06 15:26:58 UTC
Created attachment 176153 [details] [review]
Bug 38810 - SIP account level system preference overrides not properly cleared between requests

Account level syspref overrides in SIP will "leak" between SIP requests.

Basically an account level system preference override will may be set by a given request, but those overrides will never be cleared for the life of that SIP process thus "contaminating" that process until it reaches end of life".

This is because the code
    # Clear overrides from previous message handling first
    foreach my $key ( keys %ENV ) {
        delete $ENV{$key} if index($key, 'OVERRIDE_SYSPREF_') > 0;
    }
is checking if the env key contains the substring OVERRIDE_SYSPREF_ at an index greater than 0. The problem is that for all overrides the subtring *starts at 0* since it's always the first part of the string. If the substring is not part of the string index will return -1. TLDR we have an "off by one" error. We need to check that the return value is zero instead of any positive value.

1) Set your SIP server params so that only one SIP process should run,
  <server-params
    min_servers='1'
    min_spare_servers='0'
    max_spare_servers='0'
    max_servers='0'
    custom_tcp_keepalive='0'
    custom_tcp_keepalive_time='7200'
    custom_tcp_keepalive_intvl='75'
  />
2) Set noissuescharge to $5
3) Create two SIP accounts
4) Set an account level override for noissuescharge to $500
5) Create a patron that owes $10 in fines
6) Run a patron information request on the "normal" SIP account for the
   patron, the patron flags should show the patron being not able to
   check out items
7) Run a patron information request of the "overridden" SIp account for
   the patron, the patron flags should show the patron being allowed to
   check out items
8) Run a second patron information request on the "normal" SIP account
   for the patron, the patron flags should show the patron being able to
   check out even though they should not be able to check out
9) Apply this patch
10) Restart all the things!
11) Repeast steps 6-8
12) The flags should be correct!

Signed-off-by: Jake Deery <jake.deery@ptfs-europe.com>
Comment 7 Jake Deery 2025-01-06 15:27:39 UTC
Followed the plan, had a play, and couldn't break nowt. Signed off.

Jake.
Comment 8 Martin Renvoize (ashimema) 2025-01-07 15:40:08 UTC
Created attachment 176198 [details] [review]
Bug 38810 - SIP account level system preference overrides not properly cleared between requests

Account level syspref overrides in SIP will "leak" between SIP requests.

Basically an account level system preference override will may be set by a given request, but those overrides will never be cleared for the life of that SIP process thus "contaminating" that process until it reaches end of life".

This is because the code
    # Clear overrides from previous message handling first
    foreach my $key ( keys %ENV ) {
        delete $ENV{$key} if index($key, 'OVERRIDE_SYSPREF_') > 0;
    }
is checking if the env key contains the substring OVERRIDE_SYSPREF_ at an index greater than 0. The problem is that for all overrides the subtring *starts at 0* since it's always the first part of the string. If the substring is not part of the string index will return -1. TLDR we have an "off by one" error. We need to check that the return value is zero instead of any positive value.

1) Set your SIP server params so that only one SIP process should run,
  <server-params
    min_servers='1'
    min_spare_servers='0'
    max_spare_servers='0'
    max_servers='0'
    custom_tcp_keepalive='0'
    custom_tcp_keepalive_time='7200'
    custom_tcp_keepalive_intvl='75'
  />
2) Set noissuescharge to $5
3) Create two SIP accounts
4) Set an account level override for noissuescharge to $500
5) Create a patron that owes $10 in fines
6) Run a patron information request on the "normal" SIP account for the
   patron, the patron flags should show the patron being not able to
   check out items
7) Run a patron information request of the "overridden" SIp account for
   the patron, the patron flags should show the patron being allowed to
   check out items
8) Run a second patron information request on the "normal" SIP account
   for the patron, the patron flags should show the patron being able to
   check out even though they should not be able to check out
9) Apply this patch
10) Restart all the things!
11) Repeast steps 6-8
12) The flags should be correct!

Signed-off-by: Jake Deery <jake.deery@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Kyle M Hall (khall) 2025-01-07 15:59:48 UTC
Thanks yinz!
Comment 10 Katrin Fischer 2025-01-10 14:01:56 UTC
Amended bug description ... Bug 38810: ;)
Comment 11 Katrin Fischer 2025-01-10 14:03:23 UTC
Small change, big test plan :D
Comment 12 Katrin Fischer 2025-01-10 17:00:40 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 13 David Nind 2025-01-26 21:59:51 UTC
I couldn't figure out what this means for libraries, so have added a release-notes-needed keyword.

Please add a release not for this change that will make sense for library staff.

Thanks!
Comment 14 Paul Derscheid 2025-04-17 12:45:39 UTC
Nice work everyone!

Pushed to 24.11.x for 24.11.04