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>
This commit is contained in:
Martin Renvoize
Pedro Amorim
parent
42ae1e0bf0
commit
bbb7f406d2
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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 %]
|
||||
|
|
|
|||
|
|
@ -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>
|
||||
|
|
|
|||
Loading…
Reference in New Issue