validate postId in the site-editor.php #7188

mi5t4n · 2024-08-13T15:28:39Z

Added validation for the postId parameter in site-editor.php to ensure that the post exists before proceeding. If the post ID is invalid or the post does not exist, the script will now terminate with an error message, preventing potential errors or unintended behavior.

Trac ticket: https://core.trac.wordpress.org/ticket/61796

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.

github-actions · 2024-08-13T15:28:59Z

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props mi5t4n, hellofromtonya, peterwilsoncc, costdev, davidbaumwald, benniledl.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-08-13T15:41:42Z

Test using WordPress Playground

The 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

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Added validation for the `postId` parameter in `site-editor.php` to ensure that the post exists before proceeding. If the post ID is invalid or the post does not exist, the script will now terminate with an error message, preventing potential errors or unintended behavior.

peterwilsoncc

I've added a couple of notes inline.

While testing I noticed that going to wp-admin/site-editor.php?postType=page&canvas=edit ignores the postType parameter and loads the Blog Home template.

It suggests that further validation of the user input via the URL is required

src/wp-admin/site-editor.php

dream-encode · 2024-08-14T01:01:48Z

Though it's probably beyond the scope of the original ticket, I see a lot of duplication of isset( $_GET['postType'] ) and sanitize_key( $_GET['postType'] ).

Wondering if we could have something like a default $current_post_type = false|null then check and sanitize it once for reuse in the rest of the file. Same could pro be done for $_GET['postId'] and $_GET['path']?

Could probably move the early bails a bit higher in the file as well, to save some unnecessary code running when it's not needed? Just based off a brief look, unless I am missing something, I don't see anything between lines 76 and 118 that is in any way dependent on any of the code running above.

benniledl · 2024-08-20T13:04:56Z

would be nice to have an error message for various post statuses as in #7215

Co-authored-by: Peter Wilson <[email protected]>

…quest. Co-authored-by: Peter Wilson <[email protected]>

…stType"].

…ary code does not run.

mi5t4n · 2024-08-23T17:06:34Z

@dream-encode @peterwilsoncc Thank you for the suggestions. I have made the changes.

peterwilsoncc

I've added a few notes inline for additional reuse of $post_type_param and to avoid a potential PHP error if the value is not set.

src/wp-admin/site-editor.php

Additional reuse of `$post_type_param` and to avoid a potential PHP error if the value is not set. Co-authored-by: Peter Wilson <[email protected]>

hellofromtonya · 2024-10-04T16:50:55Z

Before 6.6.0, it redirected to site-editor.php?path=%2Fpage&canvas=view. Granted that did not indicate the why.

Why wp_die()?

My concern is two-fold:

Before this bug: It redirected, but stayed in the Site Editor.
Will the user understand how to get back to the editor, i.e. get out of this error page?

I'm wondering:

Could a lighter approach might be a better user experience? "lighter" meaning -> retain the previous behavior to keep the user in the Site Editor, while also displaying a message to inform the user why their requested page did not show.

@dream-encode @peterwilsoncc What do you think?

peterwilsoncc · 2024-10-07T01:03:41Z

@hellofromtonya How about passing link_url and link_text to the function call to return to the site editor?

Untested:

wp_die(
   __( 'Invalid page ID.' ),
   '',
   array(
      'link_url' => admin_url( 'site-editor.php' ),
      'link_text' => 'Return to site editor'
   )
);

I think hitting these error messages will require some degree of messing around with the URLs so it's safe to assume a certain amount of advanced user knowledge.

costdev · 2024-10-08T06:32:47Z

src/wp-admin/site-editor.php

+			break;
+
+		case 'wp_template':
+			$block_template = get_block_template( $_GET['postId'] );


Noting that $_GET['postId'] isn't sanitized here or in the next case.

This has been fixed

hellofromtonya · 2024-10-08T18:16:04Z

@hellofromtonya How about passing link_url and link_text to the function call to return to the site editor?

Untested:
wp_die(
   __( 'Invalid page ID.' ),
   '',
   array(
      'link_url' => admin_url( 'site-editor.php' ),
      'link_text' => 'Return to site editor'
   )
);
I think hitting these error messages will require some degree of messing around with the URLs so it's safe to assume a certain amount of advanced user knowledge.

That works in my testing. It mitigates one of my concerns:

Will the user understand how to get back to the editor, i.e. get out of this error page?

Let's adding that to each of the new wp_die() instances.

hellofromtonya · 2024-10-08T18:17:48Z

src/wp-admin/site-editor.php

+			$post = get_post( (int) $_GET['postId'] );
+
+			if ( null === $post || 'page' !== get_post_type( $post ) ) {
+				wp_die( __( 'Invalid page ID.' ) );


To help with the user's experience, please add the following as suggested by @peterwilsoncc :

Suggested change

wp_die( __( 'Invalid page ID.' ) );

wp_die(

__( 'Invalid page ID.' ),

'',

array(

'link_url' => admin_url( 'site-editor.php' ),

'link_text' => 'Return to site editor',

)

);

Resolved ff15ab1

hellofromtonya · 2024-10-08T18:19:06Z

src/wp-admin/site-editor.php

+			$post = get_post( (int) $_GET['postId'] );
+
+			if ( null === $post || 'wp_block' !== get_post_type( $post ) ) {
+				wp_die( __( 'Invalid pattern ID.' ) );


Same here, but maybe should link to the Patterns page.

Resolved 64e135c

hellofromtonya · 2024-10-08T18:19:17Z

src/wp-admin/site-editor.php

+			$block_template = get_block_template( $_GET['postId'] );
+
+			if ( null === $block_template ) {
+				wp_die( __( 'Invalid template ID.' ) );


Same here, but maybe should link to the Templates page.

Resolved 25ecd75

hellofromtonya · 2024-10-08T18:20:11Z

src/wp-admin/site-editor.php

+			$block_template = get_block_template( $_GET['postId'], 'wp_template_part' );
+
+			if ( null === $block_template ) {
+				wp_die( __( 'Invalid template part ID.' ) );


Same here, but maybe should link to the Templates page.

Resolved 39669ab

…ist.

…ot exist.

… not exist.

… part does not exist.

mi5t4n · 2024-11-25T16:49:25Z

@costdev @hellofromtonya Thanks a lot for the feedback. I have implemented the requested changes.

costdev · 2024-11-25T17:36:54Z

Hi @mi5t4n, thanks for the ping!

My contributions to WordPress Core are currently on hold. I am therefore unable to perform a follow-up review at this time.

Please drop a comment on the ticket to ask if other contributors are able to perform a follow-up review.

mi5t4n force-pushed the fix/61796 branch 2 times, most recently from 01c46f2 to d01e1e6 Compare August 13, 2024 17:08

mi5t4n force-pushed the fix/61796 branch from d01e1e6 to 4683dbe Compare August 13, 2024 17:21

peterwilsoncc reviewed Aug 14, 2024

View reviewed changes

src/wp-admin/site-editor.php Outdated Show resolved Hide resolved

src/wp-admin/site-editor.php Outdated Show resolved Hide resolved

src/wp-admin/site-editor.php Outdated Show resolved Hide resolved

src/wp-admin/site-editor.php Outdated Show resolved Hide resolved

mi5t4n and others added 6 commits August 23, 2024 22:28

Merge branch 'WordPress:trunk' into fix/61796

038bead

Ensure the post type is not wp_block

84c3c8a

Co-authored-by: Peter Wilson <[email protected]>

Ensure the post type is not page.

bb0bb39

Co-authored-by: Peter Wilson <[email protected]>

Add a check to ensure postType exists before proceeding with the re…

8fd4278

…quest. Co-authored-by: Peter Wilson <[email protected]>

Introduced $post_type_param variable with sanitize value of $_GET["po…

a7b6460

…stType"].

Bail early if postId and postType are not valid, so that the unnecess…

32b11c0

…ary code does not run.

removed the new line.

a0b8164

mi5t4n requested a review from peterwilsoncc August 23, 2024 17:15

peterwilsoncc reviewed Aug 24, 2024

View reviewed changes

src/wp-admin/site-editor.php Outdated Show resolved Hide resolved

src/wp-admin/site-editor.php Outdated Show resolved Hide resolved

src/wp-admin/site-editor.php Outdated Show resolved Hide resolved

src/wp-admin/site-editor.php Outdated Show resolved Hide resolved

Reuse and avoid a PHP error if the value is not set

fbcd239

Additional reuse of `$post_type_param` and to avoid a potential PHP error if the value is not set. Co-authored-by: Peter Wilson <[email protected]>

costdev reviewed Oct 8, 2024

View reviewed changes

hellofromtonya reviewed Oct 8, 2024

View reviewed changes

mi5t4n added 2 commits November 25, 2024 22:28

Added a link to the site editor in wp_die() when the page does not ex…

ff15ab1

…ist.

Added a link to the patterns page in wp_die() when the pattern does n…

64e135c

…ot exist.

mi5t4n added 2 commits November 25, 2024 22:29

Added a link to the templates page in wp_die() when the template does…

25ecd75

… not exist.

Added a link to the template parts page in wp_die() when the template…

39669ab

… part does not exist.

mi5t4n requested review from hellofromtonya and costdev November 25, 2024 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validate postId in the site-editor.php #7188

validate postId in the site-editor.php #7188

mi5t4n commented Aug 13, 2024

github-actions bot commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024

peterwilsoncc left a comment

dream-encode commented Aug 14, 2024 •

edited

Loading

benniledl commented Aug 20, 2024

mi5t4n commented Aug 23, 2024

peterwilsoncc left a comment

hellofromtonya commented Oct 4, 2024

peterwilsoncc commented Oct 7, 2024

costdev Oct 8, 2024

mi5t4n Nov 25, 2024

hellofromtonya commented Oct 8, 2024

hellofromtonya Oct 8, 2024

mi5t4n Nov 25, 2024

hellofromtonya Oct 8, 2024

mi5t4n Nov 25, 2024

hellofromtonya Oct 8, 2024

mi5t4n Nov 25, 2024

hellofromtonya Oct 8, 2024

mi5t4n Nov 25, 2024

mi5t4n commented Nov 25, 2024

costdev commented Nov 25, 2024

validate postId in the site-editor.php #7188

Are you sure you want to change the base?

validate postId in the site-editor.php #7188

Conversation

mi5t4n commented Aug 13, 2024

github-actions bot commented Aug 13, 2024 • edited Loading

github-actions bot commented Aug 13, 2024

Test using WordPress Playground

Some things to be aware of

peterwilsoncc left a comment

Choose a reason for hiding this comment

dream-encode commented Aug 14, 2024 • edited Loading

benniledl commented Aug 20, 2024

mi5t4n commented Aug 23, 2024

peterwilsoncc left a comment

Choose a reason for hiding this comment

hellofromtonya commented Oct 4, 2024

peterwilsoncc commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellofromtonya commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mi5t4n commented Nov 25, 2024

costdev commented Nov 25, 2024

github-actions bot commented Aug 13, 2024 •

edited

Loading

dream-encode commented Aug 14, 2024 •

edited

Loading