Skip html encoding rich-text fields

[colemanw]

Overview

Fixes civicrm/org.civicrm.contactlayout#21

Before

Rich text custom fields saved with api were being escaped

After

Html is preserved as-is

Comments

Ugh.

[eileenmcnaughton]

@colemanw @totten @seamuslee001 @mlutfy this works to fix the issue. However, I just want to cover off the security issues before we merge.

Performance - ideally once we have finalised some caching discussion we would store this in a cache so Redis or Memcache etc can be used. However, in the meantime this should mean only one extra query per process as it is on a singleton property.

Security
Basically we have some fields in which data is stored in the database as raw html. For all other fields the data is encoded before being stored - so the html will not actually run. We have an existing whitelist of fields where the data is stored in html. This is perhaps not the perfect solution but to the extent that this fix is just extending the whitelist it's worth talking about whether rich text custom data fields differ from the other fields in the whitelist in any meaningful way that means we are less secure with this.

The only thing that strikes me about the custom fields compared to the existing fields is that none of the existing fields are likely to be exposed to front end users. However, to create a new contact or contribution, other than using an existing form - which should be unchanged - the user would need Contact.create or Contribution.create permission - which seems likely to only be given to 'trusted users'.

I'm about 50-50 on whether we should add an extra permission 'save html to database' and at least in this place in the code require it for any skipped fields.

[eileenmcnaughton]

Also note that I couldn't just add script tags & have them save

<script>alert(1);</script>

[colemanw]

Also note that we use (at least in the layout extension) the smarty {$string|purify} modifier to filter out malicious html before displaying. We ought to do that in other places for these whitelisted fields.

[eileenmcnaughton]

Any reply I give to that is going to take us into a whole different scope of conversation but yes - good that use purify in the extension!

[seamuslee001]

@eileenmcnaughton i think that is the IDS there kicking in, however as Sean pointed out that isn't a solution its self. the problem is that this list of fields doesn't know what entity it is for so any field that matches custom_x would be skipped but given its custom fields they will only ever belong to one entity so i think this is probably ok but adding to this array always makes me nervous

[colemanw]

Just looking at this patch, without getting sidetracked onto other related topics, I think this is safe to merge, because it mirrors the behavior in the DAO. Saving these custom fields via DAO will not escape them, so now the api functions the same way.
Whether or not the architecture of the DAO should be changed is an entirely different conversation.

[eileenmcnaughton]

@colemanw right - be people can access the api from js whereas they have to go through some form or other to get to the DAO

[colemanw]

Well the js api enforces check_permissions always, and those permission checks are at least as strict as the form permissions (sometimes stricter).

[eileenmcnaughton]

I'd like @totten to comment

[totten]

• Agree with @seamuslee001's point - the entity issue is addressed implicitly because custom-field ID's are globally unique.
• On performance, the results are effectively stored in a static/singleton. Agree it would be a slight improvement for some systems to use a short cache (but not in a long cache) -- and agree it's a minor point.
• The main caveat is that this is fixing a wonky API contract, which means it is technically a BC-break. Put yourself in the shoes of an Android app developer: you observed responses as escaped HTML in v5.1.0, and you don't have (or don't believe that you have) a way to change that, so you compensate by including encode/decode steps in the app. Then v5.8.0 changes the convention.

I'm trying to reconcile myself to the interface change. Some ways:

1. If one can show that the mobile app was already broken (i.e. the old contract was fundamentally unusable), then it's easy to merge and move on.
2. If the old contract was valid-but-wonky, then the next question -- does anyone actually manipulate rich-text custom-data via APIv3. If not, then it's just hypothetical -- we can make the change and move on.
3. If we think that this is a real break and want to proceed anyway, then there's a higher communication bar. Perhaps we could send a ping on the api channel and send email to skornien and ramesh?

(I haven't sent the pings -- would like to hear about #1 before bugging more people.)

[colemanw]

I would argue that scenario 1 is the case: it is fundamentally broken as-is and there is no workaround that doesn't involve circumventing the api with a direct db query or doing something complex like implementing a custom api wrapper (I'm not even sure an api wrapper could fix this encoding it happens at such a low level).

@totten you mentioned an android app - is there an android dev that mentioned this issue? I filed it because of the Layout extension.

[totten]

(b) No one's pushing the point now. (It's too early in the process for anyone to get potentially bitten.) I raised an Android app as a hypothetical.

(a) 👍 Your statement that the old behavior is fundamentally broken checks out (IMHO). In playing around with a read/write cycle of the API, I wind up with munged data that no longer displays properly in the main UI.

[eileenmcnaughton]

I feel like this is resolved now per the last 2 comments - merging