Skip to content

Fix SQL Table - #477

Open
andrewdalpino wants to merge 3 commits into
masterfrom
fix-sql-table
Open

Fix SQL Table#477
andrewdalpino wants to merge 3 commits into
masterfrom
fix-sql-table

Conversation

@andrewdalpino

Copy link
Copy Markdown
Member
  • Removed $connection->quote($table) (produced a string literal like 'pets' in FROM → MySQL ERROR 1149)
  • Added strict identifier whitelist: /^[A-Za-z_][A-Za-z0-9_](.[A-Za-z_][A-Za-z0-9_])?$/ → throws InvalidArgumentException otherwise (src/Extractors/SQLTable.php:64-66)
  • Query changed to portable LIMIT {$batchSize} OFFSET :offset (valid on MySQL, SQLite, PostgreSQL), replacing the MySQL/SQLite-only LIMIT :offset, N (src/Extractors/SQLTable.php:95)

@andrewdalpino
andrewdalpino requested review from a team and a lite review from Copilot September 7, 2026 03:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

It introduces typed class constants (protected const string ...) which will cause a parse error on the repo’s supported PHP versions (composer.json declares PHP >=7.4).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the SQLTable extractor to fix invalid table name quoting, add table-identifier validation to prevent SQL injection via the table parameter, and make pagination syntax portable across MySQL/SQLite/PostgreSQL.

Changes:

  • Added a strict identifier whitelist for table names and throw InvalidArgumentException for invalid identifiers.
  • Removed use of PDO::quote() for the table name and now interpolate the validated identifier directly.
  • Updated the query pagination from LIMIT :offset, N to LIMIT N OFFSET :offset, and added a unit test covering invalid identifiers.
File summaries
File Description
src/Extractors/SQLTable.php Removes value-quoting of table names, adds identifier validation, and switches to portable LIMIT ... OFFSET ... syntax.
tests/Extractors/SQLTableTest.php Adds a test ensuring invalid table identifiers are rejected via InvalidArgumentException.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Extractors/SQLTable.php Outdated
andrewdalpino and others added 2 commits September 6, 2026 22:40
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.

2 participants