From f7f18ce21baedb463629b704dc7451acdf12ffb6 Mon Sep 17 00:00:00 2001
From: Owen Leonard <oleonard@myacpl.org>
Date: Thu, 7 Apr 2022 09:36:00 +0000
Subject: [PATCH] Bug 30482: Potential for bad string concatenation in
 cataloging validation error message
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit

In validating the basic MARC editor form before submission, we run
AreFieldsNotOk() twice (once with the parameter "false") and concatenate
the result for output. This creates the potential for the error string
to be appended with "0" if AreFieldsNotOk() returns false.

This patch improves the logic around building the error string.

To test, apply the patch and make sure one of your MARC frameworks
contains at least one mandatory field and at least one important field.

- Edit or create a MARC record in the basic MARC editor.
- Submit the form in various states of completion:
  - If a mandatory and an important field are empty, you should see two
    error messages at the top. "The following mandatory subfields aren't
    filled" and "The following important subfields aren’t filled."
  - If a mandatory OR an important field is empty, you should see a
    single message.
    - If it's the important field which is empty, a confirmation will
      ask if you want to save the record anyway. Test that both answers
      to this confirmation work correctly.
  - If no mandatory or important fields are empty the form should
    submit.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
---
 .../prog/en/modules/cataloguing/addbiblio.tt  | 49 ++++++++++---------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
index 4d903160ef..8737d3278d 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
@@ -540,7 +540,7 @@ function PopupMARCFieldDoc(field) {
         var subfields = new Array();
         var tab = new Array();
         var label = new Array();
-        var flag=0;
+        var flag = false;
         var tabflag= new Array();
         var StrAlert = "<div id='form-errors' class='dialog alert list'>";
         var notFilledClass = "subfield_not_filled";
@@ -609,7 +609,7 @@ function PopupMARCFieldDoc(field) {
                 var tag=tagsubfieldid.substr(0,3);
                 var subfield=tagsubfieldid.substr(3,1);
                 StrAlert += "<li>"+_("Tag %s subfield %s %s in tab %s").format(tag, subfield, formatFieldName( tabflag[tagsubfieldid][1] ), tabflag[tagsubfieldid][2]) + ' <a class="linkfield btn btn-link" href="#" data-tab="' + tabflag[tagsubfieldid][2] + '" data-field="' + tabflag[tagsubfieldid][3] + '"><i class="fa fa-arrow-right" aria-hidden="true"></i> ' + _("Go to field") + '</a></li>';
-                flag=1;
+                flag = true;
             }
         }
         StrAlert += "</ul>";
@@ -663,7 +663,7 @@ function PopupMARCFieldDoc(field) {
             }
 
             if(isempty){
-                flag = 1;
+                flag = true;
                 if (mandatory) {
                     mandatoryFields[ arr[0] ] = {
                         importance: "mandatory",
@@ -694,7 +694,7 @@ function PopupMARCFieldDoc(field) {
             StrAlert += "</ul>";
         }
         StrAlert += "</div>";
-        if (flag) {
+        if ( flag ) {
             $("#show-errors").html('<button type="button" class="btn btn-danger show-errors"><i class="fa fa-warning"></i> ' + _("Errors") + '</span>');
             return StrAlert;
         } else {
@@ -703,33 +703,36 @@ function PopupMARCFieldDoc(field) {
     }
 
     /**
-     *
-     *
+     * Run checks for mandatory and important fields
+     * Output errors if necessary, or submit the form
      */
     function Check(){
         var StrAlert = AreFieldsNotOk();
-        if( ! StrAlert ){
-            var StrWarning = AreFieldsNotOk(false);
-            if (StrWarning){
-
-                // Check important fields
-                $("#check_errors").html( AreFieldsNotOk(false) );
-                $('html, body').animate({ scrollTop: 0 }, 'fast');
+        var StrWarning = AreFieldsNotOk( false );
+        if( !StrAlert && StrWarning ){
+            // Check important fields
+            $("#check_errors").html( StrWarning );
+            $('html, body').animate({ scrollTop: 0 }, 'fast');
 
-                var r=confirm(_("Important fields(s) are not filled. Are you sure you want to save?"));
-                if (! r){
-                    return false;
-                }
+            var r=confirm( _("Important fields(s) are not filled. Are you sure you want to save?" ) );
+            if (! r){
+                return false;
+            } else {
+                document.f.submit();
+                return true;
             }
-            document.f.submit();
-            return true;
-        } else {
-
-            // Call AreFieldsNotOk() twice to check both mandatory and important fields
-            $("#check_errors").html( AreFieldsNotOk() + AreFieldsNotOk(false) );
+        } else if( StrAlert ){
+            var strAll = StrAlert;
+            if( StrWarning ){
+                strAll += StrWarning;
+            }
+            $("#check_errors").html( strAll );
             $('html, body').animate({ scrollTop: 0 }, 'fast');
             Sticky.hcSticky('refresh');
             return false;
+        } else if( !StrAlert && !StrWarning ){
+            document.f.submit();
+            return true;
         }
     }
 
-- 
2.39.5