From cef9b6c9302b97d0288d769ebb14f4e565195e3b Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Thu, 24 Sep 2026 13:00:29 -0700 Subject: [PATCH 1/4] Strip Excel-illegal control characters from report cells Imported metadata can contain stray control characters -- in this case a U+001F unit separator embedded in a contributor name. openpyxl refuses to write those into a worksheet and raises IllegalCharacterError, which failed the entire inventory report task. Sanitize string values in _cell_value, the shared conversion point for the CSV and Excel writers, so both outputs stay identical. The stripping uses openpyxl's own ILLEGAL_CHARACTERS_RE -- the same regex its cell validation checks against -- so anything that survives is guaranteed writable and the two can't drift apart. Tab, newline and carriage return are not matched by that regex and are preserved. Co-Authored-By: Claude Opus 5 --- .../generate_inventory_and_hold_reports.py | 22 ++++++- ...est_generate_inventory_and_hold_reports.py | 65 +++++++++++++++++++ 2 files changed, 85 insertions(+), 2 deletions(-) diff --git a/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py b/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py index e08651c8f9..5e58bf0dd0 100644 --- a/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py +++ b/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py @@ -14,6 +14,7 @@ from celery import shared_task from openpyxl import Workbook from openpyxl.cell import WriteOnlyCell +from openpyxl.cell.cell import ILLEGAL_CHARACTERS_RE from openpyxl.styles.numbers import FORMAT_TEXT from sqlalchemy import ( bindparam, @@ -268,21 +269,38 @@ def _stringify_cell_value(value: Any) -> str: return str(value) +def _sanitize_cell_value(value: str) -> str: + """Strip characters that Excel does not permit in worksheet cells. + + Some imported metadata contains stray control characters (e.g. a U+001F unit + separator embedded in a contributor name). openpyxl rejects these with an + ``IllegalCharacterError``, which would fail the whole report, so we remove + them. The same regex openpyxl validates with is used here, so anything that + survives is guaranteed to be writable. Tab, newline and carriage return are + not matched by that regex and are therefore preserved. + """ + return ILLEGAL_CHARACTERS_RE.sub("", value) + + def _cell_value(key: str, value: Any, stringify_cols: frozenset[str]) -> Any: """Convert a cell value for report output (shared by CSV and Excel writers). For columns in stringify_cols, forces the value to a plain string. Enum values are converted using their .value attribute. Timezone-aware datetimes are formatted as strings for Excel compatibility. + String values are stripped of characters that Excel disallows, so that the + CSV and Excel outputs stay identical. """ if key in stringify_cols: - return _stringify_cell_value(value) + return _sanitize_cell_value(_stringify_cell_value(value)) if value is None: return "" if isinstance(value, enum.Enum): - return str(value.value) + return _sanitize_cell_value(str(value.value)) if isinstance(value, datetime) and value.tzinfo is not None: return value.strftime("%Y-%m-%d %H:%M:%S.%f") + if isinstance(value, str): + return _sanitize_cell_value(value) return value diff --git a/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py b/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py index 58bec1ddc9..688e7ba5f0 100644 --- a/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py +++ b/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py @@ -277,6 +277,71 @@ def test_generate_csv_report_quotes_target_age( assert '"5-8"' in csv_content +def test_generate_excel_report_strips_illegal_characters( + db: DatabaseTransactionFixture, +): + """Control characters in the data don't blow up Excel generation. + + Imported metadata sometimes carries stray control characters (e.g. a U+001F + unit separator inside a contributor name). openpyxl refuses to write them, + so they are stripped rather than failing the whole report. + """ + query = select( + literal("Sofi\x1fa Pescarin").label("author"), + literal("Ti\x0btle").label("title"), + literal("line1\nline2\ttabbed").label("notes"), + ) + + excel_file = io.BytesIO() + excel_file.name = "test_report.xlsx" + + generate_excel_report( + db=db.session, + excel_file=excel_file, + sql_params={}, + query=query, + ) + + excel_file.seek(0) + wb = load_workbook(io.BytesIO(excel_file.getvalue())) + ws = wb.active + assert ws is not None + headers = [ws.cell(row=1, column=c).value for c in range(1, ws.max_column + 1)] + values = { + header: ws.cell(row=2, column=index + 1).value + for index, header in enumerate(headers) + } + assert values["author"] == "Sofia Pescarin" + assert values["title"] == "Title" + # Tab, newline and carriage return are legal and must be preserved. + assert values["notes"] == "line1\nline2\ttabbed" + + +def test_generate_csv_report_strips_illegal_characters( + db: DatabaseTransactionFixture, +): + """The CSV output is sanitized identically to the Excel output.""" + query = select( + literal("Sofi\x1fa Pescarin").label("author"), + literal(9780306406157).label("identifier"), + ) + + csv_file = io.StringIO() + csv_file.name = "test_report.csv" + + generate_csv_report( + db=db.session, + csv_file=csv_file, + sql_params={}, + query=query, + columns_to_stringify={"identifier"}, + ) + + csv_file.seek(0) + rows = list(csv.reader(io.StringIO(csv_file.getvalue()))) + assert rows[1][0] == "Sofia Pescarin" + + def test_only_active_collections_are_included( db: DatabaseTransactionFixture, services_fixture: ServicesFixture ): From 91074b836e98e9e352c178fef35fa6f97f1cb0fb Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Thu, 24 Sep 2026 13:07:20 -0700 Subject: [PATCH 2/4] Use a generic placeholder name in the sanitization tests The test data was lifted verbatim from the production traceback, which put a real person's name in the codebase. Replace it with a generic placeholder; the control character being exercised is unchanged. Co-Authored-By: Claude Opus 5 --- .../tasks/test_generate_inventory_and_hold_reports.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py b/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py index 688e7ba5f0..e2b8a60043 100644 --- a/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py +++ b/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py @@ -287,7 +287,7 @@ def test_generate_excel_report_strips_illegal_characters( so they are stripped rather than failing the whole report. """ query = select( - literal("Sofi\x1fa Pescarin").label("author"), + literal("Ja\x1fne Doe").label("author"), literal("Ti\x0btle").label("title"), literal("line1\nline2\ttabbed").label("notes"), ) @@ -311,7 +311,7 @@ def test_generate_excel_report_strips_illegal_characters( header: ws.cell(row=2, column=index + 1).value for index, header in enumerate(headers) } - assert values["author"] == "Sofia Pescarin" + assert values["author"] == "Jane Doe" assert values["title"] == "Title" # Tab, newline and carriage return are legal and must be preserved. assert values["notes"] == "line1\nline2\ttabbed" @@ -322,7 +322,7 @@ def test_generate_csv_report_strips_illegal_characters( ): """The CSV output is sanitized identically to the Excel output.""" query = select( - literal("Sofi\x1fa Pescarin").label("author"), + literal("Ja\x1fne Doe").label("author"), literal(9780306406157).label("identifier"), ) @@ -339,7 +339,7 @@ def test_generate_csv_report_strips_illegal_characters( csv_file.seek(0) rows = list(csv.reader(io.StringIO(csv_file.getvalue()))) - assert rows[1][0] == "Sofia Pescarin" + assert rows[1][0] == "Jane Doe" def test_only_active_collections_are_included( From 07b7752ae926beca3982a795576efca78fd3f7fb Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Thu, 24 Sep 2026 19:57:09 -0700 Subject: [PATCH 3/4] Cover the stringified-column branch of the sanitizer _cell_value sanitizes along two paths, and only the plain-string one was asserted. The stringified-column path (identifier/isbn/target_age, which come from imported feeds just like author) was executed by existing tests but never checked, so a regression there would have gone unnoticed -- line coverage was satisfied while the behavior was not. Put a control character in a stringified column in both the CSV and Excel tests and assert it is removed. Verified by mutation: dropping the sanitize call from that branch now fails both tests. Co-Authored-By: Claude Opus 5 --- .../test_generate_inventory_and_hold_reports.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py b/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py index e2b8a60043..f3dc0a4d23 100644 --- a/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py +++ b/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py @@ -290,6 +290,7 @@ def test_generate_excel_report_strips_illegal_characters( literal("Ja\x1fne Doe").label("author"), literal("Ti\x0btle").label("title"), literal("line1\nline2\ttabbed").label("notes"), + literal("97803064\x1f06157").label("identifier"), ) excel_file = io.BytesIO() @@ -300,6 +301,7 @@ def test_generate_excel_report_strips_illegal_characters( excel_file=excel_file, sql_params={}, query=query, + columns_to_stringify={"identifier"}, ) excel_file.seek(0) @@ -312,6 +314,8 @@ def test_generate_excel_report_strips_illegal_characters( for index, header in enumerate(headers) } assert values["author"] == "Jane Doe" + # Stringified columns go through a separate branch of the sanitizer. + assert values["identifier"] == "9780306406157" assert values["title"] == "Title" # Tab, newline and carriage return are legal and must be preserved. assert values["notes"] == "line1\nline2\ttabbed" @@ -320,10 +324,14 @@ def test_generate_excel_report_strips_illegal_characters( def test_generate_csv_report_strips_illegal_characters( db: DatabaseTransactionFixture, ): - """The CSV output is sanitized identically to the Excel output.""" + """The CSV output is sanitized identically to the Excel output. + + ``identifier`` is a stringified column, so it exercises the + ``_stringify_cell_value`` branch of the sanitizer as well. + """ query = select( literal("Ja\x1fne Doe").label("author"), - literal(9780306406157).label("identifier"), + literal("97803064\x1f06157").label("identifier"), ) csv_file = io.StringIO() @@ -339,7 +347,7 @@ def test_generate_csv_report_strips_illegal_characters( csv_file.seek(0) rows = list(csv.reader(io.StringIO(csv_file.getvalue()))) - assert rows[1][0] == "Jane Doe" + assert rows[1] == ["Jane Doe", "9780306406157"] def test_only_active_collections_are_included( From 017a88f1aa4c7084ff15fda20bb4731093b2e0c6 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 28 Sep 2026 15:48:26 -0700 Subject: [PATCH 4/4] Don't sanitize enum values Per review: enum values reaching the reports are LicensePoolStatus and LicenseStatus, both declared as literals in our own source. They can't carry illegal characters, and LicenseStatus.get() maps any unrecognized feed value onto a known member, so feed data never becomes an arbitrary enum value. Sanitizing them implied a trust boundary that isn't there, and the branch couldn't be meaningfully tested without inventing a fake enum. Co-Authored-By: Claude Opus 5 --- .../celery/tasks/generate_inventory_and_hold_reports.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py b/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py index 5e58bf0dd0..2942d2c0f3 100644 --- a/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py +++ b/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py @@ -288,15 +288,17 @@ def _cell_value(key: str, value: Any, stringify_cols: frozenset[str]) -> Any: For columns in stringify_cols, forces the value to a plain string. Enum values are converted using their .value attribute. Timezone-aware datetimes are formatted as strings for Excel compatibility. - String values are stripped of characters that Excel disallows, so that the - CSV and Excel outputs stay identical. + String values from the database are stripped of characters that Excel + disallows, so that the CSV and Excel outputs stay identical. """ if key in stringify_cols: return _sanitize_cell_value(_stringify_cell_value(value)) if value is None: return "" if isinstance(value, enum.Enum): - return _sanitize_cell_value(str(value.value)) + # Enum values are declared in our own source, so they can't carry + # illegal characters and don't need sanitizing. + return str(value.value) if isinstance(value, datetime) and value.tzinfo is not None: return value.strftime("%Y-%m-%d %H:%M:%S.%f") if isinstance(value, str):