security: guard delimited spreadsheet formulas - #404
codeforester wants to merge 2 commits into
Conversation
|
|
||
| return _table_cell(_cell_value(value)) | ||
| cell = _table_cell(_cell_value(value)) | ||
| if formula_guard and cell[:1] in {"=", "+", "-", "@"}: |
There was a problem hiding this comment.
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.
|
|
||
| return _table_cell(_cell_value(value)) | ||
| cell = _table_cell(_cell_value(value)) | ||
| if formula_guard and cell[:1] in {"=", "+", "-", "@"}: |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
|
Recall-biased review note (non-inline, file not part of this diff): No |
Fixes #380
Summary
formula_guard=Falseopt-out for trusted downstream consumersValidation
UV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra dev pytest -q tests/test_output.pyHosted checks are expected to run on this branch.