Skip to content

Commit

Permalink
Simplify with wp_insert_term_data vs pre_insert_term
Browse files Browse the repository at this point in the history
  • Loading branch information
@mboynes
mboynes committed Mar 22, 2022
1 parent c299d4d commit c2bd3a0
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 75 deletions.
105 changes: 37 additions & 68 deletions php/context/class-fieldmanager-context-term.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) {
Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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;
}

/**
Expand Down
18 changes: 13 additions & 5 deletions php/context/class-fieldmanager-context.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -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;
}
Expand All @@ -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' ) );
}

Expand Down Expand Up @@ -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
Expand All @@ -122,5 +131,4 @@ protected function render_field( $args = array() ) {
return $nonce . $field;
}
}

}
33 changes: 31 additions & 2 deletions tests/php/test-fieldmanager-context-term.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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 ) );
}
Expand Down

0 comments on commit c2bd3a0

Please sign in to comment.