WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 5 weeks ago

Last modified 5 weeks ago

#53915 closed enhancement (fixed)

Add show show_in_rest param to register_sidebar

Reported by: spacedmonkey Owned by: TimothyBlynJacobs
Milestone: 5.9 Priority: normal
Severity: normal Version: 5.8
Component: REST API Keywords: has-patch has-unit-tests
Focuses: rest-api Cc:

Description

Sidebars can be queries via the sidebar REST API, added in WordPress 5.8. However this data is not public, as requires the current user to be have rights to edit the widget to see the sidebar in the api. Sidebars are registered in PHP, so adding show_in_rest param, should be easy. This opt-in feature would make sidebar data, including embedded widgets, public, if the dev opts in.

Change History (27)

#1 @TimothyBlynJacobs
4 months ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

I think this makes a lot of sense to add. Feel free to milestone for 5.9.

This ticket was mentioned in PR #1578 on WordPress/wordpress-develop by spacedmonkey.


4 months ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

#3 @spacedmonkey
4 months ago

  • Milestone changed from Future Release to 5.9

Thanks @TimothyBlynJacobs

I have a PR at #1578.

It is a surprisingly simple change.

#4 @prbot
4 months ago

TimothyBJacobs commented on PR #1578:

Won't we need a change in the widgets controller to check if a widget is part of a public sidebar?

#5 @prbot
4 months ago

spacedmonkey commented on PR #1578:

@TimothyBJacobs Updated the PR with your feedback.

One question. Should we / can we change raw instance values to only display in edit context.

https://github.com/WordPress/wordpress-develop/blob/eb6b517aec3e04b1868a4ea4e4eb01a6a08a3751/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php#L783-L787

I am wonder about sensitive data leaking?

#6 @prbot
4 months ago

TimothyBJacobs commented on PR #1578:

Thanks. Looks like the test failure may be legit.

We would need to change the whole instance value to be only be exposed in an edit context, since the encoded form is just encoded, not hidden.

I think sensitive data leakage is definitely something we should be concerned about. While a theme author may opt-in to having their sidebar available, they might not know that a widget's setting contains an API key for instance.

However, I think there is definitely value in having the instance values available, not just the rendered widget, to allow for rendering the widget yourself.

I think for now we could omit the instance entirely, but it may make sense in the future to allow for show_instance_in_rest to be overloaded to a configuration array similarly to register_meta to allow for a widget author to selectively expose certain fields.

#7 @prbot
4 months ago

spacedmonkey commented on PR #1578:

Seems like the issue here was running retrieve_widgets twice, once in permission check and once in *_items function. I added a workaround for this, with some state is seems to work. I would love some feedback on managing state like this better. Thoughts @TimothyBJacobs

#8 @prbot
4 months ago

TimothyBJacobs commented on PR #1578:

Pinging @adamziel as he's been looking into retrieve_widgets issues.

#9 @prbot
3 months ago

spacedmonkey commented on PR #1578:

We would need to change the whole instance value to be only be exposed in an edit context, since the encoded form is just encoded, not hidden.

So should only instance be changed to 'context' => array( 'edit' ), or instance, instance.encoded, instance.hash and instance.raw be changed?

https://github.com/WordPress/wordpress-develop/blob/52fe9ffbd59955c30e1e91ab0ba0f04e72243d8d/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php#L767-L789

#10 @prbot
3 months ago

adamziel commented on PR #1578:

I added a workaround for this, with some state is seems to work. I would love some feedback on managing state like this better.

@spacedmonkey I think the mental model of `retrieve_widgets` is broken right now. The name is confusing, that function doesn't just retrieve stuff, it actually updates the database. It's like "switching themes breaks the stored data, so we need to fix it every time we use it". I would like to make it more "we never break the data in the first place" but that's a larger project. For now I just proposed a more accurate name and it has commit label so you might want to update this PR once that change is merged.

Since your issue seems to come from running that function twice and we've seen similar problems, I wonder if this fix might help you.

Other than that, I don't think that we have a good way around running that function multiple times like you do now. Well, maybe aside of maybe a hook that would run when a route is matched – that way we could have only a single call that would cover both the permissions check and the actual handler.

#11 @prbot
3 months ago

spacedmonkey commented on PR #1578:

@adamziel Do you think that https://github.com/WordPress/wordpress-develop/pull/1525 blocks this PR? Would it effect anything in your PR? Did you review this PR?

#12 @prbot
3 months ago

adamziel commented on PR #1578:

l Do you think that #1525 blocks this PR?

@spacedmonkey I wouldn't say it blocks, no. Maybe it would allow you to get rid of this check:

{{{php

if ( $this->widgets_retrieved ) {

return;

}

}}}

But I'm not 100% certain about that since I don't have a good grasp of what the problem was.

Would it effect anything in your PR?

I don't this PR would affect #1525 much – a rebase, if necessary, should be pretty easy. Feel free to move forward with this one once you have approvals.

Did you review this PR?

I read the code and it looks good, but I did not test much. I'm happy to help with testing once you confirm the code is where you want it to be, sync_registered_widgets or not.

Also, one more question – should this PR be moved to the Gutenberg repo so that 5.7+plugin may benefit from these changes? That's what we did with https://github.com/WordPress/gutenberg/pull/34230, it may or may not make sense here too

#13 @prbot
3 months ago

spacedmonkey commented on PR #1578:

Also, one more question – should this PR be moved to the Gutenberg repo so that 5.7+plugin may benefit from these changes? That's what we did with WordPress/gutenberg#34230, it may or may not make sense here as well

Yeah, it can be backported.

#14 @prbot
3 months ago

spacedmonkey commented on PR #1578:

I wouldn't say it blocks, no. Maybe it would allow you to get rid of this check:

So we should wait until your work is merged before this goes in?

#15 @prbot
3 months ago

adamziel commented on PR #1578:

If it takes a few days then it would be convenient – potentially one check less in your PR. If it takes more, it's probably not worth it – we could re-evaluate that check at some later time.

#16 @prbot
3 months ago

adamziel commented on PR #1578:

Yeah, it can be backported.

Why not Gutenberg first? It would be picked up during the next major WP version as a part of the regular workflow. See the discussion in https://github.com/WordPress/gutenberg/issues/33810

#17 @prbot
3 months ago

adamziel commented on PR #1578:

Yeah, it can be backported.

Why not Gutenberg first? It would be picked up during the next major WP version as a part of the regular workflow. See the discussion in https://github.com/WordPress/gutenberg/issues/33810

#18 @prbot
3 months ago

adamziel commented on PR #1578:

Yeah, it can be backported.

Why not Gutenberg-first? These "double" changes have been challenging lately, see the discussion in https://github.com/WordPress/gutenberg/issues/33810 and https://github.com/WordPress/gutenberg/pull/34536.

The sidebars endpoint is getting removed from the next Gutenberg version, so no problem there. It's all about having a consistent widgets endpoint now.

#19 @prbot
3 months ago

spacedmonkey commented on PR #1578:

Why not Gutenberg-first? These "double" changes have been challenging lately

This change doesn't make sense to backport. It has little use in the gutenberg plugin and it an opt in feature for devs. It is most useful for headless applications of WordPress.

#20 @prbot
3 months ago

adamziel commented on PR #1578:

This change doesn't make sense to backport. It has little use in the gutenberg plugin and it an opt in feature for devs. It is most useful for headless applications of WordPress.

I see, thank you for explaining! I am only concerned about merging changes later on. If we keep updating the same endpoints both in core and in the Gutenberg repo, we may end up with conflicting changes. On the other hand, I think I discussed that with @gziolo in https://github.com/WordPress/gutenberg/issues/33810 and the conclusion was that in practice this rarely becomes a problem.

#21 @prbot
3 months ago

spacedmonkey commented on PR #1578:

this rarely becomes a problem so maybe we're good here?

If there was any value in adding this to the gutenberg plugin, I would have done it both places. I will do that future PRs.

#22 @prbot
2 months ago

spacedmonkey commented on PR #1578:

Unit tests are passing!

This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.


8 weeks ago

#24 @prbot
6 weeks ago

adamziel commented on PR #1578:

PHPUnit failures seem to be just flaky CI. I think this PR is pretty close 👍 I left some last notes and proposed a few tests.

This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.


5 weeks ago

#26 @TimothyBlynJacobs
5 weeks ago

  • Owner set to TimothyBlynJacobs
  • Resolution set to fixed
  • Status changed from new to closed

In 52016:

REST API: Allow sidebars and their widgets to be public.

By default, only users with the edit_theme_options capability can access the sidebars and widgets REST API endpoints. In this commit, A new show_in_rest parameter is added to the register_sidebar function. When enabled, all users will be able to access that sidebar and any widgets belonging to that sidebar.

This commit reduces the context for a widget's instance information to only edit. This is to ensure that internal widget data is not inadvertently exposed to the public. A future ticket may expose additional APIs to allow widget authors to indicate that their instance data can be safely exposed. REST API consumers intending to access this instance information should take care to explicitly set the context parameter to edit.

Props spacedmonkey, zieladam.
Fixes #53915.

Note: See TracTickets for help on using tickets.