Bug 28929

Summary: No filtering on borrowers.flags on member entry pages (OPAC, self registration, staff interface)
Product: Koha Reporter: Andreas Jonsson <andreas.jonsson>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: Pushed to oldoldoldstable --- QA Contact: Testopia <testopia>
Severity: critical    
Priority: P1 - high CC: 1joynelson, andreas.jonsson, bastien.gonckel, corinne.hayet, dcook, didier.gautheron, eric.begin, f.demians, fridolin.somers, jonathan.druart, katrin.fischer, kyle, laurent.ducos, lucas, m.de.rooy, magnus, martin.renvoize, mengu, mik, mtj, nick, nicolas.legrand, nugged, philippe.blouin, roger.grossmann, sonia.bouis, ulrich.kleiber, victor, ztajoli
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28941
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.04, 20.11.10, 20.05.16, 19.11.22
Bug Depends on:    
Bug Blocks: 28935, 28947    
Attachments: Bug 28929: Prevent flags to be sent during selfreg
Bug 28929: Add selenium tests
[Alternate] Bug 28929: Filter user input through allowlist for patrons
[Alternate] Bug 28929: Filter user input through allowlist for patrons
Bug 28929: Centralize the allow list
Bug 28929: [SIGNED_OFF] Prevent flags to be sent during selfreg
Bug 28929: Prevent flags to be sent during patron's edition
Bug 28929: Add selenium tests
Bug 28929: Prevent flags to be sent during patron's edition
Bug 28929: (follow-up) Add exec flag to tests
Bug 28929: Add selenium tests
Bug 28929: Prevent flags to be sent during patron's edition
Bug 28929: (follow-up) Add exec flag to tests
Bug 28929: Add selenium tests
Bug 28929: Prevent flags to be sent during patron's edition
Bug 28929: Add selenium tests

Description Andreas Jonsson 2021-08-31 12:29:43 UTC
There is no server side filtering of the borrower fields in the OPAC self-registration.  This makes it trivial to self register as a super librarian by simply adding an input element to the registration form using the DOM inspector:

<input type="hidden" name="borrower_flags" value="1" />

Any field can be modified using this.  (It is not possible to edit existing by adding a borrower_borrowernumber field, although this seems to be coincidental rather than by design.)

Fields that should not be editable in the OPAC must be filtered from the borrower data on the server side before adding the borrower to the database.
Comment 1 Jonathan Druart 2021-08-31 13:42:39 UTC
whooot!?
Comment 2 Jonathan Druart 2021-08-31 13:49:38 UTC
Confirmed
Comment 3 Marcel de Rooy 2021-08-31 13:55:21 UTC
(In reply to Andreas Jonsson from comment #0)
> There is no server side filtering of the borrower fields in the OPAC
> self-registration.  This makes it trivial to self register as a super
> librarian by simply adding an input element to the registration form using
> the DOM inspector:

Great catch !
Comment 4 Jonathan Druart 2021-08-31 13:56:10 UTC
Created attachment 124308 [details] [review]
Bug 28929: Prevent flags to be sent during selfreg
Comment 5 Jonathan Druart 2021-08-31 13:56:59 UTC
This patch fixes the problem but does not feel very robust.

We will need a selenium test.
Comment 6 Jonathan Druart 2021-08-31 13:58:30 UTC
Self modification seems safe.
Comment 7 Andreas Jonsson 2021-08-31 14:18:29 UTC
While the field flags is the most serious problem, there are additional fields that should not be editable.  For instance these: dateenrolled, dateexpiry, date_renewed, lost, debarred, debarredcomment, borrowernote, contactnote, opacnote, sort1, sort2, login_attempts, lastseen.  Maybe others as well.

borrowernumber should be explicitly filtered.

Also, fields in PatronSelfRegistrationBorrowerUnwantedField should be filtered I think.

Also, patron attributes that should not be editable in OPAC.
Comment 8 Jonathan Druart 2021-08-31 15:13:24 UTC
Created attachment 124310 [details] [review]
Bug 28929: Add selenium tests
Comment 9 Jonathan Druart 2021-08-31 15:15:18 UTC
I am done for today.
Comment 10 David Cook 2021-09-01 01:10:26 UTC
(In reply to Andreas Jonsson from comment #7)
> While the field flags is the most serious problem, there are additional
> fields that should not be editable.  For instance these: dateenrolled,
> dateexpiry, date_renewed, lost, debarred, debarredcomment, borrowernote,
> contactnote, opacnote, sort1, sort2, login_attempts, lastseen.  Maybe others
> as well.
> borrowernumber should be explicitly filtered.
> 
> Also, fields in PatronSelfRegistrationBorrowerUnwantedField should be
> filtered I think.
> 
> Also, patron attributes that should not be editable in OPAC.

Lots of good points, although we often let people update their sort1 and sort2 as those fields get used for all kinds of different purposes.
Comment 11 David Cook 2021-09-01 01:33:36 UTC
I think that we'd need an allow list to specify what columns are allowed. While we could use a deny list, it's harder to remember to add items to it. Plus, garbage data is much more difficult to filter out then.

I suppose we could "use ./koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/borrowers.json" to provide an allow list, although I'm not sure it's available in all supported Koha versions...

I think Andreas has a point about filtering out unwanted fields too which would need to cover these 3 system preferences:
  - BorrowerUnwantedField
  - PatronSelfModificationBorrowerUnwantedField
  - PatronSelfRegistrationBorrowerUnwantedField


Looking at ./members/memberentry.pl and it looks like you could do the same thing. It implements a bit of a denylist, but not comprehensive enough it seems.

For a different project, I've actually had to do per-column authorization by role, and it's not necessarily that easy to do comprehensively when different roles have different authorizations. In this case, we wouldn't necessarily have to go that far.
Comment 12 David Cook 2021-09-01 01:54:17 UTC
An allow list looks like it's problematic too though, as not everything that passes through ParseCgiForBorrower is actually a column. For example: password2, gdpr_proc_consent.

That said, we could just add those to our list of allowed fields I suppose. 

An additive rather than subtractive process will be more secure in my opinion, although it could lead to more bugs in the short-term if something is missed from the allow list.

It looks like patron attributes are handled separately by ParsePatronAttributes().
Comment 13 David Cook 2021-09-01 02:08:04 UTC Comment hidden (obsolete)
Comment 14 David Cook 2021-09-01 02:18:15 UTC
In 20.11, I was able to update my own flags via the Staff Interface as well.
Comment 15 David Cook 2021-09-01 04:08:34 UTC
After reviewing https://owasp.org/www-project-proactive-controls/v3/en/c5-validate-inputs, I think we really should be using an allow list.

I'll have a crack at a first draft.
Comment 16 David Cook 2021-09-01 04:08:34 UTC Comment hidden (obsolete)
Comment 17 David Cook 2021-09-01 05:23:43 UTC Comment hidden (obsolete)
Comment 18 Andreas Jonsson 2021-09-01 06:23:13 UTC
I agree with David that an allow list is a better solution.
Comment 19 David Cook 2021-09-01 06:36:32 UTC Comment hidden (obsolete)
Comment 20 David Cook 2021-09-01 06:43:58 UTC
I've marked as Needs Signoff, but it should probably be "In Discussion".

I don't love my patch, and it needs more testing (especially for GDPR and the unit tests could be expanded), but it seems to be correct and effective so far. 

I would actually like to have more context-specific allowlists based on the user authorizations, but hopefully I've left enough room in the approach for that to be possible. 

Most of Koha's authorizations are done at the scrpit-level, so it would be nice to actually have a system-wide granular authorization system, but I wasn't going to try to create that today...

I figure this is simple enough yet comprehensive enough for these member entry pages, but curious to hear what you all think.
Comment 21 David Cook 2021-09-01 06:46:37 UTC
Created attachment 124316 [details] [review]
[Alternate] Bug 28929: 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
Comment 23 Andreas Jonsson 2021-09-01 07:19:33 UTC
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' ){
           ...
Comment 24 Jonathan Druart 2021-09-01 08:49:59 UTC
Created attachment 124317 [details] [review]
Bug 28929: 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.
Comment 26 Jonathan Druart 2021-09-01 08:52:35 UTC
(In reply to Andreas Jonsson from 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.
Comment 27 Jonathan Druart 2021-09-01 08:58:19 UTC
(In reply to Jonathan Druart from comment #6)
> Self modification seems safe.

This was wrong, self modification is not safe either.
Comment 28 Katrin Fischer 2021-09-01 09:17:12 UTC
I am a little worried at seeing the complexity of the proposed fix. I wonder if in light of backporting to older affected versions blocking flags as a first important step is the way to go and having/discussing a more complete solution as a second step. 

Especially as we won't be able to provide fixed for all possibly affected old versions, a simple "here do this" might be nice.

If people can edit fields they should not, that's not great, but I'd not qualify it as a security issue.
Comment 29 Katrin Fischer 2021-09-01 09:33:19 UTC
This goes back far, just verified the problem with OPAC self registration and OPAC self modification in 18.11.16.

For self modification it shows "flags" as changed field on moderating the change request, but this still could be missed easily.
Comment 30 Jonathan Druart 2021-09-01 09:45:19 UTC
(In reply to Katrin Fischer from comment #28)
> Especially as we won't be able to provide fixed for all possibly affected
> old versions, a simple "here do this" might be nice.

This change should do it and should be applicable to old versions.

diff --git a/members/memberentry.pl b/members/memberentry.pl
index 5844b9c3f90..c9460583af7 100755
--- a/members/memberentry.pl
+++ b/members/memberentry.pl
@@ -215,6 +215,7 @@ if ( $op eq 'insert' || $op eq 'modify' || $op eq 'save' || $op eq 'duplicate' )
 # remove keys from %newdata that is not part of patron's attributes
 {
     my @keys_to_delete = (
+        qr/^flags$/,
         qr/^BorrowerMandatoryField$/,
         qr/^category_type$/,
         qr/^check_member$/,
diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl
index 8945cc339ba..c940a6e1af6 100755
--- a/opac/opac-memberentry.pl
+++ b/opac/opac-memberentry.pl
@@ -522,6 +522,7 @@ sub ParseCgiForBorrower {
     # Replace checkbox 'agreed' by datetime in gdpr_proc_consent
     $borrower{gdpr_proc_consent} = dt_from_string if  $borrower{gdpr_proc_consent} && $borrower{gdpr_proc_consent} eq 'agreed';
 
+    delete $borrower{flags};
     return %borrower;
 }
Comment 31 Katrin Fischer 2021-09-01 09:46:39 UTC
Can someone verify if turning off PatronSelfRegistration and PatronDetails is enough to prevent this bug as an immediate action?

PatronDetails only seems to deactivate the form fields on first glance. I tried also adding the submit button back in DOM and can submit, but get no change request - so I didn't manage to exploit it yet. But would love if someone could verify.
Comment 32 Jonathan Druart 2021-09-01 09:51:35 UTC
Below a copy of the email I sent to some people.


Hello everybody,

If you get this email I am expecting from you to talk about it to whom
could be concerned.

Yesterday a bug (28929) was reported about a possible privilege escalation.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28929
It is confirmed and is highlighly critical in some cases, and stay
critical in all other cases. It's really bad.
Basically we are missing filtering on patron's data when we create or
edit a patron. Which means a user can edit whichever info they want,
by modifying the DOM of the page.
And, it includes borrowers.flags (that's where it hurts really badly).

It impacts both OPAC and Staff interfaces.

What you should do:
1. Reduce the risks!
 * Disable PatronSelfRegistration (!)
 * Disable AutoApprovePatronProfileSettings (!) or you can totally
disable OPACPatronDetails
2. Make sure the staff interface is not accessible to the world
3. Remove old librarian accounts
4. Something else you have in mind? If so, please share!

Then you should (or ask your team to) help us. We will need people on
the bug to:
1. Write patches and discuss the best approach to fix the different
issues (there are patches already)
2. Test! and test! We will have to test on five different versions
(master+4 stables)!
3. QA, and *really* review, all the different versions (we really want
to prevent a mess publishing a fix that will introduce regressions).

We will then need to coordinate.
A plan (to discuss) could be:
1. Tell the big actors to be ready for an urgent release (This is the
goal of this email!)
2. Make the patches ready for the different versions (see the above)
3. Have all the packages ready, before we communicate publicly (Mason,
will you be available?)
4. Tell the people we know to upgrade (so, you, and those who are
aware of the problem)
5. Announce publicly

What do you think?

This info should not be public, but please communicate as much as you
can around you (people you trust of course).
No need to over communicate either, as we are not ready yet!

If you don't have access to the bug, just tell me (or Tomas, Katrin,
Martin, Nick, etc.)

Cheers,
Jonathan
Comment 33 Jonathan Druart 2021-09-01 10:14:37 UTC
(In reply to Katrin Fischer from comment #31)
> Can someone verify if turning off PatronSelfRegistration and PatronDetails
> is enough to prevent this bug as an immediate action?
> 
> PatronDetails only seems to deactivate the form fields on first glance. I
> tried also adding the submit button back in DOM and can submit, but get no
> change request - so I didn't manage to exploit it yet. But would love if
> someone could verify.

I managed to create a modification request, even with PatronDetails and PatronSelfRegistration turned off (modified initials to flags, set 1, added the submit button, and copied csrf from another form).
Comment 34 Marcel de Rooy 2021-09-01 11:28:34 UTC
(In reply to Jonathan Druart from comment #30)

> diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl
> index 8945cc339ba..c940a6e1af6 100755
> --- a/opac/opac-memberentry.pl
> +++ b/opac/opac-memberentry.pl
> @@ -522,6 +522,7 @@ sub ParseCgiForBorrower {
>      # Replace checkbox 'agreed' by datetime in gdpr_proc_consent
>      $borrower{gdpr_proc_consent} = dt_from_string if 
> $borrower{gdpr_proc_consent} && $borrower{gdpr_proc_consent} eq 'agreed';
>  
> +    delete $borrower{flags};
>      return %borrower;
>  }

Why not use sub CheckForInvalidFields ?
Comment 35 Marcel de Rooy 2021-09-01 12:18:05 UTC
Created attachment 124323 [details] [review]
Bug 28929: [SIGNED_OFF] Prevent flags to be sent during selfreg

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested OPAC and staff side. Prevents mangling flags column.
Comment 36 Jonathan Druart 2021-09-01 12:19:46 UTC
Focusing on the privileges escalation here.
Comment 37 Jonathan Druart 2021-09-01 12:21:43 UTC
(In reply to Marcel de Rooy from comment #34)
> (In reply to Jonathan Druart from comment #30)
> 
> > diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl
> > index 8945cc339ba..c940a6e1af6 100755
> > --- a/opac/opac-memberentry.pl
> > +++ b/opac/opac-memberentry.pl
> > @@ -522,6 +522,7 @@ sub ParseCgiForBorrower {
> >      # Replace checkbox 'agreed' by datetime in gdpr_proc_consent
> >      $borrower{gdpr_proc_consent} = dt_from_string if 
> > $borrower{gdpr_proc_consent} && $borrower{gdpr_proc_consent} eq 'agreed';
> >  
> > +    delete $borrower{flags};
> >      return %borrower;
> >  }
> 
> Why not use sub CheckForInvalidFields ?

The idea was to make sure the fix would be backportable/applicable easily on older versions. Feel free to suggest an alternative patch if you found something better :)
Comment 38 Marcel de Rooy 2021-09-01 12:22:10 UTC
I would be in favor of pushing the simple patch now first (easy to backport).
It seems that flags is the most critical field. I have been testing a bit with trying to modify borrowernumber but that seems to work fine. So we cannot change another borrower by adding an input borrowernumber.

Surely we do not want to hack cardnumber, dateexpiry, debarred and so, but that could follow soon.
Comment 39 Jonathan Druart 2021-09-01 12:25:26 UTC
Created attachment 124325 [details] [review]
Bug 28929: Prevent flags to be sent during patron's edition

* selfreg and selfmod for OPAC
* patron's edition on staff

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested OPAC and staff side. Prevents mangling flags column.
Comment 40 Jonathan Druart 2021-09-01 12:27:56 UTC
The two other patches have been moved to bug 28935.
Comment 41 Jonathan Druart 2021-09-01 13:19:08 UTC
(In reply to Andreas Jonsson from comment #7)
> While the field flags is the most serious problem, there are additional
> fields that should not be editable.  For instance these: dateenrolled,
> dateexpiry, date_renewed, lost, debarred, debarredcomment, borrowernote,
> contactnote, opacnote, sort1, sort2, login_attempts, lastseen.  Maybe others
> as well.
> 
> borrowernumber should be explicitly filtered.

Done on bug 28935 (with the patches moved there).

> Also, fields in PatronSelfRegistrationBorrowerUnwantedField should be
> filtered I think.

Additional patch added on bug 28935

> Also, patron attributes that should not be editable in OPAC.

Didn't recreate that, but still enforced on the last patch on bug 28935.
Comment 42 Nick Clemens 2021-09-01 13:38:39 UTC
Created attachment 124364 [details] [review]
Bug 28929: Add selenium tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 43 Nick Clemens 2021-09-01 13:38:43 UTC
Created attachment 124365 [details] [review]
Bug 28929: Prevent flags to be sent during patron's edition

* selfreg and selfmod for OPAC
* patron's edition on staff

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested OPAC and staff side. Prevents mangling flags column.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 44 Nick Clemens 2021-09-01 13:38:47 UTC
Created attachment 124366 [details] [review]
Bug 28929: (follow-up) Add exec flag to tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 45 Kyle M Hall 2021-09-01 15:49:32 UTC
Created attachment 124405 [details] [review]
Bug 28929: Add selenium tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 46 Kyle M Hall 2021-09-01 15:49:44 UTC
Created attachment 124406 [details] [review]
Bug 28929: Prevent flags to be sent during patron's edition

* selfreg and selfmod for OPAC
* patron's edition on staff

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested OPAC and staff side. Prevents mangling flags column.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 47 Kyle M Hall 2021-09-01 15:49:48 UTC
Created attachment 124407 [details] [review]
Bug 28929: (follow-up) Add exec flag to tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 48 David Cook 2021-09-01 23:46:27 UTC
(In reply to Katrin Fischer from comment #28)
> I am a little worried at seeing the complexity of the proposed fix. I wonder
> if in light of backporting to older affected versions blocking flags as a
> first important step is the way to go and having/discussing a more complete
> solution as a second step. 

I was thinking the same thing yesterday. Initially, we need a simple fix that is easily tested and distributed. 

It looks like OPAC users can only modify their own user, so the only field that should really be immediately dangerous is "flags".
Comment 49 Marcel de Rooy 2021-09-02 05:58:37 UTC
(In reply to Kyle M Hall from comment #46)
> Created attachment 124406 [details] [review] [review]
> Bug 28929: Prevent flags to be sent during patron's edition
> 
> * selfreg and selfmod for OPAC
> * patron's edition on staff
> 
> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
> Tested OPAC and staff side. Prevents mangling flags column.
> 
> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
> 
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Three members of the QA team already signed off on this one!
Kyle, is your signoff a QA stamp ?
Comment 50 David Cook 2021-09-02 07:17:35 UTC
(In reply to Marcel de Rooy from comment #49)
> Three members of the QA team already signed off on this one!
> Kyle, is your signoff a QA stamp ?

I was wondering this too.

The QA tools returned OK.

I tried running the full unit test suite but got a lot of failures (my koha-testing-docker is probably outdated). They'll be unrelated to this change though. 

I'd be happy to add my QA stamp if needed.
Comment 51 Katrin Fischer 2021-09-02 07:19:14 UTC
We've implementd and tested this fix for 18.11 yesterday - so I probably should not be signing off. But can confirm that it works :)

I'll take the chance to set to PQA.
Comment 52 Jonathan Druart 2021-09-02 07:31:50 UTC
Please continue the work on bug 28935. I think it must be included with this one in (at least) the last two stables.
Comment 53 David Cook 2021-09-02 23:31:47 UTC
(Are we missing any other entry points? Is it possible to modify users using a kiosk via SIP? It doesn't look like it at a glance.)
Comment 54 Victor Grousset/tuxayo 2021-09-03 04:09:57 UTC
Is the screenshot taking still useful in the test?
↓↓↓
    $driver->capture_screenshot('selenium_failure_x.png');
Comment 55 Marcel de Rooy 2021-09-03 06:16:51 UTC
(In reply to Victor Grousset/tuxayo from comment #54)
> Is the screenshot taking still useful in the test?
> ↓↓↓
>     $driver->capture_screenshot('selenium_failure_x.png');

Probably not ;)
Comment 56 Victor Grousset/tuxayo 2021-09-04 00:07:55 UTC
Ok, I can add a patch to remove that unless there is an objection.

Also, I missed this on the first review:

    $driver->get($opac_base_url . 'opac-main.pl');

    $driver->get($opac_base_url . 'opac-memberentry.pl');

Is going to the homepage useful before going directly via URL to the self registration form?


Otherwise, code look good and seems stable after a few hundred runs trial.
Comment 57 Victor Grousset/tuxayo 2021-09-04 03:16:52 UTC
Tested on 20.05.x:
- reproduced self registration attack and the patch prevents it
- reproduced self modification attack and the patch prevents it
- self registration with all the fields filled: no data loss
- self modification with all the fields filled: no data loss

What else should be tested?
Comment 58 Jonathan Druart 2021-09-09 08:06:31 UTC
Created attachment 124683 [details] [review]
Bug 28929: Add selenium tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 28929: (follow-up) Add exec flag to tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 59 Jonathan Druart 2021-09-09 08:06:37 UTC
Created attachment 124684 [details] [review]
Bug 28929: Prevent flags to be sent during patron's edition

* selfreg and selfmod for OPAC
* patron's edition on staff

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested OPAC and staff side. Prevents mangling flags column.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 60 Jonathan Druart 2021-09-09 08:08:35 UTC
Created attachment 124685 [details] [review]
Bug 28929: Add selenium tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 28929: (follow-up) Add exec flag to tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 61 Jonathan Druart 2021-09-09 08:10:30 UTC
First and third patches squashed.

(In reply to Victor Grousset/tuxayo from comment #54)
> Is the screenshot taking still useful in the test?
> ↓↓↓
>     $driver->capture_screenshot('selenium_failure_x.png');

Indeed, was for debugging, removed.

(In reply to Victor Grousset/tuxayo from comment #56)
> Ok, I can add a patch to remove that unless there is an objection.
> 
> Also, I missed this on the first review:
> 
>     $driver->get($opac_base_url . 'opac-main.pl');
> 
>     $driver->get($opac_base_url . 'opac-memberentry.pl');
> 
> Is going to the homepage useful before going directly via URL to the self
> registration form?

Indeed, I initially wanted to click the "Register here" from opac-main but it didn't work (?). I removed the first get.
Comment 62 Victor Grousset/tuxayo 2021-09-16 07:35:53 UTC
Backported: Pushed to 20.05.x security branch for 20.05.16
Comment 63 Jonathan Druart 2021-09-30 07:22:16 UTC
Backported to 21.05.04, 20.11.10, 20.05.16, 19.11.22
Comment 64 Jonathan Druart 2021-09-30 07:51:56 UTC
Pushed to master for 21.11, thanks to everybody involved!