From b1dad0de2a12a0feb0608d1f68f3f1e8144592e6 Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Sun, 11 Feb 2024 15:32:02 -0500 Subject: [PATCH] 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. --- ChangeLog | 8 ++++++ include/qpdf/QPDFFormFieldObjectHelper.hh | 4 ++- libqpdf/QPDFFormFieldObjectHelper.cc | 30 +++++++++++++++++++---- qpdf/qtest/qpdf/button-set-out.pdf | 6 ++--- qpdf/qtest/qpdf/button-set.pdf | 2 +- qpdf/test_driver.cc | 4 ++- 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 + + * 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 * 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);