Skip to content

🛡️ Sentinel: [CRITICAL] Fix SQL Injection in PRAGMA table_info#107

Open
mattjoyce wants to merge 1 commit into
mainfrom
sentinel-fix-sqlite-pragma-sqli-4686737539554573183
Open

🛡️ Sentinel: [CRITICAL] Fix SQL Injection in PRAGMA table_info#107
mattjoyce wants to merge 1 commit into
mainfrom
sentinel-fix-sqlite-pragma-sqli-4686737539554573183

Conversation

@mattjoyce
Copy link
Copy Markdown
Owner

@mattjoyce mattjoyce commented May 28, 2026

🚨 Severity: CRITICAL
💡 Vulnerability: Used fmt.Sprintf to construct PRAGMA table_info query with dynamic table names, allowing potential SQL injection.
🎯 Impact: Could allow arbitrary SQL execution if the table parameter can be controlled by an attacker.
🔧 Fix: Replaced PRAGMA statement with SQLite's table-valued function pragma_table_info(?) which fully supports parameterized queries. This ensures all input is properly sanitized. Added journal entry documenting learning.
✅ Verification: Ran make test locally to verify changes, confirming existing test suite functions correctly and the logic handles sql.ErrNoRows precisely as before.


PR created automatically by Jules for task 4686737539554573183 started by @mattjoyce

Summary by CodeRabbit

  • Bug Fixes
    • Fixed a SQL injection vulnerability in the database layer by implementing secure parameterized query handling. This improvement enhances the security and stability of database operations.

Review Change Stack

Co-authored-by: mattjoyce <278869+mattjoyce@users.noreply.github.com>
@google-labs-jules
Copy link
Copy Markdown
Contributor

👋 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 @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 28, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
ductile e421dc3 Commit Preview URL

Branch Preview URL
May 28 2026, 11:46 AM

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 28, 2026

📝 Walkthrough

Walkthrough

This PR eliminates a SQL injection vulnerability in SQLite column-existence checking by replacing string-interpolated PRAGMA table_info queries with parameterized pragma_table_info(?) function calls. The fix is documented in a security sentinel entry and implemented in the sqliteColumnExists function.

Changes

SQL Injection Fix for Column Existence Check

Layer / File(s) Summary
SQL injection fix: parameterized column existence check
.jules/sentinel.md, internal/storage/sqlite.go
sqliteColumnExists switches from string-interpolated PRAGMA table_info iteration to a parameterized SELECT 1 FROM pragma_table_info(?) WHERE name = ? LIMIT 1 query using bound parameters. A dated security sentinel entry documents the risk and solution approach.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A breach in the pragma, now sealed with a bind,
Parameters guard where interpolation's unkind,
Sentinel stands watch with a date and a note,
Column existence checks float a safer boat! 🛡️

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically identifies the main change: fixing a SQL injection vulnerability in PRAGMA table_info queries using sentinel documentation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sentinel-fix-sqlite-pragma-sqli-4686737539554573183

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: The file's first line uses a level-2 heading ("## 2024-05-18 -
Parameterized Table-Valued Functions") which triggers markdownlint MD041; change
that first line to a top-level heading by replacing "##" with "#" so the file
begins with "# 2024-05-18 - Parameterized Table-Valued Functions" (ensure no
blank lines precede 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: 9ecdbba9-fbe3-4a52-8d9d-38531d4dec14

📥 Commits

Reviewing files that changed from the base of the PR and between 68c5b08 and e421dc3.

📒 Files selected for processing (2)
  • .jules/sentinel.md
  • internal/storage/sqlite.go

Comment thread .jules/sentinel.md
@@ -0,0 +1,5 @@
## 2024-05-18 - Parameterized Table-Valued Functions
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use a top-level heading on the first line.

Line 1 starts with ##, which triggers markdownlint MD041 and can fail lint gates for docs.

Suggested fix
-## 2024-05-18 - Parameterized Table-Valued Functions
+# 2024-05-18 - Parameterized Table-Valued Functions
📝 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.

Suggested change
## 2024-05-18 - Parameterized Table-Valued Functions
# 2024-05-18 - Parameterized Table-Valued Functions
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[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, The file's first line uses a level-2 heading
("## 2024-05-18 - Parameterized Table-Valued Functions") which triggers
markdownlint MD041; change that first line to a top-level heading by replacing
"##" with "#" so the file begins with "# 2024-05-18 - Parameterized Table-Valued
Functions" (ensure no blank lines precede it).

mattjoyce added a commit that referenced this pull request Jun 7, 2026
…vault fail-open

ductile-admin source diligence found the secret_ref-for-plugins scheme cannot work: PluginConf has
no secret_ref (only webhook/relay do); the only plugin secret path is the spawn-time stdin secrets
map, needing principal==plugin-name (kebab, no normalization) AND the plugin reading that map.
#107 = the dev workstream to make first-party plugins vault-native (the real secret-holder enforce
path). #108 = vault compose is fail-OPEN on an unknown principal (silent no-secret) — out of step
with privsep fail-closed spine.
mattjoyce added a commit that referenced this pull request Jun 7, 2026
…locate, secrets not a config reconcile

Relocating plugin code to /opt invalidates attestation → #93 fail-safe downgrades to untrusted
until plugin lock re-records fingerprints (witnessed live). Plugin secrets need the #107 vault-native
workstream (PluginConf has no secret_ref; principals must be kebab; vault compose fail-open #108).
migrate-everything splits 3 ways: keyless-now / secret-holders-after-#107 / unconfinable-admin-#106.
mattjoyce added a commit that referenced this pull request Jun 7, 2026
…substitution

Tested live on discord_notify: config check rejects webhook_url_ref as not satisfying required
webhook_url; runtime "No webhook_url configured". Vault→plugin is ONLY the stdin secrets map →
per-plugin kebab-rename + code change to read secrets[...]. Not a config tweak.
mattjoyce added a commit that referenced this pull request Jun 7, 2026
Enforced data-plane gateway live on :8081: vault carried, 5 keyless integrations enforced+attested
on default(1001), config/plugin lock, admission re-enabled, all wall-bites pass. #93 downgrade proven
live. Remaining carded as dev workstreams: #107 (HEAVY, secret-holders), #106 (admin instance), #108
(vault fail-open), #105 (FHS package). Old --user decommission deferred until #106+#107 land.
mattjoyce added a commit that referenced this pull request Jun 7, 2026
…l (+ schema vault_file)

The vault rejects non-kebab principal names; #107 would otherwise need every secret-holding plugin
RENAMED to kebab (breaking config/pipeline refs). New plugins.<name>.vault_principal lets a snake_case
plugin (discord_notify) compose its secrets under a kebab principal (discord-notify) WITHOUT a rename;
attestation still uses the plugin name. This is the repo-side enabler — the remaining #107 work (each
plugin reading the stdin secrets map) lives in the external plugin repos. Also added the missing
secrets.vault_file to the schema (drift: live key, would fail strict validate). +test.
mattjoyce added a commit that referenced this pull request Jun 7, 2026
mattjoyce added a commit that referenced this pull request Jun 7, 2026
…live)

discord_notify proved end-to-end: webhook from the encrypted vault over stdin, uid 1001, attested,
requires_vault fail-closed. Recorded the reusable 5-step recipe on card #107 (read stdin secrets map,
manifest required:[], kebab principal, vault_principal+requires_vault, relocate+lock) + a runbook
pointer. NOTE: the discord_notify plugin code change is in the external ductile-plugins repo.
mattjoyce added a commit that referenced this pull request Jun 7, 2026
github_repo_sync et al are vault-native + ready but exit 127 under enforce: uv run --script needs
uv on PATH + writable HOME/cache and walks config up-tree into the 0700 cwd. Affects the 4 git
plugins; disabled for now. Fix: account-friendly uv (UV_CACHE_DIR/HOME + cwd off 0700) or de-uv to
plain python3. Found during the #107 push.
mattjoyce added a commit that referenced this pull request Jun 7, 2026
…on + fail-closed proven live

Recipe step 1 generalized: name the vault secret via a config key (salt_secret/token_secret) so a
shared base plugin stays reusable across instances. Status: discord_notify + ap_canary vault-native
+ live (B form: snake instance + vault_principal + requires_vault); fail-closed PROVEN live (bogus
principal → refused spawn). Plugin code @ ductile-plugins a1934e5. git/uv plugins → #109.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant