fix(server): prevent SQL injection in UQI filter service projection and sortBy columns#41594
Open
fix(server): prevent SQL injection in UQI filter service projection and sortBy columns#41594
Conversation
…nd sortBy columns Validate projection and sortBy column names against the data schema before SQL construction, matching the existing WHERE clause validation pattern. Also escape backtick characters in identifiers as defense-in-depth and reject backticks in column names during schema generation.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds pre-validation for projection and sort-by inputs, escapes backtick-quoted identifiers in SQL generation, moves data lifecycle into a guarded block for cleanup, and rejects backticks in generated schema column names. Tests expanded to cover projection/sort validation and injection protections. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-interfaces/src/main/java/com/appsmith/external/services/ce/FilterDataServiceCE.java`:
- Around line 296-308: The validateSortByColumns method currently only checks
column names; update it to also validate the sort order (SORT_BY_TYPE_KEY) in
the same loop: for each sortCondition in validateSortByColumns (used with
isSortConditionEmpty), check that sortCondition.get(SORT_BY_TYPE_KEY) is not
null/blank and equals one of the allowed values (e.g., "ASC" or "DESC",
case-insensitive) before returning, and if invalid or missing throw an
AppsmithPluginException with AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR
and a clear message including the columnName and the bad/missing order; this
prevents addSortCondition from calling
sortCondition.get(SORT_BY_TYPE_KEY).toUpperCase() on null and centralizes
validation.
- Around line 165-166: Validation of projection/sort columns is performed after
the temp table is created/filled causing leaked tables on validation failure;
move the calls to validateProjectionColumns(...) and validateSortByColumns(...)
to occur before generateTable(...) and insertAllData(...) inside filterDataNew,
and ensure dropTable(...) is executed in a finally block so the temp table is
always removed even on exceptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 351668b8-34d6-4700-b40e-dcb99b54dfcd
📒 Files selected for processing (2)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/services/ce/FilterDataServiceCE.javaapp/server/appsmith-interfaces/src/test/java/com/appsmith/external/services/FilterDataServiceTest.java
...appsmith-interfaces/src/main/java/com/appsmith/external/services/ce/FilterDataServiceCE.java
Outdated
Show resolved
Hide resolved
...appsmith-interfaces/src/main/java/com/appsmith/external/services/ce/FilterDataServiceCE.java
Show resolved
Hide resolved
Failed server tests
|
… creation, add try/finally cleanup, validate sort order - Move validateProjectionColumns/validateSortByColumns into filterDataNew() before generateTable() so invalid input is rejected before the H2 temp table is created, preventing table leaks on validation failure. - Wrap table operations in try/finally to guarantee dropTable() runs even if insertAllData or executeFilterQueryNew throws. - Add sort order validation in validateSortByColumns to reject null/blank and invalid sort types early, preventing NPE in addSortCondition. - Add tests for null sort order, invalid sort order, and table leak prevention.
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.
Description
TL;DR: Fix a High-severity SQL injection vulnerability (GHSA-7634-7vm9-xg5q, CVSS 7.7) in
FilterDataServiceCEwhere unsanitized projection and sortBy column names allow arbitrary SQL injection into the H2 in-memory database, enabling server-side file reads viaFILE_READ()and cross-user data exposure.Root Cause
addProjectionCondition()andaddSortCondition()inFilterDataServiceCEconstruct SQL queries by wrapping user-supplied column names in backticks without escaping backtick characters within the input. An authenticated attacker with developer-level workspace access can break out of the backtick quoting and inject arbitrary SQL expressions into the H2 in-memory database.The WHERE clause was already protected via schema validation (
validConditionList()), but this same validation was never applied to projection or sortBy column names — an inconsistency that this fix addresses.Fix (Defense in Depth — 3 Layers)
Schema validation (primary fix): Added
validateProjectionColumns()andvalidateSortByColumns()methods that whitelist column names against the data schema before any SQL is constructed. This eliminates the entire class of identifier injection — any column name not present in the actual data is rejected with anAppsmithPluginException.Backtick escaping (defense in depth): Added
escapeBacktickIdentifier()that escapes backtick characters (`→``) inaddProjectionConditionandaddSortCondition, preventing breakout even if Layer 1 were bypassed.Schema generation hardening: Extended the existing quote rejection in
generateSchema()to also reject backtick characters in data column names, preventing the attack character from entering the system via the data source itself.Affected Plugins
RowsGetMethod) — uses both projection and sortBy from formDataAmazonS3Plugin) — uses sortBy from formData (projection is null)Test Coverage
Added 8 new unit tests covering:
All 29 tests pass (21 existing + 8 new).
Fixes https://github.com/appsmithorg/appsmith/security/advisories/GHSA-7634-7vm9-xg5q
Fixes https://linear.app/appsmith/issue/APP-14992/bug-prevent-sql-injection-in-uqi-filter-service-projection-and-sortby
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Important
🟣 🟣 🟣 Your tests are running.
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/22853341442
Commit: 75ed724
Workflow:
PR Automation test suiteTags:
@tag.AllSpec: ``
Mon, 09 Mar 2026 12:26:01 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Tests