Fix SQL injection, TLS, and stored XSS findings - #31797
Conversation
github-actions
Bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple security findings across the ingestion connectors and UI by (1) binding SQL parameters / dialect-quoting identifiers to reduce SQL injection risk, (2) removing Elasticsearch’s “unverified TLS” fallback, and (3) removing stored-XSS sinks in advanced search dropdown labels and feed mention rendering.
Changes:
- UI: Replace
dangerouslySetInnerHTML-based highlighting with React text/<mark>rendering for advanced search dropdowns, and render feed mention suggestions as React text instead of interpolated HTML strings. - Ingestion: Bind SQL values and quote identifiers across Redshift, Snowflake, Databricks, Doris, QuestDB, Hive, and Oracle; remove Elasticsearch unverified TLS context fallback.
- Tests: Add focused unit coverage to assert binding/quoting behavior, TLS enforcement, and XSS payloads render as text.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/src/utils/AdvancedSearchUtils.tsx | Removes dangerouslySetInnerHTML and renders highlighted matches as React nodes. |
| openmetadata-ui/src/main/resources/ui/src/utils/AdvancedSearchUtils.test.tsx | Adds regression test ensuring dropdown labels render payloads as text (no executable HTML). |
| openmetadata-ui/src/main/resources/ui/src/utils/AdvancedSearchPureUtils.ts | Escapes HTML + regex metacharacters when generating <mark>-based highlight HTML strings. |
| openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/FeedEditor/FeedEditor.tsx | Renders mention suggestion items via React into a DOM container instead of string-built HTML. |
| openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/FeedEditor/FeedEditor.test.tsx | Adds regression coverage ensuring mention names/breadcrumbs render as text (no XSS). |
| ingestion/tests/unit/topology/database/test_redshift_utils.py | Adds unit test asserting schema filter is bound (not interpolated) for Redshift schema-column query. |
| ingestion/tests/unit/topology/database/test_redshift_incremental_table_processor.py | Adds unit test asserting database/start_date are passed as bound params. |
| ingestion/tests/unit/topology/database/test_questdb.py | Adds unit test asserting table name is bound in QuestDB table_columns query. |
| ingestion/tests/unit/topology/database/test_oracle.py | Updates tests for bound schema param in stored procedure queries; adds injection regression. |
| ingestion/tests/unit/topology/database/test_hive.py | Updates tests to pass a quoting dialect; adds injection regression for catalog/table quoting. |
| ingestion/tests/unit/topology/database/test_doris.py | Adds unit test validating identifier quoting escapes embedded backticks. |
| ingestion/tests/unit/topology/database/test_databricks_query_reduction.py | Adds unit test verifying identifier helper quoting keeps injected text inside backticks. |
| ingestion/tests/unit/source/search/elasticsearch/test_connection.py | Adds test asserting empty cert config does not disable TLS verification (raises). |
| ingestion/tests/unit/source/database/snowflake/test_queries.py | Adds tests for identifier quoting and schema binding in Snowflake table-name queries. |
| ingestion/src/metadata/ingestion/source/search/elasticsearch/connection.py | Removes unverified TLS context fallback; errors on incomplete SSL cert configuration. |
| ingestion/src/metadata/ingestion/source/database/snowflake/utils.py | Adds identifier quoting helpers; switches multiple queries to use bound params / safer formatting. |
| ingestion/src/metadata/ingestion/source/database/snowflake/queries.py | Converts interpolated Snowflake query strings to bind parameters and quoted identifier slots. |
| ingestion/src/metadata/ingestion/source/database/snowflake/metadata.py | Quotes database/schema identifiers for SHOW ... IN ... queries; binds stream name. |
| ingestion/src/metadata/ingestion/source/database/redshift/utils.py | Binds schema param for schema-column query instead of interpolating. |
| ingestion/src/metadata/ingestion/source/database/redshift/queries.py | Replaces interpolated schema/database/start_date with bound parameters. |
| ingestion/src/metadata/ingestion/source/database/redshift/metadata.py | Binds database name for external table location query. |
| ingestion/src/metadata/ingestion/source/database/redshift/incremental_table_processor.py | Binds database/start_date parameters when querying table changes. |
| ingestion/src/metadata/ingestion/source/database/questdb/utils.py | Executes QuestDB columns query with a bound parameter. |
| ingestion/src/metadata/ingestion/source/database/questdb/queries.py | Updates QuestDB query to use :table_name bind parameter. |
| ingestion/src/metadata/ingestion/source/database/oracle/queries.py | Replaces interpolated schema with :schema bind parameter. |
| ingestion/src/metadata/ingestion/source/database/oracle/metadata.py | Executes stored procedure query using bound schema parameter. |
| ingestion/src/metadata/ingestion/source/database/hive/utils.py | Quotes identifiers for DESCRIBE/SHOW CREATE paths to avoid injection. |
| ingestion/src/metadata/ingestion/source/database/hive/queries.py | Adjusts DESCRIBE template to accept a pre-qualified (quoted) table name. |
| ingestion/src/metadata/ingestion/source/database/doris/queries.py | Adjusts SHOW templates to accept a pre-qualified (quoted) table name. |
| ingestion/src/metadata/ingestion/source/database/doris/metadata.py | Adds qualified-identifier helper; uses it for SHOW FULL COLUMNS / SHOW PARTITIONS queries. |
| ingestion/src/metadata/ingestion/source/database/databricks/queries.py | Removes raw backtick interpolation points; uses identifier placeholders. |
| ingestion/src/metadata/ingestion/source/database/databricks/metadata.py | Adds identifier quoting helpers and formats identifier substitutions via the dialect. |
Suppressed comments (1)
ingestion/src/metadata/ingestion/source/database/snowflake/utils.py:347
- Same as
get_table_names:parameterscontains formatting-only keys that are not bind params in the formatted SQL. Passing them toexecute()may fail with “text() construct doesn't define a bound parameter …”. Filter to only the bind params actually used by the query.
cursor = connection.execute(text(query.format(**parameters)), parameters)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
github-actions
Bot
added
safe to test
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
ingestion/src/metadata/ingestion/source/database/snowflake/utils.py:348
- Same issue as
get_table_names:_get_query_parametersreturns keys used for string formatting (include_transient_tables,include_views,account_usage) in addition to bind values. Passing the full dict toexecute()can fail due to extra, undefined bind parameters.
query = queries["views"]
cursor = connection.execute(text(query.format(**parameters)), parameters)
result = SnowflakeTableList(
Jest test CoverageUI tests summary
|
There was a problem hiding this comment.
🔵 Needs a closer look
It spans security-sensitive changes across multiple connectors, TLS behavior, and UI sinks, which warrants final human review despite the added regression coverage.
Review details
- Files reviewed: 46/46 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The changes span multiple connectors plus UI rendering/security paths, so a final human review is needed to validate runtime behavior across environments beyond the available unit/E2E regressions.
Review details
- Files reviewed: 46/46 changed files
- Comments generated: 0 new
- Review effort level: Lite
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
Conflict resolutions: - snowflake/queries.py, query_parser.py, tests/.../test_queries.py: main moved QUERY_TAG to a driver session parameter at connection creation (open-metadata#32481), which removes the SQL-string path the branch was hardening. Dropped set_session_tag_query and its test; kept _snowflake_string_literal, which build_get_ddl_query still uses. - playwright BigQueryIngestionClass.ts: main landed the same select-flake fix as a shared selectOptionWithRetry util, so took main's version and dropped the branch-local selectReactAriaOption duplicate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Clean auto-merge: main's 8 new commits touch no file this branch changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Snowflake tag-ingestion queries in snowflake/metadata.py still format database_name/schema_name into SQL templates that use string-literal interpolation, leaving confirmed SQL-injection sinks in modified code paths.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
ingestion/src/metadata/ingestion/source/database/snowflake/metadata.py:391
database_nameis still formatted into the Snowflake database-tag query template (which useswhere OBJECT_DATABASE = '{database_name}'), so a malicious database name containing quotes can break out of the string literal. Please switch these tag queries to bind:database_name(and any other value filters) and pass the parameters toexecute(), leaving only{account_usage}as a formatted/quoted identifier.
for row in conn.execute(
text(
SNOWFLAKE_FETCH_DATABASE_TAGS.format(
database_name=database_name,
account_usage=quote_account_usage_schema(self.service_connection.accountUsageSchema),
)
)
ingestion/src/metadata/ingestion/source/database/snowflake/metadata.py:633
- The fallback path that wraps names with
f'"{...}"'is not safe quoting (embedded"will break the identifier) and it still relies on string formatting into the SQL template. Prefer using the Snowflake identifier-quoting helpers for identifier parts and bind parameters for value filters, rather than constructing quoted SQL fragments with f-strings.
logger.warning(f"Error fetching tags {exc}. Trying with quoted names")
result = self.connection.execute(
text(
SNOWFLAKE_FETCH_TABLE_TAGS.format(
database_name=f'"{self.context.get().database}"',
schema_name=f'"{self.context.get().database_schema}"',
account_usage=quote_account_usage_schema(self.service_connection.accountUsageSchema),
)
)
)
- Files reviewed: 45/45 changed files
- Comments generated: 2
- Review effort level: Lite
The three ACCOUNT_USAGE.tag_references templates interpolated
database_name/schema_name into `where OBJECT_DATABASE = '{...}'` string
literals. Snowflake permits quotes in quoted identifiers, so a database
created as "PROD' OR 1=1 UNION SELECT ..." escapes the literal and runs
attacker SQL under the ingestion role.
Bind both object names instead, leaving only {account_usage} as a
formatted (and already quoted) identifier.
The yield_tag retry path wrapped the context names in f'"{...}"'. That was
never valid quoting for a value comparison -- tag_references stores plain
names, so '"MYDB"' matches nothing -- and embedded quotes broke it anyway.
It now binds fqn.unquote_name(...) of the context names, which makes the
retry meaningful for names that arrive already quoted.
Reported by Copilot on open-metadata#31797.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
There was a problem hiding this comment.
🔵 Needs a closer look
One Oracle ingestion module uses runtime-evaluated subscripted type annotations without postponed evaluation, which should be hardened to avoid import-time typing issues across SQLAlchemy versions.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
ingestion/src/metadata/ingestion/source/database/oracle/metadata.py:18
process_resultuses a parameterizedRow[Any]type annotation, but this module does not enable postponed evaluation of annotations. Addingfrom __future__ import annotations(as used in other ingestion database modules) avoids runtime evaluation of subscripted typing and keeps import-time behavior robust across SQLAlchemy versions.
- Files reviewed: 45/45 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
There was a problem hiding this comment.
🔵 Needs a closer look
It makes broad, security-sensitive changes across multiple connectors and UI sinks that warrant final human verification of runtime behavior beyond the included regression tests.
Review details
- Files reviewed: 45/45 changed files
- Comments generated: 0 new
- Review effort level: Lite
Two CI gates broke on the previous commit, both from moving those expressions rather than from new behaviour: - basedpyright: the baseline matches entries by column range, so reindenting `self.context.get().database` into the bind dict orphaned its baseline entries and the errors resurfaced. Tag the three lines with `# pyright: ignore[reportAttributeAccessIssue]`, as the ~15 other TopologyContext reads in this file already do. - check_ruff_suppressions: the G004 baseline keys on a hash of the log message, so rewording the retry warning registered as a new f-string log. Switch it to lazy %-style logging, which drops the violation outright, and prune the now-obsolete baseline entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ngs' into harshach/fix-security-review-findings
There was a problem hiding this comment.
🔵 Needs a closer look
It touches multiple security-critical paths (SQL construction, TLS verification, and XSS sinks) across many connectors/modules and should receive final human verification despite the added tests.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
openmetadata-ui/src/main/resources/ui/src/utils/AdvancedSearchPureUtils.ts:14
- This file now imports
OldJsonTreefrom@react-awesome-query-builder/antd, but the rest of the UI codebase (including QueryBuilder helpers and the advance-search provider) consistently imports these types from@react-awesome-query-builder/ui. Keeping this consistent avoids type drift/confusion across query-builder utilities.
openmetadata-ui/src/main/resources/ui/src/utils/AdvancedSearchUtils.test.tsx:15 FieldOrGroupis imported from@react-awesome-query-builder/antdhere, but the production code under test (AdvancedSearchUtils.tsx) imports it from@react-awesome-query-builder/ui. Align the import to the same entrypoint to avoid accidental type/API mismatches between test and implementation.
- Files reviewed: 46/46 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It is a large, security-sensitive, cross-connector change spanning multiple SQL dialects and UI rendering paths, so it warrants final human review despite strong regression coverage.
Review details
- Files reviewed: 46/46 changed files
- Comments generated: 1
- Review effort level: Lite
| */ | ||
|
|
||
| import { FieldOrGroup } from '@react-awesome-query-builder/ui'; | ||
| import { FieldOrGroup } from '@react-awesome-query-builder/antd'; |
Code Review ✅ Approved 2 resolved / 2 findingsHardens database connector queries by binding SQL values and dialect-quoting identifiers across Databricks, Doris, Hive, Oracle, QuestDB, Redshift, and Snowflake connectors. Removes Elasticsearch's unverified TLS fallback and secures activity-feed mentions and advanced-search labels against stored XSS by rendering through React-safe text instead of HTML injection sinks. Addresses ReactDOM.render memory leak on mention renders and fixes unescaped account_usage identifier interpolation. Includes 176 focused Python tests plus four subtests and 65 Jest tests; Ruff and targeted ESLint also passed. ✅ 2 resolved✅ Performance: ReactDOM.render into detached node leaks on every mention render
✅ Security: account_usage identifier still interpolated into SQL unescaped
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
There was a problem hiding this comment.
🔵 Needs a closer look
The change set is security-critical and spans many connectors and UI rendering paths, so it warrants final human review to confirm no behavioral regressions across all affected integrations.
Review details
- Files reviewed: 46/46 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
|
Addresses the 18 OpenMetadata findings from security run 32324515413 by binding SQL values and dialect-quoting identifiers across the affected database connectors.
Removes Elasticsearch's unverified TLS fallback and renders feed mentions and advanced-search labels as safe React text.
Adds focused regression coverage for the connector query paths, TLS configuration, and both stored-XSS sinks.
Validation: 176 focused Python tests plus four subtests and 65 Jest tests passed; Ruff and targeted ESLint also passed.
Greptile Summary
The PR hardens database connector queries by binding values and dialect-quoting identifiers, removes Elasticsearch’s unverified TLS fallback, and replaces unsafe UI HTML rendering with React-safe text.
Confidence Score: 4/5
The PR is not yet safe to merge because Snowflake GET_DDL reflection still passes object names as bind parameters and can omit ingested definitions.
Snowflake’s table, view, stream, and semantic-view definition paths still bind the object-name argument despite the connector’s documented string-literal requirement, so those reflection calls can fail and leave schema definitions missing.
Files Needing Attention: ingestion/src/metadata/ingestion/source/database/snowflake/queries.py, ingestion/src/metadata/ingestion/source/database/snowflake/utils.py
Important Files Changed
Reviews (8): Last reviewed commit: "Merge branch 'main' into harshach/fix-se..." | Re-trigger Greptile
Context used: