fix(metadata): sanitize interpolated identifiers (SQL injection) + tests — rebased #2#12
Merged
Merged
Conversation
Add 51 tests across all three source files covering PacketType::from_u8, sqlite_to_tds_type, value_to_tds_type, build_columns, decode_utf16le, TdsSession transaction state management, and skip_all_headers.
Add sanitize_identifier() that strips non-alphanumeric/underscore/period characters from table and schema names before they are interpolated into generated SQL. Applied at extraction time in extract_table_name(), extract_where_value_original(), and sp_columns parsing so all MetadataQuery variants are protected.
Cover edge case values (integer/float extremes, empty text/blob, large payloads, Unicode/emoji), error paths (nonexistent table, wrong param count, SQL syntax errors), mixed-type rows, sequential query-execute patterns, and hrana client conversion fallbacks (non-numeric integer parse, invalid base64, overflow) plus URL trailing-slash handling.
Adds 70+ roundtrip tests via translate() covering complex SELECTs (GROUP BY, HAVING, DISTINCT, LIMIT/OFFSET, aggregates, CASE WHEN, subqueries), JOINs (INNER, LEFT, RIGHT, CROSS, self-join, multi-join), set operations (UNION, INTERSECT, EXCEPT), CTEs (simple, multiple, recursive), complex DML (INSERT...SELECT, multi-row INSERT, UPDATE/DELETE with subqueries), DDL (CREATE/DROP TABLE IF [NOT] EXISTS, CREATE INDEX, CREATE UNIQUE INDEX), and transaction statements (BEGIN, COMMIT, ROLLBACK, SAVEPOINT, RELEASE, ROLLBACK TO). Tests cover MySQL, PostgreSQL, and TDS dialects.
Tests builder construction, chaining, invalid address handling, the no-frontends error path, and serve() smoke tests for each frontend.
Add comprehensive tests for the handler module covering ok_response, translate_sql, transaction state logic, param_to_value conversion dispatch, and handler initialization. Add edge case tests for types.rs covering empty strings, whitespace, additional SQL type variants, and case sensitivity for BYTEA.
Tests cover sqlite_to_pg_type type mapping (21 tests), value_to_pg_type inference (5 tests), encode_value serialization (11 tests), extract_params portal parameter extraction (15 tests), and pg_error construction (2 tests). Added bytes as a dev-dependency for constructing binary parameter fixtures.
…n after #11 Brings the stale `add-comprehensive-tests` branch (PR #2, open since 2026-05-04) up to date with current main (699e634, the #11 framework wire-compat merge). PR #2 had diverged (ahead 13 / behind 11) and was `mergeable_state: dirty`. Changes to make it apply cleanly on top of #11: - metadata.rs: keep the PR's `sanitize_identifier()` fix (strips anything that isn't alphanumeric / `_` / `.` from extracted table & schema identifiers before they are `format!`-interpolated into generated SQL). This now also guards the new #11 interpolation sites — the Laravel `Schema::hasTable` existence probe (`name='{table}'`) and the WordPress `SHOW FULL COLUMNS` path (`pragma_table_info('{table}')`). - metadata.rs: add injection tests for every `{table}`/`{schema}` interpolation site introduced/kept by #11 (TableExists, ShowColumns, ShowCreateTable, ShowIndex, InformationSchemaColumns, PgCatalogColumns). - litewire-mysql/handler.rs: adapt the PR's handler tests to #11's new `LiteWireHandler::new(backend, cache).await` (async, 2-arg) signature and the `backend` -> `conn` field rename. `memory_handler()` now block_on's the async constructor; a `memory_handler_async()` helper serves the `#[tokio::test]` cases. - rusqlite_backend.rs: preserve main's per-connection isolation tests (#6) and append the PR's new backend edge-case tests. - Replace `3.14` test literals with `2.5` to satisfy clippy::approx_constant. Verified: cargo fmt --check, cargo test --workspace (635 pass, 0 fail), cargo clippy --workspace --all-targets -D warnings all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ChVRC9ZrvQarrctDwSy1S5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebase of #2 onto current main (after #11), conflicts resolved and tests adapted to #11's async handler API.
Why #2 stalled: it predated this repo's CI (ci.yml landed 2026-07-10, ~67d after #2's commit) so it never got a signal, then went stale/conflicted and its tests stopped compiling against #11.
Security substance (still valid, now more relevant):
metadata.rs::to_sqlite_sql()format!-interpolates table/schema identifiers into SQLite/PRAGMA SQL. #11 added new interpolation sites (TableExistsname='{table}',SHOW FULL COLUMNSpragma_table_info) fed by the same extractors #2 wraps insanitize_identifier()— so this fix covers #11's new surface.Closes #2. Design note for review: sanitizer silently strips injected chars (safe: mangled name -> empty result) rather than rejecting — worth a conscious call.
🤖 Generated with Claude Code