Skip to content

Commit

Permalink
Merge pull request #832 from alleyinteractive/hotfix/term-context-sec…
Browse files Browse the repository at this point in the history 
…ondary-term-update-guard

Protect against side effect term updates
  • Loading branch information
@mboynes
mboynes authored Mar 30, 2022
2 parents 04d5fb7 + c2bd3a0 commit 623e2ad
Show file tree
Hide file tree
Showing 3 changed files with 249 additions and 5 deletions.
79 changes: 79 additions & 0 deletions php/context/class-fieldmanager-context-term.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,15 @@ class Fieldmanager_Context_Term extends Fieldmanager_Context_Storable {
*/
private $current_taxonomy;

/**
* Store data for inserted terms to ensure, as much as possible, that FM
* only stores data to the term being created and not any other term created
* as a side effect.
*
* @var array|null
*/
protected $inserting_term_data;

/**
* Instantiate this context. You can either pass an array of all args
* (preferred), or pass them individually (deprecated).
Expand Down Expand Up @@ -161,6 +170,7 @@ 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 );
}

Expand Down Expand Up @@ -288,6 +298,37 @@ public function term_fields( $html_template, $taxonomy, $term = null ) {
* @param string $taxonomy The term taxonomy.
*/
public function save_term_fields( $term_id, $tt_id, $taxonomy ) {
// phpcs:disable WordPress.Security.NonceVerification.Missing
// 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'] ) ) {
// Ensure this was the created_term action.
if ( ! doing_action( 'created_term' ) ) {
return;
}

if ( empty( $this->inserting_term_data ) ) {
// Something has gone awry, this shouldn't happen, so bail.
return;
}

// 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']
|| $term->parent !== $this->inserting_term_data['parent']
|| $taxonomy !== $this->inserting_term_data['taxonomy']
) {
return;
}

// This term appears to check out. Remove the recorded insert data.
unset( $this->inserting_term_data );
}
// phpcs:enable WordPress.Security.NonceVerification.Missing

// Make sure this field is attached to the taxonomy being saved and this is the appropriate action.
// phpcs:ignore WordPress.PHP.StrictInArray.MissingTrueStrict -- baseline
if ( ! in_array( $taxonomy, $this->taxonomies ) ) {
Expand All @@ -310,6 +351,44 @@ public function save_term_fields( $term_id, $tt_id, $taxonomy ) {
$this->save_to_term_meta( $term_id, $taxonomy );
}

/**
* 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 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(
'name' => $data['name'],
'taxonomy' => $taxonomy,
'parent' => ! empty( $args['parent'] ) ? max( (int) $args['parent'], 0 ) : 0,
);
}

return $data;
}

/**
* Helper to save an array of data to term meta.
*
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;
}
}

}
157 changes: 157 additions & 0 deletions tests/php/test-fieldmanager-context-term.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -408,4 +408,161 @@ public function test_unserialize_data_mixed_depth() {
$this->assertStringContainsString('name="base_group[test_group][deep_text]"', $html );
$this->assertStringContainsString( '>' . $data['test_group']['deep_text'] . '</textarea>', $html );
}

/**
* @see https://github.com/alleyinteractive/wordpress-fieldmanager/issues/831
*/
public function test_term_saving_side_effects_on_term_update() {
$name = 'side-effect';
$value = 'Fieldmanager was here';

// Register a Fieldmanager Field and add the term context.
$fm = new Fieldmanager_TextField( compact( 'name' ) );
$fm->add_term_meta_box( 'Testing Side Effects', [ $this->taxonomy ] );

// Set the POST data.
$_POST = [
'tag_ID' => $this->term_id,
'taxonomy' => $this->taxonomy,
'name' => 'News',
'slug' => 'news',
'description' => 'General news',
'parent' => '-1',
"fieldmanager-{$name}-nonce" => wp_create_nonce( "fieldmanager-save-{$name}" ),
$name => $value,
];

// Trigger the intended save.
do_action(
'edited_term',
$this->term_id,
$this->tt_id,
$this->taxonomy
);

// Fake a side effect.
$side_effect_term = self::factory()->term->create_and_get(
[
'taxonomy' => $this->taxonomy,
]
);
do_action(
'edited_term',
$side_effect_term->term_id,
$side_effect_term->term_taxonomy_id,
$this->taxonomy
);

$this->assertSame( $value, get_term_meta( $this->term_id, $name, true ) );
$this->assertSame( [], get_term_meta( $side_effect_term->term_id, $name ) );
}

/**
* @see https://github.com/alleyinteractive/wordpress-fieldmanager/issues/831
*/
public function test_term_saving_side_effects_on_term_create() {
$name = 'side-effect';
$value = 'Fieldmanager was here';
$new_term_name = 'New Term';

// Register a Fieldmanager Field and add the term context.
$fm = new Fieldmanager_TextField( compact( 'name' ) );
$fm->add_term_meta_box( 'Testing Side Effects', [ $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,
];

// Trigger the intended save.
$new_term = wp_insert_term( $new_term_name, $this->taxonomy, $_POST );

// Fake a side effect.
$side_effect_term = self::factory()->term->create_and_get(
[
'taxonomy' => $this->taxonomy,
]
);
do_action(
'edited_term',
$side_effect_term->term_id,
$side_effect_term->term_taxonomy_id,
$this->taxonomy
);

$this->assertSame( $value, get_term_meta( $new_term['term_id'], $name, true ) );
$this->assertSame( [], get_term_meta( $side_effect_term->term_id, $name ) );
}

/**
* @see https://github.com/alleyinteractive/wordpress-fieldmanager/issues/831
*/
public function test_term_meta_saving_on_term_create_when_a_filter_alters_the_term_name() {
$name = 'testing';
$value = 'Fieldmanager was here';
$new_term_name = 'New Term';

// 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,
];

// Manipualte the term name prior to insert.
add_filter(
'pre_insert_term',
function( $term_name ) {
return "Edited: {$term_name}";
}
);

// Insert the term.
$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 ) );
}
}

0 comments on commit 623e2ad

Please sign in to comment.