🛡️ Sentinel: [HIGH] Fix authorization bypass in database row mutation APIs#483
🛡️ Sentinel: [HIGH] Fix authorization bypass in database row mutation APIs#483Dexploarer wants to merge 1 commit into
Conversation
This patch adds `assertTableExists` checks to the `PUT` (handleUpdateRow) and `DELETE` (handleDeleteRow) endpoints in the database API. Previously, these endpoints used the raw tableName path parameter in raw SQL strings, which constituted an authorization bypass because malicious users could theoretically mutate arbitrary or internal tables. Now, update and delete mutations use the exact same validation mechanism as inserts and reads.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| return; | ||
| } | ||
|
|
||
| if (!(await assertTableExists(runtime, tableName))) { | ||
| sendJsonError(res, `Table "${tableName}" not found`, 404); | ||
| return; | ||
| } | ||
|
|
||
| const setClauses = Object.entries(body.data).map(([col, val]) => | ||
| sqlAssign(col, val), | ||
| ); |
There was a problem hiding this comment.
Security: Risk of SQL Injection in Update Handler
The code constructs SQL SET and WHERE clauses for the UPDATE statement by directly interpolating user-supplied data using custom helpers (sqlAssign, sqlPredicate). Unless these helpers are proven to be robust against all forms of SQL injection, this approach is highly risky. Attackers could potentially craft input that bypasses escaping and executes arbitrary SQL.
Recommendation:
Use parameterized queries provided by your database library or ORM instead of manual string interpolation. For example, with Drizzle or pg, use placeholders and pass values as parameters to ensure safe query execution.
Example (conceptual):
await db.execute('UPDATE table SET col1 = $1 WHERE col2 = $2', [val1, val2]);| return; | ||
| } | ||
|
|
||
| if (!(await assertTableExists(runtime, tableName))) { | ||
| sendJsonError(res, `Table "${tableName}" not found`, 404); | ||
| return; | ||
| } | ||
|
|
||
| const whereClauses = Object.entries(body.where).map(([col, val]) => | ||
| sqlPredicate(col, val), | ||
| ); |
There was a problem hiding this comment.
Security: Risk of SQL Injection in Delete Handler
The construction of the WHERE clause for the DELETE statement uses direct mapping of user input via sqlPredicate, which may not be sufficient to prevent SQL injection. This is a critical security vulnerability if the helper does not fully sanitize all possible input cases.
Recommendation:
Switch to parameterized queries for all user-supplied data in SQL statements. Avoid manual string construction for SQL commands. Use the parameterization features of your database adapter to ensure all values are safely escaped and injected.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an authorization bypass vulnerability by implementing table existence checks in the update and delete row endpoints, ensuring that operations are restricted to valid user tables. It also adds a corresponding entry to the sentinel log documenting the vulnerability and its prevention. The review feedback suggests optimizing these handlers by moving the existence checks before the request body parsing to follow a fail-fast pattern and improve resource efficiency.
| if (!(await assertTableExists(runtime, tableName))) { | ||
| sendJsonError(res, `Table "${tableName}" not found`, 404); | ||
| return; | ||
| } |
There was a problem hiding this comment.
While adding the assertTableExists check correctly addresses the authorization bypass, its placement after readJsonBody is less than optimal. Moving this check to the very beginning of the function (before parsing the request body) would follow the 'fail-fast' principle, avoiding unnecessary resource consumption (CPU/memory for JSON parsing) when the target table does not exist. This would also align with the implementation in handleGetRows.
| if (!(await assertTableExists(runtime, tableName))) { | ||
| sendJsonError(res, `Table "${tableName}" not found`, 404); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Similar to handleUpdateRow, the assertTableExists check should ideally be performed at the start of the function, before readJsonBody. This prevents the server from processing potentially large or malformed request bodies for non-existent or unauthorized tables, improving efficiency and DoS resilience.
1 participant
🚨 Severity: HIGH
💡 Vulnerability: Authorization bypass in database API. The
PUTandDELETEendpoints for modifying database rows did not verify that the target table was a legitimate user-facing base table using theassertTableExistscheck.🎯 Impact: An attacker could potentially modify or delete rows in arbitrary tables, including internal system tables, by manually crafting a request to
/api/database/tables/:table/rowswith an unauthorized or internal table name.🔧 Fix: Added
await assertTableExists(runtime, tableName)tohandleUpdateRowandhandleDeleteRow, returning a 404 error if the table is invalid, mirroring the behavior ofhandleInsertRowandhandleGetRows.✅ Verification: Local tests pass. Ran
bun run test src/api/database.security.test.ts test/database-api.e2e.test.tsto ensure core API flow and security tests remain green.PR created automatically by Jules for task 2888632438088747809 started by @Dexploarer