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

[dbernstein]

Description

Stops attacker-influenced report values from being written as live spreadsheet formulas.

Follows #3768 (PP-5246), now merged; this branch is rebased onto main and contains only its own two commits.

The two output formats need different mitigations, so the escaping lives in the writers rather than the shared _cell_value:

• xlsx — pin str values to the text data type (cell.data_type = "s"). No data is altered. Worth calling out: the number_format = FORMAT_TEXT that the stringified columns already carry is presentation only and does not prevent formula evaluation — identifier, isbn and target_age were unprotected despite appearing otherwise.
• csv — prefix the conventional apostrophe. CSV carries no cell-type metadata, so neutralizing a formula there necessarily changes the value. The apostrophe is visible in the CSV: Excel only hides one that was typed into a cell, so on import it is part of the value, and any script reading the file gets it too. Blocking the formula is worth that cost, but the data really does change. The trigger set is wider than xlsx's (= + - @ and leading tab/CR) because Excel's CSV importer acts on all of them, while its xlsx parser only treats = as a formula. Only str values are touched, so genuine numbers keep their type and negative numbers are unaffected.

This means the CSV and Excel outputs now differ by that one apostrophe — a partial walk-back of the "identical outputs" guarantee previously documented on _write_reports. That is unavoidable in a format without types; the docstrings on _write_reports, _cell_value and _escape_csv_formula now all say so explicitly, so nobody later "fixes" the difference away or writes a test comparing the two files cell by cell.

Motivation and Context

Raised by Greptile on #3768. It is pre-existing behavior on main, not a regression from that PR — openpyxl stores any =-prefixed string as a formula cell however it arrived. #3768 only changed the narrow <control char>=… case, which previously crashed the report outright; a payload with no control-character prefix already works on main today. Splitting the hardening out keeps the crash fix reviewable on its own.

Verified against openpyxl directly:

'=HYPERLINK(...)' , no number format  -> data_type='f'   (formula)
'=HYPERLINK(...)' , FORMAT_TEXT       -> data_type='f'   (still a formula)
'=HYPERLINK(...)' , data_type='s'     -> data_type='s'   (text, value intact)

Risk in context. The reachable path is a compromised or malicious content vendor injecting into an imported feed; it is not reachable by patrons or anonymous users. The reports contain bibliographic metadata and aggregate counts only — Patron appears in the queries solely inside func.count(), so there is no patron PII to exfiltrate. Exploitation further requires staff to open the file in desktop Excel and click through Protected View, and modern Excel disables DDE by default while .xlsx cannot carry macros. So this is hardening against a low-likelihood vector rather than an urgent fix — but it is cheap and the phishing variant (=HYPERLINK() rendering an attacker-controlled link inside a trusted internal report) needs no DDE at all.

How Has This Been Tested?

New tests in tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py:

• Excel: a =HYPERLINK(...) payload is stored with data_type == "s" and its text preserved verbatim.
• CSV: parametrized over all six escaped prefixes (= + - @ plus tab and carriage return) with two negative cases (5-8, a plain name) confirming ordinary values are left alone.
• CSV: a genuine negative number (-5, -5.5) stays an unquoted number while the string "-5" is escaped. Asserted against the raw CSV text, since csv.reader yields strings either way and would hide the difference.

Review feedback from Claude, Greptile and @tdilauro is addressed in the second commit.

All 30 tests in the file pass. mypy clean; pre-commit clean. generate_csv_report / generate_excel_report have no callers outside this task module, so the blast radius is limited to these reports.

Same local-environment caveat as #3768: tox -e py312-docker fails here building the MinIO image, so I ran pytest against a throwaway postgres:16 container with the PALACE_TEST_DATABASE_URL_* variables tox would set. CI will exercise the full environment.

Checklist

• I have updated the documentation accordingly.
• All new and existing tests passed.

🤖 Generated with Claude Code

[claude]

Claude finished @dbernstein's task in 1m 50s —— View job

---

Summary

The fix is correct, and the earlier review feedback has been addressed. openpyxl's write-only writer emits a cell with data_type == "s" as an inlineStr, so Excel reads =HYPERLINK(...) as text and the value is unchanged. The CSV prefix set matches OWASP's list (= + - @ \t \r). Because the isinstance(value, str) guard skips non-strings, the numeric columns keep their type: the Decimal hold ratio, the -1 sentinels and the date_part floats. target_age comes from numericrange_to_string and never starts with -, so it won't pick up an apostrophe it doesn't need. I found only two small follow-ups.

Details

Minor: tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py:368-382

The PR's main claim is that `identifier`, `isbn` and `target_age` were unprotected because `FORMAT_TEXT` doesn't stop openpyxl from storing a formula. This test never passes `columns_to_stringify`, though, so the `FORMAT_TEXT` path never runs. Suppose a later refactor moves `cell.data_type = "s"` into an `else` branch of `if key in stringify_cols`, which is exactly the mistake the new code comment warns against. This test would still pass. Add a stringified column to the query so the test covers that case:
```python
literal("=1+1").label("identifier"),
...
columns_to_stringify={"identifier"},
...
assert cells["identifier"].data_type == "s"
```
https://github.com/ThePalaceProject/circulation/blob/456150bae5e608bcabf3a6c3568e9602ecd5a899/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py#L368-L382

Nit: src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py:426-430

"The two files are no longer byte-identical" has two problems. A CSV and an xlsx file were never byte-identical, since they are different formats. And "no longer" describes this PR's history, which will read oddly once it merges. What the docstring means is that cell values match except for the CSV apostrophe, so say that directly. For example: "Cell values are identical in both files, except that formula-like strings get a leading apostrophe in the CSV (see `_escape_csv_formula`)…". The same phrase appears in the `_escape_csv_formula` docstring at line 305.
https://github.com/ThePalaceProject/circulation/blob/456150bae5e608bcabf3a6c3568e9602ecd5a899/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py#L426-L430

---

I couldn't run the tests or openpyxl here because this environment doesn't allow Python commands. The openpyxl behavior described above comes from reading its write-only cell writer (openpyxl/cell/_writer.py), not from running it.

[greptile-apps]

Confidence Score: 5/5

[Critical risk] Prevents formula injection in exported reports from untrusted data.

The PR appears safe to merge; no outstanding finding was identified.

Summary

The PR prevents imported report values from becoming spreadsheet formulas.

• Excel writes strings as text cells without changing their values.
• CSV prefixes formula-like strings with an apostrophe and documents the resulting difference between formats.
• Tests cover formula payloads, all six CSV prefixes, and negative numeric values.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Report query results] --> B[Shared cell conversion]
  B --> C[CSV writer]
  B --> D[Excel writer]
  C --> E[Prefix formula-like strings]
  D --> F[Store strings as text cells]

Loading

_{Reviews (5) · Last reviewed commit: "Merge branch 'main' into bugfix/report-f..."}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.75%. Comparing base (c5344ad) to head (456150b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3769      +/-   ##
==========================================
- Coverage   93.75%   93.75%   -0.01%     
==========================================
  Files         510      510              
  Lines       46470    46477       +7     
  Branches     6310     6312       +2     
==========================================
+ Hits        43568    43574       +6     
- Misses       1874     1875       +1     
  Partials     1028     1028              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tdilauro]

I gree with both of these comments from Claude.

Minor: src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py:410-413

This docstring still says the two writers "guarantee that the CSV and Excel files contain identical data". _cell_value's docstring (line 315) still says "the CSV and Excel outputs stay identical". Both are now false for any string starting with = + - @ \t \r. The PR description says the difference "is noted in the code", but neither of these docstrings nor _escape_csv_formula mentions it. Please update both docstrings, for example: "…identical data, except that formula-like strings in the CSV get a leading apostrophe (see _escape_csv_formula)". That way nobody later "fixes" the difference back or writes a test that compares the two files cell by cell.

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

Lines 410 to 413 in 6d28f57

	"""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.

Minor: src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py:296-298

The docstring says "the spreadsheet consumes it and displays the original text". Excel only hides a leading apostrophe when it's typed into a cell. When Excel or LibreOffice open a CSV, the apostrophe is imported as part of the value, so staff will see '=HYPERLINK(...). The apostrophe also stays in the data for any script that reads the CSV. Blocking the formula is still the right trade-off, but please reword the docstring (and the PR description) to say the apostrophe is visible in the CSV. Future maintainers should know the CSV data has actually changed.

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

Lines 296 to 298 in 6d28f57

	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.

[tdilauro]

greptile noted one of these, as well and added another, with which I also agree. I also think there should be a test for a simple negative number.

P2 Two CSV prefixes lack tests The new test covers four of the six protected prefixes, but not tab or carriage return. The sanitizer preserves both, so adding cases for them would catch a regression in those escape paths.

[tdilauro]

Agree with greptile here. And should probably test a regular old negative number here, too (e.g., Literal(5)).

[dbernstein]

I will make the set of changes suggested by Claude, Greptile and you @tdilauro (as well as resolve the conflicts). Thanks for the review.

[dbernstein]

@tdilauro : I made the changes suggested by Claude, Greptile and yourself (as well as resolve the conflicts). This one should be good now.