Skip to content

Fix: src/tablib/formats/_xlsx.py's dset_sheet() does cell.value = col... - #676

Open
M001N wants to merge 1 commit into
jazzband:masterfrom
M001N:oss-engine/ebd432fa-3bcba4d9
Open

Fix: src/tablib/formats/_xlsx.py's dset_sheet() does cell.value = col...#676
M001N wants to merge 1 commit into
jazzband:masterfrom
M001N:oss-engine/ebd432fa-3bcba4d9

Conversation

@M001N

@M001N M001N commented Aug 16, 2026

Copy link
Copy Markdown

Summary

Added a _sanitize_value() helper in src/tablib/formats/_xlsx.py that, for string cell values, strips characters matched by openpyxl's own openpyxl.cell.cell.ILLEGAL_CHARACTERS_RE (reused directly rather than reimplemented) and additionally strips lone surrogate characters ([\ud800-\udfff]) which openpyxl accepts but which corrupt the exported XML. dset_sheet() now assigns cell.value = _sanitize_value(col) instead of the raw value, before the existing ValueError fallback (kept for other non-string failures like arrays).

Problem

jazzband/tablib issue reference: #370

Root Cause

src/tablib/formats/_xlsx.py's dset_sheet() does cell.value = col inside try/except ValueError: cell.value = str(col). openpyxl's IllegalCharacterError is raised for strings containing ASCII control characters (via openpyxl's own ILLEGAL_CHARACTERS_RE = [\000-\010]|[\013-\014]|[\016-\037]) and is not a ValueError subclass, so it propagates uncaught. Even if caught, the str(col) fallback would not help since the illegal character is already present as a plain string and would raise identically on retry. Separately, lone Unicode surrogates (e.g. \ud800) are accepted silently by openpyxl at assignment/save time but corrupt the resulting XML, producing a file that fails to reload (xml.etree.ElementTree.ParseError on read) — a related but distinct failure mode implied by the issue title.

Testing

PASS - all 25 xlsx-related tests pass and the full suite (164 tests) passes, including the new regression test which exports a value containing chr(31) and verifies the illegal character is stripped on reload via openpyxl.load_workbook.

Related Issue

#370

openpyxl raises IllegalCharacterError (not a subclass of ValueError)
when a string cell value contains an XML-illegal ASCII control
character, so the existing except ValueError never caught it, and
the export crashed instead of writing a sanitized value.

Strip these characters using openpyxl's own ILLEGAL_CHARACTERS_RE
before assigning to a cell, so behavior stays consistent with
whatever openpyxl considers illegal. Also strip lone surrogate
characters, which openpyxl accepts silently but which corrupt the
resulting XML and make the file unreadable.

Fixes jazzband#370

@MohammedAlkindi MohammedAlkindi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This works, and the surrogate half is the part worth keeping. Verified on Windows 11, Python 3.13.13, openpyxl 3.1.5.

I exported each case and then loaded the bytes back, because openpyxl accepting a write does not mean the file is readable:

                         master                                          this PR
control char \x0b        IllegalCharacterError                           ok, round-trip 'beforeafter'
control char \x1f        IllegalCharacterError                           ok, round-trip 'ab'
lone surrogate \ud800    ParseError: reference to invalid character      ok, round-trip 'xy'

The third row is the interesting one. \ud800 does not raise on write at all, it produces a workbook that fails to parse when you open it, so ILLEGAL_CHARACTERS_RE alone would not have covered it. Reusing openpyxl's regex and adding surrogates on top is the right split.

Suite, full, with all extras installed:

master    182 passed, 1 failed
this PR   183 passed, 1 failed

Same single failure both sides, UtilsGetDateTestCase.test_getDate_datetime_timestamp, pre-existing and unrelated. test_xlsx_export_strips_illegal_characters fails on unmodified source with IllegalCharacterError: ab cannot be used in worksheets, checked by restoring only _xlsx.py from master.

One gap, and it is small. The except ValueError fallback on the next line still assigns an unsanitized string:

try:
    cell.value = _sanitize_value(col)
except ValueError:
    cell.value = str(col)

_sanitize_value returns non-strings unchanged, so a value openpyxl rejects by type goes to str(col), and if that text carries a control character it raises the same error the PR is fixing. Reachable on this branch:

class Weird:
    def __str__(self):
        return "bad\x0bvalue"

data.append([Weird()])
data.export("xlsx")
# IllegalCharacterError: badvalue cannot be used in worksheets.

_sanitize_value(str(col)) on that line closes it. Your own docstring already makes the case, since IllegalCharacterError is not a ValueError and so the except cannot catch the retry failing the same way.

Worth saying out loud in the changelog either way: this silently drops characters rather than substituting them, so an export can now differ from its input without telling anyone. That is almost certainly better than raising, but it is a behaviour change someone diffing exports will notice.

Not a maintainer, just reporting what ran here.

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.

2 participants