Refs: dev/core#3671 Fix regression involving CiviCRM Webform + Cases.

[stesi561]

Ignores permissions on api4 call in Recent::getTitle()

Overview

Addresses regression identified by colemanw/webform_civicrm#756

Before

Permissions check on api4 call when getting the default title.

After

Permissions ignored on api4 call when getting the default title.

Comments

Please review cautiously - I've not looked at ramifications of removing this permissions check on calls from elsewhere - merely followed the breadcrumbs from -> colemanw/webform_civicrm#756

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[stesi561]

In addition to above. Had some more time to look at this. The permissions loosening is on a private method that is only called by the create method of the class - and this class isn't extended anywhere.

[eileenmcnaughton]

@totten @colemanw I think we should plan to put this out in a point release this week

[demeritcowboy]

I'm thinking overall this should be merged, but have put some followup thoughts in a new ticket in https://lab.civicrm.org/dev/core/-/issues/3677

[totten]

Hmm, unfortunately, I believe this has created a security regression.

Steps to reproduce:

1. Login as a limited user (eg advisor on dmaster; has permission access CiviCRM but is limited by ACL-groups)
2. Determine a contact ID that is not visible to the limited user (eg I chose cid 173 for contact named "Global Empowerment Association")
3. Open JS console and run:

   CRM.api4('RecentItem', 'create', {
     values: {"entity_type":"Contact", "entity_id":173}
   })

4. Reload the page - look at the list of recent items. If you see #173 and their name, then you've learned new information about a restricted record.

On 5.50.3 and 5.51-rc (pre-23836), this sequence has a secure outcome -- step 3 raises an error, and step 4 does not reveal any information about contact 173. But with the patch, the user advisor can see restricted records like 173. They can just loop through and get all the names.

The patch here would have made sense in 5.48 -- before Civi\Api4\RecentItem was made available via AJAX. If RecentItem is fully trusting on both writes and reads, then it can be a vector getting extra info. There needs to be restriction somewhere.

I think the basic question is this: if you use webform (etal) to access a record with elevated privileges (beyond your normal RBAC privileges), should that item be added to "Recent Items"? Consider:

• "Yes" - You viewed an item. It was recent. You should put it on "Recent Items".
• "No" - The "Recent Items" are things that you can go back and see. You can't necessarily go back to a webform. The link sends to the wrong place - somewhere you probably can't even see.

Or even more basically... Does anybody really want webform-views to register as normal views on the "Recent Items"?

[eileenmcnaughton]

I think the creation of recentItems is a default behaviour - A judicious try-catch might be the right fix

[demeritcowboy]

While looking at this I noticed there exists a skipRecentView param for other elements, but it seemed awkward and almost a one-off, but maybe is one approach.

[totten]

Grepping for calls to Recent::add, it appears to be a mix - some low-ish level calls (eg CRM_Contact_BAO_Group::create calls CRM_Utils_Recent::add()) and some higher-level form-based calls (eg CRM_Contact_Page_View). It's probably the BAO-based calls that muck up the webform-elevated-pageviews.

If we don't actually want webform-elevated-pageviews to register as "Recent Items", then perhaps CRM_Utils_Recent::add() should have an additional permission-check? (Only add this item to the list if we're really sure that the current user has standard access to the item.) It's liable to fire some extra SELECTs, but it would help secure a couple dataflows (ie the AJAX-RecentItem-API dataflow and the automagic-BAO-dataflows).

[demeritcowboy]

Just adding that the gist of my spun-off lab ticket is that core should check permissions, but could also provide a way for that external code to say "for everything that follows during this api call I'm making, turn all permission checking off". Not just for Utils_Recent, because this is going to come up with any other secondary api calls made from the primary api call.

So if the idea here is to limit to Utils_Recent, then ok for 5.51, but then going forward is that extendable to all secondary api calls not just Utils_Recent?

[totten]

@demeritcowboy I tend to agree with the POV in dev/core#3677 that there should be a way to express a default that propagates across multiple calls. (I'll comment more about that on the ticket.) But... it seems to me that this scenario is interesting as a counter-example.

Suppose it propagated more systemically. I would expect it play out like this:

• The user fills out a webform //example.com/new-subscriber.
• Webform sets checkPermissions=>false which propagates to Contact.create API which calls a BAO or fires some hooks and eventually propagates to RecentItem API.
• With no permission checks, you can add anything to the RecentItem list.
• All is permitted, so it adds CID 123 (aka //example.com/civicrm/contact/view?id=123).
• But if the user clicks the link for CID 123 (//example.com/civicrm/contact/view?id=123), then they'll get a permission-denied error.
• That's not the right page for this user. The user never had permission to look at that page. Nobody intends for them to view that link.

There are a couple weak-links that we could attack to resolve this:

1. Move responsibility for RecentItems out of the BAO. It belongs in a form. (Academically, maybe you'd want add a config-option to enable webform<=>RecentItem integration? But it doesn't feel pressing to me...)
2. Keep it in the BAO, but do a regular permission-check while writing to RecentItems (despite the privilege-escalation that webform does).
3. Keep it in the BAO, and do a regular permission-check while reading from RecentItems (despite the privilege-escalation that webform does).

Moving responsibility out of BAO (#1) feels a bit more "correct" but less practical (IMHO; it'll be a game of whack-a-mole). Adding more permission-checks (#2 or #3) feels more manageable...

[colemanw]

The entire implementation of RecentItems feels shoehorned in. But enforcing permissions is a good solution regardless of other cleanup we end up doing.