Bug 39866

Summary: Acquisitions statistics fails when filling only the To date
Product: Koha Reporter: Adolfo Rodríguez Taboada <adolfo.rodriguez>
Component: ReportsAssignee: Adolfo Rodríguez Taboada <adolfo.rodriguez>
Status: Pushed to stable --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: david, fridolin.somers, jonathan.druart, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Trivial patch Documentation contact:
Documentation submission: Text to go in the release notes:
This fixes an internal server error when using the acquisitions statistics wizard report. The error was generated when only the "To" date was filled in for either the "Placed on" and "Received on" options.
Version(s) released in:
25.11.00,25.05.01
Circulation function:
Attachments: Bug 39866: Acquisitions statistics fails when filling only the To date
Bug 39866: Acquisitions statistics fails when filling only the To date
Bug 39866: Acquisitions statistics fails when filling only the To date
Bug 39866: (QA follow-up) Rudimentary validation of line/column

Description Adolfo Rodríguez Taboada 2025-05-09 08:58:47 UTC
The Acquisitions statistics returns an Internal Server Error if you fill only the To date. The error happens with both "Placed on" and "Received on".
When executing the query, the script checks that the To date is filled but it doesn't check if the From date is also filled.

if ( (@linefilter) and ( $linefilter[1] ) ) {
    $sth->execute( $linefilter[0], $linefilter[1] );

However, it checks both when building the SQL

    if (@linefilter) {
        if ( $linefilter[1] ) {
            if ( $linefilter[0] ) {
                $strsth .= " AND $line BETWEEN ? AND ? ";
            } else {
                $strsth .= " AND $line <= ? ";
            }
        } elsif (
            ( $linefilter[0] )
            and (  ( $line =~ /closedate/ )
                or ( $line =~ /received/ ) )
            )
        {
            $strsth .= " AND $line >= ? ";
        } elsif ( $linefilter[0] ) {
            $linefilter[0] =~ s/\*/%/g;
            $strsth .= " AND $line LIKE ? ";
        }
    }

This discrepancy is what creates the error, as it passes the SQL more variables than it expects.
Comment 1 Adolfo Rodríguez Taboada 2025-05-09 09:01:34 UTC
Created attachment 182160 [details] [review]
Bug 39866: Acquisitions statistics fails when filling only the To date

This patch fixes the exection of the report Acquisitions statistics if you fill only the To date
Test plan:
1 Execute the report Acquisitions statistics filling only To date filter in Placed on or Received on (/cgi-bin/koha/reports/acquisitions_stats.pl) Koha returns an Internal Server Error
2 Apply patch, restart services
3 Repeat step 1, now the report returns the results page as expected
Comment 2 Roman Dolny 2025-05-09 20:29:01 UTC
Created attachment 182227 [details] [review]
Bug 39866: Acquisitions statistics fails when filling only the To date

This patch fixes the exection of the report Acquisitions statistics if you fill only the To date
Test plan:
1 Execute the report Acquisitions statistics filling only To date filter in Placed on or Received on (/cgi-bin/koha/reports/acquisitions_stats.pl) Koha returns an Internal Server Error
2 Apply patch, restart services
3 Repeat step 1, now the report returns the results page as expected

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 3 Jonathan Druart 2025-05-14 14:12:39 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2025-05-30 07:59:35 UTC
Looking here
Comment 5 Marcel de Rooy 2025-05-30 09:48:23 UTC
Created attachment 182854 [details] [review]
Bug 39866: Acquisitions statistics fails when filling only the To date

This patch fixes the exection of the report Acquisitions statistics if you fill only the To date
Test plan:
1 Execute the report Acquisitions statistics filling only To date filter in Placed on or Received on (/cgi-bin/koha/reports/acquisitions_stats.pl) Koha returns an Internal Server Error
2 Apply patch, restart services
3 Repeat step 1, now the report returns the results page as expected

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Removed superfluous COALESCEs.
Comment 6 Marcel de Rooy 2025-05-30 09:48:25 UTC
Created attachment 182855 [details] [review]
Bug 39866: (QA follow-up) Rudimentary validation of line/column

Prevent SQL injection.

Note: output_error in Output.pm needs further attention, but out
of scope here. (The error passed gets replaced by 404 here too.)

Test plan:
Pass something else than table.field in Line or Column URL param.
(Manipulate URL.)
You should be redirected to 404 error instead of 500.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2025-05-30 09:48:59 UTC
QA Comment:
Thx for patching this! This script is not optimal indeed.

my $line           = $input->param("Line");
WHERE $line IS NOT NULL
This script has issues with parameter validation all together. We can post stuff to it without using the template. And it does not expect that.
We should be sure that this is not some 'nice' SQL injection thing. Same for $column btw.
Prevent 500s like
CGI::Compile::ROOT::usr_share_koha_reports_acquisitions_stats_2epl::calculate(): DBI Exception: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ';
        FROM aqorders
          LEFT JOIN aqbasket ON (aqorders.basketno = ...' at line 1 at /usr/share/koha/reports/acquisitions_stats.pl line 70
=> Adding a simple bailout in follow-up. (Did not add checking the actual field in the regex here.)

-        WHERE $line IS NOT NULL AND $line <> '' ";
+        WHERE $line IS NOT NULL AND COALESCE($line,'') <> '' ";
Same for $column
$column is a database field now. If it is NULL the first part is false. If it is not NULL, the second part is the same as $line <> "". No need to change.
Comment 8 Lucas Gass (lukeg) 2025-06-02 21:27:46 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 9 Fridolin Somers 2025-06-19 07:44:29 UTC
I see in 25.05.x
Comment 10 Fridolin Somers 2025-06-19 07:46:02 UTC
Does not apply easy on 24.11.x
Please provide a rebased patch if needed.