[Security] Values in raw where/having fragments stay inside their quotes

[bunnykong]

Values passed to raw SQL fragments, such as where("name = ? AND role = ?", a, b), now stay inside their quotes. Before this, a value containing ? let the next value be inserted outside its quotes, so crafted input could change the meaning of the query.

Reported privately to the maintainers first; Sam preferred open disclosure, since Roundhouse hasn't been security-audited and isn't widely deployed. Thanks to Greg Molnar for reviewing the report and the patch.

Source: User.where("name = ? AND role = ?", "what?", "admin")
Before: SQL error: "admin" is inserted inside the quotes of "what?"
After:  WHERE (name = 'what?' AND role = 'admin')

With a crafted second value, the same mechanism turned a query that should match no rows into one that returned every row, on CRuby and on native Spinel. Rails returns no rows for the same input.

Cause. Relation#substitute_binds (relation.rb:1824–1830) filled placeholders one at a time with String#sub, and each call searched the string built so far, including values already inserted. sub also treats backslashes in its replacement string specially, so a value such as path\1 lost its backslash and matched the wrong row.

Fix. Split the original fragment on ? once and interleave the escaped values, as ActiveRecord::Base.sanitize_sql already does. Inserted values are never searched again, so question marks and backslashes inside them stay data. Missing arguments still leave their placeholders, and Array arguments are escaped as before.

Named binds (:name with a Hash), added in 12bf080, already scan the original fragment once and are unchanged.

Affected. The Ruby, JRuby and Spinel targets, which share this runtime: where, where! and where.not with a fragment and two or more values (where, not, add_condition), having (having), and scopes that pass their arguments into such a fragment. The param-binds setting makes no difference, because raw fragments are substituted inline either way.

Tests. raw_where_substitution_ruby and raw_where_substitution_spinel run real queries covering question marks, backslashes and replacement escapes, quoting, negation, HAVING, Arrays and argument-count edges. Both fail on main with scalar value consumed later placeholder and pass with this change.

Validation on main c49721be: cargo test --locked --lib 969 passed; the bind suite 6 passed (3 Ruby, 3 native Spinel); runtime integration 5 passed; the CI planner 50 passed. Emitted Ruby, JRuby and Spinel trees change only in substitute_binds. The RBS inference tracker reports no new unresolved sites (0 on main and 0 with this change).

This complements #459, which casts limit, offset and order; both edit relation.rb and the typing test, so whichever merges second needs a small rebase. #403 (draft) carries an equivalent change and will drop it once this merges.

Part of the Conduit worklist: bunnykong#1

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Raw query conditions in where, not, and having clauses now substitute values in placeholder order. Question marks inside values are preserved rather than treated as additional placeholders.
  • When there are more placeholders than values, unmatched placeholders remain unchanged; extra values are ignored.
  • Special characters, arrays, hash-based conditions, and edge cases in raw query conditions are handled correctly. Named hash-based substitution remains supported.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 0a15fda5-3a14-497e-81d0-37a3e28a5ca9

📥 Commits

Reviewing files that changed from the base of the PR and between 85b9336 and 30b8f26.

📒 Files selected for processing (4)

• runtime/ruby/active_record/relation.rb
• scripts/ci-plan.py
• tests/ci_plan_test.py
• tests/inference_on_spinel_blog_runtime_with_rbs.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/inference_on_spinel_blog_runtime_with_rbs.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

---

📝 Walkthrough

Walkthrough

substitute_binds now rebuilds SQL from the original placeholder positions. Database-backed tests cover raw predicate substitution, and CI routing includes the new test.

Changes

Raw WHERE Bind Substitution

Layer / File(s)	Summary
Rebuild SQL from original placeholders runtime/ruby/active_record/relation.rb	substitute_binds escapes arguments for available placeholders, preserves placeholders without arguments, and does not treat question marks in inserted values as placeholders.
Test raw predicate substitution tests/param_binds_raw_where.rb, tests/param_binds.rs, scripts/ci-plan.py, tests/ci_plan_test.py, tests/inference_on_spinel_blog_runtime_with_rbs.rs	The database-backed probe checks raw conditions in where, not, and having, plus argument and placeholder edge cases. Ruby and ignored Spinel test entry points invoke it. CI routing selects the parameter-bind suite for the probe. The RBS measurement annotation records a zero delta and ceiling and notes that named-bind dispatch and RBS signatures remain unchanged.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: thomasklemm

Merge Risk: ⚪ Minimal · up to 30b8f

Raw where/having fragments no longer let one inserted value affect a later placeholder, and the change adds test coverage and CI routing for this. No merge-blocking risk was found.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 30b8f

The change strengthens separation between SQL and supplied values without adding database authority or new callers. No introduced security defect was established. Assurance remains limited across other runtimes, custom adapters, and application-specific usage.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The relevant exposure is application data selected through raw relation predicates when externally controlled values reach their positional arguments. Effective tenant, table, and database scope depends on application queries and database permissions; those deployment-specific boundaries are not established here.

Security Findings and Attack Paths

• inferred — The pre-existing substitution path could consume a question mark inside an already-escaped value when processing a later argument, undermining value-to-SQL separation. The head removes that specific mechanism by limiting placeholder matching to the original fragment. This is a repaired path, not an introduced architecture concern.

Trust Boundaries and Controls

• observed — Each matched positional value crosses the existing adapter escaping control before insertion. The inspected SQLite implementations double apostrophes inside quoted strings, and the new reconstruction appends their output without replacement-string interpretation.

Resilience and Maintainability Implications

• observed — The CI change adds the new regression file to an existing focused-suite ownership rule. The corresponding routing assertion checks param_binds selection and existing core/framework jobs; it does not introduce a production public endpoint.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the security fix in raw WHERE/HAVING fragment substitution: inserted values remain within their intended quoted context.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}

[thomasklemm]

Superseded by #476 (merge bd997d68f8eaad8fa4760b93854f4c1080b164f5 on main).

One-pass positional ? substitution in Relation#substitute_binds (question marks and backslashes stay data), named-bind Hash dispatch, tests/param_binds_raw_where.rb, and the CI planner routing all landed. Stacked in 64d6d6d5.

Closing this PR; no remaining material from this patch is missing on main.