koolreport icon indicating copy to clipboard operation
koolreport copied to clipboard

Process/Group bad implementation

Open jazzkutya opened this issue 6 years ago • 2 comments

Hi,

in koolreport/src/processes/Group.php you calculate $index by concatenating group column values and then: $index = strtolower($index) . md5($index); there are 2 big issues here:

  • case sensitivity depends on md5() having a collision or not
  • it totally breaks when one group column value optionally starts with something that another group column value optionally ends with You should do implode($sepchar,$groupvalues) instead where $sepchar is one character that you need to ensure is not in the input by deleting them before. Good candidate is "\x00".

jazzkutya avatar Feb 15 '19 09:02 jazzkutya

May be the "\x05" ENQ (enquiry) character is a better choice to avoid PHP's weakness of \x00 characters in strings (not all php function handle it well), It should not appear in ordinary text data.

jazzkutya avatar Feb 15 '19 10:02 jazzkutya

Thank you very much for your spots:

  1. We made the grouping is case-sensitive. If user do not want the case sensitive he may convert to lower case before grouping. Now I am thinking it could be better if there is option in Group process to allow the case insensitive.
  2. You are right, we will fix that.

koolphp avatar Feb 15 '19 16:02 koolphp