-
-
Notifications
You must be signed in to change notification settings - Fork 285
Refactor: Declarative ESC key handling system with security fixes #23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
22b70f4
cc57afb
6fe8427
9ac3fe5
62c9eb8
9de93c2
df03752
b0d8e7f
56bc4d1
0fdbbdb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -502,4 +502,80 @@ final class MySQLDriver: DatabaseDriver { | |
| let result = try await execute(query: "SHOW DATABASES") | ||
| return result.rows.compactMap { row in row.first.flatMap { $0 } } | ||
| } | ||
|
|
||
| /// Escape a value for safe use in a single-quoted SQL string literal. | ||
| /// | ||
| /// This helper is intended *only* for contexts where the value will be placed | ||
| /// inside single quotes (e.g. `WHERE TABLE_SCHEMA = '...'`) and should not be | ||
| /// used for identifiers (such as database, table, or column names). | ||
| private func escapeForSQLStringLiteral(_ value: String) -> String { | ||
| // Escape single quotes by doubling them, per SQL standard. | ||
| return value.replacingOccurrences(of: "'", with: "''") | ||
| } | ||
|
|
||
| /// Fetch metadata for a specific database | ||
| func fetchDatabaseMetadata(_ database: String) async throws -> DatabaseMetadata { | ||
| // Escape database name for use as a SQL string literal in information_schema queries | ||
| let escapedDbLiteral = escapeForSQLStringLiteral(database) | ||
|
|
||
| // Query for table count | ||
| let countQuery = """ | ||
| SELECT COUNT(*) as table_count | ||
| FROM information_schema.TABLES | ||
| WHERE TABLE_SCHEMA = '\(escapedDbLiteral)' | ||
| """ | ||
| let countResult = try await execute(query: countQuery) | ||
| let tableCount = Int(countResult.rows.first?[0] ?? "0") ?? 0 | ||
|
|
||
| // Query for size | ||
| let sizeQuery = """ | ||
| SELECT SUM(DATA_LENGTH + INDEX_LENGTH) as size | ||
| FROM information_schema.TABLES | ||
| WHERE TABLE_SCHEMA = '\(escapedDbLiteral)' | ||
| """ | ||
| let sizeResult = try await execute(query: sizeQuery) | ||
| let sizeString = sizeResult.rows.first?[0] ?? "0" | ||
| let sizeBytes = Int64(sizeString) ?? 0 | ||
|
|
||
| // Determine if system database | ||
| let systemDatabases = ["information_schema", "mysql", "performance_schema", "sys"] | ||
| let isSystem = systemDatabases.contains(database) | ||
|
|
||
| return DatabaseMetadata( | ||
| id: database, | ||
| name: database, | ||
| tableCount: tableCount, | ||
| sizeBytes: sizeBytes, | ||
| lastAccessed: nil, // Could track separately if needed | ||
| isSystemDatabase: isSystem, | ||
| icon: isSystem ? "gearshape.fill" : "cylinder.fill" | ||
| ) | ||
| } | ||
|
|
||
| /// Create a new database | ||
| func createDatabase(name: String, charset: String, collation: String?) async throws { | ||
| // Escape backticks in database name | ||
| let escapedName = name.replacingOccurrences(of: "`", with: "``") | ||
|
|
||
| // Validate charset (basic validation - should be expanded) | ||
| let validCharsets = ["utf8mb4", "utf8", "latin1", "ascii"] | ||
| guard validCharsets.contains(charset) else { | ||
| throw DatabaseError.queryFailed("Invalid character set: \(charset)") | ||
| } | ||
|
|
||
| var query = "CREATE DATABASE `\(escapedName)` CHARACTER SET \(charset)" | ||
|
|
||
| // Validate collation if provided | ||
| if let collation = collation { | ||
| // Collation must match charset prefix and only contain safe identifier characters | ||
| let allowedChars = CharacterSet.alphanumerics.union(CharacterSet(charactersIn: "_")) | ||
| let isSafe = collation.unicodeScalars.allSatisfy { allowedChars.contains($0) } | ||
| guard collation.hasPrefix(charset), isSafe else { | ||
| throw DatabaseError.queryFailed("Invalid collation for charset") | ||
| } | ||
| query += " COLLATE \(collation)" | ||
|
||
| } | ||
|
|
||
| _ = try await execute(query: query) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -423,4 +423,71 @@ final class PostgreSQLDriver: DatabaseDriver { | |
| let result = try await execute(query: "SELECT datname FROM pg_database WHERE datistemplate = false ORDER BY datname") | ||
| return result.rows.compactMap { row in row.first.flatMap { $0 } } | ||
| } | ||
|
|
||
| /// Fetch metadata for a specific database | ||
| func fetchDatabaseMetadata(_ database: String) async throws -> DatabaseMetadata { | ||
| // Escape single quotes for SQL string literals | ||
| let escapedDbLiteral = database.replacingOccurrences(of: "'", with: "''") | ||
|
datlechin marked this conversation as resolved.
|
||
|
|
||
| // Query for table count | ||
| let countQuery = """ | ||
| SELECT COUNT(*) | ||
| FROM information_schema.tables | ||
| WHERE table_schema = 'public' AND table_catalog = '\(escapedDbLiteral)' | ||
| """ | ||
| let countResult = try await execute(query: countQuery) | ||
| let tableCount = Int(countResult.rows.first?[0] ?? "0") ?? 0 | ||
|
|
||
| // Query for size | ||
| let sizeQuery = """ | ||
| SELECT pg_database_size('\(escapedDbLiteral)') | ||
| """ | ||
| let sizeResult = try await execute(query: sizeQuery) | ||
| let sizeString = sizeResult.rows.first?[0] ?? "0" | ||
| let sizeBytes = Int64(sizeString) ?? 0 | ||
|
|
||
| // Determine if system database | ||
| let systemDatabases = ["postgres", "template0", "template1"] | ||
| let isSystem = systemDatabases.contains(database) | ||
|
|
||
| return DatabaseMetadata( | ||
| id: database, | ||
| name: database, | ||
| tableCount: tableCount, | ||
| sizeBytes: sizeBytes, | ||
| lastAccessed: nil, | ||
| isSystemDatabase: isSystem, | ||
| icon: isSystem ? "gearshape.fill" : "cylinder.fill" | ||
| ) | ||
| } | ||
|
|
||
| /// Create a new database | ||
| func createDatabase(name: String, charset: String, collation: String?) async throws { | ||
| // Escape double quotes in database name (PostgreSQL identifiers) | ||
| let escapedName = name.replacingOccurrences(of: "\"", with: "\"\"") | ||
|
|
||
| // Validate charset (basic validation) | ||
| let validCharsets = ["UTF8", "LATIN1", "SQL_ASCII"] | ||
| let normalizedCharset = charset.uppercased() | ||
| guard validCharsets.contains(normalizedCharset) else { | ||
| throw DatabaseError.queryFailed("Invalid encoding: \(charset)") | ||
| } | ||
|
|
||
| var query = "CREATE DATABASE \"\(escapedName)\" ENCODING '\(normalizedCharset)'" | ||
|
||
|
|
||
| // Validate and add collation if provided | ||
| if let collation = collation { | ||
| // Strict validation: allow only typical locale/collation characters | ||
| let allowedCollationChars = CharacterSet(charactersIn: "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_.-") | ||
| let isValidCollation = collation.unicodeScalars.allSatisfy { allowedCollationChars.contains($0) } | ||
| guard isValidCollation else { | ||
| throw DatabaseError.queryFailed("Invalid collation") | ||
| } | ||
| // Escape single quotes for safe SQL literal usage | ||
| let escapedCollation = collation.replacingOccurrences(of: "'", with: "''") | ||
| query += " LC_COLLATE '\(escapedCollation)'" | ||
| } | ||
|
|
||
| _ = try await execute(query: query) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| // | ||
| // EscapeKeyCoordinator.swift | ||
| // TablePro | ||
| // | ||
| // Coordinates ESC key handling across the app using SwiftUI environment. | ||
| // | ||
|
|
||
| import AppKit | ||
| import Combine | ||
| import SwiftUI | ||
|
|
||
| // MARK: - Coordinator | ||
|
|
||
| /// Manages global ESC key handling and coordinates with SwiftUI environment | ||
| @MainActor | ||
| public final class EscapeKeyCoordinator: ObservableObject { | ||
| public static let shared = EscapeKeyCoordinator() | ||
|
|
||
| /// The current environment context (updated by root view) | ||
| @Published private(set) var currentContext: EscapeKeyContext = EscapeKeyContext() | ||
|
|
||
| /// NSEvent monitor for capturing ESC key globally | ||
| private var eventMonitor: Any? | ||
|
|
||
| private init() {} | ||
|
|
||
| // MARK: - Setup | ||
|
|
||
| /// Install global ESC key monitor | ||
| /// Should be called once at app startup | ||
| public func install() { | ||
| guard eventMonitor == nil else { return } | ||
|
|
||
| eventMonitor = NSEvent.addLocalMonitorForEvents(matching: .keyDown) { [weak self] event in | ||
| guard let self = self else { return event } | ||
|
|
||
| // Check if it's the ESC key (keyCode 53) | ||
| if event.keyCode == 53 { | ||
| // Process through environment handlers | ||
| if self.handleEscape() { | ||
| return nil // Consumed | ||
| } | ||
| } | ||
|
|
||
| return event // Pass through | ||
| } | ||
| } | ||
|
|
||
| /// Remove global ESC key monitor | ||
| public func uninstall() { | ||
| if let monitor = eventMonitor { | ||
| NSEvent.removeMonitor(monitor) | ||
| eventMonitor = nil | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Context Management | ||
|
|
||
| /// Update the current context (called by root view's environment reader) | ||
| public func updateContext(_ context: EscapeKeyContext) { | ||
| self.currentContext = context | ||
| } | ||
|
|
||
| // MARK: - Processing | ||
|
|
||
| /// Process ESC key through all registered handlers | ||
| /// Returns true if handled, false if ignored by all | ||
| public func handleEscape() -> Bool { | ||
| // Check for special cases first (popups, autocomplete) | ||
| if shouldIgnoreForSpecialWindows() { | ||
| return false | ||
| } | ||
|
|
||
| // Process handlers in priority order (highest first) | ||
| let handlers = currentContext.sortedHandlers() | ||
|
|
||
| for handler in handlers { | ||
| let result = handler.handle() | ||
|
|
||
| switch result { | ||
| case .handled: | ||
| // Handler consumed the ESC key, stop propagation | ||
| return true | ||
|
|
||
| case .ignored: | ||
| // Handler didn't process, try next | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| // No handler processed the ESC key | ||
| return false | ||
| } | ||
|
|
||
| // MARK: - Special Cases | ||
|
|
||
| /// Check if ESC should be ignored for special windows (autocomplete, popups, etc.) | ||
| private func shouldIgnoreForSpecialWindows() -> Bool { | ||
| // Check if autocomplete/popup window is visible | ||
| if let frontmostWindow = NSApp.keyWindow, | ||
| frontmostWindow.level == .popUpMenu, | ||
| frontmostWindow.isVisible { | ||
| // Let the popup handle ESC | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The charset parameter is validated against a whitelist but is not escaped when interpolated into the SQL query. While the validation provides some protection, it would be safer to escape the charset value or use parameterized queries to fully prevent potential SQL injection if the validation logic is ever modified or bypassed.