Please undo all the (incorrect) CS changes and only make the change you are actually proposing.

Also, I've had a quick look at the get_block_wrapper_attributes() function and the $key used is not escaped and as it is not a hard-coded value, this leaves the code wide-open to all sorts of injection, so until that function is actually made safe, I would not recommend adding it to the "safe" functions list. This should probably be flagged up for the @WordPress/security-team ...

https://developer.wordpress.org/reference/functions/get_block_wrapper_attributes/

@jrfnl Sorry, I'm finding my way around pull requests for the first time. I only wanted to edit one line, but the Basic QA checks failed.

@jrfnl Sorry, I'm finding my way around pull requests for the first time. I only wanted to edit one line, but the Basic QA checks failed.

That's fine. You can run composer check-cs on the command line to see what is being flagged and composer fix-cs should be able to auto-fix most issues.

For additional guidelines - like adding tests to cover your change - please have a look at the CONTRIBUTING docs.

Thanks for your patience, @jrfnl. I got there in the end though. 😅

@lewis-elborn No worries, but now (aside from the PR needing a test) the question remains whether the function should be added, see my remark about this above.

P.S.: thanks for persisting in trying to get the CI to pass ;-)

It looks like the function only supports a fixed set of attributes, see source code reference.

However, I think it would be good to modify WordPress to use esc_attr( $key ) for hardening purposes prior to adding the function to the safe list. Doing so will prevent issues in the event a bug is introduced at a latter date.

Does that work for you both?

It looks like the function only supports a fixed set of attributes, see source code reference.

@peterwilsoncc Except it doesn't - arbitrary attributes passed via the parameter which are not in that list are added without any validation: https://github.com/WordPress/wordpress-develop/blob/2bb5679d666474d024352fa53f07344affef7e69/src/wp-includes/class-wp-block-supports.php#L198-L202

I'll pass you a code sample in private (wouldn't want to give anyone any ideas).

However, I think it would be good to modify WordPress to use esc_attr( $key ) for hardening purposes prior to adding the function to the safe list. Doing so will prevent issues in the event a bug is introduced at a latter date.

I'd highly recommend improving the security. The security bug already exists, this is not a hypothetical future situation.

I wonder if this shouldn't use sanitize_key() instead of esc_attr() ?

arbitrary attributes passed via the parameter which are not in that list are added without any validation

Thanks @jrfnl, I misread that line of code.

As discussed elsewhere, I am not sure what is best to use for the escaping of attribute names. Reviewing the attribute spec, it looks like sanitize_title is closer than esc_attr. https://html.spec.whatwg.org/multipage/syntax.html#attributes-2

Just chiming in that we have a PR opened that adds whole lot of other safe functions to the list (#2082).

IMO this PR should be closed, and an issue opened instead.

There we can discuss the problem of whether the get_block_wrapper_attributes() is correctly escaped or not, and should be added to the list.

Closing this PR now as the function should not be on the safe list as-is.