RTC: Use prepared queries instead of *_post_meta functions#11325
RTC: Use prepared queries instead of *_post_meta functions#11325chriszarate wants to merge 2 commits intoWordPress:trunkfrom
*_post_meta functions#11325Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
chriszarate
force-pushed
the
fix/sync-post-meta-prepared-queries
branch
from
c6314bc to
bfb5f12
Compare
| // | ||
| // This operation corresponds to this function call (single=false): | ||
| // add_post_meta( $post_id, self::SYNC_UPDATE_META_KEY, $update, false ); | ||
| $meta_value = wp_unslash( $update ); |
There was a problem hiding this comment.
Should we remove the wp_unslash() call? Or is $update always slashed here? The method docstring doesn't say that $update must be slashed which makes me think wp_unslash() is present here because it's also present in add_metadata(). Is there any chance that $update could contain a legitimate \" byte sequence that should be preserved verbatim? Right now we'd lose that slash. It may be an important slash!
Also, should we skip the sanitize_meta call? I understand it is brought over from add_metadata, but what kind of sanitization would we expect the plugin authors to hook into here? Would it be safer to start without this extension point until a clear use-case arises? This data is being inserted into post meta today, but it may be moved to a separate table later on – if these sanitization hooks fire in here, we'll need to consider how to handle the registered hooks for inserts targeting the new database table. Not firing those hooks would make that easier later on.
As a side note, WordPress escaping and sanitization strategies always confuse me. Why would we sanitize anything for storage? It's just data. It needs different treatment depending on the context: raw processing, displaying in an HTML attribute, displaying in an HTML text node, embedding in JavaScript. We can't apply a context-specific treatment without damaging the data if the stored value is already preemptively treated with stripslashes or htmlentities. I don't mean that about your work in this PR specifically, it's just a series of weird choices WordPress makes all over the place. I believe WordPress becomes better when we avoid those choices in new code.
There was a problem hiding this comment.
I struggled with these questions. I coded it to match the existing functions mostly to pre-empt questions and spurious security concerns. But I think you're right that if we go with this approach, we should tailor it tightly to the use case.
Is there any chance that $update could contain a legitimate " byte sequence that should be preserved verbatim?
Here, no, it will always be Base-64 encoded binary data. In the awareness update, possibly. Good catch.
| $meta_value = sanitize_meta( self::SYNC_UPDATE_META_KEY, $meta_value, 'post', self::POST_TYPE ); | ||
| $meta_value = maybe_serialize( $meta_value ); | ||
|
|
||
| $result = $wpdb->insert( |
There was a problem hiding this comment.
@dmsnell have you ever found a way to run native prepared statements via $wpdb? Or is concatenating strings via $wpdb->insert() the best alternative that $wpdb offers?
| // add_post_meta( $post_id, self::SYNC_UPDATE_META_KEY, $update, false ); | ||
| $meta_value = wp_unslash( $update ); | ||
| $meta_value = sanitize_meta( self::SYNC_UPDATE_META_KEY, $meta_value, 'post', self::POST_TYPE ); | ||
| $meta_value = maybe_serialize( $meta_value ); |
There was a problem hiding this comment.
Do we need maybe_unserialize here? If it's in here because that's what add_metadata() do, not because we actually expect an already-serialized value, we could replace it with serialize(). Or, even better – with json_encode() as that would prevent an entire class of unserialize()-based vulnerabilities.
| // | ||
| // This operation corresponds to this function call: | ||
| // update_post_meta( $post_id, self::AWARENESS_META_KEY, $awareness ); | ||
| $meta_value = wp_unslash( $awareness ); |
There was a problem hiding this comment.
| } | ||
|
|
||
| // Return true even if the database operation fails, since this operation is | ||
| // not critical for collaboration state. |
There was a problem hiding this comment.
This is outside of scope of this PR. I'm just noticing it because it stood out to me.
Why not tell the caller whether the operation failed or succeeded and decide what to do? It's fine if it chooses to ignore the failure and it's fine if it chooses to do something about it. However, the current public API never offers that choice since it always communicates success.
Also, even more interestingly – a failure to insert or update returns true, but a failure to find the $post_id returns false. Why is one failure mode treated differently from the other one?
2 participants
Opening this for conversation: What if we used direct database operations instead of post meta functions? Does that alleviate concerns about changing the APIs? What are the risks, aside from the security of the operations themselves?
Trac ticket: https://core.trac.wordpress.org/ticket/64696
AI usage
Unit tests written with assistance from Claude Opus 4.6.