🔒 Fix potential SQL Injection in SQLite schema modification#27
Open
Gunnarguy wants to merge 1 commit into
Open
🔒 Fix potential SQL Injection in SQLite schema modification#27Gunnarguy wants to merge 1 commit into
Gunnarguy wants to merge 1 commit into
Conversation
Co-authored-by: Gunnarguy <110250624+Gunnarguy@users.noreply.github.com>
|
👋 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. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds strict identifier validation to SQLiteFullTextService.ensureColumnExists to prevent potential SQL injection when constructing schema-modifying SQL statements from table/column parameters. Sequence diagram for updated ensureColumnExists validation flowsequenceDiagram
participant Caller
participant SQLiteFullTextService
participant Log
participant SQLite
Caller->>SQLiteFullTextService: ensureColumnExists(table, column, definition)
SQLiteFullTextService->>SQLiteFullTextService: check database
alt invalid table identifier
SQLiteFullTextService->>Log: Log.error("[SQLiteFTS5] Invalid table name for schema modification")
SQLiteFullTextService-->>Caller: return
else invalid column identifier
SQLiteFullTextService->>Log: Log.error("[SQLiteFTS5] Invalid column name for schema modification")
SQLiteFullTextService-->>Caller: return
else identifiers valid
SQLiteFullTextService->>SQLite: sqlite3_prepare_v2(db, "PRAGMA table_info(table)")
SQLite-->>SQLiteFullTextService: statement
SQLiteFullTextService->>SQLite: sqlite3_step / sqlite3_finalize
SQLite-->>SQLiteFullTextService: result
SQLiteFullTextService-->>Caller: schema ensured
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider extracting the identifier regex into a shared constant or helper (e.g., a static on SQLiteFullTextService or a small IdentifierValidator) so that future identifier validation stays consistent and is easier to reuse and update.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the identifier regex into a shared constant or helper (e.g., a static on SQLiteFullTextService or a small IdentifierValidator) so that future identifier validation stays consistent and is easier to reuse and update.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR hardens SQLite schema-migration logic in SQLiteFullTextService by validating schema identifiers before constructing dynamic PRAGMA table_info(...) and ALTER TABLE ... ADD COLUMN ... SQL strings, reducing the risk of SQL injection if the helper is ever called with non-hardcoded values.
Changes:
- Added strict regex validation for
tableandcolumnidentifiers inensureColumnExists. - Logged and returned early when invalid identifiers are provided to prevent executing the constructed SQL.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3061
to
+3071
| // Security fix: Validate table and column names to prevent SQL injection | ||
| let validIdentifierRegex = "^[a-zA-Z_][a-zA-Z0-9_]*$" | ||
| guard table.range(of: validIdentifierRegex, options: .regularExpression) != nil else { | ||
| Log.error("[SQLiteFTS5] Invalid table name for schema modification: \(table)", category: .vectorDB) | ||
| return | ||
| } | ||
| guard column.range(of: validIdentifierRegex, options: .regularExpression) != nil else { | ||
| Log.error("[SQLiteFTS5] Invalid column name for schema modification: \(column)", category: .vectorDB) | ||
| return | ||
| } | ||
|
|
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
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.
🎯 What: Fixed a potential SQL injection vulnerability in
ensureColumnExistswithinSQLiteFullTextService.swiftwhere table and column names were unsafely interpolated into SQL strings.PRAGMA table_info(...)andALTER TABLE ... ADD COLUMN ...queries. If used dynamically in the future, it could allow an attacker to inject arbitrary SQL statements or alter schema in unintended ways.🛡️ Solution: Implemented early validation of the
tableandcolumnparameters against a strict regular expression^[a-zA-Z_][a-zA-Z0-9_]*$. This ensures that only valid SQL identifiers can be passed. If validation fails, an error is logged and the function returns early, preventing the query from being executed.(Note: Tests could not be run directly due to the lack of Swift toolchain in the environment, but the changes were carefully reviewed manually and follow standard Swift regex validation patterns and logging practices.)
PR created automatically by Jules for task 5969573126713650346 started by @Gunnarguy
Summary by Sourcery
Bug Fixes: