security(gateway): prevent SQL injection in schema introspection query builders - #1625
Open
matheusfrancisco wants to merge 1 commit into
Open
security(gateway): prevent SQL injection in schema introspection query builders#1625matheusfrancisco wants to merge 1 commit into
matheusfrancisco wants to merge 1 commit into
Conversation
Contributor
Migration Safety AnalysisNo database migrations were changed in this PR. Safe to deploy to sandbox. |
PR Summary by QodoHarden schema introspection builders against identifier injection
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Contributor
📋 API ChangelogAPI Changelog unknown vs. unknownNo changes detected |
Code Review by Qodo
Context used✅ Compliance rules (platform):
26 rules 1. Oracle missing params misreported
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+365
to
369
| // Allows only letters, numbers, underscores, hyphens, and dots with | ||
| // length between 1 and 128 characters (see validateSchemaIdentifier). | ||
| if err := validateSchemaIdentifier("database name", dbName); err != nil { | ||
| return fmt.Errorf("invalid database name. Only alphanumeric characters, underscore, hyphen and dot are allowed with length between 1 and 128 characters") | ||
| } |
There was a problem hiding this comment.
1. Validator error can drift 🐞 Bug ⚙ Maintainability
validateDatabaseName delegates acceptance to validateSchemaIdentifier but discards the returned error and emits its own hardcoded message, so future changes to the shared validator’s constraints/message can diverge silently. This undermines the stated goal of having a single canonical validator for schema identifiers.
Agent Prompt
## Issue description
`validateDatabaseName` calls `validateSchemaIdentifier("database name", dbName)` but ignores its returned error and returns a separate hardcoded error string. This reintroduces duplication and can drift if `validateSchemaIdentifier` changes.
## Issue Context
`validateSchemaIdentifier` is intended to be the canonical schema identifier validator used by the schema query builders.
## Fix Focus Areas
- gateway/api/connections/helpers.go[363-371]
- gateway/api/connections/queries_schema.go[17-24]
## Suggested fix
- Replace the `if err := validateSchemaIdentifier(...); err != nil { return fmt.Errorf(...) }` block with `return validateSchemaIdentifier("database name", dbName)` (or wrap it, if you need to preserve the exact historical string), ensuring there is a single source of truth for both validation and messaging.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+99
to
+105
| if err := validateSchemaIdentifier("table name", tableName); err != nil { | ||
| return "", err | ||
| } | ||
| if err := validateSchemaIdentifier("schema name", schemaName); err != nil { | ||
| return "", err | ||
| } | ||
| return getOracleDBColumnsQuery(tableName, schemaName), nil |
There was a problem hiding this comment.
2. Oracle missing params misreported 🐞 Bug ≡ Correctness
With the new OracleDB schema-name validation, GetTableColumns requests that omit both schema and database now fail as HTTP 422 "invalid schema name" because the handler defaults schemaName to dbName (which can be empty for Oracle). This is a confusing client-facing error path for what is effectively a missing required parameter for Oracle column introspection.
Agent Prompt
## Issue description
`getColumnsQuery` now validates `schemaName` for OracleDB and rejects empty values. In `GetTableColumns`, OracleDB does not require `database`, and `schemaName` defaults to `dbName` when omitted—so requests with neither `schema` nor `database` will now return a generic 422 "invalid schema name".
## Issue Context
This behavior is introduced by the new validation; prior behavior would execute a query with an empty owner filter (likely returning no results). Now it fails early but with a misleading message compared to an explicit “missing schema/database” response.
## Fix Focus Areas
- gateway/api/connections/database_explorer.go[352-392]
- gateway/api/connections/queries_schema.go[98-105]
## Suggested fix
- In `GetTableColumns`, add an OracleDB-specific guard before calling `getColumnsQuery`, e.g.:
- If `currentConnectionType == pb.ConnectionTypeOracleDB` and `schema` is empty, require one explicit source of schema (either `schema` query param, or decide to treat `database` as schema and require it to be non-empty).
- Return a clear client error message (and consistent status) like `"schema parameter is required for OracleDB"` instead of surfacing the generic validator error.
- Keep the validator in `getColumnsQuery` (do not relax it), so injection protection remains intact.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
✅ Build Completed with Success, Version=1625.0.0-g17b44ba |
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.
tableNameinto a shell command (aws dynamodb describe-table --table-name %s) [0/6603]unvalidated.
Since these scripts execute against the target database via
clientexecwith the connection's credentials, a crafted identifierallowed predicate manipulation or statement injection.
Validation now lives inside the query builders themselves (
validateSchemaIdentifier: alphanumeric,_,-,., 1–128 chars —rejects backticks, quotes, whitespace, and separators), so every call path is covered regardless of caller behavior.
TablesQueryFor/ColumnsQueryForreturn(string, error); callers surface the error as HTTP 422 or an MCP tool error.📣 User-facing impact
None — security hardening of internal schema introspection; no behavior change for valid database, table, or schema names.
🔗 Related Issue
Fixes #
🚀 Type of Change
📋 Changes Made
gateway/api/connections/queries_schema.go: addedvalidateSchemaIdentifier;getTablesQuery/getColumnsQuery(and exportedTablesQueryFor/ColumnsQueryFor) now validate every interpolated identifier and return(string, error)gateway/api/connections/helpers.go:validateDatabaseNamedelegates tovalidateSchemaIdentifier(same regex, same error message)so the two cannot drift
gateway/api/connections/database_explorer.go:ListTables/GetTableColumnshandle the builder error and return HTTP 422;table/schemaare now validated on the columns endpointgateway/api/mcpserver/tools_schema.go:connectionTablesHandler/connectionColumnsHandlerhandle the builder error and return anMCP tool error result
gateway/api/connections/queries_schema_test.go: added injection-rejection tests (backtick/quote breakouts per connection type) anda valid-identifier acceptance test
🧪 Testing
Ran
go test ./gateway/api/connections/ ./gateway/api/mcpserver/ -count=1— all pass, including pre-existingTestValidateDatabaseNameand MySQL quoting tests. Fullgo build ./gateway/... ./common/...clean; repo-wide grep confirms no othercallers of the changed signatures.
Test Configuration:
Tests performed:
📸 Screenshots (if applicable)
N/A
✅ Checklist
📄 Additional Notes
getColumnsQuery— validation is per connection type, matching exactly the identifiers each builder interpolates(e.g. MongoDB ignores
schema, OracleDB ignoresdatabase, DynamoDB uses onlytable).validateDatabaseNameregex, so no previously accepted name is rejected.patch(security fix, no API surface change).