CXP-784 Escape identifiers in remaining unescaped Snowflake SQL sites#136
CXP-784 Escape identifiers in remaining unescaped Snowflake SQL sites#136JavierCarnelli-ConductorOne wants to merge 7 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Connector PR Review: CXP-784 Escape identifiers in remaining unescaped Snowflake SQL sitesBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. The new commit ( Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
… sites The CXP-784 fix only covered unquoting grantee names coming back from SHOW GRANTS. Several other query-building sites still interpolated raw role/user/database names - and pagination cursors, which are themselves resource names - into quoted identifiers or LIKE clauses without escaping, so a name containing an embedded " or ' breaks the statement. Escapes role/user/database names in ListAccountRoleGrantees, GetAccountRole, GrantAccountRole, RevokeAccountRole, ListSecrets, UserRsa, and GetDatabase, and escapes the pagination cursor in ListAccountRoles, ListDatabases, and ListUsers, matching the pattern already used in table.go and GetUser. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GetAccountRole and GetDatabase pass their name through escapeLikePattern, which backslash-escapes % and _, but neither statement declared ESCAPE '\\'. Snowflake's LIKE has no default escape character, so without that clause the backslash is inert and \_ /\% are parsed as a literal backslash plus wildcard - the exact match then fails for any role/database name containing % or _, which is a very common naming pattern (e.g. DATA_ENGINEER, PROD_DB). This is a regression introduced by the prior escaping commit, not present before it. Adds ESCAPE '\\' to both statements, matching the pattern GetTable already used, and adds regression tests covering names with embedded _ and %. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
66799b9 to
9f7872f
Compare
database_test.go's new test added a third literal "kind" occurrence, tripping goconst's threshold (min 3) against the two pre-existing raw "Kind"/"kind" literals in database.go and table.go's column maps. Adds structFieldKind/columnKind alongside the existing column-name constants and uses them in both maps instead of the raw strings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Snowflake's SHOW DATABASES/ROLES/TABLES commands use a simplified LIKE '<pattern>' filter that does not support an ESCAPE clause at all (per Snowflake's own SQL reference) - only the general SQL LIKE predicate/ WHERE usage supports ESCAPE. A prior commit added "ESCAPE '\'" to GetAccountRole and GetDatabase to try to neutralize _/% wildcards, which made those statements syntactically invalid and caused every call to fail with 422 Unprocessable Entity - this broke the CI integration test, since databaseBuilder.Grants() calls GetDatabase unconditionally for every database resource. GetTable (table.go) had the identical pre-existing defect from an earlier merged PR, just never exercised in CI because it's only called as a fallback. Fixed it the same way here since it's the same root cause. Since SHOW's LIKE has no escape mechanism, backslash-escaping _/% is both ineffective (no ESCAPE clause to interpret it) and harmful (the inserted backslash must then literally appear in the name to match). Switches GetAccountRole, GetDatabase, and GetTable to escapeSingleQuote only - the sole character that must be escaped to keep the SQL string literal well-formed - and removes the now-dead escapeLikePattern helper. _ and % remain unavoidable wildcards for these SHOW commands; there is no Snowflake syntax to suppress that. Also extracts shared structFieldKind/columnKind and structFieldSchemaName/columnSchemaName constants to keep goconst happy after the new regression tests added a third occurrence of each literal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
SHOW ROLES LIKE is a pattern match, not an exact match, and has no ESCAPE clause to neutralize _/% wildcards in roleName. LIMIT 1 means at most one row comes back, but if roleName contains a wildcard character, that row can belong to a different role that merely matches the loose pattern. GetAccountRole previously took accountRoles[0] unconditionally, silently returning the wrong role with no error. GetTable already guards this by filtering for an exact name match, and GetDatabase guards it by erroring on more than one row; this adds the equivalent exact-match check to GetAccountRole. All three existing call sites already treat a nil role as "not found" and skip gracefully, so returning nil here instead of a mismatched role is safe. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GetAccountRoleGrantees read row[1]/row[2]/row[3] by hardcoded position. A Snowflake behavior bundle that reorders or adds columns to SHOW GRANTS OF ROLE's output would silently corrupt RoleName/GranteeType/ GranteeName with no error. TableGrant (SHOW GRANTS ON TABLE/VIEW) already avoids this by parsing via ResultSetMetadata.ParseRow + column-name lookup; this brings AccountRoleGrantee in line with that pattern. Column layout (rowType) is only present on the partition-0 response - partitions 1..N return bare data with no metadata - so the pagination cursor is redesigned from a colon-delimited string to a JSON-encoded accountRoleGranteesCursor that carries RowTypes forward, mirroring tableGrantsCursor's existing solution to the identical problem for table grants. Also extracts shared columnRole/columnGrantOption/columnGrantedBy constants (and reuses structFieldGrantedTo/structFieldGranteeName) to avoid re-triggering goconst across account_role.go and table.go. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| // remain active wildcards; there is no Snowflake syntax to suppress that for SHOW commands. | ||
| queries := []string{ | ||
| fmt.Sprintf("SHOW ROLES LIKE '%s' LIMIT 1;", roleName), | ||
| fmt.Sprintf("SHOW ROLES LIKE '%s' LIMIT 1;", escapeSingleQuote(roleName)), |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): escapeSingleQuote only doubles ', but Snowflake processes backslash escape sequences inside single-quoted string literals (\', \\, etc.). A value ending in a lone backslash (e.g. a role named foo\) would produce 'foo\', where the trailing \' escapes the closing quote and leaves the literal unterminated — breaking the query, and in adversarially-named-object cases potentially enabling breakout. This is a net improvement over the previously-unescaped code and the same escapeSingleQuote is used for the ... FROM '%s' cursors and other SHOW ... LIKE sites, so please verify Snowflake's backslash semantics here; if backslash is significant, also escape \ → \\ before doubling quotes.
| // remain active wildcards; there is no Snowflake syntax to suppress that for SHOW commands. | ||
| queries := []string{ | ||
| fmt.Sprintf("SHOW DATABASES LIKE '%s' LIMIT 1;", name), | ||
| fmt.Sprintf("SHOW DATABASES LIKE '%s' LIMIT 1;", escapeSingleQuote(name)), |
There was a problem hiding this comment.
[Client] GetDatabase is missing the exact-name guard that the sibling lookups in this PR added
This PR removed escapeLikePattern and now relies on a post-query exact-name check to compensate for _ and % staying live wildcards in SHOW ... LIKE '<pattern>' (Snowflake's SHOW-command grammar documents the filter only as LIKE '<pattern>' with %/_ as active wildcards, and does not document an ESCAPE clause the way the general SQL LIKE predicate does). GetAccountRole (accountRoles[0].Name == roleName) and GetTable both got that compensating guard — but GetDatabase did not. The result-handling block below (if len(databases) == 0 { ... } else if len(databases) > 1 { ... }; return &databases[0]) returns whatever single row LIMIT 1 produced, without checking its name. Because _ matches any single char and is ubiquitous in Snowflake identifiers, a request for MY_DB can silently resolve to a different database (e.g. MYADB). The len(databases) > 1 branch is dead under LIMIT 1, so nothing else catches it. Note the gap is independent of the ESCAPE question: whether or not the wildcards could be escaped, GetDatabase currently does neither escaping nor guarding.
Fix in the result block just below (outside the diff view) — mirror the sibling guards:
if len(databases) == 0 || databases[0].Name != name {
return nil, resp.StatusCode, fmt.Errorf("database with name %s not found", name)
}
return &databases[0], resp.StatusCode, nil3 participants
The CXP-784 fix only covered unquoting grantee names coming back from SHOW GRANTS. Several other query-building sites still interpolated raw role/user/database names - and pagination cursors, which are themselves resource names - into quoted identifiers or LIKE clauses without escaping, so a name containing an embedded " or ' breaks the statement.
Escapes role/user/database names in ListAccountRoleGrantees, GetAccountRole, GrantAccountRole, RevokeAccountRole, ListSecrets, UserRsa, and GetDatabase, and escapes the pagination cursor in ListAccountRoles, ListDatabases, and ListUsers, matching the pattern already used in table.go and GetUser.