From 5d70d058f9062cb8ebe8536215f8429f6884d615 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Thu, 24 Sep 2026 20:00:50 -0700 Subject: [PATCH 1/2] Don't let imported metadata become spreadsheet formulas Report values come from third-party OPDS/ODL feeds, so they are attacker-influenced. A title or contributor name beginning with "=" is written into the report as a live formula and evaluated when library staff open it -- CSV injection, reachable through the normal metadata import path. This is pre-existing behavior on main, not a regression: openpyxl stores any "="-prefixed string as a formula cell regardless of how it got there. The two formats need different mitigations: - xlsx: pin string values to the text data type. Note that the number_format the stringified columns already carry is presentation only and does NOT prevent formula evaluation -- identifier/isbn/target_age were unprotected despite looking otherwise. - csv: prefix the conventional apostrophe. CSV has no cell-type metadata, so neutralizing a formula there means changing the value; the spreadsheet consumes the apostrophe and displays the original text. The escape set is wider than xlsx's because Excel's CSV importer also acts on + - @ and leading whitespace controls. The outputs therefore differ by that apostrophe in the CSV, which is unavoidable in a format without types. Co-Authored-By: Claude Opus 5 --- .../generate_inventory_and_hold_reports.py | 35 ++++++++- ...est_generate_inventory_and_hold_reports.py | 78 +++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) 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 fe4966f0dd..90ca02ebaf 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 @@ -284,6 +284,29 @@ def _sanitize_cell_value(value: str) -> str: return ILLEGAL_CHARACTERS_RE.sub("", value) +# Leading characters that a spreadsheet application treats as the start of a +# formula when it parses a CSV. Excel's own xlsx parser only does this for "=", +# but its CSV importer (and Google Sheets) also act on these. +_CSV_FORMULA_PREFIXES = frozenset({"=", "+", "-", "@", "\t", "\r"}) + + +def _escape_csv_formula(value: Any) -> Any: + """Neutralize a value a spreadsheet would otherwise parse as a formula. + + Imported metadata is attacker-influenced -- it comes from third-party feeds -- + so a title or contributor name beginning with ``=`` would be evaluated when + library staff open the CSV in Excel or Sheets. Prefixing with an apostrophe is + the conventional mitigation; the spreadsheet consumes it and displays the + original text. + + Only ``str`` values are touched, so genuine numbers (written unquoted by + ``QUOTE_NONNUMERIC``) keep their type and negative numbers are unaffected. + """ + if isinstance(value, str) and value[:1] in _CSV_FORMULA_PREFIXES: + return f"'{value}" + return 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). @@ -318,7 +341,10 @@ def _write_csv_rows( writer.writerow(keys) for row in rows: writer.writerow( - [_cell_value(key, row.get(key, ""), stringify_cols) for key in keys] + [ + _escape_csv_formula(_cell_value(key, row.get(key, ""), stringify_cols)) + for key in keys + ] ) csv_file.flush() @@ -344,6 +370,13 @@ def _write_excel_rows( for key in keys: value = _cell_value(key, row.get(key, ""), stringify_cols) cell = WriteOnlyCell(ws, value=value) + if isinstance(value, str): + # openpyxl stores any "="-prefixed string as a formula cell, which + # Excel then evaluates. Pin string values to the text type so + # imported metadata can't become executable. This is a no-op for + # strings openpyxl already typed as text. Note that number_format + # is presentation only and does NOT prevent this. + cell.data_type = "s" if key in stringify_cols: cell.number_format = FORMAT_TEXT data_row.append(cell) 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 25b717279c..95adf71f08 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 @@ -355,6 +355,84 @@ def test_generate_csv_report_strips_illegal_characters( assert rows[1] == ["Jane Doe", "9780306406157"] +def test_generate_excel_report_does_not_create_formulas( + db: DatabaseTransactionFixture, +): + """Imported metadata is never written as an executable formula. + + openpyxl stores any "="-prefixed string as a formula cell, so a contributor + name or title from a third-party feed could otherwise be evaluated when + library staff open the report. + """ + payload = '=HYPERLINK("http://example.com/evil","Click")' + query = select( + literal(payload).label("author"), + literal("=1+1").label("title"), + literal("normal value").label("collection_name"), + ) + + 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) + ws = load_workbook(io.BytesIO(excel_file.getvalue())).active + assert ws is not None + headers = [ws.cell(row=1, column=c).value for c in range(1, ws.max_column + 1)] + cells = { + header: ws.cell(row=2, column=index + 1) for index, header in enumerate(headers) + } + + # Stored as text, not as a formula, and the text is preserved verbatim. + assert cells["author"].data_type == "s" + assert cells["author"].value == payload + assert cells["title"].data_type == "s" + assert cells["title"].value == "=1+1" + assert cells["collection_name"].value == "normal value" + + +@pytest.mark.parametrize( + "value,expected", + [ + pytest.param( + '=HYPERLINK("http://x","c")', '\'=HYPERLINK("http://x","c")', id="equals" + ), + pytest.param("+1+1", "'+1+1", id="plus"), + pytest.param("-1+1", "'-1+1", id="minus"), + pytest.param("@SUM(A1)", "'@SUM(A1)", id="at"), + pytest.param("5-8", "5-8", id="target-age-untouched"), + pytest.param("Jane Doe", "Jane Doe", id="plain-untouched"), + ], +) +def test_generate_csv_report_escapes_formulas( + db: DatabaseTransactionFixture, + value: str, + expected: str, +): + """Formula-like strings are neutralized in the CSV, ordinary values are not.""" + query = select(literal(value).label("author")) + + csv_file = io.StringIO() + csv_file.name = "test_report.csv" + + generate_csv_report( + db=db.session, + csv_file=csv_file, + sql_params={}, + query=query, + ) + + csv_file.seek(0) + rows = list(csv.reader(io.StringIO(csv_file.getvalue()))) + assert rows[1][0] == expected + + def test_only_active_collections_are_included( db: DatabaseTransactionFixture, services_fixture: ServicesFixture ): From 5d328692cf3a0ee3352fec4828754db36f901454 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 28 Sep 2026 16:05:54 -0700 Subject: [PATCH 2/2] Address review feedback Docstrings (Claude, Greptile): _write_reports and _cell_value still promised the CSV and Excel outputs were identical, which the formula escaping made false. Both now say what actually differs and why not to "fix" it. _escape_csv_formula also claimed the spreadsheet consumes the apostrophe and shows the original text. That is only true of an apostrophe typed into a cell -- on CSV import it is part of the value, so staff see '=HYPERLINK(...) and scripts reading the CSV get it too. Say so plainly; the CSV data really does change. Tests (Greptile): cover the tab and carriage-return prefixes, which were in the escape set but untested. Tests (Tim): a genuine negative number must not be mistaken for a formula. Asserted against the raw CSV text, because csv.reader yields strings either way and would hide the difference between an unquoted number and a quoted string. Co-Authored-By: Claude Opus 5 --- .../generate_inventory_and_hold_reports.py | 25 ++++++++++---- ...est_generate_inventory_and_hold_reports.py | 34 +++++++++++++++++++ 2 files changed, 53 insertions(+), 6 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 90ca02ebaf..54b944f6fb 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 @@ -296,11 +296,17 @@ def _escape_csv_formula(value: Any) -> Any: Imported metadata is attacker-influenced -- it comes from third-party feeds -- so a title or contributor name beginning with ``=`` would be evaluated when library staff open the CSV in Excel or Sheets. Prefixing with an apostrophe is - the conventional mitigation; the spreadsheet consumes it and displays the - original text. + the conventional mitigation. + + Note that this genuinely changes the CSV data: Excel only hides a leading + apostrophe that was typed into a cell, so on CSV import the apostrophe is part + of the value. Staff will see ``'=HYPERLINK(...)`` and any script reading the + CSV gets the apostrophe too. Blocking the formula is worth that cost, but the + CSV is no longer byte-identical to the xlsx (see ``_write_reports``). Only ``str`` values are touched, so genuine numbers (written unquoted by - ``QUOTE_NONNUMERIC``) keep their type and negative numbers are unaffected. + ``QUOTE_NONNUMERIC``) keep their type -- a negative *number* is unaffected, + though a negative number that arrives as a *string* is escaped. """ if isinstance(value, str) and value[:1] in _CSV_FORMULA_PREFIXES: return f"'{value}" @@ -314,7 +320,8 @@ def _cell_value(key: str, value: Any, stringify_cols: frozenset[str]) -> Any: Enum values are converted using their .value attribute. Timezone-aware datetimes are formatted as strings for Excel compatibility. String values from the database are stripped of characters that Excel - disallows, so that the CSV and Excel outputs stay identical. + disallows, so that both writers see the same value. The CSV writer may then + add a formula-escaping apostrophe on top of this; see ``_write_reports``. """ if key in stringify_cols: return _sanitize_cell_value(_stringify_cell_value(value)) @@ -413,8 +420,14 @@ def _write_reports( ) -> None: """Execute a query once and write both CSV and Excel from the buffered results. - This avoids running the same heavy query twice and guarantees that the CSV - and Excel files contain identical data from the same query execution. + This avoids running the same heavy query twice, so both files are built from + a single query execution. + + The two files are no longer byte-identical: formula-like strings get a leading + apostrophe in the CSV (see ``_escape_csv_formula``), while the xlsx blocks the + same formulas by pinning the cell to the text type and so keeps the value + unchanged. Don't "fix" that difference away, and don't compare the two files + cell by cell. """ stringify_cols = frozenset(columns_to_stringify or ()) keys, rows = _fetch_report_rows(db, query, sql_params, row_transform) 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 95adf71f08..ac5b24f7ba 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 @@ -406,6 +406,8 @@ def test_generate_excel_report_does_not_create_formulas( pytest.param("+1+1", "'+1+1", id="plus"), pytest.param("-1+1", "'-1+1", id="minus"), pytest.param("@SUM(A1)", "'@SUM(A1)", id="at"), + pytest.param("\tlead-tab", "'\tlead-tab", id="tab"), + pytest.param("\rlead-cr", "'\rlead-cr", id="carriage-return"), pytest.param("5-8", "5-8", id="target-age-untouched"), pytest.param("Jane Doe", "Jane Doe", id="plain-untouched"), ], @@ -433,6 +435,38 @@ def test_generate_csv_report_escapes_formulas( assert rows[1][0] == expected +def test_generate_csv_report_leaves_negative_numbers_numeric( + db: DatabaseTransactionFixture, +): + """A real negative number is not mistaken for a formula. + + "-" is in the CSV escape set, but only ``str`` values are escaped, so numeric + columns keep their type. Asserted on the raw CSV text rather than the parsed + row, because ``csv.reader`` yields strings either way and would hide the + difference between an unquoted number and a quoted string. + """ + query = select( + literal(-5).label("int_col"), + literal(-5.5).label("float_col"), + literal("-5").label("str_col"), + ) + + csv_file = io.StringIO() + csv_file.name = "test_report.csv" + + generate_csv_report( + db=db.session, + csv_file=csv_file, + sql_params={}, + query=query, + ) + + csv_file.seek(0) + data_line = csv_file.getvalue().splitlines()[1] + # Numbers stay unquoted and unescaped; only the string form is escaped. + assert data_line == '-5,-5.5,"\'-5"' + + def test_only_active_collections_are_included( db: DatabaseTransactionFixture, services_fixture: ServicesFixture ):