Trigger _doing_it_wrong for dangerous REST API endpoint option

[rmccue]

REST API endpoints can be registered with a permission_callback option. It's really easy to accidentally write permissions_callback instead, and tough to catch during peer review, making this a potential footgun.

We should trigger a _doing_it_wrong() for this.

[TimothyBlynJacobs]

Discussed in slack:

• ​https://wordpress.slack.com/archives/C02RQC26G/p1552590412266800
• ​https://wordpress.slack.com/archives/C02RQC26G/p1585669262124700

Examples in the wild:

• ​https://blog.sucuri.net/2019/03/vulnerability-disclosure-siteground-optimizer-caldera-forms.html
• ​https://www.wordfence.com/blog/2020/03/critical-vulnerabilities-affecting-over-200000-sites-patched-in-rank-math-seo-plugin/

One of the things we discussed is whether we should allow permission_callback => true as a shortcut for __return_true.

[rmccue]

Replying to TimothyBlynJacobs:

One of the things we discussed is whether we should allow permission_callback => true as a shortcut for __return_true.

IMO no, as that's a potential footgun too; if you accidentally cast to true or something crazy. I think it also complicates the API surface for no real gain, given __return_true already exists.

Note that I'm only suggesting the misspelling here, not the lack of permission_callback; while that would be OK, it's a bit of an API breakage, and would cause a lot of warnings.

[sorenbronsted]

I will work on this

Added a notice when permission_callback is misspelled

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

[TimothyBlynJacobs]

IMO no, as that's a potential footgun too; if you accidentally cast to true or something crazy. I think it also complicates the API surface for no real gain, given __return_true already exists.

Works for me.

Note that I'm only suggesting the misspelling here, not the lack of permission_callback; while that would be OK, it's a bit of an API breakage, and would cause a lot of warnings.

Ah, I misunderstood. Yeah I'm conflicted about it, I kinda hoped that the issue last year would be a "one and done" type of deal, but it has cropped up again. I'm not sure if it is worth the noise.

[prbot]

​TimothyBJacobs commented on ​PR #265:

Thanks for working on a patch @sorenbronsted! Left a couple of comments.

[TimothyBlynJacobs]

I think it makes the most sense to add the notice if the permission_callback is missing at all. We've seen multiple examples of that issue in the wild and it can be a pretty bad vulnerability when it happens.

Looking for permissions_callback specifically would work for one misspelling, but others are easy as well.

The notice would be annoying for people who are intentionally omitting a permission callback, but adding a __return_true is a quite simple fix.

Chatted about this with @SergeyBiryukov, we also have precedent for this kind of thing in Core with the notices added in map_meta_cap if the object's type isn't registered. That behavior isn't necessarily unsafe, but Core can't be sure, and so it is safer to add a notice.

[whyisjake]

Sounds like a good idea, I think we should get a dev note ASAP as this will start triggering a lot of notices...

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

[TimothyBlynJacobs]

Thanks for taking a look @whyisjake! I've added a patch implementing this. The error message renders like so:

The REST API route definition for <code>my-ns/my-route</code> is missing the required <code>permission_callback</code> argument. For REST API routes that are intended to be public, use <code>__return_true</code> as the permission callback.

[whyisjake]

Code looks good, if @SergeyBiryukov is good with the notices, works for me!

[TimothyBlynJacobs]

Got sign off from @SergeyBiryukov. Going to commit and add it to the overall dev note for the REST API.

[TimothyBlynJacobs]

In 48526:

REST API: Issue a _doing_it_wrong when registering a route without a permission callback.

The REST API treats routes without a permission_callback as public. Because this happens without any warning to the user, if the permission callback is unintentionally omitted or misspelled, the endpoint can end up being available to the public. Such a scenario has happened multiple times in the wild, and the results can be catostrophic when it occurs.

For REST API routes that are intended to be public, it is recommended to set the permission callback to the __return_true built in function.

Fixes #50075.
Props rmccue, sorenbronsted, whyisjake, SergeyBiryukov, TimothyBlynJacobs.

[sc0ttkclark]

Wondering in the interest of saving energy from all the error log storage that will quickly fill up :) should core pull back from the _doing_it_wrong() in WP 5.5 and instead do that in WP 5.6 with a strong dev note ahead of time to start updating code right away?

[TimothyBlynJacobs]

Wondering in the interest of saving energy from all the error log storage that will quickly fill up :) should core pull back from the _doing_it_wrong() in WP 5.5 and instead do that in WP 5.6 with a strong dev note ahead of time to start updating code right away?

That's a possibility. For me, I think the developers who read the dev notes will quickly update their plugins before WordPress 5.5 releases. For the ones that aren't, they won't know about it coming in 5.6. So pushing the notice to the next release doesn't really change things.

[TimothyBlynJacobs]

In 48571:

REST API: Remove textdomain from doing it wrong message.

See #50075.
Props dlh.

[TimothyBlynJacobs]

Covered in ​https://make.wordpress.org/core/2020/07/22/rest-api-changes-in-wordpress-5-5/