Skip to content

Commit f544f39

Browse files
datlechindatlechin
committed
fix: Address code quality and security issues
- Add centralized notification constant for loadQueryIntoEditor - Fix SQL injection vulnerabilities in QueryHistoryStorage: * Refactor fetchHistory to use parameterized queries * Refactor fetchBookmarks to use parameterized queries * Replace string interpolation with sqlite3_bind_* functions - Remove debug print statements (~40 prints removed): * QueryHistoryStorage.swift * HistoryListViewController.swift * MainContentView.swift Security improvements: - All SQL queries now use parameter binding - Eliminated potential SQL injection attack vectors - Proper handling of UUID, date, and search text parameters Code quality improvements: - Centralized notification names in extension - Removed debug noise from production code - Cleaner error handling in storage layer
1 parent 90b98a6 commit f544f39

6 files changed

Lines changed: 87 additions & 90 deletions

File tree

‎OpenTable.xcodeproj/project.xcworkspace/xcuserdata/ngoquocdat.xcuserdatad/UserInterfaceState.xcuserstate‎

0 Bytes
Binary file not shown.

‎OpenTable/Core/Storage/QueryHistoryManager.swift‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import Foundation
1212
extension Notification.Name {
1313
static let queryHistoryDidUpdate = Notification.Name("queryHistoryDidUpdate")
1414
static let queryBookmarksDidUpdate = Notification.Name("queryBookmarksDidUpdate")
15+
static let loadQueryIntoEditor = Notification.Name("loadQueryIntoEditor")
1516
}
1617

1718
/// Thread-safe manager for query history and bookmarks

‎OpenTable/Core/Storage/QueryHistoryStorage.swift‎

Lines changed: 82 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,6 @@ final class QueryHistoryStorage {
200200

201201
var statement: OpaquePointer?
202202
guard sqlite3_prepare_v2(self.db, sql, -1, &statement, nil) == SQLITE_OK else {
203-
print("[QueryHistory] Failed to prepare INSERT statement")
204203
DispatchQueue.main.async { completion?(false) }
205204
return
206205
}
@@ -228,12 +227,7 @@ final class QueryHistoryStorage {
228227
let result = sqlite3_step(statement)
229228
let success = result == SQLITE_DONE
230229

231-
if !success {
232-
let errorMsg = String(cString: sqlite3_errmsg(self.db))
233-
print("[QueryHistory] INSERT failed: \(errorMsg)")
234-
} else {
235-
print("[QueryHistory] Successfully recorded query")
236-
}
230+
// Silently handle errors - logging can be added via proper logging framework if needed
237231

238232
DispatchQueue.main.async { completion?(success) }
239233
}
@@ -250,37 +244,52 @@ final class QueryHistoryStorage {
250244
return queue.sync {
251245
var entries: [QueryHistoryEntry] = []
252246

253-
// Build query
254-
var sql = "SELECT id, query, connection_id, database_name, executed_at, execution_time, row_count, was_successful, error_message FROM history"
255-
var whereClauses: [String] = []
256-
257-
if let connectionId = connectionId {
258-
whereClauses.append("connection_id = '\(connectionId.uuidString)'")
259-
}
260-
261-
if let startDate = dateFilter.startDate {
262-
whereClauses.append("executed_at >= \(startDate.timeIntervalSince1970)")
263-
}
247+
// Build query with placeholders
248+
var sql: String
249+
var bindIndex: Int32 = 1
250+
var hasConnectionFilter = false
251+
var hasDateFilter = false
264252

265253
// Use FTS5 for full-text search if search text provided
266254
if let searchText = searchText, !searchText.isEmpty {
267-
let ftsQuery = searchText.replacingOccurrences(of: "'", with: "''")
268255
sql = """
269256
SELECT h.id, h.query, h.connection_id, h.database_name, h.executed_at, h.execution_time, h.row_count, h.was_successful, h.error_message
270257
FROM history h
271258
INNER JOIN history_fts ON h.rowid = history_fts.rowid
272-
WHERE history_fts MATCH '\(ftsQuery)'
259+
WHERE history_fts MATCH ?
273260
"""
274261

275-
// Add other where clauses if needed
262+
// Add additional filters
263+
if connectionId != nil {
264+
sql += " AND h.connection_id = ?"
265+
hasConnectionFilter = true
266+
}
267+
268+
if dateFilter.startDate != nil {
269+
sql += " AND h.executed_at >= ?"
270+
hasDateFilter = true
271+
}
272+
} else {
273+
sql = "SELECT id, query, connection_id, database_name, executed_at, execution_time, row_count, was_successful, error_message FROM history"
274+
275+
var whereClauses: [String] = []
276+
277+
if connectionId != nil {
278+
whereClauses.append("connection_id = ?")
279+
hasConnectionFilter = true
280+
}
281+
282+
if dateFilter.startDate != nil {
283+
whereClauses.append("executed_at >= ?")
284+
hasDateFilter = true
285+
}
286+
276287
if !whereClauses.isEmpty {
277-
sql += " AND " + whereClauses.joined(separator: " AND ")
288+
sql += " WHERE " + whereClauses.joined(separator: " AND ")
278289
}
279-
} else if !whereClauses.isEmpty {
280-
sql += " WHERE " + whereClauses.joined(separator: " AND ")
281290
}
282291

283-
sql += " ORDER BY executed_at DESC LIMIT \(limit) OFFSET \(offset);"
292+
sql += " ORDER BY executed_at DESC LIMIT ? OFFSET ?;"
284293

285294
var statement: OpaquePointer?
286295
guard sqlite3_prepare_v2(db, sql, -1, &statement, nil) == SQLITE_OK else {
@@ -289,6 +298,28 @@ final class QueryHistoryStorage {
289298

290299
defer { sqlite3_finalize(statement) }
291300

301+
let SQLITE_TRANSIENT = unsafeBitCast(-1, to: sqlite3_destructor_type.self)
302+
303+
// Bind parameters in order
304+
if let searchText = searchText, !searchText.isEmpty {
305+
sqlite3_bind_text(statement, bindIndex, searchText, -1, SQLITE_TRANSIENT)
306+
bindIndex += 1
307+
}
308+
309+
if let connectionId = connectionId, hasConnectionFilter {
310+
sqlite3_bind_text(statement, bindIndex, connectionId.uuidString, -1, SQLITE_TRANSIENT)
311+
bindIndex += 1
312+
}
313+
314+
if let startDate = dateFilter.startDate, hasDateFilter {
315+
sqlite3_bind_double(statement, bindIndex, startDate.timeIntervalSince1970)
316+
bindIndex += 1
317+
}
318+
319+
sqlite3_bind_int(statement, bindIndex, Int32(limit))
320+
bindIndex += 1
321+
sqlite3_bind_int(statement, bindIndex, Int32(offset))
322+
292323
while sqlite3_step(statement) == SQLITE_ROW {
293324
if let entry = parseHistoryEntry(from: statement) {
294325
entries.append(entry)
@@ -358,8 +389,6 @@ final class QueryHistoryStorage {
358389

359390
var statement: OpaquePointer?
360391
guard sqlite3_prepare_v2(db, sql, -1, &statement, nil) == SQLITE_OK else {
361-
let errorMsg = String(cString: sqlite3_errmsg(db))
362-
print("[QueryHistoryStorage] Failed to prepare bookmark INSERT: \(errorMsg)")
363392
return false
364393
}
365394

@@ -401,12 +430,7 @@ final class QueryHistoryStorage {
401430
let result = sqlite3_step(statement)
402431
let success = result == SQLITE_DONE
403432

404-
if !success {
405-
let errorMsg = String(cString: sqlite3_errmsg(db))
406-
print("[QueryHistoryStorage] Bookmark INSERT failed: \(errorMsg)")
407-
} else {
408-
print("[QueryHistoryStorage] Successfully saved bookmark: \(nameString)")
409-
}
433+
// Silently handle errors
410434

411435
return success
412436
}
@@ -432,8 +456,6 @@ final class QueryHistoryStorage {
432456

433457
var statement: OpaquePointer?
434458
guard sqlite3_prepare_v2(db, sql, -1, &statement, nil) == SQLITE_OK else {
435-
let errorMsg = String(cString: sqlite3_errmsg(db))
436-
print("[QueryHistoryStorage] Failed to prepare bookmark UPDATE: \(errorMsg)")
437459
return false
438460
}
439461

@@ -474,12 +496,7 @@ final class QueryHistoryStorage {
474496
let result = sqlite3_step(statement)
475497
let success = result == SQLITE_DONE
476498

477-
if !success {
478-
let errorMsg = String(cString: sqlite3_errmsg(db))
479-
print("[QueryHistoryStorage] Bookmark UPDATE failed: \(errorMsg)")
480-
} else {
481-
print("[QueryHistoryStorage] Successfully updated bookmark: \(nameString)")
482-
}
499+
// Silently handle errors
483500

484501
return success
485502
}
@@ -492,15 +509,18 @@ final class QueryHistoryStorage {
492509

493510
var sql = "SELECT id, name, query, connection_id, tags, created_at, last_used_at, notes FROM bookmarks"
494511
var whereClauses: [String] = []
512+
var bindIndex: Int32 = 1
513+
var hasSearchFilter = false
514+
var hasTagFilter = false
495515

496516
if let searchText = searchText, !searchText.isEmpty {
497-
let escaped = searchText.replacingOccurrences(of: "'", with: "''")
498-
whereClauses.append("(name LIKE '%\(escaped)%' OR query LIKE '%\(escaped)%')")
517+
whereClauses.append("(name LIKE ? OR query LIKE ?)")
518+
hasSearchFilter = true
499519
}
500520

501521
if let tag = tag, !tag.isEmpty {
502-
let escaped = tag.replacingOccurrences(of: "'", with: "''")
503-
whereClauses.append("tags LIKE '%\(escaped)%'")
522+
whereClauses.append("tags LIKE ?")
523+
hasTagFilter = true
504524
}
505525

506526
if !whereClauses.isEmpty {
@@ -516,6 +536,22 @@ final class QueryHistoryStorage {
516536

517537
defer { sqlite3_finalize(statement) }
518538

539+
let SQLITE_TRANSIENT = unsafeBitCast(-1, to: sqlite3_destructor_type.self)
540+
541+
// Bind parameters in order
542+
if let searchText = searchText, !searchText.isEmpty, hasSearchFilter {
543+
let searchPattern = "%\(searchText)%"
544+
sqlite3_bind_text(statement, bindIndex, searchPattern, -1, SQLITE_TRANSIENT)
545+
bindIndex += 1
546+
sqlite3_bind_text(statement, bindIndex, searchPattern, -1, SQLITE_TRANSIENT)
547+
bindIndex += 1
548+
}
549+
550+
if let tag = tag, !tag.isEmpty, hasTagFilter {
551+
let tagPattern = "%\(tag)%"
552+
sqlite3_bind_text(statement, bindIndex, tagPattern, -1, SQLITE_TRANSIENT)
553+
}
554+
519555
while sqlite3_step(statement) == SQLITE_ROW {
520556
if let bookmark = parseBookmark(from: statement) {
521557
bookmarks.append(bookmark)
@@ -534,8 +570,6 @@ final class QueryHistoryStorage {
534570
let sql = "DELETE FROM bookmarks WHERE id = ?;"
535571
var statement: OpaquePointer?
536572
guard sqlite3_prepare_v2(db, sql, -1, &statement, nil) == SQLITE_OK else {
537-
let errorMsg = String(cString: sqlite3_errmsg(db))
538-
print("[QueryHistoryStorage] Failed to prepare DELETE bookmark: \(errorMsg)")
539573
return false
540574
}
541575

@@ -547,16 +581,7 @@ final class QueryHistoryStorage {
547581
let result = sqlite3_step(statement)
548582
let success = result == SQLITE_DONE
549583

550-
if success {
551-
let changes = sqlite3_changes(db)
552-
print("[QueryHistoryStorage] Deleted bookmark \(idString), rows affected: \(changes)")
553-
if changes == 0 {
554-
print("[QueryHistoryStorage] WARNING: Delete succeeded but no rows were affected!")
555-
}
556-
} else {
557-
let errorMsg = String(cString: sqlite3_errmsg(db))
558-
print("[QueryHistoryStorage] Failed to DELETE bookmark: \(errorMsg), result: \(result)")
559-
}
584+
// Silently handle success/failure
560585

561586
return success
562587
}

0 commit comments

Comments
 (0)