Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synced Patterns: Ensure recursion prevention is working correctly #56455

Closed
annezazu opened this issue Nov 22, 2023 · 4 comments · Fixed by #56511
Closed

Synced Patterns: Ensure recursion prevention is working correctly #56455

annezazu opened this issue Nov 22, 2023 · 4 comments · Fixed by #56511
Assignees
Labels
[Block] Pattern Affects the Patterns Block Needs Technical Feedback Needs testing from a developer perspective. [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@annezazu
Copy link
Contributor

A report on X was brought to my attention where someone placed a reusable block (synced pattern) within itself causing a 500 error. This is reminiscent of a previously reported and handled problem described here #18704. As a result of that prior issue, a RecursionProvider component was created as well as simple conventions for preventing recursion on the front end. As noted previously, this was originally implemented in #28405, #28456, #31455. It seems there might be something off with the server warning based on the post on X so this is an issue to follow up to ensure things are working correctly. cc @mcsf who helped implement this work previously!

@annezazu annezazu added [Type] Bug An existing feature does not function as intended Needs Technical Feedback Needs testing from a developer perspective. labels Nov 22, 2023
@talldan
Copy link
Contributor

talldan commented Nov 23, 2023

I did a quick test and it seems to work fine for the general use case.

In the block editor, there's a warning that a block cannot be placed within itself:
Screenshot 2023-11-23 at 1 28 02 pm

In the frontend, when you have debug mode on, an error message is shown there too (and without debug mode nothing renders for the affected block):
Screenshot 2023-11-23 at 1 34 59 pm

Not to say that it isn't impossible for a user. For example the site may have entirely replaced the code that renders patterns/reusable blocks (perhaps via a plugin), in which case they will have removed the code that protects against recursion.

There may also be other ways to exploit it, though I can't think of any, the PHP code is fairly simple and effective.

@mcsf
Copy link
Contributor

mcsf commented Nov 23, 2023

Thanks for the issue and the debugging, @annezazu and @talldan!

Without detailed instructions to reproduce this bug, this will be difficult to fully diagnose. That said, in thinking about how runaway recursion can occur, it struck me that the thin wp:pattern wrapper isn't guarding against it:

const clonedBlocks = selectedPattern.blocks.map( ( block ) =>
cloneBlock(
injectThemeAttributeInBlockTemplateContent( block )
)
);

$content = do_blocks( $content );

Now with the consolidation of terminology around "patterns" — synced or not synced — this has become easier to miss: synced patterns (formerly "reusable blocks") are protected, unsynced aren't.

I can confirm that one can create recursive dynamic patterns that crash both the front end and the editor, and I'll be sharing a fix soon. Due to the idiosyncratic mechanics of this block, the standard approach based on RecursionProvider will not work.

That said, the original report is odd in that only the front end crashes, but the editor correctly caught the recursion. I wonder if this could be the result of a strange mutual recursion, e.g. an unsynced pattern references a synced pattern which references the unsynced pattern back. This is pure conjecture, and perhaps a red herring.

@annezazu
Copy link
Contributor Author

I'll do what I can to reach out to the original X poster and see if we can get more info :)

@gziolo gziolo added the [Block] Pattern Affects the Patterns Block label Nov 24, 2023
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Nov 24, 2023
@mtias
Copy link
Member

mtias commented Nov 24, 2023

Nice debugging @mcsf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Pattern Affects the Patterns Block Needs Technical Feedback Needs testing from a developer perspective. [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants