CRM-21753 add pass-through support for criteria in urls on dedupe pages #11658
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8c517f5 to
83ab2ee
Compare
|
Jenkins re test this please |
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Feb 14, 2018
Per civicrm#11658 the dedupe code has a mechanism to handle a nuanced array of criteria. This PR is purely to discuss the passing of that criteria through the UI, with a proposed validation rule. The criteria in the url might look like criteria={"contact" : {"first_name" :{"IN" : ["Peter", "Paul", "Mary"]]]} This is passed to the contact api to limit the contacts to look for matches for, prior to applying permissions (which is done when actually finding the matches. My specific concern is that the criteria is passed back to the template and used in constructed urls - so I am looking to validate it on the way in for xss in this PR.
3aa512d to
3cab140
Compare
CRM/Contact/Page/DedupeFind.php
Outdated
| @@ -79,8 +81,10 @@ public function run() { | |||
| 'rgid' => $rgid, | |||
| 'gid' => $gid, | |||
| 'limit' => $limit, | |||
| 'criteria' => $criteria, | |||
| ); | |||
| $this->assign('urlQuery', CRM_Utils_System::makeQueryString($urlQry)); | |||
There was a problem hiding this comment.
makeQueryString url encodes both keys & values
ceb5101 to
2ace2a0
Compare
|
I did some more digging on this & concluded that when the urls are passed to the CRM_Utils_System::url function as an array rather than a string they are url encoded so the values exposed in the tpl (urls to click on & in once place a value to POST back) are safe. The criteria input is never passed to mysql - only the api which has it's own protections. Once url encoding was working I also added the smarty escape operator. I'm feeling confident about this now but would like to get some engagement with someone on this code because I've discovered that bugs that I fixed previously & submitted PRs for mid last year are still unfixed (I closed the PRS after they went stale because my head was no longer in the code). I'd like to refix those bugs & get them merged but feel I need this merged first so I don't get stale again. I'm going to submit the NFC change to how the urls are constructed as a separate PR as well so if I can't get movement on this I can at least get some on that one (which is quite stale-inducing) |
7cf4b90 to
e9c00c1
Compare
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Mar 20, 2018
When we pass a query it is urlencoded and any quotes in the string are not subsequently htmlentity encoded plus I think the url construction is generally cleaner civicrm#11658 Change-Id: I2300bac6aff180318f01784ce5b12ae1627a2e37
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Mar 20, 2018
This permits urls such as
civicrm/contact/dedupefind?reset=1&rgid=4&gid=&limit=&criteria={"contact":{"contact_id":205}}&action=update
The contents of $criteria['contact'] are passed to the contact api to get a list of contacts to find possible
matches for (acls are applied when generating the match list to both the matched & match list in the next step).
Note that the urls are urlencoded & hence will have %22 rather than a quote so that feels xss safe but I added some
validation before I figured that part out & I think it still makes sense to keep it.
civicrm#11658
Change-Id: I22129a0c63da4bdcb0151501e10244762651de95
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Mar 20, 2018
This permits urls such as
civicrm/contact/dedupefind?reset=1&rgid=4&gid=&limit=&criteria={"contact":{"contact_id":205}}&action=update
The contents of $criteria['contact'] are passed to the contact api to get a list of contacts to find possible
matches for (acls are applied when generating the match list to both the matched & match list in the next step).
Note that the urls are urlencoded & hence will have %22 rather than a quote so that feels xss safe but I added some
validation before I figured that part out & I think it still makes sense to keep it.
civicrm#11658
Change-Id: I22129a0c63da4bdcb0151501e10244762651de95
e9c00c1 to
4bd059b
Compare
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Mar 23, 2018
Per civicrm#11658 the dedupe code has a mechanism to handle a nuanced array of criteria. This PR is purely to support the passing of that json through the url criteria with a proposed validation rule. The criteria in the url might look like criteria={"contact" : {"first_name" :{"IN" : ["Peter", "Paul", "Mary"]]]} This is passed to the contact api to limit the contacts to look for matches for (before actually finding the matches). In discussion the biggest risk seemed to be the possibility of chaining so I have added a filter on the criteria & set check_permissions (acls are applied in the next step but having it on the api call is clearer & may reduce matches for the next step).
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Mar 27, 2018
Per civicrm#11658 the dedupe code has a mechanism to handle a nuanced array of criteria. This PR is purely to support the passing of that json through the url criteria with a proposed validation rule. The criteria in the url might look like criteria={"contact" : {"first_name" :{"IN" : ["Peter", "Paul", "Mary"]]]} This is passed to the contact api to limit the contacts to look for matches for (before actually finding the matches). In discussion the biggest risk seemed to be the possibility of chaining so I have added a filter on the criteria & set check_permissions (acls are applied in the next step but having it on the api call is clearer & may reduce matches for the next step).
4bd059b to
d4706dd
Compare
|
@seamuslee001 or @xurizaemon do you have any security concerns - I think this fine but want some other eyes on it. |
|
@JoeMurray we discussed the security in the linked pr & on the security channel |
This permits urls such as
civicrm/contact/dedupefind?reset=1&rgid=4&gid=&limit=&criteria={"contact":{"contact_id":205}}&action=update
The contents of $criteria['contact'] are passed to the contact api to get a list of contacts to find possible
matches for (acls are applied when generating the match list to both the matched & match list in the next step).
Note that the urls are urlencoded & hence will have %22 rather than a quote so that feels xss safe but I added some
validation before I figured that part out & I think it still makes sense to keep it.
d4706dd to
9f54f04
Compare
jmcclelland
pushed a commit
to jmcclelland/civicrm-core
that referenced
this pull request
Apr 10, 2018
Per civicrm#11658 the dedupe code has a mechanism to handle a nuanced array of criteria. This PR is purely to support the passing of that json through the url criteria with a proposed validation rule. The criteria in the url might look like criteria={"contact" : {"first_name" :{"IN" : ["Peter", "Paul", "Mary"]]]} This is passed to the contact api to limit the contacts to look for matches for (before actually finding the matches). In discussion the biggest risk seemed to be the possibility of chaining so I have added a filter on the criteria & set check_permissions (acls are applied in the next step but having it on the api call is clearer & may reduce matches for the next step).
|
@monishdeb I thought this was merged! |
|
thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Permit (but not at this stage promote) criteria in the dedupe params to be respected as they are for batch dedupe finding
Before
civicrm/contact/dedupefind?reset=1&rgid=4&gid=&limit=&criteria={"contact":{"contact_id":205}}&action=update does nothing
After
civicrm/contact/dedupefind?reset=1&rgid=4&gid=&limit=&criteria={"contact":{"contact_id":205}}&action=update and similarly constructed urls pass criteria through to the underlying function that finds duplicates and limits those found
Technical Details
Originally when deduping was added it was too intensive for some DBs. The idea of adding a group filter was hit upon & putting gid into the url will limit to matches of members of the group. When we wanted to add more filtering options to batch dedupe we decided that it was too restrictive & decided to accept parameters that could be passed to the api (rather than iterate on 'things we come up with).
This PR passes the url contents through so they can be used but does not promote the use of them. We have been using this patch with extensions that create urls with list of contact ids or other criteria to find dedupes for for a while now & I see this as a step on the path to generalising & extensionising this code.
An example of how we are using this is we have a 'Fishing Net' rule which 'casts a wide net' finding duplicates. We have an action to 'Find duplicates using Fishing Net' in the contact menu that redirects to a url similar to above
Comments
I have a concern about data validation. Is it possible to put xss in a valid json string? Should we add a rule validateJSON that unpacks it & checks each key or value against our XSS rule? @totten @seamuslee001 @seanmadsen
I did something a bit like that here
4b02a1c#diff-3076917760efeb467459726ebb6af282
UPDATE: I added the validation rule per the other PR. The criteria array is never used for sql - it is passed to an api call so it is validated at that point. My concern is to ensure the criteria var appended to urls is safe. The buttons on this form all carry a bunch of parameters in the urls that are required for the next action (perhaps that's bad design but this does not exacerbate that & I feel that an angular rewrite would be the way to tackle that)
@JKingsnorth would love your review on the other aspects of this