Fix previous fix to setting checkbox value (fixes #1056)

The code accepted values other than /Yes but still used /Yes as the
checked value instead of obeying the normal appearance dictionary.

6 files changed, 43 insertions, 11 deletions

diff --git a/ChangeLog b/ChangeLog
index 7f673204..294aa8cc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2024-02-11  Jay Berkenbilt  <ejb@ql.org>
+
+	* The previous fix to #1056 was incomplete. When setting a check
+	box value, the previous fix allowed any value other than /Off to
+	mean checked. Now we also set the actual value based on the
+	allowable non-/Off value in the normal appearance dictionary.
+	Fixes #1056.
+
 2024-02-03  Jay Berkenbilt  <ejb@ql.org>
 
 	* Add fuzz testing for JSON.
diff --git a/include/qpdf/QPDFFormFieldObjectHelper.hh b/include/qpdf/QPDFFormFieldObjectHelper.hh
index f31bb7bd..881a7db4 100644
--- a/include/qpdf/QPDFFormFieldObjectHelper.hh
+++ b/include/qpdf/QPDFFormFieldObjectHelper.hh
@@ -166,7 +166,9 @@ class QPDFFormFieldObjectHelper: public QPDFObjectHelper
     // either /Tx (text) or /Ch (choice), set /NeedAppearances to true. You can explicitly tell this
     // method not to set /NeedAppearances if you are going to generate an appearance stream
     // yourself. Starting with qpdf 8.3.0, this method handles fields of type /Btn (checkboxes,
-    // radio buttons, pushbuttons) specially.
+    // radio buttons, pushbuttons) specially. When setting a checkbox value, any value other than
+    // /Off will be treated as on, and the actual value set will be based on the appearance stream's
+    // /N dictionary, so the value that ends up in /V may not exactly match the value you pass in.
     QPDF_DLL
     void setV(QPDFObjectHandle value, bool need_appearances = true);
 
diff --git a/libqpdf/QPDFFormFieldObjectHelper.cc b/libqpdf/QPDFFormFieldObjectHelper.cc
index 371ed271..40627c3d 100644
--- a/libqpdf/QPDFFormFieldObjectHelper.cc
+++ b/libqpdf/QPDFFormFieldObjectHelper.cc
@@ -310,8 +310,8 @@ QPDFFormFieldObjectHelper::setV(QPDFObjectHandle value, bool need_appearances)
                 setCheckBoxValue((name != "/Off"));
             }
             if (!okay) {
-                this->oh.warnIfPossible("ignoring attempt to set a checkbox field to a value of "
-                                        "other than /Yes or /Off");
+                this->oh.warnIfPossible(
+                    "ignoring attempt to set a checkbox field to a value whose type is not name");
             }
         } else if (isRadioButton()) {
             if (value.isName()) {
@@ -415,9 +415,6 @@ QPDFFormFieldObjectHelper::setRadioButtonValue(QPDFObjectHandle name)
 void
 QPDFFormFieldObjectHelper::setCheckBoxValue(bool value)
 {
-    // Set /AS to /Yes or /Off in addition to setting /V.
-    QPDFObjectHandle name = QPDFObjectHandle::newName(value ? "/Yes" : "/Off");
-    setFieldAttribute("/V", name);
     QPDFObjectHandle AP = this->oh.getKey("/AP");
     QPDFObjectHandle annot;
     if (AP.isNull()) {
@@ -439,6 +436,29 @@ QPDFFormFieldObjectHelper::setCheckBoxValue(bool value)
     } else {
         annot = this->oh;
     }
+    std::string on_value;
+    if (value) {
+        // Set the "on" value to the first value in the appearance stream's normal state dictionary
+        // that isn't /Off. If not found, fall back to /Yes.
+        if (AP.isDictionary()) {
+            auto N = AP.getKey("/N");
+            if (N.isDictionary()) {
+                for (auto const& iter: N.ditems()) {
+                    if (iter.first != "/Off") {
+                        on_value = iter.first;
+                        break;
+                    }
+                }
+            }
+        }
+        if (on_value.empty()) {
+            on_value = "/Yes";
+        }
+    }
+
+    // Set /AS to the on value or /Off in addition to setting /V.
+    QPDFObjectHandle name = QPDFObjectHandle::newName(value ? on_value : "/Off");
+    setFieldAttribute("/V", name);
     if (!annot.isInitialized()) {
         QTC::TC("qpdf", "QPDFObjectHandle broken checkbox");
         this->oh.warnIfPossible("unable to set the value of this checkbox");
diff --git a/qpdf/qtest/qpdf/button-set-out.pdf b/qpdf/qtest/qpdf/button-set-out.pdf
index b7ceae01..d08f4586 100644
--- a/qpdf/qtest/qpdf/button-set-out.pdf
+++ b/qpdf/qtest/qpdf/button-set-out.pdf
@@ -122,7 +122,7 @@ endobj
   >>
   /P 15 0 R
   /T (checkbox1)
-  /V /Yes
+  /V /Yup
 >>
 endobj
 
@@ -589,10 +589,10 @@ endobj
   /AP <<
     /N <<
       /Off 64 0 R
-      /Yes 66 0 R
+      /Yup 66 0 R
     >>
   >>
-  /AS /Yes
+  /AS /Yup
   /F 4
   /Rect [
     118.649
diff --git a/qpdf/qtest/qpdf/button-set.pdf b/qpdf/qtest/qpdf/button-set.pdf
index fe96f336..ca65395f 100644
--- a/qpdf/qtest/qpdf/button-set.pdf
+++ b/qpdf/qtest/qpdf/button-set.pdf
@@ -3481,7 +3481,7 @@ endobj
   /AP <<
     /N <<
       /Off 24 0 R
-      /Yes 26 0 R
+      /Yup 26 0 R
     >>
   >>
   /AS /Off
diff --git a/qpdf/test_driver.cc b/qpdf/test_driver.cc
index 28d8062c..e7451576 100644
--- a/qpdf/test_driver.cc
+++ b/qpdf/test_driver.cc
@@ -1942,7 +1942,9 @@ test_51(QPDF& pdf, char const* arg2)
         } else if (Tval == "checkbox1") {
             std::cout << "turning checkbox1 on\n";
             QPDFFormFieldObjectHelper foh(field);
-            foh.setV(QPDFObjectHandle::newName("/Yes"));
+            // The value that eventually gets set is based on what's allowed in /N and may not match
+            // this value.
+            foh.setV(QPDFObjectHandle::newName("/Sure"));
         } else if (Tval == "checkbox2") {
             std::cout << "turning checkbox2 off\n";
             QPDFFormFieldObjectHelper foh(field);