[touch:9222]{t:90}when fetching data from the DB that is a serialized…

… PHP object, the REST API replaces it with a JSON error object (instead of removing the property from the JSON entity entirely). When submitting data, also watch for folks submitting these error objects and give an error if they try that
eventespresso · May 18, 2017 · 8b1e50a · 8b1e50a
1 parent 7f0f78f
commit 8b1e50a
Show file tree

Hide file tree

Showing 10 changed files with 119 additions and 134 deletions.
diff --git a/core/libraries/rest_api/ModelDataTranslator.php b/core/libraries/rest_api/ModelDataTranslator.php
@@ -60,7 +60,9 @@ public static function prepareFieldValuesFromJson(
         $requested_version,
         $timezone_string = 'UTC'
     ) {
-        if (is_array($original_value_maybe_array)) {
+        if (is_array($original_value_maybe_array)
+            && ! $field_obj instanceof EE_Serialized_Text_Field
+        ) {
             $new_value_maybe_array = array();
             foreach ($original_value_maybe_array as $array_key => $array_item) {
                 $new_value_maybe_array[$array_key] = ModelDataTranslator::prepareFieldValueFromJson(
@@ -90,7 +92,6 @@ public static function prepareFieldValuesFromJson(
      * @param mixed                $original_value_maybe_array
      * @param string               $request_version (eg 4.8.36)
      * @return array
-     * @throws ObjectDetectedException if we find a serialized PHP object while preparing data for JSON
      */
     public static function prepareFieldValuesForJson($field_obj, $original_value_maybe_array, $request_version)
     {
@@ -128,6 +129,23 @@ public static function prepareFieldValueFromJson(
         $requested_version,
         $timezone_string = 'UTC' // UTC
     ) {
+        //check if they accidentally submitted an error value. If so throw an exception
+        if (is_array($original_value)
+            && isset($original_value['error_code'], $original_value['error_message'])) {
+            throw new RestException(
+                'rest_submitted_error_value',
+                sprintf(
+                    esc_html__(
+                        'You tried to submit a JSON error object as a value for %1$s. That\'s not allowed.',
+                        'event_espresso'
+                    ),
+                    $field_obj->get_name()
+                ),
+                array(
+                    'status' => 400,
+                )
+            );
+        }
         $timezone_string = $timezone_string !== '' ? $timezone_string : get_option('timezone_string', '');
         $new_value = null;
         if ($field_obj instanceof EE_Infinite_Integer_Field
@@ -226,7 +244,6 @@ private static function parseTimezoneOffset($timezone_offset)
      * @param mixed                $original_value
      * @param string               $requested_version
      * @return mixed
-     * @throws ObjectDetectedException if we find an object while preparing data for JSON
      */
     public static function prepareFieldValueForJson($field_obj, $original_value, $requested_version)
     {
@@ -247,7 +264,10 @@ public static function prepareFieldValueForJson($field_obj, $original_value, $re
         //are we about to send an object? just don't. We have no good way to represent it in JSON.
         // can't just check using is_object() because that missed PHP incomplete objects
         if (! ModelDataTranslator::isRepresentableInJson($new_value)) {
-            throw new ObjectDetectedException($new_value);
+            $new_value = array(
+                'error_code' => 'php_object_not_return',
+                'error_message' => esc_html__('The value of this field in the database is a PHP object, which can\'t be represented in JSON.', 'event_espresso')
+            );
         }
         return apply_filters(
             'FHEE__EventEspresso\core\libraries\rest_api\Model_Data_Translator__prepare_field_for_rest_api',
@@ -338,6 +358,7 @@ public static function prepareConditionsQueryParamsForModels(
                             continue;
                         }
                     }
+
                     //writing serialized data to the DB is sensitive stuff (especially if it's not restricted to
                     //only being HTML inside.
                     // Only allow trusted users to do that

diff --git a/core/libraries/rest_api/ObjectDetectedException.php b/core/libraries/rest_api/ObjectDetectedException.php
diff --git a/core/libraries/rest_api/controllers/model/Meta.php b/core/libraries/rest_api/controllers/model/Meta.php
@@ -1,7 +1,6 @@
 <?php
 namespace EventEspresso\core\libraries\rest_api\controllers\model;
 
-use EventEspresso\core\libraries\rest_api\ObjectDetectedException;
 use Exception;
 use EE_Boolean_Field;
 use EE_Maintenance_Mode;
@@ -50,8 +49,6 @@ public static function handleRequestModelsMeta(\WP_REST_Request $request, $versi
     /*
      * Gets the model metadata resource entity
      * @return array for JSON response, describing all the models available in teh requested version
-     * @throws ObjectDetectedException if a default value has a PHP object, which should never do (and if we
-     * did, let's know about it ASAP, so let the exception bubble up)
      */
     protected function getModelsMetadataEntity()
     {

diff --git a/core/libraries/rest_api/controllers/model/Read.php b/core/libraries/rest_api/controllers/model/Read.php
@@ -671,49 +671,43 @@ protected function createBareEntityFromWpdbResults(EEM_Base $model, $db_row)
         );
         foreach ($result as $field_name => $field_value) {
             $field_obj = $model->field_settings_for($field_name);
-            try {
-                if ($this->isSubclassOfOne($field_obj, $this->getModelVersionInfo()->fieldsIgnored())) {
-                    unset($result[$field_name]);
-                } elseif ($this->isSubclassOfOne(
+            if ($this->isSubclassOfOne($field_obj, $this->getModelVersionInfo()->fieldsIgnored())) {
+                unset($result[$field_name]);
+            } elseif ($this->isSubclassOfOne(
+                $field_obj,
+                $this->getModelVersionInfo()->fieldsThatHaveRenderedFormat()
+            )
+            ) {
+                $result[$field_name] = array(
+                    'raw'      => $this->prepareFieldObjValueForJson($field_obj, $field_value),
+                    'rendered' => $this->prepareFieldObjValueForJson($field_obj, $field_value, 'pretty'),
+                );
+            } elseif ($this->isSubclassOfOne(
+                $field_obj,
+                $this->getModelVersionInfo()->fieldsThatHavePrettyFormat()
+            )
+            ) {
+                $result[$field_name] = array(
+                    'raw'    => $this->prepareFieldObjValueForJson($field_obj, $field_value),
+                    'pretty' => $this->prepareFieldObjValueForJson($field_obj, $field_value, 'pretty'),
+                );
+            } elseif ($field_obj instanceof \EE_Datetime_Field) {
+                $field_value = $field_obj->prepare_for_set_from_db($field_value);
+                $timezone = $field_value->getTimezone();
+                $field_value->setTimezone(new \DateTimeZone('UTC'));
+                $result[$field_name . '_gmt'] = ModelDataTranslator::prepareFieldValuesForJson(
                     $field_obj,
-                    $this->getModelVersionInfo()->fieldsThatHaveRenderedFormat()
-                )
-                ) {
-                    $result[$field_name] = array(
-                        'raw'      => $this->prepareFieldObjValueForJson($field_obj, $field_value),
-                        'rendered' => $this->prepareFieldObjValueForJson($field_obj, $field_value, 'pretty'),
-                    );
-                } elseif ($this->isSubclassOfOne(
+                    $field_value,
+                    $this->getModelVersionInfo()->requestedVersion()
+                );
+                $field_value->setTimezone($timezone);
+                $result[$field_name] = ModelDataTranslator::prepareFieldValuesForJson(
                     $field_obj,
-                    $this->getModelVersionInfo()->fieldsThatHavePrettyFormat()
-                )
-                ) {
-                    $result[$field_name] = array(
-                        'raw'    => $this->prepareFieldObjValueForJson($field_obj, $field_value),
-                        'pretty' => $this->prepareFieldObjValueForJson($field_obj, $field_value, 'pretty'),
-                    );
-                } elseif ($field_obj instanceof \EE_Datetime_Field) {
-                    $field_value = $field_obj->prepare_for_set_from_db($field_value);
-                    $timezone = $field_value->getTimezone();
-                    $field_value->setTimezone(new \DateTimeZone('UTC'));
-                    $result[$field_name . '_gmt'] = ModelDataTranslator::prepareFieldValuesForJson(
-                        $field_obj,
-                        $field_value,
-                        $this->getModelVersionInfo()->requestedVersion()
-                    );
-                    $field_value->setTimezone($timezone);
-                    $result[$field_name] = ModelDataTranslator::prepareFieldValuesForJson(
-                        $field_obj,
-                        $field_value,
-                        $this->getModelVersionInfo()->requestedVersion()
-                    );
-                } else {
-                    $result[$field_name] = $this->prepareFieldObjValueForJson($field_obj, $field_value);
-                }
-            } catch (ObjectDetectedException $e) {
-                //so the value had a PHP object in it. Well exclude it from the response then. That will make it obvious
-                //to API clients that we're not showing them something.
-                unset($result[$field_name]);
+                    $field_value,
+                    $this->getModelVersionInfo()->requestedVersion()
+                );
+            } else {
+                $result[$field_name] = $this->prepareFieldObjValueForJson($field_obj, $field_value);
             }
         }
         return $result;

diff --git a/docs/C--REST-API/ee4-rest-api-reading-data.md b/docs/C--REST-API/ee4-rest-api-reading-data.md
@@ -139,12 +139,40 @@ Some fields in Event Espresso can represent infinity, which isn't part of the JS
 ###Serialized PHP Objects in Responses Are Removed
 
 There are some database columns where we store serialized PHP objects, but when reading that data over the EE4 REST 
-API, entity properties containing a PHP object are removed entirely from responses. E.g., if you're reading answers, and one of those answers' `ANS_value` contains a serialized PHP object in the EE
- database, that answer entity will not have an `ANS_value` property. So you should check entities have the normal properties before using them. 
- We do this because there is no good way to represent PHP objects in JSON anyway. Also, we plan to remove these serialized PHP objects from our database (for this reason, and others). So
- you should not encounter this situation very frequently right now, and even less frequently in the future. 
-
-However, we will continue to store serialized arrays in our database. These are represented as JSON objects or arrays.
+API, we replace these values with a JSON "error" object containing keys "error_code", and "error_message".
+E.g., normally answer entities look like this
+
+```json
+{
+  "ANS_ID": 1,
+  "ANS_value": [
+    "b",
+    "c"
+  ],
+...
+}
+```
+or
+```json
+{
+  "ANS_ID": 2,
+  "ANS_value": "foobar",
+...
+}
+```
+
+but if the answer somehow contains a serialized PHP object in the database in its "ANS_value" column, "ANS_value" will be 
+replaced with a JSON error object, like this
+```json
+{
+"ANS_ID": 3,
+  "ANS_value": {
+    "error_code": "php_object_not_return",
+    "error_message": "The value of this field in the database is a PHP object, which can&#039;t be represented in JSON."
+  },
+  ...
+}
+```
 
 ### Datetimes and Timezones
 Since 4.9.0, all datetimes returned in the EE4 REST API are in the site's default timezone (see the [site_info endpoint](https://github.com/eventespresso/event-espresso-core/blob/master/docs/C--REST-API/ee4-rest-api-introduction.md#site-info)  to find which timezone that is). This is usually the timezone you want to display to users, and their input to you will usually be in this timezone too.

diff --git a/docs/C--REST-API/rest-api-changelog.md b/docs/C--REST-API/rest-api-changelog.md
@@ -5,7 +5,7 @@ This is a log of client-facing changes made to the EE4 REST API (ie, changes to
 
 ## 4.9.38
 - Added support for `POST`ing, `PUT`ing, and `DELETE`ing EE4 model data
-- JSON entities with properties containing PHP objects are REMOVED entirely from responses.
+- PHP objects in JSON responses are replaced with JSON objects with properties "error_code" and "error_message"
 - Serialized data is no longer accepted when querying
 
 ## 4.9.18

diff --git a/modules/core_rest_api/EED_Core_Rest_Api.module.php b/modules/core_rest_api/EED_Core_Rest_Api.module.php
@@ -5,7 +5,6 @@
 use EventEspresso\core\libraries\rest_api\changes\ChangesInBase;
 use EventEspresso\core\libraries\rest_api\ModelDataTranslator;
 use EventEspresso\core\libraries\rest_api\ModelVersionInfo;
-use EventEspresso\core\libraries\rest_api\ObjectDetectedException;
 
 defined('EVENT_ESPRESSO_VERSION') || exit;
 
@@ -731,8 +730,6 @@ protected function _get_read_query_params(\EEM_Base $model, $version)
      * @param boolean          $create                                       whether this is for request to create (in which case we need
      *                                                                       all required params) or just to update (in which case we don't need those on every request)
      * @return array
-     * @throws ObjectDetectedException if a default value has a PHP object, which should never do (and if we
-     *                                                                       did, let's know about it ASAP, so let the exception bubble up)
      */
     protected function _get_write_params(
         $model_name,

diff --git a/tests/testcases/core/libraries/rest_api/ModelDataTranslatorTest.php b/tests/testcases/core/libraries/rest_api/ModelDataTranslatorTest.php
@@ -177,7 +177,9 @@ public function testPrepareFieldValueFromJsonOk(
     public function dataProviderForTestPrepareFieldValueFromJsonBad()
     {
         $serializable_field = new EE_Maybe_Serialized_Simple_HTML_Field('whatever', 'Whatever', true);
+        $serializable_field->_construct_finalize('Foobar', 'test_serialized_field','Foobar');
         $text_field = new EE_Plain_Text_Field('whatever', 'whatever', true);
+        $text_field->_construct_finalize('Foobar', 'test_text_field','Foobar');
         return array(
             array('s:6:"foobar";', $serializable_field),//that's a serialized string alright!
             array('O:4:"Evil":0:{}', $serializable_field),//that's a string with a serialized object of class "Evil"
@@ -186,6 +188,16 @@ public function dataProviderForTestPrepareFieldValueFromJsonBad()
             array('O:4:"Evil":0:{}', $text_field),//double-check we don't even accept serialized text even on normal
             // text fields. Theoretically these won't get unserialized, but I don't see much need for anyone to ever
             // submit this kind of malicious junk, and having them sit around in our DB is dangerous
+            array(
+                array(
+                  'error_code' => 'php_object_not_return',
+                  'error_message' => esc_html__(
+                      'The value of this field in the database is a PHP object, which can\'t be represented in JSON.',
+                      'event_espresso'
+                  )
+                ),
+                $serializable_field
+            )
         );
     }
 
@@ -198,6 +210,7 @@ public function dataProviderForTestPrepareFieldValueFromJsonBad()
      * @param mixed $inputted_json_value
      * @param EE_Model_Field_Base $field_obj
      * @group 9222
+     * @group current
      */
     public function testPrepareFieldValueFromJsonBad($inputted_json_value, EE_Model_Field_Base $field_obj)
     {
@@ -214,6 +227,13 @@ public function dataProviderForTestPrepareFieldValuesForJson()
     {
         $field = new EE_Maybe_Serialized_Simple_HTML_Field('whatever', 'whatever', true);
         $datetime_field = new EE_Datetime_Field('whatever2', 'whatever2', true, EE_Datetime_Field::now);
+        $error_response = array(
+            'error_code' => 'php_object_not_return',
+            'error_message' => esc_html__(
+                'The value of this field in the database is a PHP object, which can\'t be represented in JSON.',
+                'event_espresso'
+            )
+        );
         return array(
             array(array('foo' => 'bar'), array('foo' => 'bar'), $field),
             array(1, 1, $field),
@@ -225,7 +245,10 @@ public function dataProviderForTestPrepareFieldValuesForJson()
                     new DateTimeZone('UTC')
                 ),
                 $datetime_field
-            )
+            ),
+            array($error_response, new stdClass(), $field),
+            array(array('obj'=> $error_response), array('obj' => new stdClass()), $field),
+            array($error_response, @unserialize('O:6:"Foobar":0:{}'), $field)
         );
     }
 
@@ -246,32 +269,6 @@ public function testPrepareFieldValuesForJson($expected, $input, $field_obj)
         );
     }
 
-    /**
-     * @return array 1st item is the expected value, 2nd is the input, 3rd is the field object to use
-     */
-    public function dataProviderForTestPrepareFieldValuesForJsonBad()
-    {
-        $field = new EE_Maybe_Serialized_Simple_HTML_Field('whatever', 'whatever', true);
-        return array(
-            array(new stdClass(), $field),
-            array(array('obj' => new stdClass()), $field),
-            array(@unserialize('O:6:"Foobar":0:{}'), $field)
-        );
-    }
-
-    /**
-     * @group        9222
-     * @dataProvider dataProviderForTestPrepareFieldValuesForJsonBad
-     * @expectedException EventEspresso\core\libraries\rest_api\ObjectDetectedException
-     * @param                     $input
-     * @param EE_Model_Field_Base $field_obj
-     */
-    public function testPrepareFieldValuesForJsonBad($input, $field_obj)
-    {
-        //duck and cover! it's gonna blow!
-        ModelDataTranslator::prepareFieldValuesForJson($field_obj, $input, '4.8.36');
-    }
-
 
 
     /**

diff --git a/tests/testcases/core/libraries/rest_api/controllers/WriteTest.php b/tests/testcases/core/libraries/rest_api/controllers/WriteTest.php
@@ -342,7 +342,6 @@ public function testInsertInvalidParamProvided()
     /**
      * Test that we tell API clients when they are using a bad parameter
      * @group 9222
-     * @group current
      */
     public function testInsertPostType()
     {