Skip to content

fix: prevent oEmbed data leaking between posts on nested calls - #23587

Open
si-kui-a wants to merge 1 commit into
Yoast:trunkfrom
si-kui-a:fix/oembed-reentrant-data-leak
Open

fix: prevent oEmbed data leaking between posts on nested calls#23587
si-kui-a wants to merge 1 commit into
Yoast:trunkfrom
si-kui-a:fix/oembed-reentrant-data-leak

Conversation

@si-kui-a

@si-kui-a si-kui-a commented Sep 3, 2026

Copy link
Copy Markdown

Context

Open_Graph_OEmbed::set_oembed_data() stored the oEmbed data, the post ID, and the post meta it was working with on shared instance properties ($this->data, $this->post_id, $this->post_meta) instead of local variables. If the oembed_response_data filter fires re-entrantly within the same request — for example a theme rebuilding an excerpt by re-running content filters for a different post while the outer oEmbed response is still being assembled — the inner call overwrites that shared state. By the time the outer call resumes and runs set_title()/set_description()/set_image(), it operates on the wrong post's meta and returns the wrong post's title/description/image. WordPress then caches that wrong result indefinitely in the original post's _oembed_* postmeta, so the bug outlives the request that caused it.

Summary

This PR can be summarized in the following changelog entry:

  • Fixes a bug where the oEmbed response for a post could contain another post's Open Graph title, description, or image when the oembed_response_data filter fired more than once in the same request.

Relevant technical choices:

  • set_oembed_data() and its three helpers (set_title, set_description, set_image) now thread $data and $post_meta through as parameters and return values instead of reading/writing instance state. This makes a single call self-contained regardless of any nested call that happens inside it.
  • Removed the $data, $post_id, and $post_meta instance properties entirely rather than keeping them alongside the new parameters — confirmed via GitHub code search (Open_Graph_OEmbed across the repo) that nothing outside this class reads them, so they were purely internal, now-redundant state.
  • Added test_set_oembed_data_is_reentrant_safe(), which mocks Meta_Surface::for_post() for post A to trigger a nested set_oembed_data() call for post B before returning post A's meta — reproducing the exact re-entrancy scenario. It fails against the pre-fix code (post A's result contains post B's title/description) and passes against this fix.
  • Disclosure: no PHP/Composer toolchain is available in the environment this PR was prepared in, so composer check-branch-cs and the PHPUnit run could not be executed locally. The new test was hand-traced line-by-line against both the old and new implementations instead of run. The existing test_set_oembed_data_with_no_data_set cases were re-traced the same way and are unaffected by the signature change from the caller's perspective (set_oembed_data()'s public signature is unchanged). Please let CI run the full suite before merge.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

  • Set an Open Graph title/description/image on two different posts (Post A and Post B) via the Yoast SEO metabox.
  • Embed Post A inside Post B's content (or in a way that causes both posts' oEmbed data to be requested within the same page load, e.g. both posts appearing on an archive/listing page that themes/plugins often filter through the_content more than once).
  • Request the oEmbed endpoint for each post (/wp-json/oembed/1.0/embed?url=<post-url>) and confirm each response's title/description/thumbnail_url matches that post's own Open Graph settings, not the other post's.
  • Run the automated test suite (Open_Graph_OEmbed_Test), in particular the new test_set_oembed_data_is_reentrant_safe case.

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • Changes should be tested on different editors (Default Block/Gutenberg/Classic/Elementor/other)
  • Changes should be tested on different browsers
  • Changes should be tested on multisite

Test instructions for QA when the code is in the RC

  • QA should use the same steps as above.

QA can test this PR by following these steps:

  • Same steps as the acceptance test above.

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

  • The oEmbed response (oembed_response_data filter) for any post with Open Graph title/description/image set — this is the only integration touched.

Other environments

  • This PR also affects Shopify.
  • This PR also affects Yoast SEO for Google Docs.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities.
  • During testing, I had activated all plugins that Yoast SEO provides integrations for.
  • I have added unit tests to verify the code works as intended.
  • If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
  • I have written this PR in accordance with my team's definition of done.
  • I have checked that the base branch is correctly set.
  • I have run grunt build:images and committed the results, if my PR introduces or edits images or SVGs.

Innovation

  • No innovation project is applicable for this PR.

Fixes #23527

🤖 Generated with Claude Code

https://claude.ai/code/session_01HG5wiw3Z8QdjNWb5bFDLiA

…nse_data calls

Open_Graph_OEmbed::set_oembed_data() stored the oEmbed data, post ID, and post
meta on shared instance properties instead of local variables. When the
oembed_response_data filter fires re-entrantly within the same request (e.g. a
theme rebuilding an excerpt by re-running content filters while the outer
oEmbed response for a different post is still being assembled), the inner
call overwrote the shared state, so the outer call's set_title/
set_description/set_image ran against the wrong post's meta and returned the
wrong post's title/description/image -- which then gets cached indefinitely
in that post's _oembed_* postmeta.

Refactored set_oembed_data() and its three helpers to thread $data and
$post_meta through as parameters/return values instead of touching instance
state, removing the now-unused $data/$post_id/$post_meta properties entirely
(confirmed via GitHub code search that nothing outside this class references
them). Added a regression test simulating the nested-call scenario -- it
fails against the old code (second post's data leaks into the first) and
passes against this fix.

Fixes Yoast#23527.

No PHP/Composer toolchain is available in this environment, so
composer check-branch-cs and the PHPUnit run were not executed locally; the
test was hand-traced against the new code instead. Disclosed in the PR body
per docs/workflows/create-pr.md step 2. CI will run the full suite.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HG5wiw3Z8QdjNWb5bFDLiA
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: set_oembed_data ignores its $post` argument — second embed in a post returns the first embed's data

2 participants