Skip to content

fix: quote SQL identifiers in LAVA writer; path-safe report names - #1759

Merged
abrignoni merged 3 commits into
mainfrom
fix/lava-quote-identifiers
Jul 26, 2026
Merged

fix: quote SQL identifiers in LAVA writer; path-safe report names#1759
abrignoni merged 3 commits into
mainfrom
fix/lava-quote-identifiers

Conversation

@abrignoni

@abrignoni abrignoni commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Problem

Artifact data_headers that sanitize to a SQL reserved word (From, To, Order, Group, Index, Select, ...) failed at report time with:

Error was near "from": syntax error

The parser ran fine, so the only symptom was a LAVA table that silently never appeared. This was hit three times in one day while writing new modules (Mastodon notifications, Apple Mail) and worked around each time by renaming the column. admin/test/scripts/test_module.py mocks the LAVA database connection, so unit-level testing passed either way.

The same class of trap existed for a / in an artifact name or category: those become HTML/TSV/KML filenames and _HTML folder names, and os.path.join() reads the / as a path separator. A Twitter/X category made the artifact fail to write its report entirely.

Changes

  • scripts/lavafuncs.py — new quote_sql_name(), applied to the table name and every column in lava_create_sqlite_table() and both CREATE TABLE branches, plus lava_insert_sqlite_data(). Embedded double quotes are doubled per the SQL standard.
  • scripts/ilapfuncs.py / ileapp.py — new sanitize_report_name() replaces / and \ in artifact names and categories used as file and folder names, and logs a one-time warning naming the artifact. Display names, the "located at" line, timeline entries and the LAVA JSON all keep the original name; only the on-disk name changes. The icons dict keys use the same safe names so the sidebar icon lookup still matches. No currently shipping artifact contains a separator, so no existing output path changes.
  • admin/test/scripts/test_lava_sql_identifiers.py — 13 regression tests against a real SQLite file, no mocks, which is the gap test_module.py leaves. Covers both CREATE TABLE branches, INSERT, a reserved-word table name, lava_process_artifact end to end, quote escaping, and sanitize_report_name. One test asserts the reserved-word corpus genuinely fails unquoted, so it cannot silently stop testing anything; another scans scripts/artifacts/ and fails if any shipped name or category contains a separator.

Verification

13/13 tests pass; 8 fail against the pre-fix code. Two full ileapp.py -t fs runs over a 300k-file image (2020 CTF, iOS 12), 691 artifacts, exit 0, zero SQL syntax errors and zero artifacts reporting errors. A probe artifact whose every column is a reserved word produced a populated table, and a slash-named artifact wrote its report; against the pre-fix code the same probes reproduced both original failures exactly.

Companion change

LAVA reads these tables and had the same unquoted-identifier bug on its side — see abrignoni/LAVA#157. Because this PR makes reserved-word columns reach LAVA for the first time, that one should ship first or together, otherwise the failure just moves downstream. The same fix is also ported to ALEAPP, RLEAPP and VLEAPP, which share lavafuncs.py.

Lint

Rebased onto main after #1758, and the pre-existing pylint debt in the three touched files is now cleared in its own commit, so CI is green. Those warnings are why PRs #1280, #1661 and #1719 are currently red — #1719 while changing nothing but lavafuncs.py — so this should unblock them once they rebase.

Real fixes there: 13 global declarations that never assign, codecs.open (deprecated) to open(encoding=...) for the TSV writer, bare except narrowed to (ValueError, TypeError) on the two json.load calls, raise ... from ex, a stray six-space indent, an unused local shadowing datetime.timezone, and two f-strings with nothing to interpolate. Suppressed with a stated reason rather than changed: the backward-compatibility re-export block in ilapfuncs.py (its own comment says those names are kept deliberately), the three wildcard imports in ileapp.py, two deliberate catch-alls that log the traceback, the global in initialize_lava, and a TODO reworded so pylint's fixme check stops failing the build over a note.

Regression evidence

A full ileapp.py -t fs run over a combined iOS 17 extraction (2,088 files, 689 artifacts) before and after: exit 0, 0 artifacts reporting errors, 50 populated LAVA tables, 29,615 rows — identical both sides. TSV files still carry their utf-8-sig BOM after the codecs change.

On the LAVA dependency below: quoting changes the SQL statement, not the stored identifier. Comparing pragma table_info for every table across a pre-fix and post-fix run gives 20/20 identical schemas, zero differences, so LAVA sees exactly what it saw before. 76 shipped columns already sanitize to SQLite keywords (mostly Offset) and keep working, quoted or not. The downstream fix therefore matters for future artifacts that use a genuinely reserved word like From, not for anything shipping today.

abrignoni and others added 2 commits July 25, 2026 19:00
Artifact data_headers that sanitize to SQL reserved words ('From', 'To',
'Order', ...) used to fail at report time with `near "from": syntax error`,
so the artifact silently produced no LAVA table even though the parser ran
fine. Hit three times on 2026-07-25 (mastodon notifications, Apple Mail).

- lavafuncs.py: new quote_sql_name(); applied to the table name and every
  column in lava_create_sqlite_table() and lava_insert_sqlite_data().
- ilapfuncs.py / ileapp.py: sanitize_report_name() replaces path separators
  in artifact names and categories used as HTML/TSV/KML file and folder
  names (a 'Twitter/X' category used to break report writing). Display
  names, timeline entries and the LAVA JSON keep the original name.
- admin/test/scripts/test_lava_sql_identifiers.py: regression tests against
  a real SQLite database. test_module.py mocks the LAVA connection, which
  is why unit-level testing never caught this. 13 tests; 8 fail against
  the pre-fix code.

Verified with full ileapp.py -t fs runs over a 300k-file image: zero SQL
errors across 691 artifacts, probe artifacts whose headers are all reserved
words produce populated tables, slash-named artifacts write their reports.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The module file path is only read for its basename, so it need not exist.
Using a synthetic name lets ALEAPP/RLEAPP/VLEAPP carry this file unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…uches

CI lints every changed Python file with warnings fatal, so touching a core
file means inheriting whatever debt it already carries. ileapp.py,
scripts/ilapfuncs.py and scripts/lavafuncs.py each carried enough to fail the
gate on their own: PRs #1280, #1661 and #1719 are all currently red for this
reason, #1719 while changing nothing but lavafuncs.py.

Real fixes:
  - drop 13 global declarations that never assign the name
  - codecs.open -> open(encoding=...) for the TSV writer; codecs.open is
    deprecated and the utf-8-sig BOM behaviour is unchanged (verified against
    a before/after run)
  - bare except -> except (ValueError, TypeError) on the two json.load calls,
    which is what they were guarding
  - raise ... from ex when rejecting an unknown timezone, plus the stray
    six-space indent on that line
  - drop an unused local that shadowed datetime.timezone, and an unused
    exception binding
  - two f-strings with nothing to interpolate

Suppressed with a reason rather than changed:
  - the backward-compatibility re-export block in ilapfuncs.py. Its own
    comment says those names are kept so older imports keep resolving, so
    unused-import does not apply. The same goes for re, codecs and leapp_name.
  - the three wildcard imports in ileapp.py. Both ileapp.py and ileappGUI.py
    resolve a large surface through them; unpicking that is a refactor of the
    entry point, not lint cleanup.
  - two deliberate catch-alls that log the traceback and keep going.
  - the global statement in initialize_lava, which is the one function that
    creates those module level singletons.
  - a TODO comment, reworded so pylint's fixme check stops failing the build
    over a note.

All four files now rate 10.00/10. A full run over the same extraction
produces byte-identical results: 0 errors, 50 populated LAVA tables, 29,615
rows, and the TSV files still carry their BOM.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@abrignoni
abrignoni force-pushed the fix/lava-quote-identifiers branch from ba3930e to 07cf5f5 Compare July 26, 2026 00:12
@abrignoni
abrignoni merged commit b6a1e45 into main Jul 26, 2026
1 check passed
@abrignoni
abrignoni deleted the fix/lava-quote-identifiers branch July 26, 2026 00:21
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