tbar0970 / jethro-pmm

Jethro Pastoral Ministry Manager
GNU General Public License v3.0
35 stars 25 forks source link

2.35-RC3: CSRF code causes stacktrace when adding a family #1022

Closed jefft closed 4 months ago

jefft commented 4 months ago

Using 2.35-RC3, if I go Families -> Add, the family+person is added but an error is logged:

image

AH01071: Got error 'PHP message: Undefined index: person_form_token - Line 934 of /home/jethro/code/2.35.0-RC3/app/db_objects/person.class.phpPHP message: Trying to access array offset on value of type null - Line 934 of /home/jethro/code/2.35.0-RC3/app/db_objects/person.class.php', referer: https://jethro/?view=families__add

More seriously, if I: 1) add a Person to an existing Family 2) add a new Family, as above:

then I get An error occurred. Please contact your system administrator for help., and a stacktrace:

AH01071: Got error 'PHP message: Synchroniser token mismatch - person could not be saved - Line 935 of /home/jethro/code/2.35.0-RC3/app/db_objects/person.class.php', referer: https://jethro/?view=families__add
jefft commented 4 months ago

The problems were introduced in https://github.com/tbar0970/jethro-pmm/commit/811a3142a65af2a5d2405534535dd8869017e594 when the csrf token was moved from $_SESSION['person_form_token'] to $_SESSION['person_form_token'][$this->id]. When adding a Person, the Person#printForm() method is called, but when adding a Family (plus person), Family#printForm() is called, not Person#printForm(), and so the csrf token is never initialized.

I don't see why person_form_token would ever be different for different person IDs. When creating a new person, $this->id is always null anyway.

For that matter, why is this csrf code in Person#printForm(), and not in db_object#printForm()?

tbar0970 commented 4 months ago

Thanks for diagnosing.

I don't see why person_form_token would ever be different for different person IDs. When creating a new person, $this->id is always null anyway.

You might edit differernt persons in different tabs at the same time. This token was only meant to apply for editing existing persons.

For that matter, why is this csrf code in Person#printForm(), and not in db_object#printForm()?

Person record has mobile number, which now has security implications. I knew that different forms worked differently and didn't want to test/troubleshoot across the whole system. But I should have tested this one!