Repository navigation
Conversation
When update_option() finds the option does not exist, it has already
passed the value through sanitize_option() and then delegates to
add_option(), which sanitizes it a second time. Any callback that is not
idempotent, such as one that encrypts, hashes or appends to the value,
therefore stored a value mangled by a second pass, and only the first
save was affected since the option exists afterwards.
Carry the fact that the value is already sanitized across the delegation
in a function-scoped static, consumed by add_option() before any of its
early returns so it cannot leak into an unrelated call. Relying on
did_filter() was considered and rejected, since sanitize_option() can be
called by anything beforehand and an unsanitized value could then reach
the database.
The signatures of add_option() and update_option() are unchanged, and
pre_update_option and pre_update_option_{$option} still fire with the
same arguments and in the same order.
Refs #21989.
Assert the sanitize callback runs exactly once for the path that creates
an option through update_option(), while direct add_option() calls and
updates to an existing option are unchanged at one pass. Count the calls
rather than only comparing the stored value, since the count is the
property the ticket is about.
Also guard the back-compat property the ticket is protecting: that
pre_update_option_{$option} still fires on the new-option path, and that
the flag does not leak into a later unrelated add_option() call.
Refs #21989.
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @skikken. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
|
I don't see any practical benefit to this over #6156, and it has comment slop, extra tests, and a private function not marked as such. Seems like AI spam, to me, like it hasn't even been reviewed by a human. |
2 participants
When update_option() finds that an option does not exist, it has already passed the value through sanitize_option() before delegating to add_option(), which sanitizes the same value a second time. A sanitize callback therefore ran twice, but only on the first save, because the option exists from then on.
This is not a performance concern in the interesting cases. Any callback that is not idempotent corrupts the stored value on that first save: one that encrypts or hashes stores an encryption of an encryption, one that appends a marker stores the marker twice. The reporter hit it through register_setting(), which uses update_option() and silently creates the option when it is missing, so a plugin's very first settings save is the affected case.
The fix carries the fact that the value is already sanitized across the delegation between the two functions, using a function-scoped static that add_option() consumes. Consuming happens before any of add_option()'s early returns, so the flag cannot survive into an unrelated later call, and a test covers that specifically.
Two alternatives were considered and rejected:
Relying on did_filter( "sanitize_option_{$option}" ) to detect that sanitization had already run. This was proposed in ticket comment 32 and withdrawn in comment 34, because anything can call sanitize_option() or the filter beforehand, and skipping sanitization on that basis could let an unsanitized value reach the database.
Simply moving the notoptions / get_option() check above the sanitize_option() call, so the diversion happens first. This is the smallest diff, but it skips the pre_update_option and pre_update_option_{$option} filters, which currently always fire when update_option() is called. Since update_option() is used everywhere, relying on the absence of those calls would be a backward compatibility break, as argued in ticket comment 43.
The signatures of add_option() and update_option() are unchanged, and no public sanitization bypass parameter is introduced. The order and arguments of the pre_update_option filters are unchanged on both paths.
An alternative shape for this fix, which extracts the database-write half of add_option() into a private helper rather than passing a flag, is available for comparison in #6156.
Trac ticket: https://core.trac.wordpress.org/ticket/21989
Use of AI Tools
AI assistance: Yes
Tool(s): Claude
Model(s): Sonnet
Used for: test cases, code review
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.