diff --git a/php/context/class-fieldmanager-context-term.php b/php/context/class-fieldmanager-context-term.php index d8def367cc..6f9ce6a5ec 100644 --- a/php/context/class-fieldmanager-context-term.php +++ b/php/context/class-fieldmanager-context-term.php @@ -170,8 +170,8 @@ public function __construct( $args, $taxonomies = array(), $show_on_add = true, foreach ( $this->taxonomies as $taxonomy ) { if ( $this->show_on_add ) { add_action( $taxonomy . '_add_form_fields', array( $this, 'add_term_fields' ), 10, 1 ); + add_filter( 'wp_insert_term_data', array( $this, 'verify_term_data_on_insert' ), PHP_INT_MAX, 3 ); add_action( 'created_term', array( $this, 'save_term_fields' ), 10, 3 ); - add_filter( 'pre_insert_term', array( $this, 'maybe_hook_into_create_term' ), 0, 2 ); } if ( $this->show_on_edit ) { @@ -302,12 +302,7 @@ public function save_term_fields( $term_id, $tt_id, $taxonomy ) { // Ensure that the term being created/updated is the term in the request. if ( ! empty( $_POST['tag_ID'] ) && $term_id !== (int) $_POST['tag_ID'] ) { return; - } elseif ( - empty( $_POST['tag_ID'] ) - && ! empty( $_POST['taxonomy'] ) - && $taxonomy === $_POST['taxonomy'] - && isset( $_POST['tag-name'], $_POST['parent'] ) - ) { + } elseif ( empty( $_POST['tag_ID'] ) ) { // Ensure this was the created_term action. if ( ! doing_action( 'created_term' ) ) { return; @@ -319,15 +314,17 @@ public function save_term_fields( $term_id, $tt_id, $taxonomy ) { } // Core expects terms to have a unique combination of [taxonomy, name, parent]. + // Therefore, this verifies that data against what was recorded prior to insert. $term = get_term( $term_id ); - if ( $term->name !== $this->inserting_term_data['name'] ) { - return; - } - if ( $this->inserting_term_data['parent'] !== $term->parent ) { + if ( + $term->name !== $this->inserting_term_data['name'] + || $term->parent !== $this->inserting_term_data['parent'] + || $taxonomy !== $this->inserting_term_data['taxonomy'] + ) { return; } - // This term appears to check out. + // This term appears to check out. Remove the recorded insert data. unset( $this->inserting_term_data ); } // phpcs:enable WordPress.Security.NonceVerification.Missing @@ -355,69 +352,41 @@ public function save_term_fields( $term_id, $tt_id, $taxonomy ) { } /** - * Hook into create_term if the term being created matches the postdata. - * - * @param string|WP_Error $name The term name to add, or a WP_Error object if there's an error. - * @param string $taxonomy Taxonomy slug. - * @return string|WP_Error $name, unmodified. + * Verify that the term being inserted is the term that Fieldmanager data + * should be added to. + * + * This method serves to protect against a plugin or theme creating one or + * more additional terms as a side effect of a term created from the form. + * In order to protect against that, it's necessary to check the inserting + * term's [name, taxonomy, parent], as WordPress requires the combination of + * these fields to be unique. In order to guard against a plugin or theme + * changing this data, it is recorded immediately prior to the term being + * insert, and then checked in `save_term_fields()`. + * + * @param array $data { + * Term data to be inserted. + * + * @type string $name Term name. + * @type string $slug Term slug. + * @type int $term_group Term group. + * } + * @param string $taxonomy Taxonomy slug. + * @param array $args Arguments passed to wp_insert_term(), which is + * raw $_POST data. + * @return array Unmodified `$data`. */ - public function maybe_hook_into_create_term( $name, $taxonomy ) { - // phpcs:disable WordPress.Security.NonceVerification.Missing - if ( - ! is_wp_error( $name ) - && ! empty( $_POST['tag-name'] ) && $_POST['tag-name'] === $name - && ! empty( $_POST['taxonomy'] ) && $_POST['taxonomy'] === $taxonomy - ) { - add_action( 'create_term', array( $this, 'verify_new_term_data_didnt_change' ), 10, 1 ); + public function verify_term_data_on_insert( $data, $taxonomy, $args ) { + // Confirm that this data has the FM nonce for this field. + if ( ! empty( $args[ $this->nonce_key() ] ) ) { // Append the data to the queued insert. $this->inserting_term_data = array( - 'depth' => 1, - 'name' => $name, + 'name' => $data['name'], 'taxonomy' => $taxonomy, - 'parent' => ! empty( $_POST['parent'] ) ? max( (int) $_POST['parent'], 0 ) : 0, + 'parent' => ! empty( $args['parent'] ) ? max( (int) $args['parent'], 0 ) : 0, ); - } elseif ( isset( $this->inserting_term_data['depth'] ) ) { - // If a term is being insert between `pre_insert_term` and `create_term`, note it. - $this->inserting_term_data['depth']++; - } - - return $name; - // phpcs:enable WordPress.Security.NonceVerification.Missing - } - - /** - * Verify the term data didn't change prior to insert. - * - * @param int $term_id Term ID. - */ - public function verify_new_term_data_didnt_change( $term_id ) { - // Early escape for a situation which shouldn't happen. - if ( empty( $this->inserting_term_data ) ) { - return; - } - - // If depth > 1, it means that this term is being inserted while the target - // term is, as a side effect. This will ignore it and reduce the tracked depth. - if ( $this->inserting_term_data['depth'] > 1 ) { - $this->inserting_term_data['depth']--; - return; - } - - $term = get_term( $term_id ); - if ( - $term->name !== $this->inserting_term_data['name'] - || $term->taxonomy !== $this->inserting_term_data['taxonomy'] - || $term->parent !== $this->inserting_term_data['parent'] - ) { - // The data was manipualted prior to insert. - $this->inserting_term_data['name'] = $term->name; - $this->inserting_term_data['taxonomy'] = $term->taxonomy; - $this->inserting_term_data['parent'] = $term->parent; } - // Since the term is now confirmed, skip any additional checks. - remove_filter( 'pre_insert_term', array( $this, 'maybe_hook_into_create_term' ), 0, 2 ); - remove_action( 'create_term', array( $this, 'verify_new_term_data_didnt_change' ), 10, 1 ); + return $data; } /** diff --git a/php/context/class-fieldmanager-context.php b/php/context/class-fieldmanager-context.php index c4763467a0..eb46dcdd7a 100644 --- a/php/context/class-fieldmanager-context.php +++ b/php/context/class-fieldmanager-context.php @@ -41,6 +41,15 @@ public function __construct() { add_filter( 'wp_refresh_nonces', array( $this, 'refresh_nonce' ) ); } + /** + * Get the nonce key for this field. + * + * @return string + */ + protected function nonce_key() { + return 'fieldmanager-' . $this->fm->name . '-nonce'; + } + /** * Include a fresh nonce for this field in a response with refreshed nonces. * @@ -50,7 +59,7 @@ public function __construct() { * @return array Updated response data. */ public function refresh_nonce( $response ) { - $response['fieldmanager_refresh_nonces']['replace'][ 'fieldmanager-' . $this->fm->name . '-nonce' ] = wp_create_nonce( 'fieldmanager-save-' . $this->fm->name ); + $response['fieldmanager_refresh_nonces']['replace'][ $this->nonce_key() ] = wp_create_nonce( 'fieldmanager-save-' . $this->fm->name ); return $response; } @@ -62,12 +71,12 @@ public function refresh_nonce( $response ) { * @return bool */ protected function is_valid_nonce() { - if ( empty( $_POST[ 'fieldmanager-' . $this->fm->name . '-nonce' ] ) ) { + if ( empty( $_POST[ $this->nonce_key() ] ) ) { return false; } // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- baseline - if ( ! wp_verify_nonce( $_POST[ 'fieldmanager-' . $this->fm->name . '-nonce' ], 'fieldmanager-save-' . $this->fm->name ) ) { + if ( ! wp_verify_nonce( $_POST[ $this->nonce_key() ], 'fieldmanager-save-' . $this->fm->name ) ) { $this->fm->_unauthorized_access( __( 'Nonce validation failed', 'fieldmanager' ) ); } @@ -113,7 +122,7 @@ protected function render_field( $args = array() ) { $data = array_key_exists( 'data', $args ) ? $args['data'] : null; $echo = isset( $args['echo'] ) ? $args['echo'] : true; - $nonce = wp_nonce_field( 'fieldmanager-save-' . $this->fm->name, 'fieldmanager-' . $this->fm->name . '-nonce', true, false ); + $nonce = wp_nonce_field( 'fieldmanager-save-' . $this->fm->name, $this->nonce_key(), true, false ); $field = $this->fm->element_markup( $data ); if ( $echo ) { // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped -- baseline @@ -122,5 +131,4 @@ protected function render_field( $args = array() ) { return $nonce . $field; } } - } diff --git a/tests/php/test-fieldmanager-context-term.php b/tests/php/test-fieldmanager-context-term.php index ededa5f610..2af9622d3b 100644 --- a/tests/php/test-fieldmanager-context-term.php +++ b/tests/php/test-fieldmanager-context-term.php @@ -481,7 +481,7 @@ public function test_term_saving_side_effects_on_term_create() { ]; // Trigger the intended save. - $new_term = wp_insert_term( $new_term_name, $this->taxonomy ); + $new_term = wp_insert_term( $new_term_name, $this->taxonomy, $_POST ); // Fake a side effect. $side_effect_term = self::factory()->term->create_and_get( @@ -532,7 +532,36 @@ function( $term_name ) { ); // Insert the term. - $term = wp_insert_term( $new_term_name, $this->taxonomy ); + $term = wp_insert_term( $new_term_name, $this->taxonomy, $_POST ); + + $this->assertSame( $value, get_term_meta( $term['term_id'], $name, true ) ); + } + + /** + * @see https://github.com/alleyinteractive/wordpress-fieldmanager/issues/831 + */ + public function test_term_meta_saving_on_term_create_when_term_name_has_special_characters() { + $name = 'testing'; + $value = 'Fieldmanager was here'; + $new_term_name = 'Aprés & Mañana™'; + + // Register a Fieldmanager Field and add the term context. + $fm = new Fieldmanager_TextField( compact( 'name' ) ); + $fm->add_term_meta_box( 'Testing', [ $this->taxonomy ] ); + + // Set the POST data. + $_POST = [ + 'taxonomy' => $this->taxonomy, + 'tag-name' => $new_term_name, + 'slug' => '', + 'description' => '', + 'parent' => '-1', + "fieldmanager-{$name}-nonce" => wp_create_nonce( "fieldmanager-save-{$name}" ), + $name => $value, + ]; + + // Insert the term. + $term = wp_insert_term( $new_term_name, $this->taxonomy, $_POST ); $this->assertSame( $value, get_term_meta( $term['term_id'], $name, true ) ); }