[Bug 43196] New: PUT requests with a bad request body to biblios endpoint blanks a record
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 Bug ID: 43196 Summary: PUT requests with a bad request body to biblios endpoint blanks a record Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: lucas@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: tomascohen@gmail.com Target Milestone: --- TO recreate: 1. Have a record in Koha with good MARC data. In KTD I'm using biblionumber=32 as an example. 2. Do a PUT requested with invalid JSON in it. I did this: curl -u koha:koha -X PUT http://localhost:8081/api/v1/biblios/32 -H "Content-Type: application/marc-in-json" -H "Accept: application/json" -d '{"leader":"00000nam a2200000 a 4500","fields":[{"245":{"ind1":"1","ind2":"0","subfields":[{"a":"STOP HERE ON PURPOSE!!!' 3. The response is 200, success. 4. Now go look at the record in Koha. It is contains nothing in the MARC record now. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |major Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- URL| |https://github.com/perl4lib | |/marc-perl/issues/27 Status|NEW |ASSIGNED --- Comment #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- I filed an issue on `marc-perl` because the `MiJ` implementation accepts being passed `undef` (i.e. the result from `$c->req->json` on invalid JSON data. I will implement defensive code to avoid the issue. By design, `MARC::Record` will always return a `MARC::Record` object, so we need to check for empty fields also for `USMARC` and `MARCXML`. The latter explodes on bad XML, but the controller catches it and throws an `unhandled exception` with error 500. This needs to be addressed here too. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203350 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203350&action=edit Bug 43196: Regression tests for PUT /biblios with malformed bodies MARC::Record::new_from_mij_structure silently returns an empty record when passed undef or an invalid structure. The PUT /biblios endpoint does not validate the parsed record before calling ModBiblio, resulting in a 200 response for clearly invalid input. This patch adds regression tests for all three supported content types: - application/marc-in-json: truncated JSON, invalid structure, empty fields - application/marcxml+xml: truncated XML, empty record - application/marc: garbage input All should return 400 but currently return 200. Note: MARC::Record::MiJ upstream should arguably croak on undef/invalid input rather than returning a blank record. Test plan: 1. Apply patch 2. Run: $ prove t/db_dependent/api/v1/biblios.t => FAIL: put() malformed body tests fail (200 instead of 400) 3. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 --- Comment #3 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203351 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203351&action=edit Bug 43196: Reject malformed MARC records in PUT /biblios MARC::Record parsers silently return empty records on invalid input: - new_from_mij_structure(undef) returns a blank record (MiJ) - new_from_usmarc(garbage) returns a blank record with warnings - new_from_xml(valid-but-empty) returns a blank record The PUT /biblios endpoint passed these blank records to ModBiblio, which overwrote existing bibliographic data with an empty record (only system-generated 005 and 999 fields survive). Wrap each parser call in try/catch (for MARCXML which dies on malformed XML) and validate the resulting record has at least one field before proceeding. Return 400 if parsing fails or produces an empty record. Note: MARC::Record::MiJ upstream has been patched to croak on undef/invalid input (perl4lib/marc-perl#28), but Koha needs this defensive check regardless for USMARC and empty-but-valid cases. Test plan: 1. Apply the regression tests from the previous commit 2. Run: $ prove t/db_dependent/api/v1/biblios.t => FAIL: put() malformed body tests fail 3. Apply this patch 4. Repeat step 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com | |, nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203350|0 |1 is obsolete| | Attachment #203351|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 --- Comment #4 from David Nind <david@davidnind.com> --- Created attachment 203354 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203354&action=edit Bug 43196: Regression tests for PUT /biblios with malformed bodies MARC::Record::new_from_mij_structure silently returns an empty record when passed undef or an invalid structure. The PUT /biblios endpoint does not validate the parsed record before calling ModBiblio, resulting in a 200 response for clearly invalid input. This patch adds regression tests for all three supported content types: - application/marc-in-json: truncated JSON, invalid structure, empty fields - application/marcxml+xml: truncated XML, empty record - application/marc: garbage input All should return 400 but currently return 200. Note: MARC::Record::MiJ upstream should arguably croak on undef/invalid input rather than returning a blank record. Test plan: 1. Apply patch 2. Run: $ prove t/db_dependent/api/v1/biblios.t => FAIL: put() malformed body tests fail (200 instead of 400) 3. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 203355 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203355&action=edit Bug 43196: Reject malformed MARC records in PUT /biblios MARC::Record parsers silently return empty records on invalid input: - new_from_mij_structure(undef) returns a blank record (MiJ) - new_from_usmarc(garbage) returns a blank record with warnings - new_from_xml(valid-but-empty) returns a blank record The PUT /biblios endpoint passed these blank records to ModBiblio, which overwrote existing bibliographic data with an empty record (only system-generated 005 and 999 fields survive). Wrap each parser call in try/catch (for MARCXML which dies on malformed XML) and validate the resulting record has at least one field before proceeding. Return 400 if parsing fails or produces an empty record. Note: MARC::Record::MiJ upstream has been patched to croak on undef/invalid input (perl4lib/marc-perl#28), but Koha needs this defensive check regardless for USMARC and empty-but-valid cases. Test plan: 1. Apply the regression tests from the previous commit 2. Run: $ prove t/db_dependent/api/v1/biblios.t => FAIL: put() malformed body tests fail 3. Apply this patch 4. Repeat step 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |emily.lamancusa@montgomeryc |y.org |ountymd.gov CC| |lisette@bywatersolutions.co | |m -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203354|0 |1 is obsolete| | Attachment #203355|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 --- Comment #6 from Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 207308 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207308&action=edit Bug 43196: Regression tests for PUT /biblios with malformed bodies MARC::Record::new_from_mij_structure silently returns an empty record when passed undef or an invalid structure. The PUT /biblios endpoint does not validate the parsed record before calling ModBiblio, resulting in a 200 response for clearly invalid input. This patch adds regression tests for all three supported content types: - application/marc-in-json: truncated JSON, invalid structure, empty fields - application/marcxml+xml: truncated XML, empty record - application/marc: garbage input All should return 400 but currently return 200. Note: MARC::Record::MiJ upstream should arguably croak on undef/invalid input rather than returning a blank record. Test plan: 1. Apply patch 2. Run: $ prove t/db_dependent/api/v1/biblios.t => FAIL: put() malformed body tests fail (200 instead of 400) 3. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 --- Comment #7 from Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 207309 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207309&action=edit Bug 43196: Reject malformed MARC records in PUT /biblios MARC::Record parsers silently return empty records on invalid input: - new_from_mij_structure(undef) returns a blank record (MiJ) - new_from_usmarc(garbage) returns a blank record with warnings - new_from_xml(valid-but-empty) returns a blank record The PUT /biblios endpoint passed these blank records to ModBiblio, which overwrote existing bibliographic data with an empty record (only system-generated 005 and 999 fields survive). Wrap each parser call in try/catch (for MARCXML which dies on malformed XML) and validate the resulting record has at least one field before proceeding. Return 400 if parsing fails or produces an empty record. Note: MARC::Record::MiJ upstream has been patched to croak on undef/invalid input (perl4lib/marc-perl#28), but Koha needs this defensive check regardless for USMARC and empty-but-valid cases. Test plan: 1. Apply the regression tests from the previous commit 2. Run: $ prove t/db_dependent/api/v1/biblios.t => FAIL: put() malformed body tests fail 3. Apply this patch 4. Repeat step 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |26.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 --- Comment #8 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Thanks everyone! Pushed to main for 26.11! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable Version(s)|26.11.00 |26.11.00,26.05.05 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43196 --- Comment #9 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to 26.05.x for the upcoming 26.05.05 release. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org