fix(security): use consistent SQL escape function for channelName parameter#1727
Closed
artylobos wants to merge 2 commits intoCap-go:mainfrom
Closed
fix(security): use consistent SQL escape function for channelName parameter#1727artylobos wants to merge 2 commits intoCap-go:mainfrom
artylobos wants to merge 2 commits intoCap-go:mainfrom
Conversation
…ameter The channelName parameter in readDeviceVersionCountsCF was using an incomplete inline escape (only replacing single quotes) while the rest of the codebase uses escapeSqlString() which also escapes backslashes. This inconsistency could potentially allow SQL injection if an attacker could control channel names with backslash characters. Uses the existing escapeSqlString() helper for consistency.
Contributor
📝 WalkthroughWalkthroughThis PR documents SQL injection vulnerabilities in the Cloudflare utility functions and implements security remediations. It includes a detailed vulnerability report, fixes one injection point with a helper sanitization function, and adds SSRF protection to webhook URL validation with multiple new security checks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SECURITY_REPORT.md`:
- Around line 5-166: The SECURITY_REPORT.md is outdated and contains inaccurate
injection claims and public PoC payloads; update the report to state it's been
re-audited against the current cloudflare.ts implementation (which now uses
escapeSqlString), remove or move weaponized PoC examples to a private advisory
channel, correct or remove the listed line-numbered findings and instead
reference the functions/readers by name (e.g., readDevicesCF, readStatsCF,
readBandwidthUsageCF, readDeviceUsageCF, readStatsVersionCF, countDevicesCF) and
note that escapeSqlString (and any validation) is applied; also add an
instruction to perform a follow-up code review to verify all user-controlled
inputs are passed through escapeSqlString or a whitelist validator and to
document any remaining gaps.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
SECURITY_REPORT.mdsupabase/functions/_backend/utils/cloudflare.tssupabase/functions/_backend/utils/webhook.ts
Comment on lines
+5
to
+166
| Multiple SQL injection vulnerabilities exist in `supabase/functions/_backend/utils/cloudflare.ts`. User-controlled values from API request bodies are interpolated directly into SQL query strings sent to the Cloudflare Analytics Engine API without sanitization or parameterization. | ||
|
|
||
| ## Severity: HIGH | ||
|
|
||
| An authenticated user with read-level API key permissions can inject arbitrary SQL into Cloudflare Analytics Engine queries, potentially accessing data belonging to other users/apps. | ||
|
|
||
| ## Affected Code | ||
|
|
||
| **File:** `supabase/functions/_backend/utils/cloudflare.ts` | ||
|
|
||
| ### Injection Point 1: `readDevicesCF` — deviceIds (line 562, 565) | ||
|
|
||
| ```typescript | ||
| // Line 562 - single device ID, no escaping | ||
| conditions.push(`blob1 = '${params.deviceIds[0]}'`) | ||
|
|
||
| // Line 565 - multiple device IDs, each quoted but NOT escaped | ||
| const devicesList = params.deviceIds.map(id => `'${id}'`).join(', ') | ||
| conditions.push(`blob1 IN (${devicesList})`) | ||
| ``` | ||
|
|
||
| ### Injection Point 2: `readDevicesCF` — search (line 574, 578) | ||
|
|
||
| ```typescript | ||
| // Line 574 | ||
| conditions.push(`position('${searchLower}' IN toLower(blob5)) > 0`) | ||
| ``` | ||
|
|
||
| ### Injection Point 3: `readDevicesCF` — cursor (line 592) | ||
|
|
||
| ```typescript | ||
| // Line 592 - cursor values split and interpolated | ||
| cursorFilter = `AND (timestamp < toDateTime('${cursorTime}') OR (timestamp = toDateTime('${cursorTime}') AND blob1 > '${cursorDeviceId}'))` | ||
| ``` | ||
|
|
||
| ### Injection Point 4: `readStatsCF` — deviceIds WITHOUT quotes (line 668-669) — MOST CRITICAL | ||
|
|
||
| ```typescript | ||
| // Line 668-669 - deviceIds joined WITHOUT any quoting | ||
| const devicesList = params.deviceIds.join(',') | ||
| deviceFilter = `AND device_id IN (${devicesList})` | ||
| ``` | ||
|
|
||
| This is the most dangerous because values are not even wrapped in single quotes, making injection trivial. | ||
|
|
||
| ### Injection Point 5: `readStatsCF` — actions (line 677, 680) | ||
|
|
||
| ```typescript | ||
| actionsFilter = `AND action = '${params.actions[0]}'` | ||
| ``` | ||
|
|
||
| ### Injection Point 6: `readStatsCF` — search (line 689-691) | ||
|
|
||
| ```typescript | ||
| searchFilter = `AND (position('${searchLower}' IN toLower(device_id)) > 0 ...)` | ||
| ``` | ||
|
|
||
| ### Injection Point 7: `readDevicesCF` — version_name (line 583) | ||
|
|
||
| ```typescript | ||
| conditions.push(`blob2 = '${params.version_name}'`) | ||
| ``` | ||
|
|
||
| ## Attack Vector | ||
|
|
||
| ### Entry Points | ||
|
|
||
| 1. **POST /private/devices** → `devices.ts` line 29 → calls `readDevices` → calls `readDevicesCF` | ||
| 2. **POST /private/stats** → `stats.ts` line 80 → calls `readStats` → calls `readStatsCF` | ||
|
|
||
| ### Authentication Required | ||
|
|
||
| Both endpoints require `middlewareV2(['read', 'write', 'all', 'upload'])` — a valid API key with at least read permission. However, any legitimate user of the platform has this. | ||
|
|
||
| ### Input Validation | ||
|
|
||
| The `devicesId` field is validated as `z.array(z.string())` — this only checks that it's an array of strings. SQL injection payloads are valid strings and pass this validation. | ||
|
|
||
| ## Proof of Concept | ||
|
|
||
| ### Attack on `readStatsCF` (line 668-669) | ||
|
|
||
| The most critical injection point — deviceIds are joined without quotes: | ||
|
|
||
| ```bash | ||
| curl -X POST https://api.capgo.app/private/stats \ | ||
| -H "Authorization: Bearer <valid-api-key>" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{ | ||
| "appId": "com.example.app", | ||
| "devicesId": ["1) OR 1=1 UNION SELECT * FROM stats WHERE app_id != '\''com.example.app'\'' --"] | ||
| }' | ||
| ``` | ||
|
|
||
| This would modify the query from: | ||
| ```sql | ||
| ... AND device_id IN (1) OR 1=1 UNION SELECT * FROM stats WHERE app_id != 'com.example.app' --) | ||
| ``` | ||
|
|
||
| ### Attack on `readDevicesCF` search parameter | ||
|
|
||
| ```bash | ||
| curl -X POST https://api.capgo.app/private/devices \ | ||
| -H "Authorization: Bearer <valid-api-key>" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{ | ||
| "appId": "com.example.app", | ||
| "search": "'\'' OR 1=1) --" | ||
| }' | ||
| ``` | ||
|
|
||
| ## Impact | ||
|
|
||
| 1. **Data exfiltration**: Read analytics data from other users' apps | ||
| 2. **Cross-tenant data access**: Break app_id isolation in multi-tenant queries | ||
| 3. **Information disclosure**: Enumerate device IDs, version names, and usage patterns of other apps | ||
|
|
||
| ## Suggested Fix | ||
|
|
||
| Replace all string interpolation with parameterized queries or strict input validation: | ||
|
|
||
| ```typescript | ||
| // Option 1: Strict allowlist validation | ||
| function validateDeviceId(id: string): string { | ||
| if (!/^[a-zA-Z0-9_\-:.]+$/.test(id)) { | ||
| throw new Error(`Invalid device ID: ${id}`) | ||
| } | ||
| return id | ||
| } | ||
|
|
||
| // Option 2: Escape single quotes (minimum fix) | ||
| function escapeSql(value: string): string { | ||
| return value.replace(/'/g, "''") | ||
| } | ||
|
|
||
| // Apply to all interpolation points: | ||
| conditions.push(`blob1 = '${escapeSql(params.deviceIds[0])}'`) | ||
| ``` | ||
|
|
||
| The systematic fix should: | ||
| 1. Create a `sanitizeSqlString()` utility function | ||
| 2. Apply it to ALL user-controlled values before SQL interpolation in `cloudflare.ts` | ||
| 3. Add input validation regex for deviceIds, search, version_name, cursor, and actions | ||
| 4. Consider using Cloudflare Analytics Engine's parameterized query support if available | ||
|
|
||
| ## Affected Functions (Complete List) | ||
|
|
||
| | Function | Line | Injected Parameter | | ||
| |----------|------|--------------------| | ||
| | `readDevicesCF` | 562 | deviceIds[0] | | ||
| | `readDevicesCF` | 565 | deviceIds (multiple) | | ||
| | `readDevicesCF` | 574, 578 | search | | ||
| | `readDevicesCF` | 583 | version_name | | ||
| | `readDevicesCF` | 592 | cursor | | ||
| | `readStatsCF` | 665 | deviceIds[0] | | ||
| | `readStatsCF` | 668-669 | deviceIds (NO QUOTES) | | ||
| | `readStatsCF` | 677, 680 | actions | | ||
| | `readStatsCF` | 689-691 | search | | ||
| | `readBandwidthUsageCF` | 401 | app_id | | ||
| | `readDeviceUsageCF` | 340 | app_id | | ||
| | `readStatsVersionCF` | ~460 | app_id | | ||
| | `countDevicesCF` | ~517 | app_id | |
Contributor
There was a problem hiding this comment.
This security report is stale against the current code and should be revised before merge.
Several vulnerability snippets (e.g., Line 19, Line 22, Line 45, Line 53, Line 65) describe unsanitized interpolation that does not match the current cloudflare.ts implementation in this PR (which uses escapeSqlString(...) on those paths). Also, the PoC section (Line 89+) contains weaponized endpoint payloads; if retained, move this to a private advisory channel or heavily redact for public repo safety.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SECURITY_REPORT.md` around lines 5 - 166, The SECURITY_REPORT.md is outdated
and contains inaccurate injection claims and public PoC payloads; update the
report to state it's been re-audited against the current cloudflare.ts
implementation (which now uses escapeSqlString), remove or move weaponized PoC
examples to a private advisory channel, correct or remove the listed
line-numbered findings and instead reference the functions/readers by name
(e.g., readDevicesCF, readStatsCF, readBandwidthUsageCF, readDeviceUsageCF,
readStatsVersionCF, countDevicesCF) and note that escapeSqlString (and any
validation) is applied; also add an instruction to perform a follow-up code
review to verify all user-controlled inputs are passed through escapeSqlString
or a whitelist validator and to document any remaining gaps.
|
Member
|
do not make security fix on PR use proper security disclosure |
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.
Summary
The
channelNameparameter inreadDeviceVersionCountsCFwas using an incomplete inline escape that only replaced single quotes, while the rest of the codebase usesescapeSqlString()which also escapes backslashes.Security Impact
This inconsistency could potentially allow SQL injection in Cloudflare Analytics Engine queries if an attacker could control channel names containing backslash characters (e.g., via channel name creation/update).
Changes
escapeSqlString()helper function for consistent escapingTesting
Note: This is a minor security hardening fix. While channel names typically come from the database (already validated), using consistent escaping is a defense-in-depth measure.
Summary by CodeRabbit
Bug Fixes
Documentation