From 7d683eb2464a61318b2fed52ddfd45ff646cee94 Mon Sep 17 00:00:00 2001 From: Donkie Date: Mon, 27 Jul 2026 19:58:52 +0200 Subject: [PATCH] security: escape CSV formulas and bound extra-field sizes Two lower-severity findings from the audit (todo.md tasks 6 and 7). A spreadsheet executes a cell that begins with =, +, -, @, tab or CR, so a vendor named `=cmd|' /C calc'!A0` runs when someone opens the CSV export (CWE-1236). Such cells are now prefixed with a single quote, which spreadsheets read as "this is literal text" and do not display. Only strings are escaped -- numbers reach the writer as numeric types, so a negative number cannot be mistaken for a formula and must not grow a stray quote. Header names need no escaping: they are fixed attribute names or extra-field keys, which are already constrained to ^[a-z0-9_]+$. Exports also gained Content-Disposition, so they download under a sensible name rather than rendering in the browser. Extra-field values had no size limit and the columns are Text(), so a single request could store megabytes: unbounded database growth, and the resulting websocket event overflowed the 1 MiB default frame limit, which killed live updates for every connected client. Values are now capped at the same 65535 characters as settings. The number of extra fields per entity is capped too -- the per-entity key count was already bounded, since unknown keys are rejected, but the registry itself could be grown without limit, and it is cached in memory and embedded in every entity response. Finally, a setting larger than the column limit returned 500; the ValueError is now caught and returned as a 400. Verified end-to-end: a 2 MB extra value is refused on both create and update, an oversized setting returns 400 and leaves the setting unset, normal values still work, and a vendor named `=cmd|' /C calc'!A0` exports as `'=cmd|' /C calc'!A0` while the JSON export keeps the raw name. --- spoolman/api/v1/export.py | 16 ++++++---- spoolman/api/v1/setting.py | 6 +++- spoolman/export.py | 28 ++++++++++++++++-- spoolman/extra_field_registry.py | 27 ++++++++++++++++- tests/test_export.py | 47 ++++++++++++++++++++++++++++++ tests/test_extra_field_registry.py | 41 ++++++++++++++++++++++++++ 6 files changed, 156 insertions(+), 9 deletions(-) create mode 100644 tests/test_export.py diff --git a/spoolman/api/v1/export.py b/spoolman/api/v1/export.py index 488d2fd2..f3b18ef8 100644 --- a/spoolman/api/v1/export.py +++ b/spoolman/api/v1/export.py @@ -40,7 +40,7 @@ async def export_spools( ] = False, ) -> Response: all_spools, _ = await spool.find(db=db, allow_archived=allow_archived) - return await _export(all_spools, fmt) + return await _export(all_spools, fmt, "spools") @router.get( @@ -54,7 +54,7 @@ async def export_filaments( fmt: ExportFormat, ) -> Response: all_filaments, _ = await filament.find(db=db) - return await _export(all_filaments, fmt) + return await _export(all_filaments, fmt, "filaments") @router.get( @@ -68,10 +68,10 @@ async def export_vendors( fmt: ExportFormat, ) -> Response: all_vendors, _ = await vendor.find(db=db) - return await _export(all_vendors, fmt) + return await _export(all_vendors, fmt, "vendors") -async def _export(objects: Iterable[Base], fmt: ExportFormat) -> Response: +async def _export(objects: Iterable[Base], fmt: ExportFormat, name: str) -> Response: """Export the objects in various formats.""" buffer = io.StringIO() media_type = "" @@ -85,4 +85,10 @@ async def _export(objects: Iterable[Base], fmt: ExportFormat) -> Response: else: raise ValueError(f"Unknown export format: {fmt}") - return Response(content=buffer.getvalue(), media_type=media_type) + return Response( + content=buffer.getvalue(), + media_type=media_type, + # Offer it as a download with a sensible name rather than letting the browser render it. + # `name` is a fixed literal per endpoint, never user input. + headers={"Content-Disposition": f'attachment; filename="{name}.{fmt.value}"'}, + ) diff --git a/spoolman/api/v1/setting.py b/spoolman/api/v1/setting.py index 1f3aaed3..3d68519f 100644 --- a/spoolman/api/v1/setting.py +++ b/spoolman/api/v1/setting.py @@ -206,7 +206,11 @@ async def update( except ValueError as e: return JSONResponse(status_code=400, content=Message(message=str(e)).dict()) - await setting.update(db=db, definition=definition, value=body) + try: + # Raised when the value exceeds the column limit; a client error, not a server one. + await setting.update(db=db, definition=definition, value=body) + except ValueError as e: + return JSONResponse(status_code=400, content=Message(message=str(e)).dict()) logger.info('Setting "%s" has been set to "%s".', key, body) else: await setting.delete(db=db, definition=definition) diff --git a/spoolman/export.py b/spoolman/export.py index db3bf0a1..0b985260 100644 --- a/spoolman/export.py +++ b/spoolman/export.py @@ -13,6 +13,29 @@ if TYPE_CHECKING: banned_attrs = {"awaitable_attrs", "metadata", "registry", "spools", "filaments"} +# A spreadsheet treats a cell starting with any of these as a formula, so a vendor named +# `=cmd|' /C calc'!A0` executes when the export is opened (CWE-1236). Prefixing with a single +# quote makes the spreadsheet read it as literal text; the quote is not part of the value. +FORMULA_PREFIXES = ("=", "+", "-", "@", "\t", "\r") + + +def escape_csv_value(value: Any) -> Any: # noqa: ANN401 + """Neutralize a value that a spreadsheet would otherwise interpret as a formula. + + Only strings are escaped. Numbers reach the writer as numeric types, so a negative number + cannot be mistaken for a formula and must not grow a stray quote. + + Args: + value: The cell value. + + Returns: + Any: The value, prefixed with a single quote if it would be read as a formula. + + """ + if isinstance(value, str) and value.startswith(FORMULA_PREFIXES): + return "'" + value + return value + async def flatten_sqlalchemy_object(obj: models.Base, parent_key: str = "", sep: str = ".") -> dict[str, Any]: """Recursively flattens a SQLAlchemy object into a dictionary with dot-separated keys.""" @@ -50,11 +73,12 @@ async def dump_as_csv(sqlalchemy_objects: Iterable[models.Base], writer: "Suppor headers = sorted(headers) # Sort headers for consistent column ordering - # Write to CSV + # Write to CSV. Header names need no escaping: they are either fixed attribute names or + # extra-field keys, which are constrained to ^[a-z0-9_]+$. csv_writer = csv.DictWriter(writer, fieldnames=headers) csv_writer.writeheader() for flattened_obj in all_flattened: - csv_writer.writerow(flattened_obj) + csv_writer.writerow({key: escape_csv_value(value) for key, value in flattened_obj.items()}) async def dump_as_json(sqlalchemy_objects: Iterable[models.Base], writer: "SupportsWrite[str]") -> None: diff --git a/spoolman/extra_field_registry.py b/spoolman/extra_field_registry.py index 957ae6ac..e6b7bf0a 100644 --- a/spoolman/extra_field_registry.py +++ b/spoolman/extra_field_registry.py @@ -57,8 +57,24 @@ class ExtraField(ExtraFieldParameters): entity_type: EntityType = Field(description="Entity type this field is for") -def validate_extra_field_value(field: ExtraFieldParameters, value: str) -> None: # noqa: C901, PLR0912 +# Matches the settings cap in spoolman/database/setting.py. The columns are Text(), so nothing +# stopped a single request storing megabytes: unbounded database growth, and the resulting +# websocket event overflowed the 1 MiB default frame limit, killing live updates for every client. +EXTRA_FIELD_VALUE_MAX_LENGTH = 2**16 - 1 + +# Extra fields are a per-entity schema, not a data store. A few dozen is already generous, and a +# bound keeps the registry -- which is cached in memory and embedded in every entity response -- +# from being grown without limit. +MAX_EXTRA_FIELDS_PER_ENTITY = 128 + + +def validate_extra_field_value(field: ExtraFieldParameters, value: str) -> None: # noqa: C901, PLR0912, PLR0915 """Validate that the value has the correct type.""" + if len(value) > EXTRA_FIELD_VALUE_MAX_LENGTH: + raise ValueError( + f"Value is too big, max size is {EXTRA_FIELD_VALUE_MAX_LENGTH} characters.", + ) + try: data = json.loads(value) except json.JSONDecodeError: @@ -193,6 +209,11 @@ def validate_extra_field_setting(key: str, value: str) -> None: # ValueError, not TypeError: this is a validation failure the caller turns into a 400. raise ValueError(f"Setting {key} must be an array of extra fields.") # noqa: TRY004 + if len(fields) > MAX_EXTRA_FIELDS_PER_ENTITY: + raise ValueError( + f"Setting {key} has {len(fields)} extra fields, the maximum is {MAX_EXTRA_FIELDS_PER_ENTITY}.", + ) + for index, obj in enumerate(fields): try: field = ExtraField.model_validate(obj) @@ -270,6 +291,10 @@ async def add_or_update_extra_field(db: AsyncSession, entity_type: EntityType, e raise ValueError("Cannot remove existing choices.") extra_fields = [field for field in extra_fields if field.key != extra_field.key] + if len(extra_fields) >= MAX_EXTRA_FIELDS_PER_ENTITY: + raise ValueError( + f"Cannot add another extra field for {entity_type.name}, the maximum is {MAX_EXTRA_FIELDS_PER_ENTITY}.", + ) extra_fields.append(extra_field) setting_def = parse_setting(f"extra_fields_{entity_type.name}") diff --git a/tests/test_export.py b/tests/test_export.py new file mode 100644 index 00000000..2898bbcc --- /dev/null +++ b/tests/test_export.py @@ -0,0 +1,47 @@ +"""Tests for CSV formula injection in exports. + +A spreadsheet executes a cell that starts with `=`, so a vendor named `=cmd|' /C calc'!A0` runs +when someone opens the exported file (CWE-1236). The attacker only needs to get the string into +the database; the victim is whoever opens the export. +""" + +import pytest + +from spoolman.export import escape_csv_value + + +@pytest.mark.parametrize( + "value", + [ + "=cmd|' /C calc'!A0", + "=1+1", + "+1", + "-1+1", + "@SUM(A1)", + "\tleading tab", + "\rleading carriage return", + ], +) +def test_formula_like_values_are_escaped(value: str): + assert escape_csv_value(value) == "'" + value + + +@pytest.mark.parametrize( + "value", + [ + "Prusament", + "eSUN PLA+", + "", + "a = b", + "1+1", + "spool-2", + ], +) +def test_ordinary_values_are_untouched(value: str): + assert escape_csv_value(value) == value + + +@pytest.mark.parametrize("value", [-1, -1.5, 0, 42, None, True]) +def test_non_strings_are_untouched(value: object): + """Numbers reach the writer as numeric types, so a negative number must not grow a quote.""" + assert escape_csv_value(value) is value diff --git a/tests/test_extra_field_registry.py b/tests/test_extra_field_registry.py index 07d5e657..e0a6c7f6 100644 --- a/tests/test_extra_field_registry.py +++ b/tests/test_extra_field_registry.py @@ -110,3 +110,44 @@ def test_invalidate_ignores_unrelated_settings(monkeypatch: pytest.MonkeyPatch): def test_invalidate_is_safe_when_nothing_is_cached(monkeypatch: pytest.MonkeyPatch): monkeypatch.setattr(registry, "extra_field_cache", {}) registry.invalidate_extra_field_cache("extra_fields_spool") + + +def test_oversized_value_is_rejected(): + """A 2 MB value on one spool used to be accepted: unbounded growth from a single request.""" + spec = ExtraField.model_validate(field()) + oversized = json.dumps("x" * (registry.EXTRA_FIELD_VALUE_MAX_LENGTH + 1)) + + with pytest.raises(ValueError, match="too big"): + registry.validate_extra_field_value(spec, oversized) + + +def test_value_at_the_limit_is_accepted(): + spec = ExtraField.model_validate(field()) + at_limit = json.dumps("x" * (registry.EXTRA_FIELD_VALUE_MAX_LENGTH - 2)) + assert len(at_limit) == registry.EXTRA_FIELD_VALUE_MAX_LENGTH + + registry.validate_extra_field_value(spec, at_limit) + + +def test_oversized_value_is_rejected_through_the_dict_validator(): + """The spool/filament/vendor endpoints go through here, and they 400 on ValueError.""" + spec = ExtraField.model_validate(field()) + oversized = json.dumps("x" * (registry.EXTRA_FIELD_VALUE_MAX_LENGTH + 1)) + + with pytest.raises(ValueError, match="too big"): + registry.validate_extra_field_dict([spec], {"batch": oversized}) + + +def test_too_many_fields_are_rejected(): + payload = json.dumps( + [field(key=f"f{i}", name=f"Field {i}") for i in range(registry.MAX_EXTRA_FIELDS_PER_ENTITY + 1)], + ) + with pytest.raises(ValueError, match="maximum is"): + registry.validate_extra_field_setting("extra_fields_spool", payload) + + +def test_field_count_at_the_limit_is_accepted(): + payload = json.dumps( + [field(key=f"f{i}", name=f"Field {i}") for i in range(registry.MAX_EXTRA_FIELDS_PER_ENTITY)], + ) + registry.validate_extra_field_setting("extra_fields_spool", payload)