CRM-18504: Fix error in validating sub type by entity #8301

JKingsnorth · 2016-05-05T14:55:33Z

CRM-18504: validateSubTypeByEntity checks value instead of key

seamuslee001 · 2016-05-05T22:00:48Z

@eileenmcnaughton @totten Can one of you review this please, On my naked eye i think it works but this is clearly related to the recent security release. Eileen i know you patched this up for us (AUG). It would be good to just get a check of this.

seamuslee001 · 2016-05-05T22:00:56Z

Jenkins re test this please

xurizaemon · 2016-05-06T04:14:56Z

@seamuslee001 @JKingsnorth I don't think this code looks right. It is hardcoded to handle Contact subtypes when it appears intended for general Entity consumption?

Rephrasing (took me a while to realise CustomGroup refers to option groups, what I think of as fieldsets for better or worse).

We have a Custom Option Group named "Exiting" with ID=19
civicrm_views_custom_data_cache() calls CRM_Core_BAO_CustomGroup::getTree() which calls CRM_Core_BAO_CustomGroup::validateSubTypeByEntity()
I'm confused by the naming here - are we validating a custom field attached to an Entity (eg custom fieldset on Case), or are we validating that the subtype extends a core entity (eg School extends Organization)?

JKingsnorth · 2016-05-06T08:13:40Z

Must admit that I didn't look in to much depth as to why the method was implemented in the first place - I was focusing on getting the site not to fatal on every page.

are we validating a custom field attached to an Entity (eg custom fieldset on Case), or are we validating that the subtype extends a core entity (eg School extends Organization)?

I had the impression it was the latter - which I think is backed up by the steps to reproduce: https://issues.civicrm.org/jira/browse/CRM-18504

Is the logic: If the contact is a certain 'type' of contact (ie: an individual/organisation - not sure when this wouldn't be the case) then verify that the type exists, and then if they are a sub-type, verify that the sub-type is a sub-type of the parent.

That said, I've seen 'Participant' checked as well - but then the check bails because of:
if (is_numeric($subType)) { return $subType; }

It looks like participant subtypes (what?) are numeric. The check doesn't seem to be called when custom fields are used on events/participants more generally, however.

@eileenmcnaughton was doing some work on this - maybe she can shed some light on what's occurring.

davejenx · 2016-05-06T11:37:54Z

Same issue occurs in 4.6.16 and this patch applies cleanly & fixes it there.

@xurizaemon @JKingsnorth My reading is that the rationale is to validate $subType to make sure it's a legitimate entity subtype, as opposed to naughty input that might find its way into a SQL query. For most entity types, subtypes are integers, so that's an easy check. The code assumes that the only entity type that has non-integer subtypes is Contact: this is discussed at https://issues.civicrm.org/jira/browse/CRM-17984?focusedCommentId=88030&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-88030 . So if $subType is not an integer, the code checks whether it is a Contact subtype: specifically, a subtype of the type $entityType.

As John found, the check on $entityType falls over if the contact type has been renamed.

JKingsnorth · 2016-05-06T12:31:59Z

@davejenx Alas I didn't have permission to view that issue - but thanks for all the background information on this!

petednz · 2016-05-06T14:14:05Z

this has killed one of our sites. in this case, looking at what is in contact_type and what is in custom_group

the name of one of the subType as used in civicrm_contact_type differed to what was in civicrm_custom_group. in the former there is a subtype with name Funder_ (had label Funder.) - and in the latter there is a group of fields supposedly extending extends_entity_column_value of Funder (ie no following underscore).

I tried cherrypicking the patch. no change. i tried change Funder_ in the contact_type field to just Funder. No joy. I tried reversing the patch in case renaming it avoided the problem. No joy (guessing what to try at 2am)

Now I am wondering whether I can try using the integer of the contact sub type, since all the other entities eg event seem to be just using an integer, but think that may to wronger. Any pointers?

petednz · 2016-05-06T14:14:27Z

above relates to 4.6.x

petednz · 2016-05-06T14:36:45Z

by chance i thought i would go check if the upgrade completed. db said it was on 4.6.16 - /civicrm/upgrade said 'already there buddy' but guess what, seconds later the all clear alerts started coming through and site was back up.

JKingsnorth · 2016-05-06T14:38:12Z

@petednz Glad it got fixed up. I guess that if you still rename one of the types you'll run in to the bug though (steps to recreate in the JIRA issue). If you get a chance to test this patch that would be fab.

petednz · 2016-05-06T14:38:51Z

fwiw the problem subtype had a description "CiviEngage - Funder" - so not sure if the subtype is coming in with that weird name and hence might be hitting others

petednz · 2016-05-06T14:41:33Z

@JKingsnorth i can put patch back in to try, but will wait on @xurizaemon to run his eye over this as it sounded like he was asking questions too. are there two levels of issue. one if you 'just' rename the subtype, and another if you have custom fields created off the subtype either before or after the renaming of it?

JKingsnorth · 2016-05-06T14:45:43Z

@petednz Yup, would certainly be worth testing that scenario.

davejenx · 2016-05-06T14:59:42Z

Hi @petednz ,

If you throw some debugging statements in, e.g.

--- CustomGroup.php.PR8301  2016-05-06 12:07:10.000000000 +0100
+++ CustomGroup.php 2016-05-06 15:29:11.000000000 +0100
@@ -631,17 +631,21 @@
    * @throws \CiviCRM_API3_Exception
    */
   protected static function validateSubTypeByEntity($entityType, $subType) {
+    CRM_Core_Error::debug_log_message(__FUNCTION__ . ": entityType: " . var_export($entityType, TRUE));
+    CRM_Core_Error::debug_log_message(__FUNCTION__ . ": subType: " . var_export($subType, TRUE));
     $subType = trim($subType, CRM_Core_DAO::VALUE_SEPARATOR);
     if (is_numeric($subType)) {
       return $subType;
     }
     $contactTypes = civicrm_api3('Contact', 'getoptions', array('field' => 'contact_type'));
+    CRM_Core_Error::debug_log_message(__FUNCTION__ . ": contactTypes: " . var_export($contactTypes, TRUE));
     if ($entityType != 'Contact' && !array_key_exists($entityType, $contactTypes['values'])) {
       // Not quite sure if we want to fail this hard. But quiet ignore would be pretty bad too.
       // Am inclined to go with this for RC release & considering softening.
       throw new CRM_Core_Exception('Invalid Entity Filter');
     }
     $subTypes = civicrm_api3('Contact', 'getoptions', array('field' => 'contact_sub_type'));
+    CRM_Core_Error::debug_log_message(__FUNCTION__ . ": subTypes: " . var_export($subTypes, TRUE));
     if (!array_key_exists($subType, $subTypes['values'])) {
       // Same comments about fail hard as above.
       throw new CRM_Core_Exception('Invalid Filter');

what do you get in CiviCRM.blah.log for the problematic case?

I wonder if revisiting the upgrade page cleared Civi caches and allowed one of your previous changes to take effect?

eileenmcnaughton · 2016-05-06T23:54:18Z

Hi,

just looking quickly & I haven't had a long enough look to be clear but I think we need 3 principles

we need to add a unit test for the input & output we are checking - that's easy check

CustomGroupTest.php#L55-55

the principle is that function should be dealing with names rather than labels. If we discover any calling functions passing in labels we should fix the calling functions.
we should ideally have a preferred input format for that function 'random format as you please' might be supported but preferably not encouraged

petednz · 2016-05-07T02:40:04Z

problem on another site - again one with CiviEngage installed

petednz · 2016-05-07T04:03:26Z

I have this in logs

  'is_error' => 0,
  'version' => 3,
  'count' => 1,
  'id' => 'Media_Outlet',
  'values' => 
  array (
    'Media_Outlet' => 'Media Outlet',
  ),
)

And what i see in DB is that contact_type has name = Media_Outlet and label of Media Outlet - and in custom_group it is Media_Outlet

Suggestion?

…Group::validateSubTypeByEntity(); instead check the returned value & use it only if it was valid (not falsey).

…if subType is invalid.

xurizaemon · 2016-05-07T07:11:02Z

The sites where this appeared most obviously had it triggered via Views - civicrm_views_custom_data_cache() is used to add CiviCRM custom fields to the $data array which contains each entity type.

This function calls CRM_Core_BAO_CustomGroup::getTree(). Custom Group here refers to option groups, and the getTree() method gets all the option groups and option fields for that entity type (in a tree structure.

One method of triggering the exceptions introduced in 7936342 is to have a subtype which is disabled. CiviCRM Views sees these subtypes (it probably shouldn't?) and tries to CRM_Core_BAO_CustomGroup::getTree() on them.

The two commits above are for 4.6 branch and seem to address the issue triggered via Views which was affecting our sites. I ought to have mentioned earlier that this issue was hitting us on the Fuzion 4.6 branch not CiviCRM 4.7.7, wish I'd seen earlier how different the file is btw 4.6 and 4.7.

As a result the above changes won't apply to 4.7, but something similar should work - I don't have availability to look into that right now though.

yurg · 2016-05-09T16:01:02Z

Sorry, it still throws an error if there are deleted contact sub-types, in our case it looks like:
"Rejected "Young_Arts_Group_Member" as not a valid subtype of "Individual"." and we have no "Young Art Group Member" contact sub-type anymore.

xurizaemon · 2016-05-09T20:20:03Z

@yurg may I confirm your Young Arts Group Member subtype is deleted and not disabled? (Disabled is represented as is_active => 0 when viewing contact types via API explorer.)

petednz · 2016-05-09T20:24:12Z

and can I ask, if the sub Contact Type is deleted, are there any Custom Fields still associated with the now missing Contact type?

yurg · 2016-05-09T20:34:08Z

@xurizaemon It's actually disabled ( have checked via civicrm/admin/options/subtype?reset=1 ) ; should we delete it?

@petednz No, there aren't (as far as I can see here civicrm/admin/custom/group?reset=1 in "Type" column ) , but Sub Contact Type isn't deleted.

xurizaemon · 2016-05-10T00:46:43Z

Thanks @yurg - no that's fine - I'm working on the issue for "disabled" but wanted to check if you were seeing some other issue. Appreciate the correction.

JKingsnorth · 2016-05-10T08:21:20Z

Feel free to close this once the discussion has run its course if there is a better patch being formulated in the Fuzion repo.

yurg · 2016-05-17T11:11:45Z

What's the current status of this one? It continues to stabs us in the back here and there at contact add / edit forms.

jaapjansma · 2016-05-17T13:54:12Z

I have the same issue now at a fresh upgrade from 4.4 to 4.6 and cannot view the contact summary anymore of contacts with a contact subtype set.

eileenmcnaughton · 2016-05-17T13:59:45Z

So, status of this is that it needs someone to read through it & do a clear QA on it. The waters are a little muddied by the suggestion there is a competing patch that is not in a PR (half way up the thread).

It definitely HAS to be in the rc release going out in a few days, leading up to a full release at the beginning of next month.

If you are dealing with a bug that is fixed in the rc & need to go live with the rc the main caution I have is that if any changes occur during the rc period you may need to do some manual fiddling to get all changes in the upgrade script to run.

eileenmcnaughton · 2016-05-17T14:06:10Z

As an aside - I'm not even slightly sold on the suggestion that we shouldn't hard-fail when the wrong input is passed into the function. I think we should just pass the right input into the function. There is no reason we can't do that & a soft fail just opens up other hard-to-debug errors

yurg · 2016-05-17T14:45:45Z

Have found this https://issues.civicrm.org/jira/browse/CRM-18567 and this #8388 (the latter looks like a fix for us after a quick tests).

davejenx · 2016-05-17T14:47:01Z

The waters are also muddied by there being several different scenarios where errors occur around validateSubTypeByEntity(), including...

CRM-18504: pervasive fatal error when there are subtypes of a renamed contact type. This was fixed for me by @JKingsnorth's initial change here (8301).
(No separate issue): @petednz noted complications e.g. here and here - not clear what scenario triggers this.
(No separate issue): pervasive fatal error when there is an enabled custom field set that extends a disabled contact subtype. Not fixed by @JKingsnorth's initial change here. I have encountered this and as an emergency fix, worked around it by accepting $subType without further checks if it consists entirely of word characters, thus avoiding SQLi without getting into the problematic subtype checks. @xurizaemon indicates above that he is working on this, I believe in the Fuzion repo?
CRM-18559: Custom field sets for other activity types appear on activity create form - very different symptoms so I created a separate issue with PR to track it / allow users to find it, but it's another problem in the same area of code. Affects 4.6 only.
CRM-18567: Trying to edit contact with multiple subtypes gives error: Invalid Filter. Different symptoms again so I created a separate issue with PR to track it / allow users to find it, but it's another problem in the same area of code. Affects 4.6 & 4.7 .

eileenmcnaughton · 2016-05-17T15:02:12Z

Thanks Dave for the great summary - we just have to try to add tests when we fixes things so we don't wind up in a game of whack-a-mole....

eileenmcnaughton · 2016-05-17T15:33:09Z

I'm happy to merge this part of the patch - #8389 - because I was able to replicate it in a test

totten · 2016-05-18T22:22:04Z

CRM/Core/BAO/CustomGroup.php

@@ -652,13 +652,13 @@ protected static function validateSubTypeByEntity($entityType, $subType) {
      return $subType;
    }
    $contactTypes = civicrm_api3('Contact', 'getoptions', array('field' => 'contact_type'));
-    if ($entityType != 'Contact' && !in_array($entityType, $contactTypes['values'])) {
+    if ($entityType != 'Contact' && !array_key_exists($entityType, $contactTypes['values'])) {


This line replicated/merged via eileen's #8389.

The other change is not replicated merged there.

eileenmcnaughton · 2016-05-18T23:14:31Z

I propose to close this PR now.

The second line of John's patch on this PR (which turned out to be consistency rather than functionality) is covered in #8405 - as is the issue around disabled contact types.

The other issues all appear to be resolved elsewhere (which much thanks to DaveJ & Seamus) with the exception of the views issue. I don't have a clear picture of the views issue other than to note it needs it's own JIRA, description, steps etc & PR.

totten · 2016-05-19T00:05:14Z

OK, closing since there are newer PR's with tests. @JKingsnorth, if that's a mistake, feel free to reopen.

jaapjansma · 2016-05-20T10:24:38Z

Sorry guys I discover that the provided in #8301 is not good when a contact has multiple subtypes. At a client we have a contact which is an organisation and has multiple subtypes.

The $subType parameter consists of the name of multiple subTypes divided by the CRM_Core_DAO::VALUE_SEPARATOR sign....

I have fixed this in CRM_Core_BAO_CustomGroup::getTree (Line: 332) with the following lines:

if ($entityID) {
  $entityID = CRM_Utils_Type::escape($entityID, 'Integer');
}
if (is_string($subTypes)) {
  $subTypes = trim($subTypes, CRM_Core_DAO::VALUE_SEPARATOR);
  $subTypes = explode(CRM_Core_DAO::VALUE_SEPARATOR, $subTypes);
}
if (!is_array($subTypes)) {
  ...

davejenx · 2016-05-20T10:31:47Z

@jaapjansma I think you gave the wrong PR number there?

seamuslee001 · 2016-05-20T10:34:21Z

@jaapjansma Just to be clear the custom group is what extends the multiple subtypes correct?

jaapjansma · 2016-05-20T10:34:54Z

@seamuslee001 Yes it is the custom group and not the contact ;-)
@davejenx you are right wrong number

davejenx · 2016-05-20T10:36:32Z

@jaapjansma which number did you mean? There are lots of PRs floating around. :-)

jaapjansma · 2016-05-20T10:37:19Z

I did mean PR #8301

davejenx · 2016-05-20T10:39:56Z

I think #8301 has been abandoned in favour of #8389 + #8405 plus there are PRs for other issues I summarised at #8301 (comment) .

jaapjansma · 2016-05-20T11:33:03Z

Ok. I see. It is very confusing espiallcy now we have backported the security fix and have lots and lots more problems... :-(

eileenmcnaughton · 2016-05-21T01:54:01Z

I believe regressions are all fixed now in the rc / 4.6 dist tarball - see dist.civicrm.org/latest

petednz · 2016-06-16T10:13:53Z

fwiw both Dave Jenkins and we have hit this or a very very similar one with upgrading to 4.6.18 so just flagging here to try and tie conversation back in with jira-18504

davejenx · 2016-06-16T11:10:16Z

Re @petednz's last comment, details of the issue I encountered on a 4.6.18 upgrade are here: https://issues.civicrm.org/jira/browse/CRM-18504?focusedCommentId=90551&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-90551

petednz · 2016-07-17T12:09:50Z

Wanting to leave these markers for others (and risking repeating earlier examples but not certain). Two more, or subsets, or scenarios where this error can bite a 4.6.x upgrade.

a/ if there are custom group fields set up for an entity (eg CiviCase) that has now been disabled. Can be fixed through UI by deleting the custom fields and then the custom group

b/ if there are custom group fields have been set up to 'extend' a contact subtype, but that subtype has been renamed, leaving 'bad' data in the civicrm_custom_group.extends_entity_column_value. Can be fixed through UI finding the Custom Groups that should be extending a subtype, and resetting them to the new name of the subtype. Note the 'extends' shows nothing on the Custom Group summary page. You probably need to go look at whether the subtype 'names' are still in alignment in the civicrm_custom_group table and the civicrm_contact_type table.

CRM-18504: Fix error in validating sub entity

2d2af48

civicrm-builder added the master label May 5, 2016

JKingsnorth changed the title ~~CRM-18504: Fix error in validating sub entity~~ CRM-18504: Fix error in validating sub type by entity May 5, 2016

totten mentioned this pull request May 6, 2016

Invitation: CiviCRM Release for June 2016 civicrm/release-management#2

Closed

JKingsnorth mentioned this pull request May 6, 2016

CRM-18511: Fix regression introduced in 79363422. #8307

Closed

xurizaemon added a commit to fuzionnz/civicrm-core that referenced this pull request May 7, 2016

Refs CRM-18504, civicrm#8301. Do not fail hard in CRM_Core_BAO_Custom…

a0bcc16

…Group::validateSubTypeByEntity(); instead check the returned value & use it only if it was valid (not falsey).

xurizaemon added a commit to fuzionnz/civicrm-core that referenced this pull request May 7, 2016

Refs CRM-18504, civicrm#8301. Better handle invalid SQL construction …

d3e9ab2

…if subType is invalid.

xurizaemon mentioned this pull request May 8, 2016

"Fail hard" is too hard #8319

Closed

totten reviewed May 18, 2016
View reviewed changes

totten closed this May 19, 2016

omarabuhussein mentioned this pull request May 27, 2016

CATL-714: Followup fixing bugs appeared after latest secuity update compucorp/civicrm-core#3

Merged

CRM-18504: Fix error in validating sub type by entity #8301

CRM-18504: Fix error in validating sub type by entity #8301

Conversation

JKingsnorth commented May 5, 2016 • edited by civicrm-builder Loading

seamuslee001 commented May 5, 2016

seamuslee001 commented May 5, 2016

xurizaemon commented May 6, 2016 • edited Loading

JKingsnorth commented May 6, 2016 • edited Loading

davejenx commented May 6, 2016

JKingsnorth commented May 6, 2016

petednz commented May 6, 2016

petednz commented May 6, 2016

petednz commented May 6, 2016

JKingsnorth commented May 6, 2016

petednz commented May 6, 2016

petednz commented May 6, 2016

JKingsnorth commented May 6, 2016

davejenx commented May 6, 2016

eileenmcnaughton commented May 6, 2016

petednz commented May 7, 2016

petednz commented May 7, 2016

xurizaemon commented May 7, 2016 • edited Loading

yurg commented May 9, 2016

xurizaemon commented May 9, 2016

petednz commented May 9, 2016

yurg commented May 9, 2016

xurizaemon commented May 10, 2016

JKingsnorth commented May 10, 2016

yurg commented May 17, 2016

jaapjansma commented May 17, 2016

eileenmcnaughton commented May 17, 2016

eileenmcnaughton commented May 17, 2016

yurg commented May 17, 2016

davejenx commented May 17, 2016 • edited by eileenmcnaughton Loading

eileenmcnaughton commented May 17, 2016

eileenmcnaughton commented May 17, 2016

totten May 18, 2016

Choose a reason for hiding this comment

eileenmcnaughton commented May 18, 2016

totten commented May 19, 2016

jaapjansma commented May 20, 2016 • edited Loading

davejenx commented May 20, 2016

seamuslee001 commented May 20, 2016

jaapjansma commented May 20, 2016

davejenx commented May 20, 2016

jaapjansma commented May 20, 2016

davejenx commented May 20, 2016

jaapjansma commented May 20, 2016

eileenmcnaughton commented May 21, 2016

petednz commented Jun 16, 2016

davejenx commented Jun 16, 2016

petednz commented Jul 17, 2016 • edited Loading

JKingsnorth commented May 5, 2016 •

edited by civicrm-builder

Loading

xurizaemon commented May 6, 2016 •

edited

Loading

JKingsnorth commented May 6, 2016 •

edited

Loading

xurizaemon commented May 7, 2016 •

edited

Loading

davejenx commented May 17, 2016 •

edited by eileenmcnaughton

Loading

jaapjansma commented May 20, 2016 •

edited

Loading

petednz commented Jul 17, 2016 •

edited

Loading