bastian/mywhoosh2garmin
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: harden FIT patcher error boundary and verify patched metadata

Browse Source 
Final-review fixes for Plan 2 (fit-rewriter). Every failure mode below now
surfaces as FitFormatError so Plan 3 can classify invalid FIT input as a
non-retryable activity error (spec 10.4).

- Range-check numeric values against the field's declared size before
  struct.pack, so an oversized serial number or a 1-byte product field
  raises FitFormatError instead of leaking a raw struct.error.
- Reject zero-size field definitions during parsing. A zero-size
  device_info field 0 read back as device_index == 0 via
  int.from_bytes(b"", ...), which could have let a paired sensor be
  rewritten as an Edge 1030 Plus (spec 10.2).
- Add DeviceFieldValue.is_creator so callers can tell the creator
  device_info record from sensor records instead of silently keeping
  whichever record appeared last.
- Implement the missing spec 10.4 post-patch step: read the patched
  buffer back and verify file_id 1/2/8 and creator device_info 2/4/27
  hold the target values. A field that could not be written (e.g. a
  product_name field too small for the target string) now fails the whole
  conversion rather than producing a silent partial patch. Verification
  runs before the output is written, so a half-rewritten file never lands
  on disk.
- Use the field's actual endianness in _read_field_value's fallback path.
- Add curated re-exports in app/fit/__init__.py for Plan 3.
- Document _iter_data_fields' caller invariant (validate the container
  first; end_offset is not clamped).
- Extend the preservation fixture with a product_name string field so the
  zero-filling string write path is covered by the byte-preservation
  proof, and test convert_fit_device against a 12-byte header.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Bastian Wagner
2026-08-15 14:29:40 +02:00
parent 95aa8507f0
commit ef9ca9eeca
5 changed files with 343 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

148
app/fit/rewriter.py
View File

@@ -13,6 +13,14 @@ from app.fit.models import (
FILE_ID_MESG_NUM = 0
DEVICE_INFO_MESG_NUM = 23
# Device-identity fields read back / patched per message type.
FILE_ID_DEVICE_FIELDS = frozenset({1, 2, 3, 8})
DEVICE_INFO_DEVICE_FIELDS = frozenset({2, 3, 4, 27})
# Maximum unsigned value per declared FIT field size, used to reject values that
# cannot be represented in the field the source file actually declares.
_MAX_UNSIGNED_BY_SIZE = {1: 0xFF, 2: 0xFFFF, 4: 0xFFFFFFFF}
class FitFormatError(ValueError):
pass
@@ -64,7 +72,15 @@ def _read_definition(
for _ in range(field_count):
if offset + 3 > end_offset:
raise FitFormatError("Truncated FIT field definition")
fields.append(FieldDefinition(data[offset], data[offset + 1], data[offset + 2]))
field_num = data[offset]
field_size = data[offset + 1]
# A zero-size field is illegal FIT. Rejecting it here (rather than
# special-casing it downstream) closes a hole where a zero-size
# device_info field 0 would read back as device_index == 0 via
# int.from_bytes(b"", ...) and make a sensor record look like the creator.
if field_size == 0:
raise FitFormatError(f"FIT field {field_num} declares an invalid size of 0")
fields.append(FieldDefinition(field_num, field_size, data[offset + 2]))
offset += 3
developer_field_size = 0
@@ -103,6 +119,13 @@ def _collect_field_offsets(
def _iter_data_fields(
data: bytearray,
) -> list[tuple[LocalDefinition, list[tuple[FieldDefinition, int]]]]:
"""Walk the FIT data section and return every data record with its field offsets.
Caller invariant: callers MUST run ``_validate_fit_container(data)`` first. The
header-derived ``end_offset`` is trusted as-is and never clamped to ``len(data)``,
so an unvalidated buffer whose declared data size exceeds its real length would be
parsed out of bounds instead of rejected cleanly.
"""
header_size = data[0]
data_size = struct.unpack_from("<I", data, 4)[0]
offset = header_size
@@ -162,6 +185,7 @@ def convert_fit_device(
header_crc = _rewrite_header_crc(data)
file_crc = _rewrite_file_crc(data)
_validate_fit_container(data)
_verify_patched_metadata(data, resolved)
output_path.parent.mkdir(parents=True, exist_ok=True)
output_path.write_bytes(data)
return FitConversionResult(source_path, output_path, patched_count, header_crc, file_crc)
@@ -170,46 +194,60 @@ def convert_fit_device(
def read_device_field_values(path: Path) -> list[DeviceFieldValue]:
data = bytearray(path.read_bytes())
_validate_fit_container(data)
return _read_device_field_values(data)
def _read_device_field_values(data: bytearray) -> list[DeviceFieldValue]:
values: list[DeviceFieldValue] = []
for definition, field_offsets in _iter_data_fields(data):
if definition.global_message_num == FILE_ID_MESG_NUM:
interesting_fields = FILE_ID_DEVICE_FIELDS
is_creator = True
elif definition.global_message_num == DEVICE_INFO_MESG_NUM:
interesting_fields = DEVICE_INFO_DEVICE_FIELDS
field_map = {field.num: (field, offset) for field, offset in field_offsets}
is_creator = _is_creator_device_info(data, definition, field_map)
else:
continue
for field, offset in field_offsets:
if definition.global_message_num == FILE_ID_MESG_NUM and field.num in {1, 2, 3, 8}:
values.append(
DeviceFieldValue(
definition.global_message_num,
field.num,
_read_field_value(data, offset, field, definition.endian),
)
)
if definition.global_message_num == DEVICE_INFO_MESG_NUM and field.num in {
2,
3,
4,
27,
}:
values.append(
DeviceFieldValue(
definition.global_message_num,
field.num,
_read_field_value(data, offset, field, definition.endian),
)
if field.num not in interesting_fields:
continue
values.append(
DeviceFieldValue(
definition.global_message_num,
field.num,
_read_field_value(data, offset, field, definition.endian),
is_creator,
)
)
return values
def _is_creator_device_info(
data: bytearray,
definition: LocalDefinition,
field_map: dict[int, tuple[FieldDefinition, int]],
) -> bool:
"""A ``device_info`` record counts as the creator only when it carries an
explicit device_index (field 0) equal to 0. Records without field 0 are never
treated as the creator, so paired sensors are never rewritten."""
device_index_entry = field_map.get(0)
if device_index_entry is None:
return False
index_field, index_offset = device_index_entry
return _read_field_value(data, index_offset, index_field, definition.endian) == 0
def _patch_device_metadata(data: bytearray, device: GarminDevice) -> int:
patched_count = 0
eligible_field_count = 0
for definition, field_offsets in _iter_data_fields(data):
field_map = {field.num: (field, offset) for field, offset in field_offsets}
if definition.global_message_num == DEVICE_INFO_MESG_NUM:
device_index_entry = field_map.get(0)
if device_index_entry is None:
continue
index_field, index_offset = device_index_entry
device_index = _read_field_value(data, index_offset, index_field, definition.endian)
if device_index != 0:
continue
if definition.global_message_num == DEVICE_INFO_MESG_NUM and not _is_creator_device_info(
data, definition, field_map
):
continue
for field, offset in field_offsets:
target_value: int | str | None = None
@@ -242,6 +280,41 @@ def _patch_device_metadata(data: bytearray, device: GarminDevice) -> int:
return patched_count
def _expected_target_value(
global_message_num: int, field_num: int, device: GarminDevice
) -> int | str | None:
"""Target value a patched device-identity field must read back as, or None for
fields that are not verified (e.g. the optional serial number)."""
if global_message_num == FILE_ID_MESG_NUM:
return {1: device.manufacturer_id, 2: device.product_id, 8: device.product_name}.get(field_num)
if global_message_num == DEVICE_INFO_MESG_NUM:
return {2: device.manufacturer_id, 4: device.product_id, 27: device.product_name}.get(field_num)
return None
def _verify_patched_metadata(data: bytearray, device: GarminDevice) -> None:
"""Spec 10.4 post-patch step: verify the expected target metadata is readable.
Reads the patched buffer back and confirms every device-identity field that was
present in the source (file_id 1/2/8 and creator device_info 2/4/27) now holds
the target value. A field that could not be written -- e.g. a product_name field
too small for the target string -- fails the whole conversion with FitFormatError
instead of silently producing a half-rewritten file.
"""
for value in _read_device_field_values(data):
if not value.is_creator:
continue
expected = _expected_target_value(value.global_message_num, value.field_num, device)
if expected is None:
continue
if value.value != expected:
raise FitFormatError(
f"Patched FIT metadata verification failed for message "
f"{value.global_message_num} field {value.field_num}: "
f"expected {expected!r}, read back {value.value!r}"
)
def _read_field_value(data: bytearray, offset: int, field: FieldDefinition, endian: str) -> int | str:
base_type = field.base_type & 0x1F
if base_type in {0x03, 0x04, 0x0B} and field.size >= 2:
@@ -254,7 +327,7 @@ def _read_field_value(data: bytearray, offset: int, field: FieldDefinition, endi
raw = raw[: raw.index(0)]
return raw.decode("utf-8", errors="replace")
raw = bytes(data[offset : offset + field.size])
return int.from_bytes(raw, "little")
return int.from_bytes(raw, "little" if endian == "<" else "big")
def _write_field_value(
@@ -274,14 +347,23 @@ def _write_field_value(
data[offset : offset + field.size] = replacement
return True
max_value = _MAX_UNSIGNED_BY_SIZE.get(field.size)
if max_value is None:
return False
# Range-check before packing: struct.pack would otherwise raise a raw
# struct.error, which callers cannot classify alongside FitFormatError.
if not isinstance(value, int) or value < 0 or value > max_value:
raise FitFormatError(
f"Value {value!r} does not fit FIT field {field.num} "
f"of declared size {field.size} (allowed range 0-{max_value})"
)
if field.size == 1:
replacement = struct.pack("B", value)
elif field.size == 2:
replacement = struct.pack(f"{endian}H", value)
elif field.size == 4:
replacement = struct.pack(f"{endian}I", value)
else:
return False
replacement = struct.pack(f"{endian}I", value)
if bytes(data[offset : offset + field.size]) == replacement:
return False