Skip to content

fix(data): robust CSV parser, migration run state machine, shell script hardening#11

Open
amar-python wants to merge 1 commit into
feature/schema-validationfrom
fix/data-integrity-safety
Open

fix(data): robust CSV parser, migration run state machine, shell script hardening#11
amar-python wants to merge 1 commit into
feature/schema-validationfrom
fix/data-integrity-safety

Conversation

@amar-python

Copy link
Copy Markdown
Owner

Data Integrity & Safety (High Priority)

Based on feature/schema-validation (PR #7) since that branch contains the restructured backend/ layout these fixes target.

4. Robust CSV Parser

  • New backend/services/csv_parser.py — dedicated CsvParser class extracted from migration_service._parse_csv_metadata (which previously swallowed every exception with a bare except: pass). It explicitly handles:
    • zero-byte / whitespace-only files (empty_file error)
    • ragged rows — data rows whose column count differs from the header, reported with line numbers
    • encoding mismatches — UTF-8 (BOM-aware) with Latin-1 fallback + explicit encoding_fallback warning
    • malicious header names — ;, --, quotes, backslashes, comment markers, and standalone SQL keywords (DROP, DELETE, …) are flagged as suspicious_header errors; legitimate headers like drop_zone_id still pass
  • migration_service now delegates to it and logs every parse issue instead of silently ignoring it.
  • backend/tests/test_csv_parser.py — 28 unit tests covering all four failure classes plus happy paths.

5. Migration Lifecycle State Machine

  • RunStatus gains explicit READY and ERROR states (joining CREATED / UPLOADING / VALIDATING / MIGRATING / COMPLETED / FAILED).
  • New explicit ALLOWED_TRANSITIONS map + can_transition() + MigrationRun.transition_to(), which raises InvalidStateTransition on illegal moves (e.g. COMPLETED → UPLOADING).
  • Upload failure mid-way now flags the run as ERROR (previously it lingered in UPLOADING); the run recovers by re-uploading.
  • Validation now ends in READY (passed) or ERROR (failed) — previously it always reverted to CREATED, even on failure (RunStatus.CREATED if passed else RunStatus.CREATED bug).
  • All services (migration_service, schema_service, execution_service) route status changes through transition_to().
  • Frontend dashboard chip colors updated for the new states.
  • backend/tests/test_run_lifecycle.py — transition-rule matrix, transition_to guard tests, and API-level tests including a simulated mid-upload disk failure → ERROR → recovery.

6. Shell Script Hardening (backend/migration/build/)

  • csv_loader.sh: strict table-name validator after sanitisation — ^[a-zA-Z_][a-zA-Z0-9_]*$ (blocks ;, --, quotes, spaces) plus PostgreSQL's 63-char identifier limit; --env token validated before use.
  • csv_utilise.sh: --env token validated; eval echo "\$PG_DB_${E}" replaced with safe bash indirect expansion (${!var}) — removes a shell-injection path. (Its table-name validator already existed and is verified by tests.)
  • Both scripts pass shellcheck with no word-splitting (SC2086) findings; all variable expansions double-quoted.

Verification

  • pytest backend/tests/108 passed (38 existing + 70 new)
  • pytest -m "unit or regression or security or snapshot" backend/migration/tests/39 passed (no regressions in legacy loader/utilise tests)
  • Manual checks: csv_loader.sh '/tmp/bad;drop.csv' → rejected; --env 'dev; rm -rf /tmp/x' → rejected by both scripts; valid config still resolves DB/schema via indirect expansion
  • bash -n + shellcheck clean; flake8 clean on all new Python

Checklist

  • Tests added
  • Verified workflow paths (no workflow changes in this PR)

…shell hardening

- Extract CSV parsing into services/csv_parser.py (CsvParser class) with
  explicit handling of zero-byte files, ragged rows, encoding fallback
  (UTF-8 BOM -> Latin-1 with warning) and SQL-injection-style headers;
  migration_service now delegates to it and logs every issue
- Add READY and ERROR states to RunStatus with an explicit allowed-
  transition map, can_transition() and MigrationRun.transition_to()
  raising InvalidStateTransition on illegal moves
- Flag runs as ERROR when an upload fails mid-way (with recovery path);
  validation now ends in READY (passed) or ERROR (failed)
- Harden csv_loader.sh: strict table-name regex (blocks ';', '--',
  quotes, spaces) + 63-char limit; validate --env token in both
  csv_loader.sh and csv_utilise.sh; replace eval with indirect
  expansion in csv_utilise.sh
- Frontend: status chip colors for new 'ready'/'error' states
- Tests: test_csv_parser.py (28 cases) + test_run_lifecycle.py (39 cases)
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.

1 participant