From bbb7f406d2ec6ddf4a78bf923842ac99ec8b1326 Mon Sep 17 00:00:00 2001
From: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Date: Wed, 5 Aug 2026 13:07:57 +0100
Subject: [PATCH] Bug 30144: (QA follow-up) Fix stored XSS in servicing
 instructions

The RM's review found two stored XSS vectors in the servicing
instruction free-text (LVT) handling:

1. neworderempty.tt embedded the servicing instruction JSON (and the
   EDIFACT_SI authorised values list) directly into an inline <script>
   block via JSON.parse('[% ... | $raw %]'). TT's html filter does not
   escape single quotes, so a free-text value like
   test' + alert(1) + ' broke out of the JS string literal and
   executed on the next page load of the edit form.

2. basket.pl built the "Servicing instruction" column display by
   concatenating raw LVC/LVT values with '<br>' and rendering the
   result with | $raw in basket.tt, so HTML/script content in a
   free-text value executed directly on the basket page.

Fix the first by moving the JSON into existing/new hidden input
value="" attributes (escaped with the html filter, which is safe for
double-quoted HTML attributes) and reading them back via jQuery's
.val() before JSON.parse(), instead of interpolating untrusted data
into a JS string literal.

Fix the second by passing the display parts to the template as a list
and escaping each with the html filter at render time, instead of
building a pre-escaped raw HTML string in Perl.

Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
---
 acqui/basket.pl                               |   6 +-
 acqui/neworderempty.pl                        |  48 +++---
 .../prog/en/modules/acqui/basket.tt           |   6 +-
 .../prog/en/modules/acqui/neworderempty.tt    | 146 ++++++++++--------
 4 files changed, 113 insertions(+), 93 deletions(-)

diff --git a/acqui/basket.pl b/acqui/basket.pl
index af35330acd..8f49543680 100755
--- a/acqui/basket.pl
+++ b/acqui/basket.pl
@@ -688,8 +688,9 @@ sub get_order_infos {
     }
 
     # Format servicing instruction for display
+    # Parts are passed to the template as plain text and HTML-escaped there ([% part | html %]),
+    # rather than being joined into a raw HTML string here (see bug 30144 security follow-up).
     if ( $line{servicing_instruction} && $edifact_si_map ) {
-        my $si_display = '';
         eval {
             my $groups = decode_json( $line{servicing_instruction} );
             if ( ref $groups eq 'ARRAY' && @$groups ) {
@@ -730,13 +731,12 @@ sub get_order_infos {
                         }
                     }
                 }
-                $si_display = join( '<br>', @display_parts ) if @display_parts;
+                $line{servicing_instruction_display_parts} = \@display_parts if @display_parts;
             }
         };
         if ($@) {
             Koha::Logger->get->warn("Failed to parse servicing_instruction JSON: $@");
         }
-        $line{servicing_instruction_display} = $si_display;
     }
 
     return \%line;
diff --git a/acqui/neworderempty.pl b/acqui/neworderempty.pl
index 5e7abb27b2..8db1e0a5a1 100755
--- a/acqui/neworderempty.pl
+++ b/acqui/neworderempty.pl
@@ -452,6 +452,10 @@ my @servicing_instruction_authorised_values = Koha::AuthorisedValues->search(
     { order_by => 'lib' }
 )->as_list;
 
+# Encode as JSON for safe embedding in a hidden form field (avoids raw interpolation into inline JS)
+my $servicing_instruction_avs_json = encode_json(
+    [ map { { value => $_->authorised_value, label => $_->lib } } @servicing_instruction_authorised_values ] );
+
 # Parse servicing instruction JSON into array of groups
 my $servicing_instruction_groups      = [];
 my $servicing_instruction_groups_json = '[]';
@@ -483,28 +487,28 @@ $template->param(
     closedate            => $basket->{'closedate'},
 
     # order details
-    suggestion                              => $suggestion,
-    biblionumber                            => $biblionumber,
-    uncertainprice                          => $data->{'uncertainprice'},
-    discount_2dp                            => sprintf( "%.2f", $bookseller->discount ),      # for display
-    discount                                => $bookseller->discount,
-    orderdiscount_2dp                       => sprintf( "%.2f", $data->{'discount'} || 0 ),
-    orderdiscount                           => $data->{'discount'},
-    order_internalnote                      => $data->{'order_internalnote'},
-    order_vendornote                        => $data->{'order_vendornote'},
-    servicing_instruction_groups            => $servicing_instruction_groups,
-    servicing_instruction_groups_json       => $servicing_instruction_groups_json,
-    servicing_instruction_authorised_values => \@servicing_instruction_authorised_values,
-    listincgst                              => $bookseller->listincgst,
-    invoiceincgst                           => $bookseller->invoiceincgst,
-    cur_active_sym                          => $active_currency->symbol,
-    cur_active                              => $active_currency->currency,
-    currencies                              => Koha::Acquisition::Currencies->search,
-    currency                                => $data->{currency},
-    vendor_currency                         => $bookseller->listprice,
-    orderexists                             => ( $new eq 'yes' ) ? 0 : 1,
-    title                                   => $data->{'title'},
-    author                                  => $data->{'author'},
+    suggestion                        => $suggestion,
+    biblionumber                      => $biblionumber,
+    uncertainprice                    => $data->{'uncertainprice'},
+    discount_2dp                      => sprintf( "%.2f", $bookseller->discount ),      # for display
+    discount                          => $bookseller->discount,
+    orderdiscount_2dp                 => sprintf( "%.2f", $data->{'discount'} || 0 ),
+    orderdiscount                     => $data->{'discount'},
+    order_internalnote                => $data->{'order_internalnote'},
+    order_vendornote                  => $data->{'order_vendornote'},
+    servicing_instruction_groups      => $servicing_instruction_groups,
+    servicing_instruction_groups_json => $servicing_instruction_groups_json,
+    servicing_instruction_avs_json    => $servicing_instruction_avs_json,
+    listincgst                        => $bookseller->listincgst,
+    invoiceincgst                     => $bookseller->invoiceincgst,
+    cur_active_sym                    => $active_currency->symbol,
+    cur_active                        => $active_currency->currency,
+    currencies                        => Koha::Acquisition::Currencies->search,
+    currency                          => $data->{currency},
+    vendor_currency                   => $bookseller->listprice,
+    orderexists                       => ( $new eq 'yes' ) ? 0 : 1,
+    title                             => $data->{'title'},
+    author                            => $data->{'author'},
     publicationyear  => $data->{'publicationyear'} ? $data->{'publicationyear'} : $data->{'copyrightdate'},
     editionstatement => $data->{'editionstatement'},
     budget_loop      => $budget_loop,
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
index 59bdfe4c29..d212532cd9 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
@@ -932,7 +932,11 @@
                                                     [% books_loo.invoice_object.invoicenumber | html %]
                                                 [% END %]
                                             </td>
-                                            <td> [% books_loo.servicing_instruction_display | $raw %] </td>
+                                            <td>
+                                                [% FOREACH part IN books_loo.servicing_instruction_display_parts %]
+                                                    [% part | html %][% UNLESS loop.last %]<br />[% END %]
+                                                [% END %]
+                                            </td>
                                             [% IF Koha.Preference('EDIFACT') && ediaccount %]
                                                 <td>[% books_loo.suppliers_report | html %]</td>
                                             [% ELSE %]
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt
index 5c6b018c24..95d5011fdc 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt
@@ -239,19 +239,25 @@
 
     // Servicing Instructions Management
     var servicingInstructionGroups = [];
-    var servicingInstructionAVs = [
-        [% FOREACH av IN servicing_instruction_authorised_values %]
-            { value: '[% av.authorised_value | html %]', label: '[% av.lib | html %]' },
-        [% END %]
-    ];
+    var servicingInstructionAVs = [];
 
     function initServicingInstructions() {
-        // Load existing data from template
+        // Load authorised values and existing data from hidden fields rather than
+        // interpolating them directly into inline JS, so free-text values can't break
+        // out of a JS string literal (see bug 30144 security follow-up).
         try {
-            var existingData = JSON.parse('[% servicing_instruction_groups_json | $raw %]');
+            var avsData = JSON.parse($("#servicing_instruction_avs_json").val() || "[]");
+            servicingInstructionAVs = Array.isArray(avsData) ? avsData : [];
+        } catch (e) {
+            console.error("Failed to parse servicing instruction authorised values:", e);
+            servicingInstructionAVs = [];
+        }
+
+        try {
+            var existingData = JSON.parse($("#servicing_instruction_groups_json").val() || "[]");
             servicingInstructionGroups = Array.isArray(existingData) ? existingData : [];
         } catch (e) {
-            console.error('Failed to parse servicing instruction groups:', e);
+            console.error("Failed to parse servicing instruction groups:", e);
             servicingInstructionGroups = [];
         }
 
@@ -259,12 +265,12 @@
         updateJSON();
 
         // Add group button handler
-        $('#add_servicing_group').on('click', function() {
+        $("#add_servicing_group").on("click", function () {
             addServicingGroup();
         });
 
         // Form submission handler - update JSON before submit
-        $('form').on('submit', function() {
+        $("form").on("submit", function () {
             updateJSON();
         });
     }
@@ -286,23 +292,27 @@
 
         // Validate: max 2 instructions per group
         if (group.length >= 2) {
-            alert('Each service group can contain at most 2 instructions (one LVC and/or one LVT).');
+            alert("Each service group can contain at most 2 instructions (one LVC and/or one LVT).");
             return;
         }
 
-        if (!type) type = 'LVC'; // Default to LVC (coded)
-        if (!value) value = '';
+        if (!type) type = "LVC"; // Default to LVC (coded)
+        if (!value) value = "";
 
         // Check if this type already exists in the group
-        var hasLVC = group.some(function(inst) { return inst.type === 'LVC'; });
-        var hasLVT = group.some(function(inst) { return inst.type === 'LVT'; });
+        var hasLVC = group.some(function (inst) {
+            return inst.type === "LVC";
+        });
+        var hasLVT = group.some(function (inst) {
+            return inst.type === "LVT";
+        });
 
-        if (type === 'LVC' && hasLVC) {
-            alert('This group already has an LVC instruction. Each group can have only one LVC.');
+        if (type === "LVC" && hasLVC) {
+            alert("This group already has an LVC instruction. Each group can have only one LVC.");
             return;
         }
-        if (type === 'LVT' && hasLVT) {
-            alert('This group already has an LVT instruction. Each group can have only one LVT.');
+        if (type === "LVT" && hasLVT) {
+            alert("This group already has an LVT instruction. Each group can have only one LVT.");
             return;
         }
 
@@ -323,7 +333,7 @@
         // Check if changing type would create a duplicate
         for (var i = 0; i < group.length; i++) {
             if (i !== instructionIndex && group[i].type === type) {
-                alert('This group already has a ' + type + ' instruction. Each group can have only one of each type.');
+                alert("This group already has a " + type + " instruction. Each group can have only one of each type.");
                 renderServicingInstructions(); // Re-render to reset the select
                 return;
             }
@@ -334,7 +344,7 @@
     }
 
     function renderServicingInstructions() {
-        var container = $('#servicing_instructions_container');
+        var container = $("#servicing_instructions_container");
         container.empty();
 
         if (servicingInstructionGroups.length === 0) {
@@ -342,22 +352,22 @@
             return;
         }
 
-        servicingInstructionGroups.forEach(function(group, groupIndex) {
+        servicingInstructionGroups.forEach(function (group, groupIndex) {
             var groupDiv = $('<div class="servicing_group"></div>');
             groupDiv.css({
-                'border': '1px solid #ddd',
-                'padding': '10px',
-                'margin-bottom': '10px',
-                'background': '#f9f9f9',
-                'border-radius': '4px'
+                border: "1px solid #ddd",
+                padding: "10px",
+                "margin-bottom": "10px",
+                background: "#f9f9f9",
+                "border-radius": "4px",
             });
 
             var groupHeader = $('<div style="margin-bottom: 10px;"></div>');
-            groupHeader.append('<strong>Service Group ' + (groupIndex + 1) + '</strong> ');
+            groupHeader.append("<strong>Service Group " + (groupIndex + 1) + "</strong> ");
             groupHeader.append(
-                $('<button type="button" class="btn btn-default btn-xs" style="margin-left: 10px;">' +
-                    '<i class="fa fa-trash"></i> Remove group</button>')
-                    .on('click', function() { removeServicingGroup(groupIndex); })
+                $('<button type="button" class="btn btn-default btn-xs" style="margin-left: 10px;">' + '<i class="fa fa-trash"></i> Remove group</button>').on("click", function () {
+                    removeServicingGroup(groupIndex);
+                })
             );
             groupDiv.append(groupHeader);
 
@@ -365,13 +375,13 @@
                 groupDiv.append('<div class="hint" style="margin-left: 20px;">Empty group</div>');
             }
 
-            group.forEach(function(instruction, instructionIndex) {
+            group.forEach(function (instruction, instructionIndex) {
                 var instructionDiv = $('<div class="servicing_instruction" style="margin-left: 20px; margin-bottom: 5px;"></div>');
 
                 var typeSelect = $('<select style="width: 100px; margin-right: 5px;"></select>')
-                    .append('<option value="LVC"' + (instruction.type === 'LVC' ? ' selected' : '') + '>LVC (Coded)</option>')
-                    .append('<option value="LVT"' + (instruction.type === 'LVT' ? ' selected' : '') + '>LVT (Text)</option>')
-                    .on('change', function() {
+                    .append('<option value="LVC"' + (instruction.type === "LVC" ? " selected" : "") + ">LVC (Coded)</option>")
+                    .append('<option value="LVT"' + (instruction.type === "LVT" ? " selected" : "") + ">LVT (Text)</option>")
+                    .on("change", function () {
                         var newType = $(this).val();
                         var valueInput = $(this).next();
                         updateInstruction(groupIndex, instructionIndex, newType, valueInput.val());
@@ -379,29 +389,28 @@
                     });
 
                 var valueInput;
-                if (instruction.type === 'LVC' && servicingInstructionAVs.length > 0) {
+                if (instruction.type === "LVC" && servicingInstructionAVs.length > 0) {
                     valueInput = $('<select style="width: 300px; margin-right: 5px;"></select>');
                     valueInput.append('<option value="">Select...</option>');
-                    servicingInstructionAVs.forEach(function(av) {
+                    servicingInstructionAVs.forEach(function (av) {
                         valueInput.append(
-                            $('<option></option>')
-                                .attr('value', av.value)
+                            $("<option></option>")
+                                .attr("value", av.value)
                                 .text(av.label)
-                                .prop('selected', av.value === instruction.value)
+                                .prop("selected", av.value === instruction.value)
                         );
                     });
                 } else {
-                    valueInput = $('<input type="text" style="width: 300px; margin-right: 5px;" />')
-                        .val(instruction.value);
+                    valueInput = $('<input type="text" style="width: 300px; margin-right: 5px;" />').val(instruction.value);
                 }
 
-                valueInput.on('change', function() {
+                valueInput.on("change", function () {
                     updateInstruction(groupIndex, instructionIndex, typeSelect.val(), $(this).val());
                 });
 
-                var removeBtn = $('<button type="button" class="btn btn-default btn-xs">' +
-                    '<i class="fa fa-trash"></i> Remove</button>')
-                    .on('click', function() { removeInstructionFromGroup(groupIndex, instructionIndex); });
+                var removeBtn = $('<button type="button" class="btn btn-default btn-xs">' + '<i class="fa fa-trash"></i> Remove</button>').on("click", function () {
+                    removeInstructionFromGroup(groupIndex, instructionIndex);
+                });
 
                 instructionDiv.append(typeSelect);
                 instructionDiv.append(valueInput);
@@ -410,32 +419,34 @@
             });
 
             // Determine what can be added to this group
-            var hasLVC = group.some(function(inst) { return inst.type === 'LVC'; });
-            var hasLVT = group.some(function(inst) { return inst.type === 'LVT'; });
+            var hasLVC = group.some(function (inst) {
+                return inst.type === "LVC";
+            });
+            var hasLVT = group.some(function (inst) {
+                return inst.type === "LVT";
+            });
             var canAddMore = group.length < 2;
 
             if (canAddMore) {
                 var btnText = '<i class="fa fa-plus"></i> Add instruction';
                 if (hasLVC && !hasLVT) {
-                    btnText += ' (LVT only)';
+                    btnText += " (LVT only)";
                 } else if (hasLVT && !hasLVC) {
-                    btnText += ' (LVC only)';
+                    btnText += " (LVC only)";
                 } else if (group.length === 0) {
-                    btnText += ' to this group';
+                    btnText += " to this group";
                 }
 
-                var addInstructionBtn = $('<button type="button" class="btn btn-default btn-xs" style="margin-left: 20px; margin-top: 5px;">' + btnText + '</button>')
-                    .on('click', function() {
-                        // Determine default type based on what's already in the group
-                        var defaultType = 'LVC';
-                        if (hasLVC && !hasLVT) defaultType = 'LVT';
-                        else if (hasLVT && !hasLVC) defaultType = 'LVC';
-                        addInstructionToGroup(groupIndex, defaultType);
-                    });
+                var addInstructionBtn = $('<button type="button" class="btn btn-default btn-xs" style="margin-left: 20px; margin-top: 5px;">' + btnText + "</button>").on("click", function () {
+                    // Determine default type based on what's already in the group
+                    var defaultType = "LVC";
+                    if (hasLVC && !hasLVT) defaultType = "LVT";
+                    else if (hasLVT && !hasLVC) defaultType = "LVC";
+                    addInstructionToGroup(groupIndex, defaultType);
+                });
                 groupDiv.append(addInstructionBtn);
             } else {
-                var maxMsg = $('<div class="hint" style="margin-left: 20px; margin-top: 5px; color: #666;">' +
-                    'Maximum 2 instructions per group (one LVC + one LVT)</div>');
+                var maxMsg = $('<div class="hint" style="margin-left: 20px; margin-top: 5px; color: #666;">' + "Maximum 2 instructions per group (one LVC + one LVT)</div>");
                 groupDiv.append(maxMsg);
             }
 
@@ -446,17 +457,17 @@
     function updateJSON() {
         // Filter out empty groups and instructions with empty values
         var filtered = servicingInstructionGroups
-            .map(function(group) {
-                return group.filter(function(instruction) {
-                    return instruction.value && instruction.value.trim() !== '';
+            .map(function (group) {
+                return group.filter(function (instruction) {
+                    return instruction.value && instruction.value.trim() !== "";
                 });
             })
-            .filter(function(group) {
+            .filter(function (group) {
                 return group.length > 0;
             });
 
         var json = JSON.stringify(filtered);
-        $('#servicing_instruction_groups_json').val(json);
+        $("#servicing_instruction_groups_json").val(json);
     }
 </script>
 [% Asset.css("css/addbiblio.css") | $raw %]
@@ -956,7 +967,8 @@
                         <!-- Servicing instruction groups will be populated here -->
                     </div>
                     <button type="button" id="add_servicing_group" class="btn btn-default btn-xs"> <i class="fa fa-plus"></i> Add servicing group </button>
-                    <input type="hidden" id="servicing_instruction_groups_json" name="servicing_instruction_groups_json" value="" />
+                    <input type="hidden" id="servicing_instruction_groups_json" name="servicing_instruction_groups_json" value="[% servicing_instruction_groups_json | html %]" />
+                    <input type="hidden" id="servicing_instruction_avs_json" value="[% servicing_instruction_avs_json | html %]" />
                     <div class="hint">EDIFACT servicing instructions (GIR LVT/LVC). Each group can contain multiple coded (LVC) and/or free-text (LVT) instructions that work together.</div>
                 </li>
                 <li>