Skip to content

Commit 2cd869d

Browse files
bpamiriclaude
andauthored
fix(model): validate query-builder values against column type to block SQL injection (#2416)
The chainable QueryBuilder advertised in CLAUDE.md and its own docblock as "injection-safe" relied on the adapter's $quoteValue, which passes integer, float, and boolean values through unquoted. Without a value-shape check at the builder boundary, a string like "0 OR 1=1" landed verbatim in the WHERE clause on integer columns and bypassed the post-hoc cfqueryparam pass — a tautology injection that would return every row. The same defect affected whereBetween (per-bound), whereIn / whereNotIn (per-element), and the 2-arg where()/orWhere(). Validate values against the declared validationtype in $quoteValue. Integer and float values must match a numeric regex; booleans must be one of 0/1/true/false/ yes/no. Mismatches throw Wheels.InvalidValue. String columns are unaffected — the adapter still quotes-and-escapes them. Adds 22 regression tests in QueryBuilderSpec.cfc covering tautology, UNION SELECT, comment-truncation, stacked-statement, and per-method coverage across where, orWhere, whereBetween, whereIn, whereNotIn — plus accept-cases for legitimate integer/float/string values. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 8f35045 commit 2cd869d

2 files changed

Lines changed: 192 additions & 1 deletion

File tree

vendor/wheels/model/query/QueryBuilder.cfc

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -475,14 +475,61 @@ component output="false" {
475475

476476
/**
477477
* Quote a value using the model's adapter for SQL injection safety.
478+
*
479+
* For integer/float/boolean columns the adapter's $quoteValue passes the value
480+
* through unquoted (the downstream WHERE-clause regex re-extracts bare numerics
481+
* into cfqueryparam). That contract assumes the caller has already constrained
482+
* the value to a numeric/boolean shape — so we enforce that here at the only
483+
* untrusted entry point. String columns are wrapped and escaped by the adapter,
484+
* so they don't need this check.
478485
*/
479486
private string function $quoteValue(required string property, required any value) {
480487
local.type = "string";
481488
local.classData = variables.modelReference.$classData();
482489
if (StructKeyExists(local.classData.properties, arguments.property)) {
483490
local.type = local.classData.properties[arguments.property].validationtype;
484491
}
485-
return local.classData.adapter.$quoteValue(str = ToString(arguments.value), type = local.type);
492+
local.strValue = ToString(arguments.value);
493+
$validateValueShape(arguments.property, local.strValue, local.type);
494+
return local.classData.adapter.$quoteValue(str = local.strValue, type = local.type);
495+
}
496+
497+
/**
498+
* Validate that a value matches the shape the adapter expects for its declared
499+
* column type. Throws Wheels.InvalidValue when the shape doesn't match — closing
500+
* the SQL-injection vector through which strings like "0 OR 1=1" would otherwise
501+
* land in the unquoted numeric/boolean path of the adapter.
502+
*/
503+
private void function $validateValueShape(required string property, required string value, required string type) {
504+
// Empty values fall through to the adapter's empty-string quoting branch.
505+
if (!Len(arguments.value)) {
506+
return;
507+
}
508+
switch (arguments.type) {
509+
case "integer":
510+
if (!ReFind("^-?[0-9]+$", arguments.value)) {
511+
$throwInvalidValue(arguments.property, arguments.value, "integer");
512+
}
513+
break;
514+
case "float":
515+
if (!ReFind("^-?[0-9]+(\.[0-9]+)?$", arguments.value)) {
516+
$throwInvalidValue(arguments.property, arguments.value, "float");
517+
}
518+
break;
519+
case "boolean":
520+
if (!ListFindNoCase("0,1,true,false,yes,no", arguments.value)) {
521+
$throwInvalidValue(arguments.property, arguments.value, "boolean");
522+
}
523+
break;
524+
}
525+
}
526+
527+
private void function $throwInvalidValue(required string property, required string value, required string expectedType) {
528+
Throw(
529+
type = "Wheels.InvalidValue",
530+
message = "The value `#EncodeForHTML(arguments.value)#` for property `#EncodeForHTML(arguments.property)#` is not a valid #arguments.expectedType#.",
531+
extendedInfo = "Values bound to #arguments.expectedType# columns must be valid #arguments.expectedType# literals so they can be safely interpolated into the WHERE clause. This check protects the chainable query builder against SQL injection through typed-numeric/boolean payloads."
532+
);
486533
}
487534

488535
/**

vendor/wheels/tests/specs/QueryBuilderSpec.cfc

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,150 @@ component extends="wheels.WheelsTest" {
265265

266266
});
267267

268+
describe("value validation for typed columns rejects SQL injection", function() {
269+
270+
// The post model has integer columns (id, authorid, views) and a float column
271+
// (averagerating). The adapter's $quoteValue passes integer/float/boolean values
272+
// through unquoted, so without value-shape validation the QueryBuilder leaks
273+
// injected SQL fragments into the WHERE clause. These tests pin down the fix.
274+
275+
it("rejects tautology injection on integer column via where(prop, op, val)", function() {
276+
expect(function() {
277+
qb.where("views", ">", "0 OR 1=1");
278+
}).toThrow("Wheels.InvalidValue");
279+
});
280+
281+
it("rejects UNION SELECT injection on integer column", function() {
282+
expect(function() {
283+
qb.where("views", "=", "1 UNION SELECT password FROM c_o_r_e_users");
284+
}).toThrow("Wheels.InvalidValue");
285+
});
286+
287+
it("rejects comment-truncation on integer column", function() {
288+
expect(function() {
289+
qb.where("views", "=", "1 --");
290+
}).toThrow("Wheels.InvalidValue");
291+
});
292+
293+
it("rejects stacked-statement injection on integer column", function() {
294+
expect(function() {
295+
qb.where("views", "=", "1; DROP TABLE c_o_r_e_posts");
296+
}).toThrow("Wheels.InvalidValue");
297+
});
298+
299+
it("rejects injection on float column", function() {
300+
expect(function() {
301+
qb.where("averagerating", ">", "3.14 OR 1=1");
302+
}).toThrow("Wheels.InvalidValue");
303+
});
304+
305+
it("rejects injection in whereBetween low bound", function() {
306+
expect(function() {
307+
qb.whereBetween(property = "views", low = "0 OR 1=1", high = 10);
308+
}).toThrow("Wheels.InvalidValue");
309+
});
310+
311+
it("rejects injection in whereBetween high bound", function() {
312+
expect(function() {
313+
qb.whereBetween(property = "views", low = 0, high = "10 OR 1=1");
314+
}).toThrow("Wheels.InvalidValue");
315+
});
316+
317+
it("rejects injection in whereIn list element", function() {
318+
expect(function() {
319+
qb.whereIn(property = "views", values = "1,2,3 OR 1=1");
320+
}).toThrow("Wheels.InvalidValue");
321+
});
322+
323+
it("rejects injection in whereIn array element", function() {
324+
expect(function() {
325+
qb.whereIn(property = "views", values = [1, 2, "3 OR 1=1"]);
326+
}).toThrow("Wheels.InvalidValue");
327+
});
328+
329+
it("rejects injection in whereNotIn list element", function() {
330+
expect(function() {
331+
qb.whereNotIn(property = "views", values = "1,2,3 OR 1=1");
332+
}).toThrow("Wheels.InvalidValue");
333+
});
334+
335+
it("rejects injection via two-argument where()", function() {
336+
expect(function() {
337+
qb.where("views", "0 OR 1=1");
338+
}).toThrow("Wheels.InvalidValue");
339+
});
340+
341+
it("rejects injection via three-argument orWhere()", function() {
342+
expect(function() {
343+
qb.orWhere("views", ">", "0 OR 1=1");
344+
}).toThrow("Wheels.InvalidValue");
345+
});
346+
347+
it("rejects injection via two-argument orWhere()", function() {
348+
expect(function() {
349+
qb.orWhere("views", "0 OR 1=1");
350+
}).toThrow("Wheels.InvalidValue");
351+
});
352+
353+
it("accepts plain integer values", function() {
354+
expect(function() {
355+
qb.where("views", ">", 18);
356+
}).notToThrow();
357+
});
358+
359+
it("accepts integer-shaped strings", function() {
360+
expect(function() {
361+
qb.where("views", "=", "42");
362+
}).notToThrow();
363+
});
364+
365+
it("accepts negative integer values", function() {
366+
expect(function() {
367+
qb.where("views", ">=", "-1");
368+
}).notToThrow();
369+
});
370+
371+
it("accepts plain float values", function() {
372+
expect(function() {
373+
qb.where("averagerating", ">", 3.14);
374+
}).notToThrow();
375+
});
376+
377+
it("accepts float-shaped strings", function() {
378+
expect(function() {
379+
qb.where("averagerating", ">", "3.14");
380+
}).notToThrow();
381+
});
382+
383+
it("accepts negative float values", function() {
384+
expect(function() {
385+
qb.where("averagerating", ">", "-2.5");
386+
}).notToThrow();
387+
});
388+
389+
it("accepts integer values in whereBetween", function() {
390+
expect(function() {
391+
qb.whereBetween(property = "views", low = 1, high = 10);
392+
}).notToThrow();
393+
});
394+
395+
it("accepts integer values in whereIn", function() {
396+
expect(function() {
397+
qb.whereIn(property = "views", values = "1,2,3");
398+
}).notToThrow();
399+
});
400+
401+
it("does not validate string columns (adapter quotes them safely)", function() {
402+
// String columns get quoted with single-quote escaping by the adapter,
403+
// so classic value-injection payloads are rendered harmless without
404+
// needing a regex check at the builder layer.
405+
expect(function() {
406+
qb.where("title", "=", "anything' OR '1'='1");
407+
}).notToThrow();
408+
});
409+
410+
});
411+
268412
});
269413

270414
}

0 commit comments

Comments
 (0)