Bugzilla – Attachment 179312 Details for
Bug 37784
Patron password hash can be fetched using report dictionary
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 37784: Do not show forbidden columns in reports dictionary
Bug-37784-Do-not-show-forbidden-columns-in-reports.patch (text/plain), 7.41 KB, created by
Marcel de Rooy
on 2025-03-14 09:50:14 UTC
(
hide
)
Description:
Bug 37784: Do not show forbidden columns in reports dictionary
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2025-03-14 09:50:14 UTC
Size:
7.41 KB
patch
obsolete
>From 46e4489ce6075ea3b4351e4df9c9c9c01b034c93 Mon Sep 17 00:00:00 2001 >From: Aleisha Amohia <aleishaamohia@hotmail.com> >Date: Fri, 30 Aug 2024 02:04:48 +0000 >Subject: [PATCH] Bug 37784: Do not show forbidden columns in reports > dictionary >Content-Type: text/plain; charset=utf-8 > >This patch prevents forbidden columns from being selected using the reports dictionary. > >To test: > >1. Go to Reports -> View dictionary >2. Choose 'new dictionary' >3. After providing a name and description, select the 'patrons' table >4. Select the 'password' column and go Next >5. Notice it is possible to see a list of all the password hashes in the 'choose' selection box when asked to specify a value > >6. Apply patch and restart services > >7. Repeat steps 1-3 >8. Notice the 'password' column and other forbidden columns are not available to be selected > >9. Open the Inspector / developer tools in your browser and find the code for the columns selection >10. Change the value of one of the options in the code to "borrowers.password". So it may end up looking like this: > ><option value="borrowers.password">Card number</option> > >11. Choose that option from the dropdown and click Next. >12. Confirm you are redirected back to this page and warned about the column being used. > >13. Ensure tests pass t/db_dependent/Reports/Guided.t > >Sponsored-by: Reserve Bank of New Zealand >Signed-off-by: David Cook <dcook@prosentient.com.au> > >Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >--- > C4/Reports/Guided.pm | 12 ++-- > .../prog/en/modules/reports/dictionary.tt | 10 +++ > reports/dictionary.pl | 64 +++++++++++-------- > t/db_dependent/Reports/Guided.t | 8 ++- > 4 files changed, 62 insertions(+), 32 deletions(-) > >diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm >index f5f65d9639..cdda357bd9 100644 >--- a/C4/Reports/Guided.pm >+++ b/C4/Reports/Guided.pm >@@ -219,11 +219,13 @@ sub _get_columns { > return [ > map { > my $column_name = $_->{Field}; >- { >- name => sprintf( "%s.%s", $table_name, $column_name ), >- description => $all_columns->{$table_name}->{$column_name}, >- }; >- >+ my $forbidden = Koha::Report->new->check_columns( undef, [$column_name] ); >+ $forbidden ? () : ( >+ { >+ name => sprintf( "%s.%s", $table_name, $column_name ), >+ description => $all_columns->{$table_name}->{$column_name}, >+ } >+ ); > } @$columns > ]; > } >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/dictionary.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/dictionary.tt >index d30ade2126..7cdfaf7e66 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/dictionary.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/dictionary.tt >@@ -150,6 +150,16 @@ > <h3>Add new definition</h3> > <form action="/cgi-bin/koha/reports/dictionary.pl" method="post"> > [% INCLUDE 'csrf-token.inc' %] >+ >+ [% IF ( passworderr ) %] >+ <div class="alert alert-warning"> >+ <strong>The following error was encountered:</strong><br /> >+ The previous column selection included a password field.<br /> >+ The column cannot be used due to security risks.<br /> >+ Please make a new selection and ensure no password columns have been selected. >+ </div> >+ [% END %] >+ > <fieldset class="rows" > ><legend>Step 1 of 5: Name the new definition</legend > ><ol> >diff --git a/reports/dictionary.pl b/reports/dictionary.pl >index 1446c0718a..86bbf1e5fd 100755 >--- a/reports/dictionary.pl >+++ b/reports/dictionary.pl >@@ -79,38 +79,50 @@ if ( $op eq 'add_form' ) { > # Choosing the values > my @columns = $input->multi_param('columns'); > my $columnstring = join( ',', @columns ); >+ my $forbidden = Koha::Report->new->check_columns( undef, \@columns ); > my @column_loop; > foreach my $column (@columns) { >- my %tmp_hash; >- $tmp_hash{'name'} = $column; >- my $type = get_column_type($column); >- if ( $type eq 'distinct' ) { >- my $values = get_distinct_values($column); >- $tmp_hash{'values'} = $values; >- $tmp_hash{'distinct'} = 1; >+ unless ($forbidden) { >+ my %tmp_hash; >+ $tmp_hash{'name'} = $column; >+ my $type = get_column_type($column); >+ if ( $type eq 'distinct' ) { >+ my $values = get_distinct_values($column); >+ $tmp_hash{'values'} = $values; >+ $tmp_hash{'distinct'} = 1; > >+ } >+ if ( $type eq 'DATE' || $type eq 'DATETIME' ) { >+ $tmp_hash{'date'} = 1; >+ } >+ if ( $type eq 'TEXT' || $type eq 'MEDIUMTEXT' ) { >+ $tmp_hash{'text'} = 1; >+ } >+ push @column_loop, \%tmp_hash; > } >- if ( $type eq 'DATE' || $type eq 'DATETIME' ) { >- $tmp_hash{'date'} = 1; >- } >- if ( $type eq 'TEXT' || $type eq 'MEDIUMTEXT' ) { >- $tmp_hash{'text'} = 1; >- } >- >- # else { >- # warn $type;# >- # } >- push @column_loop, \%tmp_hash; > } > >- $template->param( >- 'step_4' => 1, >- 'area' => $area, >- 'definition_name' => $definition_name, >- 'definition_description' => $definition_description, >- 'columns' => \@column_loop, >- 'columnstring' => $columnstring, >- ); >+ if (@column_loop) { >+ $template->param( >+ 'step_4' => 1, >+ 'area' => $area, >+ 'definition_name' => $definition_name, >+ 'definition_description' => $definition_description, >+ 'columns' => \@column_loop, >+ 'columnstring' => $columnstring, >+ ); >+ } else { >+ my $columns = get_columns( $area, $input ); >+ $template->param( >+ 'step_3' => 1, >+ 'area' => $area, >+ 'definition_name' => $definition_name, >+ 'definition_description' => $definition_description, >+ 'columns' => \@column_loop, >+ 'columnstring' => $columnstring, >+ 'passworderr' => 1, >+ ); >+ } > > } elsif ( $op eq 'cud-add_form_5' ) { > >diff --git a/t/db_dependent/Reports/Guided.t b/t/db_dependent/Reports/Guided.t >index 15059edcc9..fdb0213dd6 100755 >--- a/t/db_dependent/Reports/Guided.t >+++ b/t/db_dependent/Reports/Guided.t >@@ -617,7 +617,7 @@ subtest 'Returning passwords tests' => sub { > }; > > subtest 'get_columns' => sub { >- plan tests => 7; >+ plan tests => 8; > $schema->storage->txn_begin; > my $area = 'foo'; > my $success = eval { C4::Reports::Guided::get_columns($area) }; >@@ -631,6 +631,12 @@ subtest 'get_columns' => sub { > ok scalar grep { $_->{description} =~ m{Biblio number} } @{ $columns->{biblio} }; > ok !scalar grep { $_->{name} eq 'not_a_column' } @{ $columns->{biblio} }; > >+ $columns = C4::Reports::Guided::get_columns('PAT'); >+ ok( >+ !exists $columns->{password}, >+ "Password is a forbidden field and should not be returned" >+ ); >+ > $schema->storage->txn_rollback; > }; > >-- >2.39.5
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 37784
:
170898
|
171041
|
174817
|
177824
|
179297
| 179312 |
179313
|
179314
|
179567
|
179568
|
179569