Securing taxonomies export #872

tw2113 · 2022-06-10T23:42:27Z

No description provided.

tw2113 · 2022-06-10T23:51:10Z

Creating the PR for the sake of commenting.

One detail about the cptui_get_single_taxonomy_registery() function is that it echos by default. Perhaps a detail we should change with output buffering. As is though, line 211 isn't really assigning anything to the variable because it's not explicitly returning anything.

We're also never going to have a WP_Error since we don't throw any with the function either. In large part because it's just meant to display copy/paste ready versions of a given register_taxonomy()

For the show_graphql, sort, and queryvar parts, the disp_boolean() function is immediately casting them to string, which is what we're getting from the settings extraction anyway, so may as well not worry about type casting there.

tw2113 · 2022-06-10T23:58:08Z

Also, I'm pretty sure a lot of what this original issue stemmed from is when doing the actual registration which happens in the main custom-post-type-ui.php file. We suddenly have new labels or arguments that we are trying to set values for, but the "old" settings don't have saved indexes in their current get_option. What has typically helped in the past is "going and saving the post type/taxonomy" so that we at least update the "get_option" value to have empty, and at least existing, indexes. Those notices would tend to populate error logs because content types get dynamically registered on each page load.

That said, if this is attempting to do the "scanning" I mention in #780 then I'm still for moving forward here, but may need to ponder on some extra approaches.

emilse-webdev · 2022-06-13T13:25:21Z

Also, I'm pretty sure a lot of what this original issue stemmed from is when doing the actual registration which happens in the main custom-post-type-ui.php file. We suddenly have new labels or arguments that we are trying to set values for, but the "old" settings don't have saved indexes in their current get_option. What has typically helped in the past is "going and saving the post type/taxonomy" so that we at least update the "get_option" value to have empty, and at least existing, indexes. Those notices would tend to populate error logs because content types get dynamically registered on each page load.

That said, if this is attempting to do the "scanning" I mention in #780 then I'm still for moving forward here, but may need to ponder on some extra approaches.

Thanks! I will try to add it there too, I've added it in taxonomies from the error logs sent when the issue was reported, when I was able to replicate the error it gave error because the key was not found, that's why I check if the key exists and if not I "initialize it", I'll add to check all keys are available in the other processes. I'll add this too -> -at least update the "get_option" value to have empty, and at least existing, indexes-

emilse-webdev · 2022-06-14T13:15:19Z

All changes added and tested!

tw2113 · 2022-06-14T14:55:33Z

custom-post-type-ui.php

@@ -268,6 +268,23 @@ function cptui_create_custom_post_types() {
 		return;
 	}

+	function sample_admin_notice__warning() {


Let's move this function into the inc/utility.php file at the end of the file, and let's also rename it to cptui_undefined_index_notice.

tw2113 · 2022-06-14T14:57:15Z

custom-post-type-ui.php

+	function sample_admin_notice__warning() {
+		?>
+		<div class="notice notice-warning">
+			<p>There's some missing indexes. You might need to re-save.</p>


Need to get this passed through esc_html_e() with a textdomain of custom-post-type-ui.

Also lets have it say "Custom Post Type UI added new parameters. Please consider reviewing your post types or taxonomies and consider clicking to save them again."

tw2113 · 2022-06-14T14:58:03Z

custom-post-type-ui.php

+	if ( (is_array($cpts) && !empty($cpts)) &&
+		(is_array($cpts_override) && !empty($cpts_override)) &&
+		( array_diff( array_keys($cpts), array_keys($cpts_override) ) )) {
+		add_action( 'admin_notices', 'sample_admin_notice__warning' );


This line will need to be updated to match the function rename.

tw2113 · 2022-06-14T14:58:47Z

custom-post-type-ui.php

+	// Compare if they don't have any missing or difference in keys
+	if ( (is_array($cpts) && !empty($cpts)) &&
+		(is_array($cpts_override) && !empty($cpts_override)) &&
+		( array_diff( array_keys($cpts), array_keys($cpts_override) ) )) {


Nitpicky detail, but can we check on PHPCS for this section regarding whitespace around function calls and passed parameters?

tw2113 · 2022-06-14T15:03:42Z

inc/taxonomies.php

@@ -1503,6 +1503,20 @@ function cptui_update_taxonomy( $data = [] ) {
 		return 'error';
 	}

+	$must_have_keys = ['object_types', 'rewrite', 'rewrite_slug', 'name',


Can we turn this into a function similar to cptui_reserved_taxonomies() where it returns this array? No need to add an apply_filters() for it, just have it return the array.

Could either still assign to $must_have_keys or it could replace the variable and just be used as such:

foreach( my_function_name() as $key ) { ... }

The reason for this request is to make the array more usable in the future, if need arises.

tw2113 · 2022-06-14T15:05:38Z

inc/tools.php

+		}
+	}
+
+	if ( ! array_key_exists('labels', $taxonomy) ){


Another nitpick, but spacing around our functions and logic statements. Her and above into the foreach loop above around line 241.

I know Alok is also working on his own PHPCS review so best to not introduce new spots when able.

I'll run phpcs to check my added code

tw2113 · 2022-06-14T15:06:44Z

inc/tools.php

 ?>
 	];

-	<?php
-	$show_graphql = isset( $taxonomy['show_in_graphql'] ) ? (bool) $taxonomy['show_in_graphql'] : false;
-	?>


This looks like it was accidentally removed and we will want to have it in place still

no, it's this comment above: "For the show_graphql, sort, and queryvar parts, the disp_boolean() function is immediately casting them to string, which is what we're getting from the settings extraction anyway, so may as well not worry about type casting there." but I can leave it as it was,

tw2113 · 2022-06-14T15:07:13Z

inc/tools.php

@@ -349,7 +370,7 @@ function cptui_get_single_taxonomy_registery( $taxonomy = [] ) {
 		"rest_namespace" => "<?php echo $rest_namespace; ?>",
 		"show_in_quick_edit" => <?php echo $show_in_quick_edit; ?>,
 		"sort" => <?php echo disp_boolean( $taxonomy['sort'] ); ?>,
-<?php if ( $show_graphql ) : ?>


Same part as restoring the isset for this above.

tw2113 · 2022-06-14T15:07:43Z

inc/tools.php

+	'public', 'publicly_queryable', 'show_ui', 'show_in_nav_menus',
+	'delete_with_user', 'show_in_rest', 'has_archive', 'exclude_from_search',
+	'hierarchical', 'can_export', 'rewrite', 'rewrite_withfront', 'query_var',
+	'show_in_menu'];


Same request as for the taxonomy reusable function.

tw2113 · 2022-06-14T15:07:56Z

inc/tools.php

+		if ( array_key_exists($key, $post_type) ){
+			$post_type[$key] = '';
+		}
+	}


Same nitpick around spacing.

I'll run phpcs

emilse-webdev added 4 commits June 10, 2022 18:30

Validation if array indexes exist and if return object is not wp_error

691ca0c

Cleaning dev comments

838c79b

Adding another validation

7eaa63c

Cleaning dev comments

eb38c29

tw2113 assigned emilse-webdev Jun 10, 2022

changing a little detail

894af74

emilse-webdev added 3 commits June 13, 2022 14:17

Update

f0f9202

Still adding changes

b03c440

Update

f8d6e5c

tw2113 commented Jun 14, 2022

View reviewed changes

emilse-webdev added 3 commits June 14, 2022 16:30

changes

d55ea64

lint

7126c31

Issue 869

f6caf8e

tw2113 closed this Aug 2, 2022

tw2113 deleted the securing_taxonomies_export branch December 16, 2022 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Securing taxonomies export #872

Securing taxonomies export #872

tw2113 commented Jun 10, 2022

tw2113 commented Jun 10, 2022

tw2113 commented Jun 10, 2022

emilse-webdev commented Jun 13, 2022

emilse-webdev commented Jun 14, 2022

tw2113 Jun 14, 2022

tw2113 Jun 14, 2022

tw2113 Jun 14, 2022

tw2113 Jun 14, 2022

tw2113 Jun 14, 2022

tw2113 Jun 14, 2022

emilse-webdev Jun 14, 2022

tw2113 Jun 14, 2022

emilse-webdev Jun 14, 2022

tw2113 Jun 14, 2022

tw2113 Jun 14, 2022

tw2113 Jun 14, 2022

emilse-webdev Jun 14, 2022

Securing taxonomies export #872

Securing taxonomies export #872

Conversation

tw2113 commented Jun 10, 2022

tw2113 commented Jun 10, 2022

tw2113 commented Jun 10, 2022

emilse-webdev commented Jun 13, 2022

emilse-webdev commented Jun 14, 2022

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

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