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)