fix(sundb): escape SQL identifiers and literals in metadata/DDL paths (#1914)#2176
Open
HandSonic wants to merge 6 commits into
Open
fix(sundb): escape SQL identifiers and literals in metadata/DDL paths (#1914)#2176HandSonic wants to merge 6 commits into
HandSonic wants to merge 6 commits into
Conversation
…ata (OtterMind#1914) - DEFAULT_VALUE_PATTERN: drop quotes, commas and parentheses so a default value cannot smuggle literals, inject extra column definitions or close the column definition early (e.g. "0) --"). Paren-less SUNDB default functions (SYSDATE, CURRENT_TIMESTAMP, USER, SEQ.NEXTVAL) still pass. - SUNDBMetaData.tableDDL: quote identifiers originating from DB metadata (column names, constraint names/columns, tablespace names, index schema/table/column names) in generated DDL. - getIndexName: always quote schema and index name; drop the unquoted PRIMARY_KEY_INDEX special case. - Tests: default-pattern rejection payloads, alter-table rename escaping, copyTable executed SQL, export-comment literal escaping, getIndexName quoting (incl. PRIMARY_KEY_INDEX names).
HandSonic
marked this pull request as draft
July 26, 2026 10:32
…jection (OtterMind#1914) dd55337's bare-token-only DEFAULT_VALUE_PATTERN rejected legitimate string-literal defaults (DEFAULT 'Y', '0', 'O''Brien', '1970-01-01', ''). New pattern is an anchored alternation: \A(?:(?!.*--)[A-Za-z0-9_ .+-]+|'(?:[^']|'')*')\z - bare token: unchanged charset, plus a lookahead rejecting '--' - quoted literal: quotes may only appear as doubled '' pairs, which exactly mirrors SQL string-literal tokenization, so anything accepted is parsed by the DB as one inert string token Still rejects: 0) --, 0 --, 1, x INT, now(), unbalanced quotes, 'a' 'b', 'a'||'b', 'a'--, 'a'; DROP TABLE x--.
…Mind#1914) Extend the quoted-literal branch with an optional single leading keyword: (?:[A-Za-z]+ )?'(?:[^']|'')*' — restores DATE '2024-01-01' and TIMESTAMP '...' defaults flagged in the gp-25 regression review. The keyword is letters-only so it cannot break out; the literal still requires doubled interior quotes. Parenthesized calls (TO_DATE(...), SYS_GUID()) stay rejected and are documented as such in the PR. Tests: accept set += DATE '2024-01-01', TIMESTAMP '2024-01-01 00:00:00'; reject set += TO_DATE('2024-01-01','YYYY-MM-DD'), SYS_GUID(), DATE '2024-01-01'--. Suite: 17/17 green.
…tainer review (OtterMind#1914) - new SUNDBIdentifierProcessor (SPI ISQLIdentifierProcessor): quoteIdentifier with double-quote doubling, escapeString with single-quote doubling - SUNDBMetaData overrides getSQLIdentifierProcessor(); metadata call sites use it - builders/managers/enums use SUNDBIdentifierProcessor.INSTANCE - non-escapable validation moved to SUNDBSqlGuards (ASC/DESC, DEFAULT expression, VARCHAR unit) - SUNDBSqlEscapes removed; tests migrated (17 green)
…e for DDL paths (OtterMind#1914) - quoteIdentifier(String) is conditional again: null/blank passthrough, valid non-keyword identifiers returned unquoted, else wrapped with embedded-quote doubling (restores SPI contract for completion/matching) - new quoteIdentifierAlways(String): unconditional wrap (null -> null), used only at the DDL-generation call sites that commit 0ff5f8b migrated from the always-quote SUNDBSqlEscapes.quoteIdentifier - quoteIdentifierIgnoreCase stays the always-quote variant; the versioned overload delegates to quoteIdentifier(String) - tests cover both conditional and always behaviors incl. null passthrough (21 green)
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
2 participants
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.
Problem
The SUNDB plugin interpolated schema/table/column/index names, comments, DEFAULT values, VARCHAR unit tokens, index sort direction (ascOrDesc), and system-catalog query parameters directly into SQL text. Values containing double quotes, single quotes, or SQL keywords could break out of their context (SQL injection via metadata/DDL paths, #1914).
Fix
Add
SUNDBSqlEscapeshelper and route every interpolation through it:escapeSqlLiteral— doubles single quotes, for string-literal positions (OWNER/NAME predicates in metadata queries, table/column comments)escapeIdentifier/quoteIdentifier— strips one surrounding double-quote pair, doubles embedded double quotes, for identifier positions (DDL object names, SET SCHEMA, DROP/COPY TABLE, CREATE SCHEMA/TABLE/INDEX, ALTER TABLE, schema AUTHORIZATION, getMetaDataName)requireAscOrDesc— whitelist ASC/DESC for index columns, canonicalized to uppercase; anything else throws (closes stacked-query injection viaASC); DROP TABLE t--)DEFAULT_VALUE_PATTERN— anchored alternation\A(?:(?!.*--)[A-Za-z0-9_ .+-]+|(?:[A-Za-z]+ )?'(?:[^']|'')*')\z:(?!.*--)guard blocks comment sequences'Y','O''Brien','',DATE '2024-01-01',TIMESTAMP '...') — interior quotes must be doubled, exactly mirroring SQL string-literal tokenization, so anything accepted parses as one inert string tokenvalidateUnit(SUNDBColumnTypeEnum) — whitelist CHAR/BYTE for VARCHAR length unitsSUNDBMetaData.tableDDL/getIndexName— quote identifiers originating from DB metadata (column names, constraint names/columns, tablespace names, index schema/table/column names); the unquotedPRIMARY_KEY_INDEXspecial case is removed and all index names get uniform quotingInvalid input now fails fast with
IllegalArgumentExceptioninstead of emitting broken/injectable SQL.Documented trade-off (by design, fail-closed)
Parenthesized DEFAULT expressions (
now(),TO_DATE('2024-01-01','YYYY-MM-DD'),SYS_GUID()) are REJECTED — parens could close the column definition early, and paren-less/typed-literal forms cover the common cases. Also rejected:0) --,0 --,1, x INT, unbalanced quotes,'a' 'b','a'||'b','a'--,'a'; DROP TABLE x--,DATE '2024-01-01'--. String-literal and typed date/time defaults ARE supported (see above).Residual accepted risk
Keyword-position DB-metadata fields remain raw in exported DDL: constraint type, DESCEND, NULL_ORDER, and driver-supplied type names. Quoting them would produce invalid DDL; exploitation would require system-catalog write access (an already DDL-privileged attacker) plus a DBA rerunning the export script. Accepted as low risk.
Tests
SUNDBSqlEscapesTest(16 cases): quote-doubling helpers, identifier stripping/doubling, ascOrDesc canonicalization + injection rejection, DEFAULT-pattern accept set (CURRENT_TIMESTAMP,-1,SEQ.NEXTVAL,'Y','O''Brien','',DATE '2024-01-01',TIMESTAMP '2024-01-01 00:00:00') and reject set (0) --,0 --,1, x INT,now(),TO_DATE(...),SYS_GUID(), unbalanced'abc,'a' 'b','a'||'b','a'--,'a'; DROP TABLE x--,DATE '2024-01-01'--), create-schema / drop-table / alter-table-rename / copy-table / export-comment / index-script paths with quote-laden names, getIndexName quoting incl. PRIMARY_KEY_INDEX names. Full sundb module suite: 17 tests, 0 failures.