parsePath(…).query is a URLSearchParams, so those parameters are just strings, and it’s an untagged template string, so it is indeed a trivial SQL injection vector. And the params argument was right there! I don’t understand quite how you make a mistake like that, but it’s extremely worrying. This is fundamental, basic stuff.
The fun aspects of this are that:
(a) I could actually imagine trailbase.js contents that would make it not SQL injection: you could have parsePath(…).query.get(…) return objects with a toString() that escaped SQL. This would raise even more questions, and I was sure it wouldn’t be the case, but it’s possible.
(b) You could make it work safely, converting interpolation into parameters, by using a tagged template string. This could require only a tiny change:
return await query(
sql`SELECT Owner, Aroma, Flavor, Acidity, Sweetness
FROM coffee
ORDER BY vec_distance_L2(
embedding, '[${+aroma}, ${+flavor}, ${+acid}, ${+sweet}]')
LIMIT 100`
);
(You could even make it query`…`, but I think query(sql`…`) is probably wiser. As for the plusses I put in there, that’s to convert from strings to numbers.)
So we’re clear, the tagged template string approach gives you a string that uses parameters properly (see the example in the README of that project that does it); it’s perfectly compatible with prepared-statement caching.
And actually on second consideration, you probably can’t use binding parameters here: you’re trying to slot numbers in as JSON values inside a string! Maybe you’d have to write vec_distance_L2(embedding, ?), with parameter JSON.stringify([+aroma, +flavor, +acid, +sweet]).
:hide: thanks for pointing out this embarrassing plunder. No excuse, just feeling silly especially after being so careful throughout the rust base. It's fixed now. Please keep yelling if you find more issues
That type stinks. Kill the undefined bit by giving it an initial value, then you can skip the `?? []` later too; and replace Array<object> with the actual row type, probably naming it, maybe like this:
type Record = [string, number, number, number, number];
const [data, setData] = useState<Record[]>([]);
const params = new URLSearchParams({ aroma, flavor, acidity, sweetness });
I confess I’m puzzled about this one too, as trailbase.js suggests you know about URLSearchParams, which is what you should use for all typical query string manipulation, parsing and generation.
The fun aspects of this are that:
(a) I could actually imagine trailbase.js contents that would make it not SQL injection: you could have parsePath(…).query.get(…) return objects with a toString() that escaped SQL. This would raise even more questions, and I was sure it wouldn’t be the case, but it’s possible.
(b) You could make it work safely, converting interpolation into parameters, by using a tagged template string. This could require only a tiny change:
(You could even make it query`…`, but I think query(sql`…`) is probably wiser. As for the plusses I put in there, that’s to convert from strings to numbers.)This is a concept that’s definitely been done seriously. The first search result I found: https://github.com/blakeembrey/sql-template-tag.