Bug 42025 - Error handling in tools/upload is broken
Summary: Error handling in tools/upload is broken
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 13618
Blocks:
  Show dependency treegraph
 
Reported: 2026-03-09 09:55 UTC by Jonathan Druart
Modified: 2026-03-09 09:56 UTC (History)
2 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Crowdfunding committed: 0
Crowdfunding contact:
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2026-03-09 09:55:30 UTC
On the following commit we add the html filter to msg:

  commit dcd1f5d48c758aee17b6c6f069c6146b42efe117
  Date:   Fri Jan 23 13:18:54 2015 +0100
  Bug 13618: Add html filters to all the variables

-                ShowAlerts( [% msg %] );
+                ShowAlerts( [% msg | html %] );

msg is supposed to be a JSON encoded string:

tools/upload.pl
118     my $msg =
119           $delete ? JSON::to_json( { $fn => { code => ALERT_DELETED } } )
120         : $id     ? JSON::to_json( { $fn || $id, { code => ERR_NOT_DELETED } } )
121         :           '';

So we could use To.json in the template and remove the to_json from the controller.

BUT...
There are a couple places in the controller where we send back what we send:
38 my $msg            = $input->param('msg');
76         msg            => $msg,

Which would lead to XSS if we remove the filtering.

This script receives msg in parameter when sent from SubmitMe
462         function SubmitMe(op, id, msg ) {
465             $("#submitter #msg").val( msg );

coming from:
398         function cbUpload( status, fileid, err ) {
399             $('#fileToUpload').prop('disabled', false);
400             if( status=='done' ) {
401                 var e = err? JSON.stringify(err): '';
402                 SubmitMe( 'search', fileid, e );

Coming from
364         function StartUpload() {
387             xhr= AjaxUpload( $('#fileToUpload'), $('#fileuploadprogress'), xtra, cbUpload );

Which does...:
koha-tmpl/intranet-tmpl/prog/js/file-upload.js
 1 function AjaxUpload(input, progressbar, xtra, callback) {

10     var url = "/cgi-bin/koha/tools/upload-file.pl?" + xtra;

19     xhr.onload = function (e) {
20         var data = JSON.parse(xhr.responseText);
21         if (data.status == "done") {
22             progressbar.val(100);
23             progressbar.next(".fileuploadpercent").text("100");
24         }
25         callback(data.status, data.fileid, data.errors);
26     };

tools/upload-file.pl
 55     send_reply( 'failed', undef, $upload ? $upload->err : undef );

 63 sub send_reply {    # response will be sent back as JSON
 64     my ( $upload_status, $data, $error ) = @_;
 65     my $reply = CGI->new("");
 66     print $reply->header( -type => 'text/html', -charset => 'UTF-8' );
 67     print JSON::encode_json(
 68         {
 69             status => $upload_status,
 70             fileid => $data,
 71             errors => $error,
 72         }
 73     );

Koha/Uploader.pm
154 sub err {
155     my ($self) = @_;                                                                                  
156     my $err;
157     foreach my $f ( keys %{ $self->{files} } ) {                               
158         my $e = $self->{files}->{$f}->{errcode};                                                      
159         $err->{$f}->{code} = $e if $e;                                  
160     }
161     return $err;
162 }

All this is VERY messy...