silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 821 forks source link

Add Excel export option in GridFields (and avoid CSV injection) #9273

Open chillu opened 5 years ago

chillu commented 5 years ago

Overview

GridField allows export of data to CSV. While this format is simple, universal and easy to generate, it increases the security surface (see "Context" below), and is hard to get right (see the long discussion about encoding issues).

I propose that we push more strongly to make an Excel export option available to more users. There are a few ways to achieve this:

Note that Excel export requires third party libraries to create it (e.g. https://github.com/PHPOffice/PhpSpreadsheet). It's a complex format which is easy to get wrong. I'm sceptical if this needs to be in the core framework.

Context

SilverStripe data can be exported to CSV. CSV can be read by spreadsheet apps like Excel. Those apps allow "dynamic" cell content (macros) with complex signatures which are hard to sanitise. Before executing this dynamic content, spreadsheet apps usually warn the user. As SilverStripe content, these macros are harmless, but when users ignore the warnings within their app (outside of SilverStripe), it can become dangerous.

We've previously added baseline security measures through SS-2017-007 (see https://github.com/silverstripe-security/security-issues/issues/4), but this hasn't been a comprehensive fix. See https://www.owasp.org/index.php/CSV_Injection for potentially malicious payloads.

After much discussion on the private security issue (https://github.com/silverstripe-security/security-issues/issues/53), we have decided not to treat this as a security issue in our product (rated as CVSS 0.0).

/cc @silverstripe/core-team

chillu commented 5 years ago

Quoting OWASP:

This attack is difficult to mitigate, and explicitly disallowed from quite a few bug bounty programs. To remediate it, ensure that no cells begin with any of the following characters:

This substantiates our position in my opinion, and we've already implemented the baseline protections they mention there years ago.

sminnee commented 5 years ago

As a core add-on, I would support the idea of making a pluggable interface for export formats (so that a module could provide an implementation of that) but not adding excel to export to core.

I'd probably treat this as a "patches welcome" kind of ticket too – if someone is keen to add this feature, happy to help design it, rather than pre-emptively building it.

maxime-rainville commented 5 years ago

Back in the glorious SS3 days, I did an Export to Excel module using phpoffice/phpexcel.

It was relatively easy to implement. Could serve as a general starting point.

lekoala commented 4 years ago

I have this module that works just fine https://github.com/lekoala/silverstripe-excel-import-export

I'm not sure it belongs to the core

what would be nice though is to have a common interface to set "exportable" fields somehow so that all modules could benefit from it

i would also love to see a solution to exporting large lists (like, a 20k members list) : due to heavy usage of the orm, it tends to be very difficult (in one of my project, I add to set some kind of raw export that was getting data from a direct database query instead)

importing data can also be tricky for some customers since they don't know exactly what to put in there, in my module i added a "sample file" which is really helpful

ScopeyNZ commented 4 years ago

what would be nice though is to have a common interface to set "exportable" fields somehow so that all modules could benefit from it

Another use case for silverstripe/silverstripe-cms#2454 ? That would be only if you wanted to export what you arbitrarily define as "content". What defines "exportable" for you?

i would also love to see a solution to exporting large lists (like, a 20k members list) : due to heavy usage of the orm, it tends to be very difficult (in one of my project, I add to set some kind of raw export that was getting data from a direct database query instead)

DataList does have a getGenerator function. The support on the adapter level is patchy though I think - at least until SS5 where the adapters are a lot more memory efficient.

For this issue my vote is option F by the way. I'm going to add RFC labels here.