@ziegenberg opened this Pull Request on April 9th 2021 First_time_contributor

Description:

FIXES #17441.

Escape the first column in CSV exports because it might contain a date span, and the date span will contain a comma.

If a test to prevent regressions is desired, I would need help with implementing that.

Note: As soon PHP 8 is the required minimum escaping could be limited with str_contains().

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review was done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@ziegenberg commented on April 9th 2021

Ready for review.

@ziegenberg commented on April 9th 2021

I did not run the tests locally and they might need to be updated.

@sgiehl commented on April 9th 2021 Member

Wondering if it might make sense to use a method like this to build a correctly escaped CSV string

function convertArrayToCSV($data) {
    $fh = fopen('php://temp', 'rw');
    fputcsv($fh, array_keys(current($data)));

    foreach ($data as $row) {
        fputcsv($fh, $row);
    }
    rewind($fh);
    $csv = stream_get_contents($fh);
    fclose($fh);

    return $csv;
}

Haven't tested it though, but could maybe prevent other issues with CSV.

Powered by GitHub Issue Mirror