Description
Jonathan Druart
2021-09-01 12:23:38 UTC
Created attachment 124326 [details] [review] Bug 28935: Filter user input through allowlist for patrons This patch creates public and staff allowlists that are used to filter the user input from the Staff Interface, OPAC, and OPAC self-registration. The lists are hard-coded into modules, but they could be generated by other means in the future to make them more suited to user context. Test plan: 0. The following should be done for the Staff Interface, a logged in OPAC user, and a OPAC self-registration user 1. Go to a patron create/edit screen 2. Using F12 dev tools in your browser, add an input for "flags" or "borrower_flags" depending on the other inputs available on that screen. Give it a value of "1". 3. Submit the change 4. Check the Staff Interface or database to confirm that the flags have not been set to 1 for that user Additional test plan: 1. prove t/Koha/Patron/Allowlist.t 2. Note that all tests pass Created attachment 124327 [details] [review] Bug 28935: Centralize the allow list We want to make sure developpers won't forget to update the allow list and that we get discrepancy between the list. This patch suggests to have: 1. A global "all fields" list which must match the borrowers table columns 2. A global deny list to apply on top of this list. This deny list is the list of attributes we want to reject from patron's edition from both staff and OPAC interfaces 3. A specific deny list per interface Moreover there is now a warning when a user edit an attribute part of the deny list. From bug 28929 comment 26 (In reply to Andreas Jonsson from bug 28929 comment #23) > I think that the apply function should throw an exception if the input > parameters are incorrect. As it stands, it will silently allow everything > if this is the case. > > if ($ui_fields && ref $ui_fields eq 'HASH'){ > ... > > if ( $input && ref $input eq 'HASH' ){ > ... It depends on what we want this function to do. Either it's a filter, or a confirmation we did things correctly prior to its call. I have added a warn there and it highlights the problem: we now have a lot of warnings in the logs. [2021/09/01 08:48:09] [WARN] Forbidden - Tried to modify 'dateexpiry' with '2029-12-01' from CGI::Compile::ROOT::kohadevbox_koha_opac_opac_2dmemberentry_2epl /kohadevbox/koha/opac/opac-memberentry.pl 277 at /kohadevbox/koha/Koha/Patron/Allowlist.pm line 56. Created attachment 124350 [details] [review] Bug 28935: Enforce *UnwantedField sysprefs I didn't recreate the problem on selfmod but it existed for selfreg. Create 2 attributes, 2 visible at the OPAC but only 1 editable. Self reg and modify the input hidden value. Have to say that we are adding a lot of code for two lines (staff and OPAC variant): delete $borrower{$_} for qw/ borrowernumber date_renewed gonenoaddress lost debarred debarredcomment relationship flags privacy privacy_guarantor_fines privacy_guarantor_checkouts checkprevcheckout updated_on lastseen lang login_attempts overdrive_auth_token anonymized dateenrolled dateexpiry borrowernotes opacnote sort1 sort2 sms_provider_id autorenew_checkouts/; Note that fields not in the borrower table are "filtered" by DBIx ;) No big deal. I like how I added the UnwantedField check in the second patch. I could have been "only 2 more lines", but you duplicate the logic. Moreover I think that having the list of fields in only one place is a good thing, it would be a mess to have it in different places. And there are tests ;) The code is pretty trivial. We need a change like this to prevent the Cardnumber already exists error: diff --git a/members/memberentry.pl b/members/memberentry.pl index 23bbe65..88387c8 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -326,7 +326,7 @@ if ($op eq 'save' || $op eq 'insert'){ # If the cardnumber is blank, treat it as null. $newdata{'cardnumber'} = undef if $newdata{'cardnumber'} =~ /^\s*$/; - if (my $error_code = checkcardnumber($newdata{cardnumber},$newdata{borrowernumber})){ + if (my $error_code = checkcardnumber( $newdata{cardnumber}, $borrowernumber )){ push @errors, $error_code == 1 ? 'ERROR_cardnumber_already_exists' : $error_code == 2 Comment on attachment 124327 [details] [review] Bug 28935: Centralize the allow list Review of attachment 124327 [details] [review]: ----------------------------------------------------------------- ::: Koha/Patron/Allowlist.pm @@ +159,5 @@ > + primary_contact_method > + ); > +} > + > +sub _global_deny_list { There are a few problems with this list. Staff should be allowed to change at least the following: gonenoaddress, lost, relationship directly through the memberentry.pl script. We'll need to remove those from here and add them to the public denylist. (This is one of the things I don't like about denylists. We have to remember to make 2 changes: one to the allow list and one to the deny list. Whereas with only allowlists, there is less risk of forgetting to subtract authorization. If authorization isn't added, it'll be obvious during testing/use and better to error on the side of caution I think.) (Fwiw I missed relationship in my initial list as well, since it's an atypical field used mostly by child users.) (In reply to David Cook from comment #8) > Comment on attachment 124327 [details] [review] [review] > Bug 28935: Centralize the allow list > > Review of attachment 124327 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/Patron/Allowlist.pm > @@ +159,5 @@ > > + primary_contact_method > > + ); > > +} > > + > > +sub _global_deny_list { > > There are a few problems with this list. > > Staff should be allowed to change at least the following: gonenoaddress, > lost, relationship directly through the memberentry.pl script. We'll need to > remove those from here and add them to the public denylist. > > (This is one of the things I don't like about denylists. We have to remember > to make 2 changes: one to the allow list and one to the deny list. Whereas > with only allowlists, there is less risk of forgetting to subtract > authorization. If authorization isn't added, it'll be obvious during > testing/use and better to error on the side of caution I think.) > > (Fwiw I missed relationship in my initial list as well, since it's an > atypical field used mostly by child users.) Indeed, gonenoaddress and lost are missing. Note that contacttitle and sms_provider_id were missing from the first patch. That was a bit of a mess to compare the two lists. And that's why I think it's better with 1. the whole list of fields, 2. a global deny list, and 3. a specific deny list per interface. Moreover the fields are listed identically as how it's in the DB (and tests prove that). To me it's a real win for maintainability. As well for the readability but that is subjective apparently. Created attachment 124416 [details] [review] Bug 28935: Allow gonenoaddress and lost for staff (In reply to David Cook from comment #8) > Staff should be allowed to change at least the following: gonenoaddress, > lost, relationship directly through the memberentry.pl script. We'll need to > remove those from here and add them to the public denylist. Good point! (In reply to Jonathan Druart from comment #10) > Created attachment 124416 [details] [review] [review] > Bug 28935: Allow gonenoaddress and lost for staff And relationship? You will see patron guarantor if the patron type is Child etc. Created attachment 124418 [details] [review] Bug 28935: Allow gonenoaddress, lost and relationship for staff So... Opened bug 28941. And we need to reuse Koha::Patron::Allowlist earlier than expected, unfortunately. It must be moved to be reused, suggestions: 1. Koha::Allowlist::{Patron,Suggestion} Seems weird but maybe the best place 2. Koha::Object::{Patron,Suggestion} Seems bad, we could use it for something else than object. But we could have the lists in/under Koha::{Patron,Suggestion}, which seems convenient and correct. 3. Koha::Util::Hashref Didn't we ban 'Util'? 4. Koha::Filter::Hashref Could do the logic of filtering, and we would pass the list of fields from Koha::Patron->columns_editable_{staff,opac} 5. Something else to suggest? (In reply to Jonathan Druart from comment #14) > So... Opened bug 28941. And we need to reuse Koha::Patron::Allowlist earlier > than expected, unfortunately. > > It must be moved to be reused, suggestions: > > 1. Koha::Allowlist::{Patron,Suggestion} > Seems weird but maybe the best place > > 2. Koha::Object::{Patron,Suggestion} > Seems bad, we could use it for something else than object. But we could have > the lists in/under Koha::{Patron,Suggestion}, which seems convenient and > correct. > > 3. Koha::Util::Hashref > Didn't we ban 'Util'? > > 4. Koha::Filter::Hashref > Could do the logic of filtering, and we would pass the list of fields from > Koha::Patron->columns_editable_{staff,opac} > > 5. Something else to suggest? Koha::Filter as the parent class? Subclass it for patrons under Koha::Patron::Filter ? Koha::Suggestion::Filter etc Filter might be just a bit too vague. What you want, is filter or validate parameters from outside. Something with taint? Note that I would not mind merging opac and staff in the same class. The current distinction is a bit articifial? Created attachment 124422 [details] [review] Bug 28935: Move logic to Koha::Allowlist for reusability I think that this is not enough. See comment7. Could be mistaken. But I needed that change to get my minimalistic version of this report working on 19.11.. Since we clear borrowernumber. diff --git a/members/memberentry.pl b/members/memberentry.pl index 6b63ee7650b..0fc3d2b022b 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -48,7 +48,7 @@ use Email::Valid; use Koha::SMS::Providers; use Koha::Patron::Allowlist::Staff; -my $ui_allowlist = Koha::Patron::Allowlist::Staff->new(); +my $ui_allowlist = Koha::Patron::Allowlist->new({ interface => 'staff' }); my $input = CGI->new; (In reply to Marcel de Rooy from comment #18) > I think that this is not enough. See comment7. > Could be mistaken. But I needed that change to get my minimalistic version > of this report working on 19.11.. Since we clear borrowernumber. Sorry, I thought I fixed it yesterday. We moved the patches from bug 28929 to let it be as minimalist as possible. The intention of this bug was to deal correctly with the different fields, not to still be minimalist. Created attachment 124425 [details] [review] Bug 28935: Take borrowernumber from the original input newdata is been modified, and filtered. We want to pick the original value of borrowernumber. (In reply to Jonathan Druart from comment #19) > (In reply to Marcel de Rooy from comment #18) > > I think that this is not enough. See comment7. > > Could be mistaken. But I needed that change to get my minimalistic version > > of this report working on 19.11.. Since we clear borrowernumber. > > Sorry, I thought I fixed it yesterday. > > We moved the patches from bug 28929 to let it be as minimalist as possible. > The intention of this bug was to deal correctly with the different fields, > not to still be minimalist. No problem with that, although I preferred to backport rightaway in a minimalistic approach locally. (In reply to Jonathan Druart from comment #17) > Created attachment 124422 [details] [review] [review] > Bug 28935: Move logic to Koha::Allowlist for reusability Note that I finally didn't use Koha::Allowlist on bug 28941. But this is still needed in my opinion. Koha::Allowlist If we're going to go with "Koha::Allowlist", we should probably rename "get_ui_fields" to something more generic like "get_entries". My intent around "Koha::Patron::Allowlist" was to keep the scope of the allowlist concept very constrained, as I didn't necessarily want to apply it to other parts of Koha without further consideration. Calling get_ui_fields() in apply() could be inefficient if used in a loop. (I know it's not right now, but this module has potential to be used elsewhere in the future.) I think that we should generate our lists, and then add them to "Koha::Allowlist" objects via mutator methods like "add_allow_entries" and "add_deny_entries". I think that would be more in-line with OOP. Then apply() would be much more efficient when applied against N inputs. If we're going to warn each key/value in %blocked, it probably makes more sense to put the warning in the foreach my $key (@key) loop, rather than double-handling the data. The perldoc for get_ui_fields in "Koha::Allowlist" also mentions "Koha::Patron::Allowlist::Public". -- Koha::Patron::Allowlist Why would we want to include all fields via _get_all_fields()? Doesn't that include more information than we need to and just ultimately increase the maintenance burden? (In reply to Marcel de Rooy from comment #16) > Note that I would not mind merging opac and staff in the same class. The > current distinction is a bit articifial? The distinction is very real, as we do have two separate interfaces, but I don't have any feelings about how/where that distinction is implemented. (In reply to David Cook from comment #24) > (In reply to Marcel de Rooy from comment #16) > > Note that I would not mind merging opac and staff in the same class. The > > current distinction is a bit articifial? > > The distinction is very real, as we do have two separate interfaces, but I > don't have any feelings about how/where that distinction is implemented. Hahaha FAIL members/memberentry.pl FAIL valid Can't locate Koha/Patron/Allowlist/Staff.pm in @INC (you may need to install the Koha::Patron::Allowlist::Staff module) (@INC contains: /usr/share/koha /usr/share/koha/lib /app/qatools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.28.1 /usr/local/share/perl/5.28.1 /usr/lib/x86_64-linux-gnu/perl5/5.28 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.28 /usr/share/perl/5.28 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base /var/lib/koha/myclone/plugins) FAIL t/Koha/Patron/Allowlist.t FAIL file permissions File must have the exec flag And some warns about adding and removing modules. Indeed we should squash this stuff. (In reply to David Cook from comment #23) > If we're going to go with "Koha::Allowlist", we should probably rename > "get_ui_fields" to something more generic like "get_entries". My intent > around "Koha::Patron::Allowlist" was to keep the scope of the allowlist > concept very constrained, as I didn't necessarily want to apply it to other > parts of Koha without further consideration. +1 for get_entries > Calling get_ui_fields() in apply() could be inefficient if used in a loop. > (I know it's not right now, but this module has potential to be used > elsewhere in the future.) I think that we should generate our lists, and > then add them to "Koha::Allowlist" objects via mutator methods like > "add_allow_entries" and "add_deny_entries". I think that would be more > in-line with OOP. Then apply() would be much more efficient when applied > against N inputs. Yes, we could generate already in new. Not sure what you mean here when you want to use add_entry or deny_entry? Instead of the parameter being used now? additional_deny_list The earlier remarks about only having an allow list are still valid too? The current exercise of all fields minus deny1 and deny2 could be simpler by having one allowlist with an additional optional attribute for the interface? $allow_patron = { fieldname => 1 | { interface => 1 } , .. } > If we're going to warn each key/value in %blocked, it probably makes more > sense to put the warning in the foreach my $key (@key) loop, rather than > double-handling the data. One warn should be enough to trigger a dev to add a new field. And if someone hacks three columns, I only need one warn. Or still an exception? > Why would we want to include all fields via _get_all_fields()? Doesn't that > include more information than we need to and just ultimately increase the > maintenance burden? I think you are right. See previous remark. Will give it a go now. Created attachment 124446 [details] [review] Bug 28935: Filter user input through allow list This is a squashed commit of earlier work and a starting point for some further proposed adjustments. Bug 28935: Filter user input through allowlist for patrons This patch creates public and staff allowlists that are used to filter the user input from the Staff Interface, OPAC, and OPAC self-registration. The lists are hard-coded into modules, but they could be generated by other means in the future to make them more suited to user context. Test plan: 0. The following should be done for the Staff Interface, a logged in OPAC user, and a OPAC self-registration user 1. Go to a patron create/edit screen 2. Using F12 dev tools in your browser, add an input for "flags" or "borrower_flags" depending on the other inputs available on that screen. Give it a value of "1". 3. Submit the change 4. Check the Staff Interface or database to confirm that the flags have not been set to 1 for that user Additional test plan: 1. prove t/Koha/Patron/Allowlist.t 2. Note that all tests pass Bug 28935: Centralize the allow list We want to make sure developpers won't forget to update the allow list and that we get discrepancy between the list. This patch suggests to have: 1. A global "all fields" list which must match the borrowers table columns 2. A global deny list to apply on top of this list. This deny list is the list of attributes we want to reject from patron's edition from both staff and OPAC interfaces 3. A specific deny list per interface Moreover there is now a warning when a user edit an attribute part of the deny list. Bug 28935: Enforce *UnwantedField sysprefs I didn't recreate the problem on selfmod but it existed for selfreg. Create 2 attributes, 2 visible at the OPAC but only 1 editable. Self reg and modify the input hidden value. Bug 28935: Allow gonenoaddress, lost and relationship for staff Bug 28935: Move logic to Koha::Allowlist for reusability Bug 28935: Take borrowernumber from the original input newdata is been modified, and filtered. We want to pick the original value of borrowernumber. Created attachment 124447 [details] [review] Bug 28935: Fix module in memberentry and chmod Allowlist.t Further changes forthcoming Created attachment 124448 [details] [review] Bug 28935: Rename get_ui_fields, introduce add and remove (In reply to Marcel de Rooy from comment #31) > Created attachment 124448 [details] [review] [review] > Bug 28935: Rename get_ui_fields, introduce add and remove Submitted as concept of proposed changes (In reply to Marcel de Rooy from comment #27) > > Calling get_ui_fields() in apply() could be inefficient if used in a loop. > > (I know it's not right now, but this module has potential to be used > > elsewhere in the future.) I think that we should generate our lists, and > > then add them to "Koha::Allowlist" objects via mutator methods like > > "add_allow_entries" and "add_deny_entries". I think that would be more > > in-line with OOP. Then apply() would be much more efficient when applied > > against N inputs. > > Yes, we could generate already in new. Not sure what you mean here when you > want to use add_entry or deny_entry? Instead of the parameter being used > now? additional_deny_list > The earlier remarks about only having an allow list are still valid too? The > current exercise of all fields minus deny1 and deny2 could be simpler by > having one allowlist with an additional optional attribute for the interface? > > $allow_patron = { fieldname => 1 | { interface => 1 } , .. } > I mean something like the following: my $allowlist = Koha::Allowlist->new(); my $allow_entries = Koha::Patron::Allowlist->allow_entries({ interface => 'opac' }); my $deny_entries = Koha::Patron::Allowlist->deny_entries({ interface => 'opac' }); $allowlist->add_allow_entries($allow_entries); $allowlist->add_deny_entries($deny_entries); if ($additional_deny_entries){ $allowlist->add_deny_entries($additional_deny_entries); } foreach my $borrower ( @borrowers ) { $allowlist->apply({ input => $borrower }); } -- I know it's a lot longer, but it's more flexible and efficient. > > If we're going to warn each key/value in %blocked, it probably makes more > > sense to put the warning in the foreach my $key (@key) loop, rather than > > double-handling the data. > > One warn should be enough to trigger a dev to add a new field. And if > someone hacks three columns, I only need one warn. Or still an exception? I mean originally I thought about only dumping out %blocked via a DEBUG log level in the controller script rather than the module. I don't know if anyone else has looked, but with the status quo %blocked will have at least a few entries (like $borrowernumber) because we often send more data than we need. So raising an exception currently would break everything. Raising an exception might also be undesirable because it would signify to an attacker that they weren't successful. We don't necessarily want an attacker to know at attack time whether or not their attack worked. But that's just my 2 cents. I don't mind silently filtering in this context. But I think the logging isn't too important. Maybe we could wrap it in a DEBUG log level condition and move on? We also need: diff --git a/members/memberentry.pl b/members/memberentry.pl index 509b5ac19e..5839557c9e 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -48,7 +48,7 @@ use Email::Valid; use Koha::SMS::Providers; use Koha::Patron::Allowlist; -my $ui_allowlist = Koha::Patron::Allowlist->new({ interface => 'staff' }); +my $allowlist = Koha::Patron::Allowlist->new({ interface => 'staff' }); my $input = CGI->new; @@ -434,7 +434,9 @@ if ( defined $sms ) { $newdata{smsalertnumber} = $sms; } -$ui_allowlist->apply({ input => \%newdata }); +my $allowed_data = { %newdata }; +$allowlist->apply({ input => $allowed_data, verbose => 1 }); # TODO Should we log results? +%newdata = %$allowed_data; ### Error checks should happen before this line. $nok = $nok || scalar(@errors); my $deny_entries = Koha::Patron::Allowlist->deny_entries({ interface => 'opac' }); Hmm Reads a bit odd. (In reply to David Cook from comment #33) > I know it's a lot longer, but it's more flexible and efficient. Dont really see that. You have ->add and ->remove. So you can do whatever you want.. Created attachment 124449 [details] [review] Bug 28935: Rename get_ui_fields, introduce add and remove Also move test to t/Koha/Allowlist.t Created attachment 124450 [details] [review] Bug 28935: Apply modules change to controller scripts (In reply to David Cook from comment #33) > my $allowlist = Koha::Allowlist->new(); > > my $allow_entries = Koha::Patron::Allowlist->allow_entries({ interface => > 'opac' }); > $allowlist->add_allow_entries($allow_entries); > Did add a load and unload method to more or less implement this. Is that much more efficient? Not sure. We should now always ->new AND ->load at least.. NOTE: Lets decide over the weekend if we stick to the first 6 patches or continue with the LAST 4 patches. Patch 7 is a squash of the previous 6. Created attachment 124461 [details] [review] Bug 28935: Rename get_ui_fields, introduce add and remove Also move test to t/Koha/Allowlist.t Created attachment 124462 [details] [review] Bug 28935: Apply modules change to controller scripts Might be worth taking a look at bug 28948 where we add an allow list for fields that should be available on the public API.. I was envisaging similar methods for public_write_list, and staff_write_list to denote fields allowed to be read or written to given various interfaces. Created attachment 124499 [details] [review] Bug 28935: Filter user input through allow list This is a squashed commit of earlier work and a starting point for some further proposed adjustments. Bug 28935: Filter user input through allowlist for patrons This patch creates public and staff allowlists that are used to filter the user input from the Staff Interface, OPAC, and OPAC self-registration. The lists are hard-coded into modules, but they could be generated by other means in the future to make them more suited to user context. Test plan: 0. The following should be done for the Staff Interface, a logged in OPAC user, and a OPAC self-registration user 1. Go to a patron create/edit screen 2. Using F12 dev tools in your browser, add an input for "flags" or "borrower_flags" depending on the other inputs available on that screen. Give it a value of "1". 3. Submit the change 4. Check the Staff Interface or database to confirm that the flags have not been set to 1 for that user Additional test plan: 1. prove t/Koha/Patron/Allowlist.t 2. Note that all tests pass Bug 28935: Centralize the allow list We want to make sure developpers won't forget to update the allow list and that we get discrepancy between the list. This patch suggests to have: 1. A global "all fields" list which must match the borrowers table columns 2. A global deny list to apply on top of this list. This deny list is the list of attributes we want to reject from patron's edition from both staff and OPAC interfaces 3. A specific deny list per interface Moreover there is now a warning when a user edit an attribute part of the deny list. Bug 28935: Enforce *UnwantedField sysprefs I didn't recreate the problem on selfmod but it existed for selfreg. Create 2 attributes, 2 visible at the OPAC but only 1 editable. Self reg and modify the input hidden value. Bug 28935: Allow gonenoaddress, lost and relationship for staff Bug 28935: Move logic to Koha::Allowlist for reusability Bug 28935: Take borrowernumber from the original input newdata is been modified, and filtered. We want to pick the original value of borrowernumber. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 124500 [details] [review] Bug 28935: Fix module in memberentry and chmod Allowlist.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 124501 [details] [review] Bug 28935: Rename get_ui_fields, introduce add and remove Also move test to t/Koha/Allowlist.t And add load, unload methods. Test plan: Run Allowlist.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 124502 [details] [review] Bug 28935: Apply module changes to controller scripts Test plan: Add an unallowed field to the submit with dom inspector in opac or staff. Check if it is ignored and if log contains one warning. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 124503 [details] [review] Bug 28935: Add logging to controller scripts Logged at DEBUG level. Test plan: Adjust your logging config, add forbidden field, check log. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> The last debug line shows that an opac-memberentry UPDATE operation only shows unallowed stuff, and no longer borrowernumber or anonymized etc. FIrst I moved the merge of $borrower and %borrower out of the way. Later I decided to add an allowlist->add(borrowernumber) to remove the false borrowernumber warning. Perhaps we can remove that if we could move the line where the borrowernumber is inserted. [DEBUG] opac-memberentry.pl: allowlist blocked fields: anonymized=0, autorenew_checkouts=1, borrowernotes=2, borrowernumber=51, checkprevcheckout=inherit, date_renewed=, dateenrolled=2021-07-08, dateexpiry=2029-10-08, debarred=, debarredcomment=, flags=1, gonenoaddress=2, lang=default, lastseen=, login_attempts=0, lost=0, opacnote=1, overdrive_auth_token=, privacy=1, privacy_guarantor_checkouts=0, privacy_guarantor_fines=0, relationship=, sms_provider_id=, sort1=, sort2=, updated_on=2021-09-03 13:59:14, zzgonenoaddress=abc CGI::Compile::ROOT::usr_share_koha_opac_opac_2dmemberentry_2epl::__ANON__ /usr/share/koha/opac/opac-memberentry.pl (288) [DEBUG] opac-memberentry.pl: allowlist blocked fields: borrowernumber=51, gonenoaddress=2, gonenoaddress2=3 CGI::Compile::ROOT::usr_share_koha_opac_opac_2dmemberentry_2epl::__ANON__ /usr/share/koha/opac/opac-memberentry.pl (283) [DEBUG] opac-memberentry.pl: allowlist blocked fields: gonenoaddress=2, gonenoaddress2=3 CGI::Compile::ROOT::usr_share_koha_opac_opac_2dmemberentry_2epl::__ANON__ /usr/share/koha/opac/opac-memberentry.pl (284) (In reply to Martin Renvoize from comment #43) > Might be worth taking a look at bug 28948 where we add an allow list for > fields that should be available on the public API.. I was envisaging > similar methods for public_write_list, and staff_write_list to denote fields > allowed to be read or written to given various interfaces. Yeah. Certainly interesting. Maybe not for now directly, but we should move such things closer to each other. Created attachment 124523 [details] [review] Bug 28935: Filter user input through allowlist for patrons This patch creates public and staff allowlists that are used to filter the user input from the Staff Interface, OPAC, and OPAC self-registration. The lists are hard-coded into modules, but they could be generated by other means in the future to make them more suited to user context. Test plan: 0. The following should be done for the Staff Interface, a logged in OPAC user, and a OPAC self-registration user 1. Go to a patron create/edit screen 2. Using F12 dev tools in your browser, add an input for "flags" or "borrower_flags" depending on the other inputs available on that screen. Give it a value of "1". 3. Submit the change 4. Check the Staff Interface or database to confirm that the flags have not been set to 1 for that user Additional test plan: 1. prove t/Koha/Patron/Allowlist.t 2. Note that all tests pass Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 124524 [details] [review] Bug 28935: Centralize the allow list We want to make sure developpers won't forget to update the allow list and that we get discrepancy between the list. This patch suggests to have: 1. A global "all fields" list which must match the borrowers table columns 2. A global deny list to apply on top of this list. This deny list is the list of attributes we want to reject from patron's edition from both staff and OPAC interfaces 3. A specific deny list per interface Moreover there is now a warning when a user edit an attribute part of the deny list. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 124525 [details] [review] Bug 28935: Enforce *UnwantedField sysprefs I didn't recreate the problem on selfmod but it existed for selfreg. Create 2 attributes, 2 visible at the OPAC but only 1 editable. Self reg and modify the input hidden value. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 124526 [details] [review] Bug 28935: Allow gonenoaddress, lost and relationship for staff Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 124527 [details] [review] Bug 28935: Move logic to Koha::Allowlist for reusability Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 124528 [details] [review] Bug 28935: Take borrowernumber from the original input newdata is been modified, and filtered. We want to pick the original value of borrowernumber. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 124529 [details] [review] Bug 28935: Fix module in memberentry and chmod Allowlist.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 124530 [details] [review] Bug 28935: Rename get_ui_fields, introduce add and remove Also move test to t/Koha/Allowlist.t And add load, unload methods. Test plan: Run Allowlist.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 124531 [details] [review] Bug 28935: Apply module changes to controller scripts Test plan: Add an unallowed field to the submit with dom inspector in opac or staff. Check if it is ignored and if log contains one warning. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 124532 [details] [review] Bug 28935: Add logging to controller scripts Logged at DEBUG level. Test plan: Adjust your logging config, add forbidden field, check log. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Lets see if we can get this further now Created attachment 124640 [details] [review] Bug 28935: Filter user input through allowlist for patrons This patch creates public and staff allowlists that are used to filter the user input from the Staff Interface, OPAC, and OPAC self-registration. The lists are hard-coded into modules, but they could be generated by other means in the future to make them more suited to user context. Test plan: 0. The following should be done for the Staff Interface, a logged in OPAC user, and a OPAC self-registration user 1. Go to a patron create/edit screen 2. Using F12 dev tools in your browser, add an input for "flags" or "borrower_flags" depending on the other inputs available on that screen. Give it a value of "1". 3. Submit the change 4. Check the Staff Interface or database to confirm that the flags have not been set to 1 for that user Additional test plan: 1. prove t/Koha/Patron/Allowlist.t 2. Note that all tests pass Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 124641 [details] [review] Bug 28935: Centralize the allow list We want to make sure developpers won't forget to update the allow list and that we get discrepancy between the list. This patch suggests to have: 1. A global "all fields" list which must match the borrowers table columns 2. A global deny list to apply on top of this list. This deny list is the list of attributes we want to reject from patron's edition from both staff and OPAC interfaces 3. A specific deny list per interface Moreover there is now a warning when a user edit an attribute part of the deny list. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 124642 [details] [review] Bug 28935: Enforce *UnwantedField sysprefs I didn't recreate the problem on selfmod but it existed for selfreg. Create 2 attributes, 2 visible at the OPAC but only 1 editable. Self reg and modify the input hidden value. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 124643 [details] [review] Bug 28935: Allow gonenoaddress, lost and relationship for staff Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 124644 [details] [review] Bug 28935: Move logic to Koha::Allowlist for reusability Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 124645 [details] [review] Bug 28935: Take borrowernumber from the original input newdata is been modified, and filtered. We want to pick the original value of borrowernumber. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 124646 [details] [review] Bug 28935: Fix module in memberentry and chmod Allowlist.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 124647 [details] [review] Bug 28935: Rename get_ui_fields, introduce add and remove Also move test to t/Koha/Allowlist.t And add load, unload methods. Test plan: Run Allowlist.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 124648 [details] [review] Bug 28935: Apply module changes to controller scripts Test plan: Add an unallowed field to the submit with dom inspector in opac or staff. Check if it is ignored and if log contains one warning. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 124649 [details] [review] Bug 28935: Add logging to controller scripts Logged at DEBUG level. Test plan: Adjust your logging config, add forbidden field, check log. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 124650 [details] [review] Bug 28935: (follow-up) Correct attribute test Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Couple comments: 1 - Should we remove the deletion of 'flags' in ParseCGIForBorrower and allow the allowlist to handle it? 2 - I think we could lower the logging level to 'WARN' and add info about what was attempted to be forced - logging attempts to alter data seems like it doesn't need to be 'DEBUG' (In reply to Nick Clemens from comment #73) Thx for your signoff. > 1 - Should we remove the deletion of 'flags' in ParseCGIForBorrower and > allow the allowlist to handle it? Could be. But I would personally leave it as is. > 2 - I think we could lower the logging level to 'WARN' and add info about > what was attempted to be forced - logging attempts to alter data seems like > it doesn't need to be 'DEBUG' DEBUG is a lower level than WARN. Normally you would set your app to WARN and not receive DEBUG info. Only if you want diagnostic info, you will lower the log level to DEBUG. The debug info contains all fields that were attempted to change. Created attachment 124730 [details] [review] Bug 28935: Filter user input through allowlist for patrons This patch creates public and staff allowlists that are used to filter the user input from the Staff Interface, OPAC, and OPAC self-registration. The lists are hard-coded into modules, but they could be generated by other means in the future to make them more suited to user context. Test plan: 0. The following should be done for the Staff Interface, a logged in OPAC user, and a OPAC self-registration user 1. Go to a patron create/edit screen 2. Using F12 dev tools in your browser, add an input for "flags" or "borrower_flags" depending on the other inputs available on that screen. Give it a value of "1". 3. Submit the change 4. Check the Staff Interface or database to confirm that the flags have not been set to 1 for that user Additional test plan: 1. prove t/Koha/Patron/Allowlist.t 2. Note that all tests pass Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124731 [details] [review] Bug 28935: Centralize the allow list We want to make sure developpers won't forget to update the allow list and that we get discrepancy between the list. This patch suggests to have: 1. A global "all fields" list which must match the borrowers table columns 2. A global deny list to apply on top of this list. This deny list is the list of attributes we want to reject from patron's edition from both staff and OPAC interfaces 3. A specific deny list per interface Moreover there is now a warning when a user edit an attribute part of the deny list. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124732 [details] [review] Bug 28935: Enforce *UnwantedField sysprefs I didn't recreate the problem on selfmod but it existed for selfreg. Create 2 attributes, 2 visible at the OPAC but only 1 editable. Self reg and modify the input hidden value. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124733 [details] [review] Bug 28935: Allow gonenoaddress, lost and relationship for staff Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124734 [details] [review] Bug 28935: Move logic to Koha::Allowlist for reusability Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124735 [details] [review] Bug 28935: Take borrowernumber from the original input newdata is been modified, and filtered. We want to pick the original value of borrowernumber. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124736 [details] [review] Bug 28935: Fix module in memberentry and chmod Allowlist.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124737 [details] [review] Bug 28935: Rename get_ui_fields, introduce add and remove Also move test to t/Koha/Allowlist.t And add load, unload methods. Test plan: Run Allowlist.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124738 [details] [review] Bug 28935: Apply module changes to controller scripts Test plan: Add an unallowed field to the submit with dom inspector in opac or staff. Check if it is ignored and if log contains one warning. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124739 [details] [review] Bug 28935: Add logging to controller scripts Logged at DEBUG level. Test plan: Adjust your logging config, add forbidden field, check log. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124740 [details] [review] Bug 28935: (follow-up) Correct attribute test Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> This seems pretty well thought out and I can see how it could be applied in other cases. It all works well, has tests and qa scripts pass. Passing QA. My one minor comment was around the Koha::AllowList module name.. I had a few thoughts on nameing options but decided none of them really made any more sense. Koha::Base::AllowList Koha::Object::AllowList Something that made it clear it's a base class.. but as we have Koha::Exception for much the same sort of idea.. just as a base.. this isn't a blocker. PQA This is a pain when trying to apply it to objects for the API :( I'm still of the opinion ::Allowlist is too generic. We have lots of differing Allowlist needs.. Read, ReadOnly and ReadWrite fields and different subsets for different access points... Intranet/OPAC, API Public/Staff. I tried using the Allowlist approach in a follow-up on bug 28948 and really it just highlights this. I used it to create Public vs Staff Readable field lists for the API.. that's the opposite of what you've done here as the Allowlist is listing Writable fields. And what about the UnwantedFields stuff.. should we really be doing that at the controller level instead of in the object where it will get properly tested and caught for all interfaces... try to do that in the Koha::Object base class for the to_api method and we come really unstuck. Frankly.. whilst this passes QA in terms of the code works for the use case here and only that use case.. I feel a fundamental base like this needs some more work.... you even choose NOT to use it in clearly related bugs. We could push a very simple patch to remove some borrower fields from member entry in opac and staff, less than 10 lines. I already have it running. Then we have a bit more time to customize this. Otherwise stable versions are waiting for this discussion? It would not be hard to add an attribute to the allow list entries and have apply check that attribute. In that way you could generate various lists for borrowers based on combinations of these attributes. Yes, Marcel, please attach the patch here! Created attachment 124769 [details] [review] Bug 28935: No filtering on patron's data on member entry pages [SIMPLE] Security patch. Follow-up for 28929. Backported minimalistically. Including correction for gonenoaddress and two others. (In reply to Marcel de Rooy from comment #90) > Created attachment 124769 [details] [review] [review] > Bug 28935: No filtering on patron's data on member entry pages [SIMPLE] > > Security patch. Follow-up for 28929. Backported minimalistically. > Including correction for gonenoaddress and two others. Doesnt include the Unwantedfields fix. (In reply to Marcel de Rooy from comment #88) > It would not be hard to add an attribute to the allow list entries and have > apply check that attribute. In that way you could generate various lists for > borrowers based on combinations of these attributes. If the allowlist is too generic, another approach would be to use the columns from DBIx, put some extra code in the Schema modules to create various column lists from there. Even closer to the DB? Created attachment 124784 [details] [review] Bug 28935: No filtering on patron's data on member entry pages Security patch. Follow-up for 28929. Including correction for gonenoaddress and two others. Includes unwanted fields too now. Will move the new code elsewhere (In reply to Marcel de Rooy from comment #94) > Will move the new code elsewhere See bug 28999. Removed the patron related stuff. So only generic code. But this patch still needs a signoff. Activity enough on Bugzilla, but this security patch got no attention ;) Created attachment 124832 [details] [review] Bug 28935: No filtering on patron's data on member entry pages Security patch. Follow-up for 28929. Including correction for gonenoaddress and two others. Includes unwanted fields too now. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 124833 [details] [review] Bug 28935: (follow-up) Use BorrowerUnwantedField on staff client Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 124837 [details] [review] Bug 28935: No filtering on patron's data on member entry pages Security patch. Follow-up for 28929. Including correction for gonenoaddress and two others. Includes unwanted fields too now. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 124838 [details] [review] Bug 28935: (follow-up) Use BorrowerUnwantedField on staff client Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 124839 [details] [review] Bug 28935: No filtering on patron's data on member entry pages Security patch. Follow-up for 28929. Including correction for gonenoaddress and two others. Includes unwanted fields too now. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 124840 [details] [review] Bug 28935: (QA follow-up) Use BorrowerUnwantedField on staff client Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Exchanged signoffs. Thanks Owen and Nick ! So passed QA now. Works on 20.05.x here was my test plan: 1. log into the OPAC 2. modify your personal details 3. right click on a field => inspect 4. edit as html (wording depends on the web browser) 5. add: <input type="text" name="borrower_dateexpiry" value="2034-03-03"> 6. submit the form 7. log into the staff interface 8. review patron updates 9. see that date expiry would have been changed 10. deny 11. apply patches 12. switch to the patron and resubmit a fraudulent update request 13. switch to the librarian and see that the date expiry isn't changed anymore Backported: Pushed to 20.05.x security branch for 20.05.16 Backported to 21.05.04, 20.11.10, 20.05.16, 19.11.22 Pushed to master for 21.11, thanks to everybody involved! |