Implemented MySQLAdapter with tests and added SchemaField interface#11
Conversation
Merge pull request Solvaratech#7 from Solvaratech/main
…-core and drawline-app
| if (!this.connection) throw new Error('Not connected'); | ||
| const placeholders = documents.map(() => '(?)').join(', '); | ||
| const values = documents.map((doc) => Object.values(doc)); | ||
| const query = `INSERT INTO ${collection} VALUES ${placeholders}`; |
There was a problem hiding this comment.
Critical SQL Injection Vulnerability
|
Thanks @AryanMishra1789 for this contribution! The adapter structure is solid and your testing approach with vitest mocking is great. Before we can merge, please address these important issues: Critical: SQL Injection VulnerabilitiesMultiple methods directly interpolate table/field names without escaping:
Fix: Add an identifier escaping helper: private escapeIdentifier(id: string): string {
return '`' + id.replace(/`/g, '``') + '`';
}High: insertDocuments LogicThe current implementation may not handle column ordering correctly: const placeholders = documents.map(() => '(?)').join(', ');
const values = documents.map((doc) => Object.values(doc));This doesn't explicitly list columns. Compare with other adapters to align on the correct pattern. Medium Issues
Test CoverageConsider adding tests for:
|
|
The PR feedback has been addressed. I have also added SQLiteAdapter.ts and SQLiteAdapter.test.ts to fully implement and test secure SQLite generation. |
|
@AryanMishra1789 looks good to me. Merging it. Thank you for the contribution. |
Description
Testing
#3