Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address the Downsides to Disabling the REST API by Default #50

Closed
mphillips opened this issue Oct 16, 2019 · 4 comments
Closed

Address the Downsides to Disabling the REST API by Default #50

mphillips opened this issue Oct 16, 2019 · 4 comments
Labels
enhancement New feature or request

Comments

@mphillips
Copy link

I think the feature to disable the REST API for non-authenticated users by default should be revisited. I've seen this create problems on multiple sites where this option was overlooked and it broke critical functionality that wasn't discovered until much later.

I see the following problems with this feature:

  • There is no warning that the REST API has been disabled when activating the plugin.
  • Engineers may not aware the REST API has been disabled until a problem is discovered.
  • You don't always know if third-party plugins use the REST API or if a plugin update will add the use of the API down the road. If you install a plugin six months after you've installed the 10up Experience plugin, you likely won't remember to go back and enable the endpoints. I realize testing could catch these issues, but it's still easy to miss.

I think I understand the purpose behind this feature, but the downsides should be addressed if this continues to be the default functionality.

Some suggestions:

  • Explain this feature clearly in this repo's README
  • Add a prominent warning when installing the plugin.
  • Only disable the REST endpoints that are included with WordPress (if this is possible).

I don't know all the details behind this decision, and there may be better ways to avoid these issues that I'm not aware of, so let me know if there's something I'm missing. I'm also happy to contribute the code changes if a decision is made.

Thanks!

@mphillips mphillips added the enhancement New feature or request label Oct 16, 2019
@eugene-manuilov
Copy link

Uber projects have being blown up by this "bomb" today... We definitely need to escalate this issue and revisit how the plugin works during the initial activation and initial protection.

An alternative solution might be a setup wizard which is activated when we install and activate the plugin for the first time. Something like woocommerce does when you activate it for the first time.

cc @tylercherpak

@tlovett1
Copy link
Member

It feels like the default should probably be Restrict access to the users endpoint to authenticated users along with a much better explanation in the README. What does everyone think about that?

@TheLastCicada
Copy link

The users endpoint is a problem and we do need to disable that for public consumption when we aren't using it. The problem is we build so many sites now that rely on the REST API that we will continue to cause outages on our client sites if we block any REST API end points by default and I just don't think engineers are going to read the documentation on this plugin before installing it. I would vote for an installation wizard, or, more simply, changing the behavior not to block anything but to show a warning in the wp-admin dashboard instead and require a box be checked in the settings to acknowledge that this endpoint is public on purpose. Now that we have the support monitor rolling out, we can potentially rely on that to provide visibility into which endpoints are public in a way we can take action on it.

@darylldoyle
Copy link
Contributor

This is an old issue, and I think at this point all 10up engineers are aware that this plugin disables access to the REST API unless you're authenticated. The addition of the tenup_experience_rest_api_allowlist filter in #168, means that it's now possible for engineers to allow specific routes if they do find it causes an issue.

Closing this and I'll open a new issue to add documentation around the new tenup_experience_rest_api_allowlist filter to the readme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants