Don't let imported metadata become spreadsheet formulas in reports (PP-5253)

src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py

@@ -284,14 +284,44 @@ 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).
 
     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 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)

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
 ):