Reject backtick/backslash in SQL identifiers to close injection vector#463
Merged
Conversation
PR #462 added escapeIdent (backtick doubling) for the go/sql-injection alert, but escaping alone is not sufficient. A query first passes through the GoogleSQL (ZetaSQL) parser and is then re-emitted for the SQLite backend, and the two layers escape a backtick differently (GoogleSQL uses a backslash, SQLite doubles the backtick). A crafted name containing a backtick and a backslash survives one layer's escaping and breaks out of the quoted identifier in the other; this was verified to execute an injected DROP TABLE despite the doubling applied by escapeIdent. Add validateIdent and reject any project, dataset, table, view, routine or column name that contains a backtick or backslash. Neither character is valid in a BigQuery identifier, so this closes the injection vector (CWE-89) without depending on the backend's escaping behavior and without affecting any legitimate name. tablePath/routinePath now return an error, and every public Repository method validates its identifiers before building a query. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
41cb98b to
ec4ceb2
Compare
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.
Summary
Follow-up to #462, which addressed the CodeQL
go/sql-injectionalert(#5) by adding
escapeIdent(backtick doubling).Escaping alone is not sufficient. A query first passes through the GoogleSQL (ZetaSQL) parser and is then re-emitted for the SQLite backend, and the two layers escape a backtick differently:
\`).A crafted name containing both a backtick and a backslash survives one layer's escaping and breaks out of the quoted identifier in the other. This was verified empirically against
googlesqlite: a table name ofa\`; DROP TABLE victim; --still executed the injectedDROP TABLEdespite the doubling applied byescapeIdent.Fix
Reject identifiers rather than rely on escaping:
validateIdent, which rejects any identifier containing a backtick or backslash.tablePath/routinePathnow return an error, and every publicRepositorymethod validates its project / dataset / table / view / routine / column names before building a query.escapeIdentis kept as defense-in-depth for the remaining characters.Tests
internal/contentdata/repository_test.gocoveringvalidateIdent, the newtablePath/routinePathsignatures, and the specific injection payload that defeated escaping.go build ./...passes.go test ./server/suite passes — table create/insert/query paths are unaffected.🤖 Generated with Claude Code