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

Fix escaping to support escape of XLSX formulae #1568

Merged

Conversation

matthewhegarty
Copy link
Contributor

@matthewhegarty matthewhegarty commented Apr 3, 2023

Problem

Issue #1543. Security vulnerabilities present due to not escaping potentially malicious content.

Solution

I have introduced two new settings attributes:

  • IMPORT_EXPORT_ESCAPE_HTML_ON_EXPORT
  • IMPORT_EXPORT_ESCAPE_FORMULAE_ON_EXPORT

I have deprecated: IMPORT_EXPORT_ESCAPE_OUTPUT_ON_EXPORT

The reason for the change is that malicious strings can be present exported to a variety of formats and still be an issue. For example, exporting bad HTML to CSV might still be an issue if the user opens the exported csv into a browser somehow.

Likewise, dodgy excel formulae could be exported to csv, tsv, ods, xls and xlsx and opened in Excel, so this need addressing for all formats.

I decided that it might be better for users to have more control over escaping, hence why two settings are used. I can envisage that if someone only ever exports to HTML, there could be cases where they don't want any leading '=' characters replaced.

Acceptance Criteria

  • Integration tests
  • Manual testing
  • Documentation updates

@coveralls
Copy link

coveralls commented Apr 3, 2023

Coverage Status

Coverage: 100.0%. Remained the same when pulling c4d4c68 on matthewhegarty:1543-xlsx-vuln into fe4acc7 on django-import-export:main.

@mil1i3
Copy link

mil1i3 commented Apr 4, 2023

I have tested all 3 parameters to all formats and everything looks Ok.

@matthewhegarty matthewhegarty marked this pull request as ready for review April 4, 2023 15:21
Copy link

@pokken-magic pokken-magic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a cool, boring, totally nonsexy but deceptively important bit of code. Appreciate you doing this.

import_export/formats/base_formats.py Show resolved Hide resolved
import_export/formats/base_formats.py Show resolved Hide resolved
@matthewhegarty matthewhegarty merged commit 8e77905 into django-import-export:main Apr 6, 2023
12 checks passed
@matthewhegarty matthewhegarty deleted the 1543-xlsx-vuln branch April 6, 2023 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants