WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#32248 closed defect (bug) (fixed)

adding a term containing "&" and renamed

Reported by: olivM Owned by: boonebgorges
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.0
Component: Administration Keywords: has-patch
Focuses: administration Cc:
PR Number:

Description

when adding terms to a post, $_POST[tax_input][taxonomy] contains only the terms name.
in edit_post() before sending $post_data to wp_update_post, we try to find each term by name ( via get_terms() )

-> which wont work for term with '&' in name, as it's saved as html entities in wp_terms

if not term is found, the term stays in post_data as a string for term's name

then in wp_update_post, we use wp_set_object_terms, which will try to find the term via term_exists,
but term_exists do a sanitize_title to try to get the term by slug

-> which wont work if the slug has been changed after term creation.

so finally, wp_set_object_terms will try to create the term, but it will failed with a "term with the name provided already exists in this taxonomy" error

( => and this will also stop adding other terms to the post )

steps to reproduce the bug :

  • create a term "Bob Marley & the Wailers"
  • change the term slug from 'bob-marley-the-wailers' to 'bob-marley-and-the-wailers'
  • edit a post, add the term "Bob Marley & the Wailers"
  • submit

=> the term wont be added

Attachments (1)

32248.diff (1.9 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (14)

#1 @boonebgorges
5 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.3

Confirmed. Thanks for the detailed report.

The 'name' parameter for get_terms() (introduced in 4.2; see #30611) currently uses sanitize_text_field() on the value passed to 'name', but this is not enough: it should be run through sanitize_term_field().

Can I ask you apply 32248.diff to see if it fixes the problem for you?

@boonebgorges
5 years ago

#2 @olivM
5 years ago

Hi,

it seems do the solve the problem !

will it be included in current trunk ? or in a later one ?

#3 @boonebgorges
5 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 32353:

Improve sanitization of 'name' param in get_terms().

Values of 'name' that contain db-encoded character on insert - like an
ampersand, which is HTML-encoded in the database - will only match if they go
through the same sanitize_term_field() routine.

Fixes #32248.

#4 @boonebgorges
5 years ago

It'll be part of 4.3. Thanks again for the report!

#5 @boonebgorges
5 years ago

#32342 was marked as a duplicate.

#6 @boonebgorges
4 years ago

#30419 was marked as a duplicate.

#7 @matthewmi11er
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Unless I'm mistaken because of a similar mismatch in sanitation

term_exists

contributes to this problem.

Something like the following should be added at line 2113

$sanitized_term=sanitize_term(
  array( 
   'alias_of' => '', 
   'description' => '', 
   'parent' => 0, 'slug' => '', 
   'name'=>$term, 
   'taxonomy'=>$taxonomy
  ),
  $taxonomy);
$term=$sanitized_term['name'];

#8 @obenland
4 years ago

@boonebgorges could you double-check on this to keep this moving?

#9 @boonebgorges
4 years ago

matthewmi11er - Can you give more details on how to reproduce what you're describing? term_exists() is a low-level function. Character encoding should be handled by the function that calls term_exists(), and term_exists() ought to trust what's passed to it. Making things more complicated, the $term passed to term_exists() is checked against both the 'name' and the 'slug' columns, and the same sanitization can't be performed for both. That being said, term_exists() does do some sanitization of slugs (sanitize_title()). In short: it's a mess, and I don't recommend using it - use get_term_by() instead.

All of this is a roundabout way of saying that I'd like to avoid touching term_exists() unless it's resulting in real-life problems. I'm pretty sure that the bug described in this ticket doesn't touch term_exists(). Do you have a specific demonstration in mind? If not, and this is just a general point about term_exists(), let's open a separate ticket so we can clear out the milestone.

#10 @matthewmi11er
4 years ago

I think the change to get_terms() fixes the error in the specific circumstances listed in the original issue description. However what may be happening (if I'm tracing everything properly) is that wp_set_post_terms() is called twice when updating the post. The first time the term name is passed to it, which fails to update the tags properly. The second time the term_id is passed which succeeds.

Moving this to a new issue may make sense (I'd be fine with that). I don't have a specific demonstration but I think it will show up anywhere wp_set_object_terms() gets called with the term name (instead of terrm_id) and the term name has an ampersand and the slug has been modified.

Last edited 4 years ago by matthewmi11er (previous) (diff)

#11 @boonebgorges
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Thanks, matthewmi11er.

However what may be happening (if I'm tracing everything properly) is that wp_set_post_terms() is called twice when updating the post.

I'm not seeing this at a glance, but since it doesn't seem to be breaking the interface that's the subject of this ticket, let's call it a separate issue. If you come up with a way to reproduce what you're describing - either in a core UI, or by calling wp_insert_post() (or edit_post() or whatever) in code - please do open a ticket with details.

This ticket was mentioned in Slack in #docs by majemedia. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by majemedia. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.