Bug 28941 - No filtering on suggestion at the OPAC
Summary: No filtering on suggestion at the OPAC
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Julian Maurice
URL:
Keywords:
Depends on:
Blocks: 29416 29696
  Show dependency treegraph
 
Reported: 2021-09-02 08:16 UTC by Jonathan Druart
Modified: 2022-12-12 21:23 UTC (History)
20 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.04, 20.11.10, 20.05.16, 19.11.22


Attachments
Bug 28941: Filter suggestion inputs at the OPAC (3.44 KB, patch)
2021-09-02 09:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28941: Filter suggestion inputs at the OPAC (3.54 KB, patch)
2021-09-02 13:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28941: Filter suggestion inputs at the OPAC (3.56 KB, patch)
2021-09-10 08:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 28941: Deal with OPACSuggestionUnwantedFields (906 bytes, patch)
2021-09-10 09:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28941: Deal with OPACSuggestionUnwantedFields (1002 bytes, patch)
2021-09-10 09:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28941: Deal with OPACSuggestionUnwantedFields (999 bytes, patch)
2021-09-10 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-09-02 08:16:28 UTC
46 my $suggestion      = $input->Vars;

181         &NewSuggestion($suggestion);

acceptedby, accepteddate, STATUS, etc...
Comment 1 Jonathan Druart 2021-09-02 08:59:40 UTC
Since...
  commit 8802bbf5fe6c0b35ad4d6e00293eb102a48b92b4
  Date:   Tue Aug 25 23:56:16 2009 +0200
  Editing C4::Suggestions.pm
Comment 2 Jonathan Druart 2021-09-02 09:53:49 UTC
Created attachment 124423 [details] [review]
Bug 28941: Filter suggestion inputs at the OPAC

The following sequence is bad:
46 my $suggestion      = $input->Vars;

181         &NewSuggestion($suggestion);

All columns can be set when we insert the suggestion into the DB
We definitely want to avoid the following fields to be set by the final
user: acceptedby, accepteddate, STATUS, etc...
Comment 3 Marcel de Rooy 2021-09-02 09:58:10 UTC
You didnt need the Allow list finally
Comment 4 Marcel de Rooy 2021-09-02 13:58:04 UTC
Created attachment 124429 [details] [review]
Bug 28941: Filter suggestion inputs at the OPAC

The following sequence is bad:
46 my $suggestion      = $input->Vars;

181         &NewSuggestion($suggestion);

All columns can be set when we insert the suggestion into the DB
We definitely want to avoid the following fields to be set by the final
user: acceptedby, accepteddate, STATUS, etc...

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Julian Maurice 2021-09-10 08:45:45 UTC
Created attachment 124729 [details] [review]
Bug 28941: Filter suggestion inputs at the OPAC

The following sequence is bad:
46 my $suggestion      = $input->Vars;

181         &NewSuggestion($suggestion);

All columns can be set when we insert the suggestion into the DB
We definitely want to avoid the following fields to be set by the final
user: acceptedby, accepteddate, STATUS, etc...

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 6 Julian Maurice 2021-09-10 08:48:19 UTC
IMO this is much better like this than with the allowlists implemented in the other bugs. It's a clean and simple solution.
Comment 7 Julian Maurice 2021-09-10 08:50:15 UTC
Related: OPACSuggestionUnwantedFields only hides HTML inputs. It is possible to manipulate the DOM to submit data for unwanted fields
Comment 8 Jonathan Druart 2021-09-10 09:15:38 UTC
(In reply to Julian Maurice from comment #7)
> Related: OPACSuggestionUnwantedFields only hides HTML inputs. It is possible
> to manipulate the DOM to submit data for unwanted fields

Good catch.

And so this is where the AllowList module could be useful as we could have reused the "additional deny list" method :)
Comment 9 Marcel de Rooy 2021-09-10 09:17:40 UTC
(In reply to Jonathan Druart from comment #8)
> (In reply to Julian Maurice from comment #7)
> > Related: OPACSuggestionUnwantedFields only hides HTML inputs. It is possible
> > to manipulate the DOM to submit data for unwanted fields
> 
> Good catch.
> 
> And so this is where the AllowList module could be useful as we could have
> reused the "additional deny list" method :)

$allowlist->remove( unwantedfields );
Comment 10 Jonathan Druart 2021-09-10 09:33:49 UTC
Created attachment 124745 [details] [review]
Bug 28941: Deal with OPACSuggestionUnwantedFields

Remove fields from OPACSuggestionUnwantedFields before creating the
suggestion
Comment 11 Jonathan Druart 2021-09-10 09:34:13 UTC
Marcel or Julian, can I get another stamp on this follow-up please?
Comment 12 Marcel de Rooy 2021-09-10 09:35:50 UTC
(In reply to Jonathan Druart from comment #11)
> Marcel or Julian, can I get another stamp on this follow-up please?

Sure
Comment 13 Marcel de Rooy 2021-09-10 09:47:31 UTC
Created attachment 124747 [details] [review]
Bug 28941: Deal with OPACSuggestionUnwantedFields

Remove fields from OPACSuggestionUnwantedFields before creating the
suggestion

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Julian Maurice 2021-09-10 10:12:58 UTC
Comment on attachment 124747 [details] [review]
Bug 28941: Deal with OPACSuggestionUnwantedFields

Review of attachment 124747 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-suggestions.pl
@@ +177,4 @@
>          }
>      }
>      else {
> +        for my $f (sort split(/\s*\,\s*/, C4::Context->preference("OPACSuggestionUnwantedFields") ) ) {

why `sort` ?
Comment 15 Jonathan Druart 2021-09-10 10:34:53 UTC
(In reply to Julian Maurice from comment #14)
> why `sort` ?

Because I am lazy and copy/pasted the existing statement.
Comment 16 Jonathan Druart 2021-09-10 15:35:16 UTC
Created attachment 124775 [details] [review]
Bug 28941: Deal with OPACSuggestionUnwantedFields

Remove fields from OPACSuggestionUnwantedFields before creating the
suggestion

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

JD amended patch: remove useless sort
Comment 17 Victor Grousset/tuxayo 2021-09-17 00:32:27 UTC
Backported: Pushed to 20.05.x security branch for 20.05.16
Comment 18 Victor Grousset/tuxayo 2021-09-17 00:33:53 UTC
== testing notes ==
1. log into the OPAC and go to the suggestion form
2. add the following HTML in the form
<input type="text" name="STATUS" value="ACCEPTED">
<input type="text" name="reason" value="we will buy it">
3. submit
4. confirm the in the staff interface that it's indeed accepted
5. apply patch
6. attack doesn't work anymore
Comment 19 Jonathan Druart 2021-09-30 07:22:28 UTC
Backported to 21.05.04, 20.11.10, 20.05.16, 19.11.22
Comment 20 Jonathan Druart 2021-09-30 07:52:11 UTC
Pushed to master for 21.11, thanks to everybody involved!