Skip to content

security: guard delimited spreadsheet formulas - #404

Open
codeforester wants to merge 2 commits into
mainfrom
security/380-20260930-security-csv-tsv-output-does-not-neutralize-spreadsheet-form
Open

codeforester wants to merge 2 commits into
mainfrom
security/380-20260930-security-csv-tsv-output-does-not-neutralize-spreadsheet-form

Conversation

@codeforester

Copy link
Copy Markdown
Contributor

Fixes #380

Summary

  • neutralize CSV/TSV cells beginning with spreadsheet formula characters by default
  • add an explicit formula_guard=False opt-out for trusted downstream consumers
  • document and test the CSV/TSV behavior

Validation

  • UV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra dev pytest -q tests/test_output.py
  • Ruff and strict mypy with the Typer extra pass locally

Hosted checks are expected to run on this branch.

Comment thread lib/python/base_cli/output.py Outdated

return _table_cell(_cell_value(value))
cell = _table_cell(_cell_value(value))
if formula_guard and cell[:1] in {"=", "+", "-", "@"}:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test-coverage gap (recall-biased review): The formula guard only checks cell[:1] in {"=", "+", "-", "@"}. Issue #380's acceptance criteria call for neutralizing leading =, +, -, @, tab, and CR, with a regression test for each prefix. Tab/CR are only incidentally neutralized today because _table_cell() (an unrelated ANSI/control-char cleanup step called on the line above) happens to convert them to spaces before this check runs. If _table_cell's control-character handling ever changes (e.g. to preserve literal tabs for some other feature), a value legitimately starting with a raw tab or CR followed by a formula-trigger character would flow through unguarded and untested, silently reopening CWE-1236 for that vector. Consider adding an explicit test asserting tab/CR-prefixed cells are guarded, independent of _table_cell's behavior.

Comment thread lib/python/base_cli/output.py Outdated

return _table_cell(_cell_value(value))
cell = _table_cell(_cell_value(value))
if formula_guard and cell[:1] in {"=", "+", "-", "@"}:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Efficiency (minor): {"=", "+", "-", "@"} is a set literal rebuilt on every call to _delimited_value, which runs once per cell on the CSV/TSV row-writing path — the same path whose docstring says it's built for 'large or long-running results.' The existing _ANSI_ESCAPE_RE module-level constant right above shows the established pattern for this file: hoist this as a module-level frozenset/constant (e.g. _FORMULA_TRIGGER_CHARS = frozenset({"=", "+", "-", "@"})) instead of reallocating it per cell.

rich: bool = False,
formula_guard: bool = True,
) -> str:
"""Render records according to the shared public output contract.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs gap: render_records's own docstring (the in-code reference, distinct from docs/output-contracts.md) wasn't updated to mention the new formula_guard parameter or its default behavior. A caller who only reads this docstring (e.g. via help()/IDE tooltip) won't learn that CSV/TSV cells starting with =+-@ are now silently prefixed with ' by default, or how to opt out — the same applies to render_document's docstring a bit further down. Worth a one-line addition here for discoverability.

@codeforester

Copy link
Copy Markdown
Contributor Author

Recall-biased review note (non-inline, file not part of this diff): No CHANGELOG.md entry was added under [Unreleased] for this change, even though every other recent entry in the file follows an Added/Changed/Fixed convention and this PR changes the default byte content of CSV/TSV output (prefixing ' on cells starting with =+-@). Since issue #380 targets the v0.5.0 milestone and this is a behavior change for the project's own 'automation-friendly' delimited-output contract, a ### Changed (or ### Security) bullet would help downstream consumers notice before upgrading that raw CSV/TSV values they parse programmatically may now be prefixed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security: CSV/TSV output does not neutralize spreadsheet formula injection

1 participant