yetanalytics / lrsql

A SQL-based Learning Record Store
https://www.sqllrs.com
Apache License 2.0
89 stars 18 forks source link

Temporary (read: BAD) fix to casing issue in Reactions template vars #359

Closed cliffcaseyyet closed 8 months ago

cliffcaseyyet commented 8 months ago

@milt @invaliduser I have uncovered an issue in reactions that is breaking UI testing, and I'm not sure how it even went unnoticed by both of us. For whatever reason SQLite does not respect alias case at all, so when you do something like:

SELECT condition_XAZ.payload as condition_XAZ
FROM xapi_statement as condition_XAZ
WHERE ((json_extract(condition_XAZ.payload, ?) = ?) AND (condition_XAZ.stored <= ? AND (condition_XAZ.statement_id = ?)) AND json_extract(condition_XAZ.payload, ?) = ?);

(and BTW the default generated titles of conditions have caps) - The actual edn row data returned from SQL ends up throwing out the case:

{:condition_xaz ...}

Which is fine except the canonical name/key for the condition is condition_XAZ, and that is what is used to perform variable injection. It concerns me that SQL is just doing whatever weird column conversion stuff it does, like what about whitespace or special characters etc. It makes me nervous that users are directly inputting something that is the query pivot of all the variable injection. I tried using double quotes on the alias because most SQL engines respect that to mean "absolutely do not mess with this alias" but alas SQLite explicitly does not support that trick.

This fix sorta kinda works for me to continue my work today, but it's extremely naive. We may need to think a bit on this one.

cliffcaseyyet commented 8 months ago

Yep, I get the approach here, but as I mentioned in standup would prefer to limit what can be used for condition names to keep things transparent and not worry about uniqueness.

that's fine. I literally called this a bad solution, i knew we had choices to make.