🛡️ Sentinel: [CRITICAL] Fix SQL injection in sqliteColumnExists#97
🛡️ Sentinel: [CRITICAL] Fix SQL injection in sqliteColumnExists#97mattjoyce wants to merge 1 commit into
Conversation
🚨 Severity: CRITICAL 💡 Vulnerability: The `sqliteColumnExists` function was vulnerable to SQL injection because it used `fmt.Sprintf` to interpolate the table name directly into a `PRAGMA table_info(%s)` query. 🎯 Impact: Depending on the context where this function is called, an attacker could potentially execute arbitrary SQL queries or manipulate the schema. 🔧 Fix: Refactored the function to use the `pragma_table_info(?)` table-valued function, which safely supports parameterization for dynamic schema lookups. ✅ Verification: Ran `go test -v ./internal/storage/...` and `make test`. All tests pass successfully, confirming no regressions. Co-authored-by: mattjoyce <278869+mattjoyce@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. |
📝 WalkthroughWalkthroughThis PR fixes a SQL injection vulnerability in SQLite column existence checking. The ChangesSQL Injection Mitigation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.jules/sentinel.md:
- Line 1: Update the opening heading "2025-05-25 - Prevent SQL Injection in
PRAGMA Queries" to be a top-level H1 (change "##" to "#") and insert a single
blank line immediately after that heading to satisfy MD041 and MD022; locate the
heading by the exact text "2025-05-25 - Prevent SQL Injection in PRAGMA Queries"
and ensure there is one empty line following it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 874a64d7-c8e2-4901-9667-acc500f998b7
📒 Files selected for processing (2)
.jules/sentinel.mdinternal/storage/sqlite.go
| @@ -0,0 +1,4 @@ | |||
| ## 2025-05-25 - Prevent SQL Injection in PRAGMA Queries | |||
There was a problem hiding this comment.
Fix markdownlint heading compliance at file start.
Line 1 violates MD041 (no H1 at top) and MD022 (missing blank line after heading), which can break docs-lint CI.
📝 Proposed fix
+# Sentinel Learnings
+
## 2025-05-25 - Prevent SQL Injection in PRAGMA Queries
+
**Vulnerability:** The `sqliteColumnExists` function was vulnerable to SQL injection because it used `fmt.Sprintf` to interpolate the table name directly into a `PRAGMA table_info(%s)` string before passing it to `db.QueryContext`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 2025-05-25 - Prevent SQL Injection in PRAGMA Queries | |
| # Sentinel Learnings | |
| ## 2025-05-25 - Prevent SQL Injection in PRAGMA Queries | |
| **Vulnerability:** The `sqliteColumnExists` function was vulnerable to SQL injection because it used `fmt.Sprintf` to interpolate the table name directly into a `PRAGMA table_info(%s)` string before passing it to `db.QueryContext`. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.jules/sentinel.md at line 1, Update the opening heading "2025-05-25 -
Prevent SQL Injection in PRAGMA Queries" to be a top-level H1 (change "##" to
"#") and insert a single blank line immediately after that heading to satisfy
MD041 and MD022; locate the heading by the exact text "2025-05-25 - Prevent SQL
Injection in PRAGMA Queries" and ensure there is one empty line following it.
…face, error taxonomy) Folds the safe, high-signal feedback from the 4-panel code review (all panels approved, zero merge-blockers): - boot-time grant resolution (validateAccountGrants): a run_as grant naming an undefined account now fails at config LOAD, not per-job at first spawn — moves a knowable fault from spawn-time/late to boot-time/early (the 4/4 headline finding). Plus boot warns when the default/untrusted tiers are absent. - SecretSurfacePaths: single-sourced secrets surface that reconciles the config DIRECTORY (not just the file), closing the file-form sibling-secret gap. - ErrNoDowngradeTarget split from ErrAccountDropFailed (honest taxonomy); the dispatcher treats both as terminal/no-retry. - vocab residue cleaned from code (log keys, comments, "a account" strings, workersConfigured -> accountsConfigured); _other euid documented as unused. Deferred with reasons (card #97): ResolvedAccount sum-type (naive Confined() is a zero-value fail-open footgun), once-per-spawn attestation (risky refactor, minor). gofmt/golangci-lint/gosec clean; suite green; independent review confirmed no fail-open regression.
- #96 (ADR vocab sync) -> done. - #83 epic: luminary code review done (unanimous approve, zero blockers); Tier A+B folded; T7 finding (live host loads a vault -> compose-attestation active, but no accounts map -> privsep unconfined, enforce macOS-pending #95). Next: doc review. - #97: deferred non-blocking review follow-ups (T3, T5, T9, T15, vocab lint).
1 participant
🛡️ Sentinel: [CRITICAL] Fix SQL injection in sqliteColumnExists
🚨 Severity: CRITICAL
💡 Vulnerability: The
sqliteColumnExistsfunction was vulnerable to SQL injection because it usedfmt.Sprintfto interpolate the table name directly into aPRAGMA table_info(%s)query.🎯 Impact: Depending on the context where this function is called, an attacker could potentially execute arbitrary SQL queries or manipulate the schema.
🔧 Fix: Refactored the function to use the
pragma_table_info(?)table-valued function, which safely supports parameterization for dynamic schema lookups.✅ Verification: Ran
go test -v ./internal/storage/...andmake test. All tests pass successfully, confirming no regressions.PR created automatically by Jules for task 2072051240258675152 started by @mattjoyce
Summary by CodeRabbit