fix: sanitize array elements in list_condition_expr to prevent SQL injection#1713
Open
prabhaks wants to merge 4 commits into
Open
fix: sanitize array elements in list_condition_expr to prevent SQL injection#1713prabhaks wants to merge 4 commits into
prabhaks wants to merge 4 commits into
Conversation
…jection Closes parseablehq#1688 The list_condition_expr function was interpolating inner_value directly into ARRAY[...] SQL expressions without any escaping or validation, allowing crafted input like `1] OR 1=1; --` to break out of the ARRAY context and inject arbitrary SQL. scalar_condition_expr already handled escaping correctly (single-quote escaping, numeric/boolean validation). This commit brings list_condition_expr to the same standard by adding a sanitize_array_elements helper that: - Splits the value by comma - Validates each element as numeric, boolean, or a properly single-quoted string - Rejects any element containing bracket characters `[` or `]` that could escape the ARRAY literal context - Returns an Err if any element fails validation
Contributor
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
Contributor
WalkthroughAdds a new ChangesArray Element Sanitization
Estimated code review effort: 3 (Moderate) | ~20 minutes Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Author
|
I have read the CLA Document and I hereby sign the CLA |
nitisht
added a commit
to parseablehq/.github
that referenced
this pull request
Jul 2, 2026
The bare unquoted string branch was rejecting valid values containing spaces, colons, slashes, etc. due to an alphanumeric-only allowlist. This is inconsistent with scalar_condition_expr which accepts any string and simply escapes single quotes. The bracket character check at the top already covers the actual injection vector, making the allowlist redundant. Now mirrors scalar_condition_expr: escape single quotes and wrap in single quotes, no character allowlist.
The contains("' OR '") check was a false negative — the correctly
escaped output 'foo'' OR ''1''=''1' still contains that substring
as part of the doubled quotes. The assert_eq on the exact escaped
string is already sufficient to prove injection safety; drop the
redundant contains check.
Author
|
@nikhilsinhaparseable please review the changes whenever you get the chance. Tested locally - runs fine! |
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/alerts/alerts_utils.rs`:
- Around line 473-514: The array sanitizer in sanitize_array_elements currently
splits on every comma, which breaks quoted list items containing commas. Update
the parsing logic to be quote-aware so elements like single-quoted strings with
embedded commas stay intact, while preserving the existing trimming, bracket
rejection, and re-quoting behavior used by sanitize_array_elements and
scalar_condition_expr.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ddef110e-7b9c-401a-b0ae-e26f8bd24a62
📒 Files selected for processing (1)
src/alerts/alerts_utils.rs
Comment on lines
+473
to
+514
| fn sanitize_array_elements(value: &str) -> Result<String, String> { | ||
| let sanitized: Result<Vec<String>, String> = value | ||
| .split(',') | ||
| .map(|elem| { | ||
| let elem = elem.trim(); | ||
|
|
||
| // Reject bracket characters that could escape the ARRAY[...] context. | ||
| // This is the primary injection guard; the quoting logic below is | ||
| // consistent with scalar_condition_expr and does not rely on this check. | ||
| if elem.contains('[') || elem.contains(']') { | ||
| return Err(format!( | ||
| "Invalid array element: '{elem}' contains bracket characters" | ||
| )); | ||
| } | ||
|
|
||
| // Numeric literal — pass through as-is | ||
| if elem.parse::<f64>().is_ok() { | ||
| return Ok(elem.to_string()); | ||
| } | ||
|
|
||
| // Boolean literal — pass through as-is | ||
| if elem.parse::<bool>().is_ok() { | ||
| return Ok(elem.to_string()); | ||
| } | ||
|
|
||
| // Single-quoted string — strip outer quotes, re-escape internals, re-wrap. | ||
| // Mirrors how scalar_condition_expr handles the Some(_) column_type branch. | ||
| if elem.starts_with('\'') && elem.ends_with('\'') && elem.len() >= 2 { | ||
| let inner = &elem[1..elem.len() - 1]; | ||
| let escaped = inner.replace('\'', "''"); | ||
| return Ok(format!("'{escaped}'")); | ||
| } | ||
|
|
||
| // Bare unquoted string — escape single quotes and wrap in single quotes. | ||
| // Mirrors the ValueType::String arm in scalar_condition_expr (None branch): | ||
| // format!("'{}'", val.replace("'", "''")) | ||
| Ok(format!("'{}'", elem.replace('\'', "''"))) | ||
| }) | ||
| .collect(); | ||
|
|
||
| Ok(sanitized?.join(", ")) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find how list-type condition values are constructed/serialized to gauge whether
# a single element can contain a comma (e.g., quoted string values).
rg -nP -C3 '\blist\b|ARRAY\[|column_type' src/alerts/ src/handlers/http/query_context.rsRepository: parseablehq/parseable
Length of output: 2061
Use quote-aware splitting for list values. value.split(',') breaks quoted elements containing commas, so a single input like 'New York, NY' is turned into multiple array entries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/alerts/alerts_utils.rs` around lines 473 - 514, The array sanitizer in
sanitize_array_elements currently splits on every comma, which breaks quoted
list items containing commas. Update the parsing logic to be quote-aware so
elements like single-quoted strings with embedded commas stay intact, while
preserving the existing trimming, bracket rejection, and re-quoting behavior
used by sanitize_array_elements and scalar_condition_expr.
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.
Summary
Fixes #1688
list_condition_exprinsrc/alerts/alerts_utils.rswas interpolatinginner_valuedirectly intoARRAY[...]SQL expressions without any escaping or validation. This allowed crafted input like1] OR 1=1; --to break out of theARRAYcontext and inject arbitrary SQL.scalar_condition_exprright below it already handled this correctly (single-quote escaping, numeric/boolean validation). This PR bringslist_condition_exprto the same standard.Changes
Added a
sanitize_array_elementshelper that:[or]that could escape theARRAY[...]contextErrdescribing the first offending element if validation failslist_condition_exprnow passesinner_valuethrough this helper before interpolating it into the SQL format string.Testing
1] OR 1=1; --now returns an error instead of being interpolatedsanitize_array_elementsandlist_condition_exprReferences
Summary by CodeRabbit