fix(storage): escape address filter values to prevent SQL injection#1349
fix(storage): escape address filter values to prevent SQL injection#1349ariel-formance wants to merge 4 commits into
Conversation
filterAccountAddress was interpolating user-supplied values directly into
SQL string literals via fmt.Sprintf, allowing single-quote injection in
the exact-address path and single-quote/double-quote injection in the
JSONPath path.
Add escapeSQL ('' for single quotes) and escapeJSONPath (additionally
escapes backslashes and double quotes) helpers and apply them before
embedding values in the generated SQL fragments.
|
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 (1)
WalkthroughAdds SQL and JSONPath escaping helpers and applies them to address-filter SQL construction and transaction-level predicates; includes unit tests for normal and injection-like inputs. ChangesSQL/JSONPath Escaping for Address Filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1349 +/- ##
==========================================
+ Coverage 80.14% 80.98% +0.84%
==========================================
Files 205 205
Lines 11106 11098 -8
==========================================
+ Hits 8901 8988 +87
+ Misses 1587 1566 -21
+ Partials 618 544 -74 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/storage/ledger/utils.go (1)
41-61:⚠️ Potential issue | 🔴 CriticalSQL injection vulnerability found in
filterAccountAddressOnTransactionsin transactions.go — requires fix.The verification revealed a critical vulnerability:
filterAccountAddressOnTransactions(transactions.go, lines 272-321) uses the same unsafe pattern as the vulnerable code infilterAccountAddress. The function marshals user-controlled address input to JSON and directly interpolates it into SQL:data, err := json.Marshal([]string{address}) parts = append(parts, fmt.Sprintf("sources @> '%s'", string(data)))Since
json.Marshaldoes not escape single quotes, an address containing a single quote (e.g.,account:it's) produces malformed SQL:sources @> '["account:it's"]', allowing SQL injection.This function is called from
resource_transactions.gowith user-controlled input from filter operators. Fix this using the same escaping approach applied tofilterAccountAddress: properly escape or parameterize the JSON data before embedding in SQL.🤖 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 `@internal/storage/ledger/utils.go` around lines 41 - 61, filterAccountAddressOnTransactions in transactions.go constructs SQL by embedding json.Marshal output directly, allowing SQL injection; update it to either parameterize the query or escape the marshaled JSON the same way filterAccountAddress does (use the existing escapeSQL helper) before interpolating into the SQL string. Locate filterAccountAddressOnTransactions and change the line that does parts = append(parts, fmt.Sprintf("sources @> '%s'", string(data))) to use the escaped JSON (or a query parameter) so single quotes are handled safely; also review the call site in resource_transactions.go to ensure user-controlled address is passed through the fixed function.
🤖 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.
Outside diff comments:
In `@internal/storage/ledger/utils.go`:
- Around line 41-61: filterAccountAddressOnTransactions in transactions.go
constructs SQL by embedding json.Marshal output directly, allowing SQL
injection; update it to either parameterize the query or escape the marshaled
JSON the same way filterAccountAddress does (use the existing escapeSQL helper)
before interpolating into the SQL string. Locate
filterAccountAddressOnTransactions and change the line that does parts =
append(parts, fmt.Sprintf("sources @> '%s'", string(data))) to use the escaped
JSON (or a query parameter) so single quotes are handled safely; also review the
call site in resource_transactions.go to ensure user-controlled address is
passed through the fixed function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 56f3fe64-9138-460d-bbe3-a42e8303f415
📒 Files selected for processing (1)
internal/storage/ledger/utils.go
… address filters json.Marshal escapes double-quotes but not single-quotes, so injecting a single quote into a source/destination address filter broke out of the SQL string literal. Apply the same escapeSQL() used in filterAccountAddress().
…filters Covers escapeSQL, escapeJSONPath, filterAccountAddress, and filterAccountAddressOnTransactions with single-quote payloads, double-quote and backslash in JSONPath segments, and classic injection patterns.
Ensures valid addresses (exact, partial with colon, wildcard suffix, multi-key) continue to produce correct SQL fragments after the escaping changes.
2 participants
Summary
filterAccountAddresswas interpolating user-supplied address filter values directly into SQL string literals usingfmt.Sprintf, with no escaping. This allowed SQL injection via single-quote characters in filter values passed to the accounts and volumes endpoints.Confirmed via testing:
{"$match": {"address": "doesnt_exist' OR '1'='1"}}returned all accounts in the ledger instead of zero resultsBlast radius (low): injection is constrained to the authenticated user's ledger scope (table-level isolation holds) and the Go PostgreSQL driver blocks multi-statement execution, so no writes are possible. The risk is an authenticated user reading all account data/metadata within their ledger, bypassing any address-based filtering.
Fix
Added two helpers:
escapeSQL— escapes'→''for safe embedding in SQL string literalsescapeJSONPath— additionally escapes\and"for safe embedding in JSONPath double-quoted strings inside SQL literalsApplied to both injection points in
filterAccountAddress:address = '%s'→ usesescapeSQL$[i] == "%s"→ usesescapeJSONPathTest plan
{"$match": {"address": "doesnt_exist' OR '1'='1"}}now returns 0 results{"$match": {"address": ":foo"}}still work correctly