Skip to content

Commit

Permalink
refactor: use SQLite parser to validate SQL queries
Browse files Browse the repository at this point in the history
- Replace string-based SQL validation with SQLite parser
- Add proper error handling for invalid queries
- Maintain existing error flow and ok flag logic
- Keep changes minimal and focused on security fix

Link to Devin run: https://app.devin.ai/sessions/7bc67faddc4d4c96ad62208b48c20218

Co-Authored-By: Matt Wong <[email protected]>
  • Loading branch information
devin-ai-integration[bot] and liquidaty committed Dec 20, 2024
1 parent d33c540 commit 7340468
Showing 1 changed file with 37 additions and 12 deletions.
49 changes: 37 additions & 12 deletions app/utils/overwrite.c
Original file line number Diff line number Diff line change
Expand Up @@ -117,27 +117,44 @@ enum zsv_status zsv_overwrite_next(void *h, struct zsv_overwrite_data *odata) {
return ctx->next(ctx, odata);
}

static const char *get_safe_sql_query(const char *user_sql) {
static const char *get_safe_sql_query(sqlite3 *db, const char *user_sql) {
static const char *default_query =
"select row, column, value, timestamp, author from overwrites order by row, column";

// Handle NULL or empty input
if (!user_sql || !*user_sql)
return default_query;

// Check for dangerous tokens that could enable SQL injection
if (strstr(user_sql, ";") || strstr(user_sql, "--") || strstr(user_sql, "/*") || strstr(user_sql, "*/") ||
strstr(user_sql, "union") || strstr(user_sql, "UNION"))
if (!db)
return default_query;

// Validate that it's a SELECT query and contains required table/columns
if (!zsv_strincmp_ascii((const unsigned char *)"select ", strlen("select "), (const unsigned char *)user_sql,
strlen("select ")) &&
strstr(user_sql, "from overwrites")) {
return user_sql; // Allow the original query if it's safe and uses the right table
sqlite3_stmt *stmt = NULL;
const char *tail = NULL;

// Try to prepare the statement (parse SQL)
int rc = sqlite3_prepare_v2(db, user_sql, -1, &stmt, &tail);

// Check if parsing succeeded and we got a valid statement
if (rc != SQLITE_OK || !stmt) {
if (stmt)
sqlite3_finalize(stmt);
return default_query;
}

// Verify it's a single statement (no additional statements in tail)
if (tail && *tail != '\0') {
sqlite3_finalize(stmt);
return default_query;
}

return default_query;
// Verify it's a read-only (SELECT) statement
if (!sqlite3_stmt_readonly(stmt)) {
sqlite3_finalize(stmt);
return default_query;
}

sqlite3_finalize(stmt);
return user_sql;
}

static enum zsv_status zsv_overwrite_init_sqlite3(struct zsv_overwrite_ctx *ctx, const char *source, size_t len) {
Expand All @@ -155,7 +172,11 @@ static enum zsv_status zsv_overwrite_init_sqlite3(struct zsv_overwrite_ctx *ctx,
const char *sql = strstr(q, zsv_overwrite_sql_prefix);
if (sql) {
const char *user_sql = sql + strlen(zsv_overwrite_sql_prefix);
ctx->sqlite3.sql = get_safe_sql_query(user_sql);
ctx->sqlite3.sql = get_safe_sql_query(ctx->sqlite3.db, user_sql);
if (!ctx->sqlite3.sql) {
fprintf(stderr, "Invalid or unsafe SQL query in URL parameter\n");
return zsv_status_error;
}
}
}

Expand All @@ -173,7 +194,11 @@ static enum zsv_status zsv_overwrite_init_sqlite3(struct zsv_overwrite_ctx *ctx,
ok = 1;
} else if (len > strlen(".sqlite3") && !strcmp(source + len - strlen(".sqlite3"), ".sqlite3")) {
ctx->sqlite3.filename = strdup(source);
ctx->sqlite3.sql = get_safe_sql_query("select * from overwrites order by row, column");
ctx->sqlite3.sql = get_safe_sql_query(ctx->sqlite3.db, "select * from overwrites order by row, column");
if (!ctx->sqlite3.sql) {
fprintf(stderr, "Invalid or unsafe default SQL query\n");
return zsv_status_error;
}
ok = 1;
}

Expand Down

0 comments on commit 7340468

Please sign in to comment.