Summary: | Koha::Object->store calls column names as methods, relying on AUTOLOAD, with possibly surprising results | ||
---|---|---|---|
Product: | Koha | Reporter: | David Gustafsson <glasklas> |
Component: | Architecture, internals, and plumbing | Assignee: | David Gustafsson <glasklas> |
Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | major | ||
Priority: | P2 | CC: | 1joynelson, hayleypelham, lucas, m.de.rooy |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Small patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: |
20.05.00, 19.11.05, 19.05.10
|
Circulation function: | |
Bug Depends on: | |||
Bug Blocks: | 14957 | ||
Attachments: |
Bug 24788: Remove autoloaded column accessors in Koha::Object->store
Bug 24788: Remove autoloaded column accessors in Koha::Object->store Bug 24788: Remove autoloaded column accessors in Koha::Object->store Bug 24788: Remove autoloaded column accessors in Koha::Object->store |
Description
David Gustafsson
2020-03-03 13:28:53 UTC
Created attachment 100007 [details] [review] Bug 24788: Remove autoloaded column accessors in Koha::Object->store Columns are accessed as methods, relying on AUTOLOAD, in Koha::Object->store. This has security implications and could also be a source of strange bugs. To test: 1) Apply patches for Bug 14957 2) Follow the testing instructions, when saving a new marc rule and error is thrown. 3) Apply patch 4) Try saving a new rule once again, this should now work Created attachment 100064 [details] [review] Bug 24788: Remove autoloaded column accessors in Koha::Object->store Columns are accessed as methods, relying on AUTOLOAD, in Koha::Object->store. This has security implications and could also be a source of strange bugs. To test: 1) Apply patches for Bug 14957 2) Follow the testing instructions, when saving a new marc rule and error is thrown. 3) Apply patch 4) Try saving a new rule once again, this should now work Signed-off-by: David Nind <david@davidnind.com> (In reply to David Gustafsson from comment #0) > Koha::Object->store accesse sDBIx::Class::Row columns as $self->$col, using > AUTOLOAD, which in effect makes column names and methods share the same > namespace. I'm no security expert, but I think this should be considered bad > practice and could be an attack vector. It also has huge potential for > causing strange bugs, which happened in Bug 14957 where a column is named > "delete", calling the "delete" method instead of fetching the column value > as intended. Here is patch to fix this using a separate namespace for > setting/getting columns through get_column/set_column instead. In this case it comes from: my $columns_info = $self->_result->result_source->columns_info; just a few lines earlier. So I guess the security argument does not really apply here. And the "huge potential for bugs" is a bit overkill. But the change looks good to me :) QAing Looking at Object.t now Created attachment 100228 [details] [review] Bug 24788: Remove autoloaded column accessors in Koha::Object->store Columns are accessed as methods, relying on AUTOLOAD, in Koha::Object->store. This has security implications and could also be a source of strange bugs. To test: 1) Apply patches for Bug 14957 2) Follow the testing instructions, when saving a new marc rule and error is thrown. 3) Apply patch 4) Try saving a new rule once again, this should now work Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Yes you are correct, I was incorrect about being security issues since only existing columns are looped over. I for some reason got it in my head that it was the columns passed to the store method. I will remove this remark from the commit message. Created attachment 100242 [details] [review] Bug 24788: Remove autoloaded column accessors in Koha::Object->store Columns are accessed as methods, relying on AUTOLOAD, in Koha::Object->store. This could be a source of strange bugs if a column name clashes with an existing method name. To test: 1) Apply patches for Bug 14957 2) Follow the testing instructions, when saving a new marc rule and error is thrown. 3) Apply patch 4) Try saving a new rule once again, this should now work Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Nice work everyone! Pushed to master for 20.05 backported to 19.11.x branch for 19.11.05 backported to 19.05.x for 19.05.10 Depends on enhancement, not backporting to 18.11.x series |