Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[data grid] Sanitize cells with formulas for CSV export used in Excel #11702

Closed
cherniavskii opened this issue Jan 16, 2024 · 5 comments · Fixed by #13115
Closed

[data grid] Sanitize cells with formulas for CSV export used in Excel #11702

cherniavskii opened this issue Jan 16, 2024 · 5 comments · Fixed by #13115
Assignees
Labels
component: data grid This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature feature: Export security Pull requests that address a security vulnerability

Comments

@cherniavskii
Copy link
Member

cherniavskii commented Jan 16, 2024

Summary

https://groups.google.com/u/0/a/mui.com/g/security/c/L5bFSD3uwF8

Examples

No response

Motivation

No response

Search keywords: csv excel

@cherniavskii cherniavskii added component: data grid This is the name of the generic UI component, not the React module! feature: Export enhancement This is not a bug, nor a new feature labels Jan 16, 2024
@github-project-automation github-project-automation bot moved this to 🆕 Needs refinement in MUI X Data Grid Jan 16, 2024
@cherniavskii cherniavskii moved this from 🆕 Needs refinement to 🔖 Ready in MUI X Data Grid Jan 16, 2024
@MBilalShafi
Copy link
Member

We could do this along with introducing a value getter/serializer which would also allow the users to customize the exported values on top of sanitization.
Here's a related issue: #8193

@oliviertassinari oliviertassinari added the security Pull requests that address a security vulnerability label Jan 16, 2024
@oliviertassinari oliviertassinari changed the title [DataGrid] Sanitize cells with formulas for CSV and Excel export [data grid] Sanitize cells with formulas for CSV and Excel export Jan 16, 2024
@oliviertassinari oliviertassinari changed the title [data grid] Sanitize cells with formulas for CSV and Excel export [data grid] Sanitize cells with formulas for CSV export used in Excel Jan 16, 2024
@oliviertassinari oliviertassinari removed the security Pull requests that address a security vulnerability label Jan 16, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 16, 2024

There seem to be a couple of opportunities to improve the CSV export features. I tried to rank these:

  1. It feels like we are missing callbacks for developers to customize the exports. For example, a callback for each CSV export cell, a callback for each Excel call, a flag to say if valueFormatter should be used or not per column, etc.

  2. We could have an opt-in flag to escape formulas by default. See:
    Screenshot 2024-01-16 at 17 03 55
    http://georgemauer.net/2017/10/07/csv-injection.html

So for instance https://www.ag-grid.com/react-data-grid/csv-export/#security-concerns not being opt-in feels borderline.

Should this be opt-out? https://bughunters.google.com/learn/invalid-reports/google-products/4965108570390528/csv-formula-injection suggests that it shouldn't. CSV has other applications, we might not be able to make an escaping logic that doesn't break some other types of applications. I wouldn't be against trying, just don't know if it would work. The root problem is really Excel.

I can't find anyone who makes this opt-out and it's rare to even find CSV libraries who have an option for it, e.g. FasterXML/jackson-dataformats-text#326 (comment), but then this is very strange UX to me:

Screen.Recording.2024-01-16.at.20.56.06.mov
  1. It would be great to have interactive playground demos where developers can see the actual CSV exports https://mui.com/x/react-data-grid/export/#csv-export.
  2. I don't know about having the apiRef at the bottom of the page https://mui.com/x/react-data-grid/export/#apiref. Feel like this should be one per section (CSV, Excel, Print). I initially didn't realize how to use the CSV export. I mean, the whole page needs to be read right now, even if you are only interested in one of these 3 exports. This move could help.

@oliviertassinari oliviertassinari added the security Pull requests that address a security vulnerability label Feb 28, 2024
@mauro-ni
Copy link

mauro-ni commented May 13, 2024

Good morning,
I'm commenting on this issue to find out when a resolution is expected.

We have a dozen applications with a total of about fifty DataGridPremium instances, some of them have a lot of columns.
An ad hoc resolution could be costly and complicated (to avoid duplication of escaping logic).

An opt-in/opt-out solution with a flag on the DataGridPremium component and/or single column would be ideal.

Thank you.

Mauro

Premium subscription Order ID: 47709

@cherniavskii
Copy link
Member Author

Hi Mauro,
I'm handling the community support this week, I'll look into it.
You can expect a solution by the end of next week.

@cherniavskii cherniavskii self-assigned this May 16, 2024
@cherniavskii cherniavskii moved this from 🔖 Ready to 🏗 In progress in MUI X Data Grid May 16, 2024
Copy link

⚠️ This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

@cherniavskii: How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

@cherniavskii cherniavskii moved this from 🏗 In progress to ✅ Done in MUI X Data Grid Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature feature: Export security Pull requests that address a security vulnerability
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants