From 014a6843f7da0dde630a2d9e7cac3277e4e55ff3 Mon Sep 17 00:00:00 2001 From: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Date: Fri, 24 Nov 2023 14:35:13 +0000 Subject: [PATCH 01/12] Prevent infinite loops due to recursive patterns --- .../src/pattern/dependency-graph.js | 87 +++++++++++++++++++ packages/block-library/src/pattern/edit.js | 35 +++++++- 2 files changed, 119 insertions(+), 3 deletions(-) create mode 100644 packages/block-library/src/pattern/dependency-graph.js diff --git a/packages/block-library/src/pattern/dependency-graph.js b/packages/block-library/src/pattern/dependency-graph.js new file mode 100644 index 00000000000000..61930a7a93bf1a --- /dev/null +++ b/packages/block-library/src/pattern/dependency-graph.js @@ -0,0 +1,87 @@ +/** + * Track any patterns that require subsequent patterns, so as to prevent single + * or mutual recursion. + */ +export class PatternsDependencyGraph { + constructor() { + /** + * @type {Map>} + */ + this.deps = new Map(); + } + + /** + * Foo. + * + * @param {Object} pattern Pattern. + * @param {string} pattern.name Pattern name. + * @param {Array} pattern.blocks Pattern's block list. + * @throws {Error} If a circular dependency is detected. + */ + parsePatternDependencies( { name, blocks } ) { + const queue = [ ...blocks ]; + while ( queue.length ) { + const block = queue.shift(); + for ( const innerBlock of block.innerBlocks ?? [] ) { + queue.unshift( innerBlock ); + } + if ( block.name === 'core/pattern' ) { + this.dependsOn( name, block.attributes.slug ); + } + } + } + + /** + * Declare that pattern `a` depends on pattern `b`. If a circular + * dependency is detected, an error will be thrown. + * + * @param {string} a Slug for pattern A. + * @param {string} b Slug for pattern B. + * @throws {Error} If a circular dependency is detected. + */ + dependsOn( a, b ) { + if ( ! this.deps.has( a ) ) { + this.deps.set( a, new Set() ); + } + this.deps.get( a ).add( b ); + + if ( hasCircularDependency( a, this.deps ) ) { + throw new Error( + `Pattern ${ a } has a circular dependency and cannot be rendered.` + ); + } + } +} + +function hasCircularDependency( + id, + dependencyGraph, + visitedNodes = new Set(), + currentPath = new Set() +) { + visitedNodes.add( id ); + currentPath.add( id ); + + const dependencies = dependencyGraph.get( id ) ?? new Set(); + + for ( const dependency of dependencies ) { + if ( ! visitedNodes.has( dependency ) ) { + if ( + hasCircularDependency( + dependency, + dependencyGraph, + visitedNodes, + currentPath + ) + ) { + return true; + } + } else if ( currentPath.has( dependency ) ) { + return true; + } + } + + // Remove the current node from the current path when backtracking + currentPath.delete( id ); + return false; +} diff --git a/packages/block-library/src/pattern/edit.js b/packages/block-library/src/pattern/edit.js index e01fb37eb849e6..f1b57e911d7ca1 100644 --- a/packages/block-library/src/pattern/edit.js +++ b/packages/block-library/src/pattern/edit.js @@ -3,12 +3,21 @@ */ import { cloneBlock } from '@wordpress/blocks'; import { useSelect, useDispatch } from '@wordpress/data'; -import { useEffect } from '@wordpress/element'; +import { useState, useEffect } from '@wordpress/element'; import { + Warning, store as blockEditorStore, useBlockProps, } from '@wordpress/block-editor'; import { store as coreStore } from '@wordpress/core-data'; +import { __ } from '@wordpress/i18n'; + +/** + * Internal dependencies + */ +import { PatternsDependencyGraph } from './dependency-graph'; + +const patternsDependencyGraph = new PatternsDependencyGraph(); const PatternEdit = ( { attributes, clientId } ) => { const selectedPattern = useSelect( @@ -32,6 +41,8 @@ const PatternEdit = ( { attributes, clientId } ) => { const { getBlockRootClientId, getBlockEditingMode } = useSelect( blockEditorStore ); + const [ hasRecursionError, setHasRecursionError ] = useState( false ); + // Duplicated in packages/edit-site/src/components/start-template-options/index.js. function injectThemeAttributeInBlockTemplateContent( block ) { if ( @@ -64,7 +75,16 @@ const PatternEdit = ( { attributes, clientId } ) => { // This change won't be saved. // It will continue to pull from the pattern file unless changes are made to its respective template part. useEffect( () => { - if ( selectedPattern?.blocks ) { + if ( ! hasRecursionError && selectedPattern?.blocks ) { + try { + patternsDependencyGraph.parsePatternDependencies( + selectedPattern + ); + } catch ( error ) { + setHasRecursionError( true ); + return; + } + // We batch updates to block list settings to avoid triggering cascading renders // for each container block included in a tree and optimize initial render. // Since the above uses microtasks, we need to use a microtask here as well, @@ -93,7 +113,8 @@ const PatternEdit = ( { attributes, clientId } ) => { } }, [ clientId, - selectedPattern?.blocks, + hasRecursionError, + selectedPattern, __unstableMarkNextChangeAsNotPersistent, replaceBlocks, getBlockEditingMode, @@ -103,6 +124,14 @@ const PatternEdit = ( { attributes, clientId } ) => { const props = useBlockProps(); + if ( hasRecursionError ) { + return ( +
+ { __( 'Recursion!' ) } +
+ ); + } + return
; }; From 62e3b0b859ff91de5ca852cb82050290945e699b Mon Sep 17 00:00:00 2001 From: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Date: Fri, 24 Nov 2023 14:52:41 +0000 Subject: [PATCH 02/12] Rename, add private method, export singleton * Rename PatternsDependencyGraph to PatternRecursionDetector * Refactor module function hasCircularDependency to class private method * Export singleton PatternsDependencyGraph instance rather than class --- .../src/pattern/dependency-graph.js | 67 ++++++++++--------- packages/block-library/src/pattern/edit.js | 6 +- 2 files changed, 36 insertions(+), 37 deletions(-) diff --git a/packages/block-library/src/pattern/dependency-graph.js b/packages/block-library/src/pattern/dependency-graph.js index 61930a7a93bf1a..b609a0a6fe7ba0 100644 --- a/packages/block-library/src/pattern/dependency-graph.js +++ b/packages/block-library/src/pattern/dependency-graph.js @@ -1,8 +1,4 @@ -/** - * Track any patterns that require subsequent patterns, so as to prevent single - * or mutual recursion. - */ -export class PatternsDependencyGraph { +class PatternRecursionDetector { constructor() { /** * @type {Map>} @@ -11,11 +7,15 @@ export class PatternsDependencyGraph { } /** - * Foo. + * Parse a given pattern and traverse its contents to detect any subsequent + * patterns on which it may depend. Such occurrences will be added to an + * internal dependency graph. If a circular dependency is detected, an + * error will be thrown. * * @param {Object} pattern Pattern. * @param {string} pattern.name Pattern name. * @param {Array} pattern.blocks Pattern's block list. + * * @throws {Error} If a circular dependency is detected. */ parsePatternDependencies( { name, blocks } ) { @@ -37,6 +37,7 @@ export class PatternsDependencyGraph { * * @param {string} a Slug for pattern A. * @param {string} b Slug for pattern B. + * * @throws {Error} If a circular dependency is detected. */ dependsOn( a, b ) { @@ -45,43 +46,43 @@ export class PatternsDependencyGraph { } this.deps.get( a ).add( b ); - if ( hasCircularDependency( a, this.deps ) ) { + if ( this.#hasCircularDependency( a ) ) { throw new Error( `Pattern ${ a } has a circular dependency and cannot be rendered.` ); } } -} -function hasCircularDependency( - id, - dependencyGraph, - visitedNodes = new Set(), - currentPath = new Set() -) { - visitedNodes.add( id ); - currentPath.add( id ); + #hasCircularDependency( + id, + visitedNodes = new Set(), + currentPath = new Set() + ) { + visitedNodes.add( id ); + currentPath.add( id ); - const dependencies = dependencyGraph.get( id ) ?? new Set(); + const dependencies = this.deps.get( id ) ?? new Set(); - for ( const dependency of dependencies ) { - if ( ! visitedNodes.has( dependency ) ) { - if ( - hasCircularDependency( - dependency, - dependencyGraph, - visitedNodes, - currentPath - ) - ) { + for ( const dependency of dependencies ) { + if ( ! visitedNodes.has( dependency ) ) { + if ( + this.#hasCircularDependency( + dependency, + visitedNodes, + currentPath + ) + ) { + return true; + } + } else if ( currentPath.has( dependency ) ) { return true; } - } else if ( currentPath.has( dependency ) ) { - return true; } - } - // Remove the current node from the current path when backtracking - currentPath.delete( id ); - return false; + // Remove the current node from the current path when backtracking + currentPath.delete( id ); + return false; + } } + +export const patternRecursionDetector = new PatternRecursionDetector(); diff --git a/packages/block-library/src/pattern/edit.js b/packages/block-library/src/pattern/edit.js index f1b57e911d7ca1..574462e0627abe 100644 --- a/packages/block-library/src/pattern/edit.js +++ b/packages/block-library/src/pattern/edit.js @@ -15,9 +15,7 @@ import { __ } from '@wordpress/i18n'; /** * Internal dependencies */ -import { PatternsDependencyGraph } from './dependency-graph'; - -const patternsDependencyGraph = new PatternsDependencyGraph(); +import { patternRecursionDetector } from './dependency-graph'; const PatternEdit = ( { attributes, clientId } ) => { const selectedPattern = useSelect( @@ -77,7 +75,7 @@ const PatternEdit = ( { attributes, clientId } ) => { useEffect( () => { if ( ! hasRecursionError && selectedPattern?.blocks ) { try { - patternsDependencyGraph.parsePatternDependencies( + patternRecursionDetector.parsePatternDependencies( selectedPattern ); } catch ( error ) { From 08546e77d1e3eecdb954e6edb5020675095fc90d Mon Sep 17 00:00:00 2001 From: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Date: Fri, 24 Nov 2023 14:57:30 +0000 Subject: [PATCH 03/12] Rename module to ./recursion-detector --- .../src/pattern/dependency-graph.js | 88 ------------------- packages/block-library/src/pattern/edit.js | 6 +- .../src/pattern/recursion-detector.js | 78 ++++++++++++++++ 3 files changed, 80 insertions(+), 92 deletions(-) delete mode 100644 packages/block-library/src/pattern/dependency-graph.js create mode 100644 packages/block-library/src/pattern/recursion-detector.js diff --git a/packages/block-library/src/pattern/dependency-graph.js b/packages/block-library/src/pattern/dependency-graph.js deleted file mode 100644 index b609a0a6fe7ba0..00000000000000 --- a/packages/block-library/src/pattern/dependency-graph.js +++ /dev/null @@ -1,88 +0,0 @@ -class PatternRecursionDetector { - constructor() { - /** - * @type {Map>} - */ - this.deps = new Map(); - } - - /** - * Parse a given pattern and traverse its contents to detect any subsequent - * patterns on which it may depend. Such occurrences will be added to an - * internal dependency graph. If a circular dependency is detected, an - * error will be thrown. - * - * @param {Object} pattern Pattern. - * @param {string} pattern.name Pattern name. - * @param {Array} pattern.blocks Pattern's block list. - * - * @throws {Error} If a circular dependency is detected. - */ - parsePatternDependencies( { name, blocks } ) { - const queue = [ ...blocks ]; - while ( queue.length ) { - const block = queue.shift(); - for ( const innerBlock of block.innerBlocks ?? [] ) { - queue.unshift( innerBlock ); - } - if ( block.name === 'core/pattern' ) { - this.dependsOn( name, block.attributes.slug ); - } - } - } - - /** - * Declare that pattern `a` depends on pattern `b`. If a circular - * dependency is detected, an error will be thrown. - * - * @param {string} a Slug for pattern A. - * @param {string} b Slug for pattern B. - * - * @throws {Error} If a circular dependency is detected. - */ - dependsOn( a, b ) { - if ( ! this.deps.has( a ) ) { - this.deps.set( a, new Set() ); - } - this.deps.get( a ).add( b ); - - if ( this.#hasCircularDependency( a ) ) { - throw new Error( - `Pattern ${ a } has a circular dependency and cannot be rendered.` - ); - } - } - - #hasCircularDependency( - id, - visitedNodes = new Set(), - currentPath = new Set() - ) { - visitedNodes.add( id ); - currentPath.add( id ); - - const dependencies = this.deps.get( id ) ?? new Set(); - - for ( const dependency of dependencies ) { - if ( ! visitedNodes.has( dependency ) ) { - if ( - this.#hasCircularDependency( - dependency, - visitedNodes, - currentPath - ) - ) { - return true; - } - } else if ( currentPath.has( dependency ) ) { - return true; - } - } - - // Remove the current node from the current path when backtracking - currentPath.delete( id ); - return false; - } -} - -export const patternRecursionDetector = new PatternRecursionDetector(); diff --git a/packages/block-library/src/pattern/edit.js b/packages/block-library/src/pattern/edit.js index 574462e0627abe..b6efba4a8eb6eb 100644 --- a/packages/block-library/src/pattern/edit.js +++ b/packages/block-library/src/pattern/edit.js @@ -15,7 +15,7 @@ import { __ } from '@wordpress/i18n'; /** * Internal dependencies */ -import { patternRecursionDetector } from './dependency-graph'; +import { parsePatternDependencies } from './recursion-detector'; const PatternEdit = ( { attributes, clientId } ) => { const selectedPattern = useSelect( @@ -75,9 +75,7 @@ const PatternEdit = ( { attributes, clientId } ) => { useEffect( () => { if ( ! hasRecursionError && selectedPattern?.blocks ) { try { - patternRecursionDetector.parsePatternDependencies( - selectedPattern - ); + parsePatternDependencies( selectedPattern ); } catch ( error ) { setHasRecursionError( true ); return; diff --git a/packages/block-library/src/pattern/recursion-detector.js b/packages/block-library/src/pattern/recursion-detector.js new file mode 100644 index 00000000000000..a6d400ecbeead6 --- /dev/null +++ b/packages/block-library/src/pattern/recursion-detector.js @@ -0,0 +1,78 @@ +/** + * @type {Map>} + */ +const deps = new Map(); + +/** + * Parse a given pattern and traverse its contents to detect any subsequent + * patterns on which it may depend. Such occurrences will be added to an + * internal dependency graph. If a circular dependency is detected, an + * error will be thrown. + * + * @param {Object} pattern Pattern. + * @param {string} pattern.name Pattern name. + * @param {Array} pattern.blocks Pattern's block list. + * + * @throws {Error} If a circular dependency is detected. + */ +export function parsePatternDependencies( { name, blocks } ) { + const queue = [ ...blocks ]; + while ( queue.length ) { + const block = queue.shift(); + for ( const innerBlock of block.innerBlocks ?? [] ) { + queue.unshift( innerBlock ); + } + if ( block.name === 'core/pattern' ) { + dependsOn( name, block.attributes.slug ); + } + } +} + +/** + * Declare that pattern `a` depends on pattern `b`. If a circular + * dependency is detected, an error will be thrown. + * + * @param {string} a Slug for pattern A. + * @param {string} b Slug for pattern B. + * + * @throws {Error} If a circular dependency is detected. + */ +function dependsOn( a, b ) { + if ( ! deps.has( a ) ) { + deps.set( a, new Set() ); + } + deps.get( a ).add( b ); + + if ( hasCircularDependency( a ) ) { + throw new Error( + `Pattern ${ a } has a circular dependency and cannot be rendered.` + ); + } +} + +function hasCircularDependency( + id, + visitedNodes = new Set(), + currentPath = new Set() +) { + visitedNodes.add( id ); + currentPath.add( id ); + + const dependencies = deps.get( id ) ?? new Set(); + + for ( const dependency of dependencies ) { + if ( ! visitedNodes.has( dependency ) ) { + if ( + hasCircularDependency( dependency, visitedNodes, currentPath ) + ) { + return true; + } + } else if ( currentPath.has( dependency ) ) { + return true; + } + } + + // Remove the current node from the current path when backtracking + currentPath.delete( id ); + return false; +} From 2b7a974a41e5e44290e56053b3f21b812e710e89 Mon Sep 17 00:00:00 2001 From: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Date: Fri, 24 Nov 2023 15:16:26 +0000 Subject: [PATCH 04/12] Improve naming of variables --- .../src/pattern/recursion-detector.js | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/block-library/src/pattern/recursion-detector.js b/packages/block-library/src/pattern/recursion-detector.js index a6d400ecbeead6..a295b17ed45c41 100644 --- a/packages/block-library/src/pattern/recursion-detector.js +++ b/packages/block-library/src/pattern/recursion-detector.js @@ -1,7 +1,7 @@ /** * @type {Map>} */ -const deps = new Map(); +const patternDependencies = new Map(); /** * Parse a given pattern and traverse its contents to detect any subsequent @@ -38,27 +38,27 @@ export function parsePatternDependencies( { name, blocks } ) { * @throws {Error} If a circular dependency is detected. */ function dependsOn( a, b ) { - if ( ! deps.has( a ) ) { - deps.set( a, new Set() ); + if ( ! patternDependencies.has( a ) ) { + patternDependencies.set( a, new Set() ); } - deps.get( a ).add( b ); + patternDependencies.get( a ).add( b ); if ( hasCircularDependency( a ) ) { - throw new Error( + throw new TypeError( `Pattern ${ a } has a circular dependency and cannot be rendered.` ); } } function hasCircularDependency( - id, + slug, visitedNodes = new Set(), currentPath = new Set() ) { - visitedNodes.add( id ); - currentPath.add( id ); + visitedNodes.add( slug ); + currentPath.add( slug ); - const dependencies = deps.get( id ) ?? new Set(); + const dependencies = patternDependencies.get( slug ) ?? new Set(); for ( const dependency of dependencies ) { if ( ! visitedNodes.has( dependency ) ) { @@ -73,6 +73,6 @@ function hasCircularDependency( } // Remove the current node from the current path when backtracking - currentPath.delete( id ); + currentPath.delete( slug ); return false; } From 109fcd80237cfd3be4ae5c0f613c087080d39744 Mon Sep 17 00:00:00 2001 From: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Date: Fri, 24 Nov 2023 16:01:27 +0000 Subject: [PATCH 05/12] (m) naming --- packages/block-library/src/pattern/recursion-detector.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/block-library/src/pattern/recursion-detector.js b/packages/block-library/src/pattern/recursion-detector.js index a295b17ed45c41..020c22d24feea7 100644 --- a/packages/block-library/src/pattern/recursion-detector.js +++ b/packages/block-library/src/pattern/recursion-detector.js @@ -23,7 +23,7 @@ export function parsePatternDependencies( { name, blocks } ) { queue.unshift( innerBlock ); } if ( block.name === 'core/pattern' ) { - dependsOn( name, block.attributes.slug ); + registerDependency( name, block.attributes.slug ); } } } @@ -37,7 +37,7 @@ export function parsePatternDependencies( { name, blocks } ) { * * @throws {Error} If a circular dependency is detected. */ -function dependsOn( a, b ) { +function registerDependency( a, b ) { if ( ! patternDependencies.has( a ) ) { patternDependencies.set( a, new Set() ); } From 06edbd2a3c3bed118db37283a65056d0cbac65eb Mon Sep 17 00:00:00 2001 From: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Date: Fri, 24 Nov 2023 16:47:13 +0000 Subject: [PATCH 06/12] Patterns: prevent server-side infinite recursion --- packages/block-library/src/pattern/index.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/block-library/src/pattern/index.php b/packages/block-library/src/pattern/index.php index 436452f6853001..f8cfd41fee3762 100644 --- a/packages/block-library/src/pattern/index.php +++ b/packages/block-library/src/pattern/index.php @@ -27,6 +27,8 @@ function register_block_core_pattern() { * @return string Returns the output of the pattern. */ function render_block_core_pattern( $attributes ) { + static $seen_refs = array(); + if ( empty( $attributes['slug'] ) ) { return ''; } @@ -38,6 +40,17 @@ function render_block_core_pattern( $attributes ) { return ''; } + if ( isset( $seen_refs[ $attributes['slug'] ] ) ) { + // WP_DEBUG_DISPLAY must only be honored when WP_DEBUG. This precedent + // is set in `wp_debug_mode()`. + $is_debug = WP_DEBUG && WP_DEBUG_DISPLAY; + + return $is_debug ? + // translators: Visible only in the front end, this warning takes the place of a faulty block. + __( '[block rendering halted]' ) : + ''; + } + $pattern = $registry->get_registered( $slug ); $content = $pattern['content']; @@ -48,11 +61,14 @@ function render_block_core_pattern( $attributes ) { $content = gutenberg_serialize_blocks( $blocks ); } + $seen_refs[ $attributes['slug'] ] = true; + $content = do_blocks( $content ); global $wp_embed; $content = $wp_embed->autoembed( $content ); + unset( $seen_refs[ $attributes['slug'] ] ); return $content; } From 559623ebdd88af228badbbbf800fbe9bbb230053 Mon Sep 17 00:00:00 2001 From: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Date: Fri, 24 Nov 2023 17:05:23 +0000 Subject: [PATCH 07/12] Improve messaging by mentioning offending pattern --- packages/block-library/src/pattern/edit.js | 10 ++++++++-- packages/block-library/src/pattern/index.php | 4 ++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/block-library/src/pattern/edit.js b/packages/block-library/src/pattern/edit.js index b6efba4a8eb6eb..300549d27fc56d 100644 --- a/packages/block-library/src/pattern/edit.js +++ b/packages/block-library/src/pattern/edit.js @@ -10,7 +10,7 @@ import { useBlockProps, } from '@wordpress/block-editor'; import { store as coreStore } from '@wordpress/core-data'; -import { __ } from '@wordpress/i18n'; +import { __, sprintf } from '@wordpress/i18n'; /** * Internal dependencies @@ -123,7 +123,13 @@ const PatternEdit = ( { attributes, clientId } ) => { if ( hasRecursionError ) { return (
- { __( 'Recursion!' ) } + + { sprintf( + // translators: A warning in which %s is the name of a pattern. + __( 'Pattern "%s" cannot be rendered inside itself.' ), + selectedPattern?.name + ) } +
); } diff --git a/packages/block-library/src/pattern/index.php b/packages/block-library/src/pattern/index.php index f8cfd41fee3762..70c389e4ec8dbe 100644 --- a/packages/block-library/src/pattern/index.php +++ b/packages/block-library/src/pattern/index.php @@ -46,8 +46,8 @@ function render_block_core_pattern( $attributes ) { $is_debug = WP_DEBUG && WP_DEBUG_DISPLAY; return $is_debug ? - // translators: Visible only in the front end, this warning takes the place of a faulty block. - __( '[block rendering halted]' ) : + // translators: Visible only in the front end, this warning takes the place of a faulty block. %s represents a pattern's slug. + sprintf( __( '[block rendering halted for pattern "%s"]' ), $slug ) : ''; } From ca82ca5bf3c8127df869583001d97f9886b81df5 Mon Sep 17 00:00:00 2001 From: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Date: Mon, 27 Nov 2023 15:43:32 +0000 Subject: [PATCH 08/12] Add tests for cycle detection --- .../src/pattern/recursion-detector.js | 33 ++++++--- .../block-library/src/pattern/test/index.js | 73 +++++++++++++++++++ 2 files changed, 96 insertions(+), 10 deletions(-) create mode 100644 packages/block-library/src/pattern/test/index.js diff --git a/packages/block-library/src/pattern/recursion-detector.js b/packages/block-library/src/pattern/recursion-detector.js index 020c22d24feea7..70460e6e206d8b 100644 --- a/packages/block-library/src/pattern/recursion-detector.js +++ b/packages/block-library/src/pattern/recursion-detector.js @@ -32,29 +32,37 @@ export function parsePatternDependencies( { name, blocks } ) { * Declare that pattern `a` depends on pattern `b`. If a circular * dependency is detected, an error will be thrown. * + * Exported for testing purposes only. + * * @param {string} a Slug for pattern A. * @param {string} b Slug for pattern B. * * @throws {Error} If a circular dependency is detected. */ -function registerDependency( a, b ) { +export function registerDependency( a, b ) { if ( ! patternDependencies.has( a ) ) { patternDependencies.set( a, new Set() ); } patternDependencies.get( a ).add( b ); - if ( hasCircularDependency( a ) ) { + if ( hasCycle( a ) ) { throw new TypeError( `Pattern ${ a } has a circular dependency and cannot be rendered.` ); } } -function hasCircularDependency( - slug, - visitedNodes = new Set(), - currentPath = new Set() -) { +/** + * Determine if a given pattern has circular dependencies on other patterns. + * This will be determined by running a depth-first search on the current state + * of the graph represented by `patternDependencies`. + * + * @param {string} slug Pattern slug. + * @param {Set} [visitedNodes] Set to track visited nodes in the graph. + * @param {Set} [currentPath] Set to track and backtrack graph paths. + * @return {boolean} Whether any cycle was found. + */ +function hasCycle( slug, visitedNodes = new Set(), currentPath = new Set() ) { visitedNodes.add( slug ); currentPath.add( slug ); @@ -62,9 +70,7 @@ function hasCircularDependency( for ( const dependency of dependencies ) { if ( ! visitedNodes.has( dependency ) ) { - if ( - hasCircularDependency( dependency, visitedNodes, currentPath ) - ) { + if ( hasCycle( dependency, visitedNodes, currentPath ) ) { return true; } } else if ( currentPath.has( dependency ) ) { @@ -76,3 +82,10 @@ function hasCircularDependency( currentPath.delete( slug ); return false; } + +/** + * Exported for testing purposes only. + */ +export function clearPatternDependencies() { + patternDependencies.clear(); +} diff --git a/packages/block-library/src/pattern/test/index.js b/packages/block-library/src/pattern/test/index.js new file mode 100644 index 00000000000000..9ac2156e76d54c --- /dev/null +++ b/packages/block-library/src/pattern/test/index.js @@ -0,0 +1,73 @@ +/** + * Internal dependencies + */ +import { + parsePatternDependencies, + registerDependency, + clearPatternDependencies, +} from '../recursion-detector'; + +describe( 'core/pattern', () => { + beforeEach( () => { + clearPatternDependencies(); + } ); + + describe( 'parsePatternDependencies', () => { + it( "is silent for patterns that don't require other patterns", () => { + const pattern = { + name: 'test/benign-pattern', + blocks: [ { name: 'core/paragraph' } ], + }; + expect( () => { + parsePatternDependencies( pattern ); + } ).not.toThrow(); + } ); + it( 'catches self-referencing patterns', () => { + const pattern = { + name: 'test/evil-pattern', + blocks: [ { name: 'core/pattern', slug: 'test/evil-pattern' } ], + }; + expect( () => { + parsePatternDependencies( pattern ); + } ).toThrow(); + } ); + } ); + + describe( 'registerDependency', () => { + it( 'is silent for patterns with no circular dependencies', () => { + expect( () => { + registerDependency( 'a', 'b' ); + } ).not.toThrow(); + } ); + it( 'catches self-referencing patterns', () => { + expect( () => { + registerDependency( 'a', 'a' ); + } ).toThrow(); + } ); + it( 'catches mutually-referencing patterns', () => { + registerDependency( 'a', 'b' ); + expect( () => { + registerDependency( 'b', 'a' ); + } ).toThrow(); + } ); + it( 'catches longer cycles', () => { + registerDependency( 'a', 'b' ); + registerDependency( 'b', 'c' ); + registerDependency( 'b', 'd' ); + expect( () => { + registerDependency( 'd', 'a' ); + } ).toThrow(); + } ); + it( 'catches any pattern depending on a tainted one', () => { + registerDependency( 'a', 'b' ); + registerDependency( 'b', 'c' ); + registerDependency( 'b', 'd' ); + expect( () => { + registerDependency( 'd', 'a' ); + } ).toThrow(); + expect( () => { + registerDependency( 'e', 'd' ); + } ).toThrow(); + } ); + } ); +} ); From 16a8feccab0fb789e05afc8302490289d842fc4a Mon Sep 17 00:00:00 2001 From: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Date: Mon, 27 Nov 2023 15:53:56 +0000 Subject: [PATCH 09/12] Add disclaimer about detection method --- .../block-library/src/pattern/recursion-detector.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/block-library/src/pattern/recursion-detector.js b/packages/block-library/src/pattern/recursion-detector.js index 70460e6e206d8b..7c2cc6bc990e29 100644 --- a/packages/block-library/src/pattern/recursion-detector.js +++ b/packages/block-library/src/pattern/recursion-detector.js @@ -1,3 +1,15 @@ +/** + * THIS MODULE IS INTENTIONALLY KEPT WITHIN THE PATTERN BLOCK'S SOURCE. + * + * This is because this approach for preventing infinite loops due to + * recursively rendering blocks is specific to the way that the `core/pattern` + * block behaves in the editor. Any other block types that deal with recursion + * SHOULD USE THE STANDARD METHOD for avoiding loops: + * + * @see https://github.com/WordPress/gutenberg/pull/31455 + * @see packages/block-editor/src/components/recursion-provider/README.md + */ + /** * @type {Map>} */ From 940e68b2daa1bbe22cb30d6c65e7169dea3f7a84 Mon Sep 17 00:00:00 2001 From: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Date: Thu, 14 Dec 2023 10:17:05 +0000 Subject: [PATCH 10/12] Add usePatternRecursionDetector with registry-based WeakMap --- packages/block-library/src/pattern/edit.js | 3 +- .../src/pattern/recursion-detector.js | 69 ++++++++++++------- .../block-library/src/pattern/test/index.js | 35 +++++----- 3 files changed, 63 insertions(+), 44 deletions(-) diff --git a/packages/block-library/src/pattern/edit.js b/packages/block-library/src/pattern/edit.js index 300549d27fc56d..165bd60ef2fa25 100644 --- a/packages/block-library/src/pattern/edit.js +++ b/packages/block-library/src/pattern/edit.js @@ -15,7 +15,7 @@ import { __, sprintf } from '@wordpress/i18n'; /** * Internal dependencies */ -import { parsePatternDependencies } from './recursion-detector'; +import { usePatternRecursionDetector } from './recursion-detector'; const PatternEdit = ( { attributes, clientId } ) => { const selectedPattern = useSelect( @@ -40,6 +40,7 @@ const PatternEdit = ( { attributes, clientId } ) => { useSelect( blockEditorStore ); const [ hasRecursionError, setHasRecursionError ] = useState( false ); + const parsePatternDependencies = usePatternRecursionDetector(); // Duplicated in packages/edit-site/src/components/start-template-options/index.js. function injectThemeAttributeInBlockTemplateContent( block ) { diff --git a/packages/block-library/src/pattern/recursion-detector.js b/packages/block-library/src/pattern/recursion-detector.js index 7c2cc6bc990e29..a390ea98f41405 100644 --- a/packages/block-library/src/pattern/recursion-detector.js +++ b/packages/block-library/src/pattern/recursion-detector.js @@ -11,9 +11,11 @@ */ /** - * @type {Map>} + * WordPress dependencies */ -const patternDependencies = new Map(); +import { useRegistry } from '@wordpress/data'; + +const patternDependenciesRegistry = new WeakMap(); /** * Parse a given pattern and traverse its contents to detect any subsequent @@ -21,13 +23,16 @@ const patternDependencies = new Map(); * internal dependency graph. If a circular dependency is detected, an * error will be thrown. * - * @param {Object} pattern Pattern. - * @param {string} pattern.name Pattern name. - * @param {Array} pattern.blocks Pattern's block list. + * Exported for testing purposes only. + * + * @param {Map>} deps Map of pattern dependencies. + * @param {Object} pattern Pattern. + * @param {string} pattern.name Pattern name. + * @param {Array} pattern.blocks Pattern's block list. * * @throws {Error} If a circular dependency is detected. */ -export function parsePatternDependencies( { name, blocks } ) { +export function parsePatternDependencies( deps, { name, blocks } ) { const queue = [ ...blocks ]; while ( queue.length ) { const block = queue.shift(); @@ -35,7 +40,7 @@ export function parsePatternDependencies( { name, blocks } ) { queue.unshift( innerBlock ); } if ( block.name === 'core/pattern' ) { - registerDependency( name, block.attributes.slug ); + registerDependency( deps, name, block.attributes.slug ); } } } @@ -46,18 +51,19 @@ export function parsePatternDependencies( { name, blocks } ) { * * Exported for testing purposes only. * - * @param {string} a Slug for pattern A. - * @param {string} b Slug for pattern B. + * @param {Map>} deps Map of pattern dependencies. + * @param {string} a Slug for pattern A. + * @param {string} b Slug for pattern B. * * @throws {Error} If a circular dependency is detected. */ -export function registerDependency( a, b ) { - if ( ! patternDependencies.has( a ) ) { - patternDependencies.set( a, new Set() ); +export function registerDependency( deps, a, b ) { + if ( ! deps.has( a ) ) { + deps.set( a, new Set() ); } - patternDependencies.get( a ).add( b ); + deps.get( a ).add( b ); - if ( hasCycle( a ) ) { + if ( hasCycle( deps, a ) ) { throw new TypeError( `Pattern ${ a } has a circular dependency and cannot be rendered.` ); @@ -69,20 +75,26 @@ export function registerDependency( a, b ) { * This will be determined by running a depth-first search on the current state * of the graph represented by `patternDependencies`. * - * @param {string} slug Pattern slug. - * @param {Set} [visitedNodes] Set to track visited nodes in the graph. - * @param {Set} [currentPath] Set to track and backtrack graph paths. - * @return {boolean} Whether any cycle was found. + * @param {Map>} deps Map of pattern dependencies. + * @param {string} slug Pattern slug. + * @param {Set} [visitedNodes] Set to track visited nodes in the graph. + * @param {Set} [currentPath] Set to track and backtrack graph paths. + * @return {boolean} Whether any cycle was found. */ -function hasCycle( slug, visitedNodes = new Set(), currentPath = new Set() ) { +function hasCycle( + deps, + slug, + visitedNodes = new Set(), + currentPath = new Set() +) { visitedNodes.add( slug ); currentPath.add( slug ); - const dependencies = patternDependencies.get( slug ) ?? new Set(); + const dependencies = deps.get( slug ) ?? new Set(); for ( const dependency of dependencies ) { if ( ! visitedNodes.has( dependency ) ) { - if ( hasCycle( dependency, visitedNodes, currentPath ) ) { + if ( hasCycle( deps, dependency, visitedNodes, currentPath ) ) { return true; } } else if ( currentPath.has( dependency ) ) { @@ -95,9 +107,14 @@ function hasCycle( slug, visitedNodes = new Set(), currentPath = new Set() ) { return false; } -/** - * Exported for testing purposes only. - */ -export function clearPatternDependencies() { - patternDependencies.clear(); +export function usePatternRecursionDetector() { + const registry = useRegistry(); + + if ( ! patternDependenciesRegistry.has( registry ) ) { + patternDependenciesRegistry.set( + registry, + parsePatternDependencies.bind( null, new Map() ) + ); + } + return patternDependenciesRegistry.get( registry ); } diff --git a/packages/block-library/src/pattern/test/index.js b/packages/block-library/src/pattern/test/index.js index 9ac2156e76d54c..0f682f6cce67b9 100644 --- a/packages/block-library/src/pattern/test/index.js +++ b/packages/block-library/src/pattern/test/index.js @@ -4,12 +4,13 @@ import { parsePatternDependencies, registerDependency, - clearPatternDependencies, } from '../recursion-detector'; describe( 'core/pattern', () => { + const deps = new Map(); + beforeEach( () => { - clearPatternDependencies(); + deps.clear(); } ); describe( 'parsePatternDependencies', () => { @@ -19,7 +20,7 @@ describe( 'core/pattern', () => { blocks: [ { name: 'core/paragraph' } ], }; expect( () => { - parsePatternDependencies( pattern ); + parsePatternDependencies( deps, pattern ); } ).not.toThrow(); } ); it( 'catches self-referencing patterns', () => { @@ -28,7 +29,7 @@ describe( 'core/pattern', () => { blocks: [ { name: 'core/pattern', slug: 'test/evil-pattern' } ], }; expect( () => { - parsePatternDependencies( pattern ); + parsePatternDependencies( deps, pattern ); } ).toThrow(); } ); } ); @@ -36,37 +37,37 @@ describe( 'core/pattern', () => { describe( 'registerDependency', () => { it( 'is silent for patterns with no circular dependencies', () => { expect( () => { - registerDependency( 'a', 'b' ); + registerDependency( deps, 'a', 'b' ); } ).not.toThrow(); } ); it( 'catches self-referencing patterns', () => { expect( () => { - registerDependency( 'a', 'a' ); + registerDependency( deps, 'a', 'a' ); } ).toThrow(); } ); it( 'catches mutually-referencing patterns', () => { - registerDependency( 'a', 'b' ); + registerDependency( deps, 'a', 'b' ); expect( () => { - registerDependency( 'b', 'a' ); + registerDependency( deps, 'b', 'a' ); } ).toThrow(); } ); it( 'catches longer cycles', () => { - registerDependency( 'a', 'b' ); - registerDependency( 'b', 'c' ); - registerDependency( 'b', 'd' ); + registerDependency( deps, 'a', 'b' ); + registerDependency( deps, 'b', 'c' ); + registerDependency( deps, 'b', 'd' ); expect( () => { - registerDependency( 'd', 'a' ); + registerDependency( deps, 'd', 'a' ); } ).toThrow(); } ); it( 'catches any pattern depending on a tainted one', () => { - registerDependency( 'a', 'b' ); - registerDependency( 'b', 'c' ); - registerDependency( 'b', 'd' ); + registerDependency( deps, 'a', 'b' ); + registerDependency( deps, 'b', 'c' ); + registerDependency( deps, 'b', 'd' ); expect( () => { - registerDependency( 'd', 'a' ); + registerDependency( deps, 'd', 'a' ); } ).toThrow(); expect( () => { - registerDependency( 'e', 'd' ); + registerDependency( deps, 'e', 'd' ); } ).toThrow(); } ); } ); From 18c8edcadee35425d07b2cc27defc9d8b82caf2c Mon Sep 17 00:00:00 2001 From: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Date: Thu, 14 Dec 2023 12:49:33 +0000 Subject: [PATCH 11/12] Update cache name, add comments --- packages/block-library/src/pattern/edit.js | 4 +- .../src/pattern/recursion-detector.js | 57 +++++++++++++------ 2 files changed, 43 insertions(+), 18 deletions(-) diff --git a/packages/block-library/src/pattern/edit.js b/packages/block-library/src/pattern/edit.js index 165bd60ef2fa25..7befaa9cb2dee6 100644 --- a/packages/block-library/src/pattern/edit.js +++ b/packages/block-library/src/pattern/edit.js @@ -15,7 +15,7 @@ import { __, sprintf } from '@wordpress/i18n'; /** * Internal dependencies */ -import { usePatternRecursionDetector } from './recursion-detector'; +import { useParsePatternDependencies } from './recursion-detector'; const PatternEdit = ( { attributes, clientId } ) => { const selectedPattern = useSelect( @@ -40,7 +40,7 @@ const PatternEdit = ( { attributes, clientId } ) => { useSelect( blockEditorStore ); const [ hasRecursionError, setHasRecursionError ] = useState( false ); - const parsePatternDependencies = usePatternRecursionDetector(); + const parsePatternDependencies = useParsePatternDependencies(); // Duplicated in packages/edit-site/src/components/start-template-options/index.js. function injectThemeAttributeInBlockTemplateContent( block ) { diff --git a/packages/block-library/src/pattern/recursion-detector.js b/packages/block-library/src/pattern/recursion-detector.js index a390ea98f41405..f2e80087dd4b07 100644 --- a/packages/block-library/src/pattern/recursion-detector.js +++ b/packages/block-library/src/pattern/recursion-detector.js @@ -15,7 +15,45 @@ */ import { useRegistry } from '@wordpress/data'; -const patternDependenciesRegistry = new WeakMap(); +/** + * Naming is hard. + * + * @see useParsePatternDependencies + * + * @type {WeakMap} + */ +const cachedParsers = new WeakMap(); + +/** + * Hook used by PatternEdit to parse block patterns. It returns a function that + * takes a pattern and returns nothing but throws an error if the pattern is + * recursive. + * + * @example + * ```js + * const parsePatternDependencies = useParsePatternDependencies(); + * parsePatternDependencies( selectedPattern ); + * ``` + * + * @see parsePatternDependencies + * + * @return {Function} A function to parse block patterns. + */ +export function useParsePatternDependencies() { + const registry = useRegistry(); + + // Instead of caching maps, go straight to the point and cache bound + // functions. Each of those functions is bound to a different Map that will + // keep track of patterns in the context of the given registry. + if ( ! cachedParsers.has( registry ) ) { + const deps = new Map(); + cachedParsers.set( + registry, + parsePatternDependencies.bind( null, deps ) + ); + } + return cachedParsers.get( registry ); +} /** * Parse a given pattern and traverse its contents to detect any subsequent @@ -23,7 +61,7 @@ const patternDependenciesRegistry = new WeakMap(); * internal dependency graph. If a circular dependency is detected, an * error will be thrown. * - * Exported for testing purposes only. + * EXPORTED FOR TESTING PURPOSES ONLY. * * @param {Map>} deps Map of pattern dependencies. * @param {Object} pattern Pattern. @@ -49,7 +87,7 @@ export function parsePatternDependencies( deps, { name, blocks } ) { * Declare that pattern `a` depends on pattern `b`. If a circular * dependency is detected, an error will be thrown. * - * Exported for testing purposes only. + * EXPORTED FOR TESTING PURPOSES ONLY. * * @param {Map>} deps Map of pattern dependencies. * @param {string} a Slug for pattern A. @@ -62,7 +100,6 @@ export function registerDependency( deps, a, b ) { deps.set( a, new Set() ); } deps.get( a ).add( b ); - if ( hasCycle( deps, a ) ) { throw new TypeError( `Pattern ${ a } has a circular dependency and cannot be rendered.` @@ -106,15 +143,3 @@ function hasCycle( currentPath.delete( slug ); return false; } - -export function usePatternRecursionDetector() { - const registry = useRegistry(); - - if ( ! patternDependenciesRegistry.has( registry ) ) { - patternDependenciesRegistry.set( - registry, - parsePatternDependencies.bind( null, new Map() ) - ); - } - return patternDependenciesRegistry.get( registry ); -} From d09b7c9ccd5fd70b42147837a5b0f98751aecbcf Mon Sep 17 00:00:00 2001 From: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Date: Fri, 15 Dec 2023 16:37:18 +0000 Subject: [PATCH 12/12] Add E2E guarding against pattern recursion --- .../e2e-tests/plugins/pattern-recursion.php | 22 ++++++++++++ .../editor/plugins/pattern-recursion.spec.js | 35 +++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 packages/e2e-tests/plugins/pattern-recursion.php create mode 100644 test/e2e/specs/editor/plugins/pattern-recursion.spec.js diff --git a/packages/e2e-tests/plugins/pattern-recursion.php b/packages/e2e-tests/plugins/pattern-recursion.php new file mode 100644 index 00000000000000..eb3a2051551a26 --- /dev/null +++ b/packages/e2e-tests/plugins/pattern-recursion.php @@ -0,0 +1,22 @@ + 'Evil recursive', + 'description' => 'Evil recursive', + 'content' => '

Hello

', + ) + ); + } +); diff --git a/test/e2e/specs/editor/plugins/pattern-recursion.spec.js b/test/e2e/specs/editor/plugins/pattern-recursion.spec.js new file mode 100644 index 00000000000000..72498bcdf9914d --- /dev/null +++ b/test/e2e/specs/editor/plugins/pattern-recursion.spec.js @@ -0,0 +1,35 @@ +/** + * WordPress dependencies + */ +const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' ); + +test.describe( 'Preventing Pattern Recursion', () => { + test.beforeAll( async ( { requestUtils } ) => { + await requestUtils.activatePlugin( + 'gutenberg-test-protection-against-recursive-patterns' + ); + } ); + + test.beforeEach( async ( { admin } ) => { + await admin.createNewPost(); + } ); + + test.afterAll( async ( { requestUtils } ) => { + await requestUtils.deactivatePlugin( + 'gutenberg-test-protection-against-recursive-patterns' + ); + } ); + + test( 'prevents infinite loops due to recursive patterns', async ( { + editor, + } ) => { + await editor.insertBlock( { + name: 'core/pattern', + attributes: { slug: 'evil/recursive' }, + } ); + const warning = editor.canvas.getByText( + 'Pattern "evil/recursive" cannot be rendered inside itself' + ); + await expect( warning ).toBeVisible(); + } ); +} );