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..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 @@ -284,6 +284,35 @@ 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. + + 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 -- 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}" + 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). @@ -291,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)) @@ -318,7 +348,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 +377,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) @@ -380,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 25b717279c..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 @@ -355,6 +355,118 @@ 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("\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"), + ], +) +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_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 ):