Database generates invalid insert command if the there's an non-existent column data

[adamkiss]

Description

If you pass an array with one or more keys that don't exist as columns on a table, kirby generates invalid sql by dropping the column/keys, but keeping the commas and values

Expected behavior

• don't validate the columns at all and let database engine report the failure properly (invalid column XYZ vs error near ',')
• or clean the data properly, and remove the binding/value as well

To reproduce

Tested with SQLite:

1. Create a simple SQLite table with fields "id" and "name":

   CREATE TABLE "kirby_db_test" ("id" integer,"name" TEXT, PRIMARY KEY (id));

2. try to insert an array with excess data3.

   Db::insert('kirby_db_test', [
   'id' => 1,
   'doesnt_exist' => 'oh no',
   'name' => 'test'
   ]);

3. Observe:

   • Db::lastQuery() - INSERT INTO "kirby_db_test" ("id", , "name") VALUES (:value_YK5BnEGI, :value_PILOWDiC, :value_X3mUasTS)
   • Db::lastError() - SQLSTATE[HY000]: General error: 1 near ",": syntax error"

Your setup

Kirby Version
3.6.2

Your system (please complete the following information)

• PHP v8.0.15
• Sqlite v3.37.2

[adamkiss]

I've tracked the error down to Sql->valueList() - https://github.com/getkirby/kirby/blob/3de403cabcd6755bdd95c159bd282ec63ec14658/src/Database/Sql.php#L850, which validates the key/column and returns null if it's invalid, but doesn't skip it or its bindings, just happily sets the column name to null and chugs along. :)

[lukasbestle]

Thanks for the issue report. BTW, we validate existing tables and columns to make SQL injections at least a bit more unlikely. If a table/column exists, that means that the query has not been crafted in a malicious way.

[afbora]

If a table/column exists, that means that the query has not been crafted in a malicious way.

Not exactly for columns. In fact, this usage provides convenience. I remember a few frameworks like this. I don't need to create new data array just for the columns in the database. The system extracts the data I have for me.

[lukasbestle]

Yes, also convenience. But the security consideration about injections was the reason why I added the validation at first.

[bastianallgeier]

✅

[adamkiss]

@lukasbestle @afbora - No, I love the convenience/safety, absolutely. it's just that the result should be a valid sql :)

Thank you all