WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 2 months ago

Last modified 2 months ago

#50075 closed defect (bug) (fixed)

Trigger _doing_it_wrong for dangerous REST API endpoint option

Reported by: rmccue Owned by: TimothyBlynJacobs
Milestone: 5.5 Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: has-patch has-unit-tests commit has-dev-note
Focuses: Cc:

Description

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.

Change History (20)

#2 in reply to: ↑ 1 @rmccue
5 months ago

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.

#3 @sorenbronsted
5 months ago

I will work on this

This ticket was mentioned in PR #265 on WordPress/wordpress-develop by sorenbronsted.


5 months ago

Added a notice when permission_callback is misspelled

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

#5 @sorenbronsted
5 months ago

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

#6 @TimothyBlynJacobs
4 months ago

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.

#7 @prbot
4 months ago

TimothyBJacobs commented on PR #265:

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

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


3 months ago

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


2 months ago

#10 @TimothyBlynJacobs
2 months ago

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.

#11 @whyisjake
2 months ago

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

#13 @TimothyBlynJacobs
2 months ago

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.

#14 @whyisjake
2 months ago

  • Keywords needs-dev-note commit added

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

#15 @TimothyBlynJacobs
2 months ago

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

#16 @TimothyBlynJacobs
2 months ago

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

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.

#17 @sc0ttkclark
2 months ago

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?

#18 @TimothyBlynJacobs
2 months ago

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.

#19 @TimothyBlynJacobs
2 months ago

In 48571:

REST API: Remove textdomain from doing it wrong message.

See #50075.
Props dlh.

Note: See TracTickets for help on using tickets.