WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#35795 closed defect (bug) (fixed)

`update_metadata()` double-unslashes meta key when using `add_metadata()`

Reported by: jdgrimes Owned by: boonebgorges
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-patch needs-testing
Focuses: Cc:

Description

When update_metadata() detects that the metadata doesn't exist in the database yet, it calls add_metadata() instead of continuing on with the "update" itself. Because the $meta_key and $meta_value are unslashed by both the update_metadata() and add_metadata() functions, the values saved in the database will be incorrect. This was fixed for the $meta_value in #17343. However, the $meta_key is still unslashed twice.

It isn't possible to work around this by just slashing the value twice before passing it to update_metadata(), because if the value already existed in the database (and so add_metadata() wouldn't be used) the value would only be unslashed once.

So there is no way to pass the data to update_metadata() in such a way that it won't end up being either too slashed or too unslashed part of the time.

I discovered this because I was saving some metadata for some objects whose slugs I had designed to\be\namespaced in some cases. The slug (slashed once with wp_slash()) was being used as the meta key, which worked except when using update_metadata() when the meta key didn't yet exist in the database—then all the slashes were ripped out.

// Assumes that the meta key 'test\slashing' doesn't exist.
update_post_meta( $post_id, wp_slash( 'test\slashing' ), 'value' );

// get_metadata() doesn't unslash the key, so we don't need to slash it as above.
$value = get_post_meta( $post_id, 'test\slashing', true );

// $value will be '', should be 'value'.

// Now $value will be 'value'.
$value = get_post_meta( $post_id, 'testslashing', true );

Related: #27421

Attachments (2)

35795.diff (1.8 KB) - added by boonebgorges 4 years ago.
35795.2.diff (1.9 KB) - added by jdgrimes 4 years ago.
Same as previous, but using wp_slash()

Download all attachments as: .zip

Change History (13)

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


4 years ago

#2 @boonebgorges
4 years ago

I think you've actually understated the problem. Not only do keys get double-slashed when update_metadata() falls back on add_metadata(), but also when update_metadata() does its "has anything changed?" check by calling get_metadata().

Until such time as the slashing mess is cleared up, these functions should be reworked so that two copies of $meta_key are kept internally: one that is unslashed (this is the one used for database queries), and one that is not-unslashed (this is the one used to pass to other meta API functions). This change should be supported by copious tests.

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

#3 @boonebgorges
4 years ago

Actually, scratch my comment about get_metadata() above - that function expects unslashed keys.

@boonebgorges
4 years ago

#4 follow-up: @boonebgorges
4 years ago

  • Keywords has-patch needs-testing added

See 35795.diff for what I have in mind. I'm a bit cross-eyed from the number of slashes required to make the unit test work as expected, but I think it's correct.

@jdgrimes
4 years ago

Same as previous, but using wp_slash()

#5 in reply to: ↑ 4 @jdgrimes
4 years ago

Replying to boonebgorges:

See 35795.diff for what I have in mind. I'm a bit cross-eyed from the number of slashes required to make the unit test work as expected, but I think it's correct.

Thanks for looking into this @boonebgorges. I've uploaded 35795.2.diff to use wp_slash() in the tests instead of placing the double slashes in the strings themselves, because I find that easier to follow.

#6 @boonebgorges
4 years ago

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

In 36509:

Don't double-unslash meta key when update_metadata() falls back on add_metadata().

Props jdgrimes.
Fixes #35795.

#7 @boonebgorges
4 years ago

  • Milestone changed from Awaiting Review to 4.5

#8 follow-up: @jdgrimes
4 years ago

I wonder if this fix should be backported to prior versions? I'm not sure how that decision is made, but I thought I'd mention it.

#9 in reply to: ↑ 8 ; follow-up: @boonebgorges
4 years ago

Replying to jdgrimes:

I wonder if this fix should be backported to prior versions? I'm not sure how that decision is made, but I thought I'd mention it.

We backport to the current stable branch for regressions or security fixes, and to earlier branches for security fixes only. This bug doesn't fall into either category.

#10 in reply to: ↑ 9 @jdgrimes
4 years ago

Replying to boonebgorges:

Replying to jdgrimes:

I wonder if this fix should be backported to prior versions? I'm not sure how that decision is made, but I thought I'd mention it.

We backport to the current stable branch for regressions or security fixes, and to earlier branches for security fixes only. This bug doesn't fall into either category.

OK. Thanks for clarifying that for me.

#11 @johnbillion
4 years ago

  • Version trunk deleted
Note: See TracTickets for help on using tickets.