Merge pull request #4297 from activepieces/fix/db

fix(mysql/postgres): add markdown warning how to prevent sql injections
activepieces · Mar 28, 2024 · 2d9abc0 · 2d9abc0
2 parents 5b3bf68 + 62ebfe9
commit 2d9abc0
Show file tree

Hide file tree

Showing 11 changed files with 74 additions and 61 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -174,6 +174,7 @@
     "soap": "1.0.0",
     "socket.io": "4.7.2",
     "sqlite3": "5.1.7",
+    "sqlstring": "2.3.3",
     "ssh2-sftp-client": "9.1.0",
     "string-replace-async": "3.0.2",
     "stripe": "14.3.0",
@@ -243,6 +244,7 @@
     "@types/quill": "2.0.10",
     "@types/semver": "7.5.6",
     "@types/snowflake-sdk": "1.6.20",
+    "@types/sqlstring": "2.3.2",
     "@types/ssh2-sftp-client": "9.0.0",
     "@types/tinycolor2": "1.4.5",
     "@types/xml2js": "0.4.14",
@@ -296,4 +298,4 @@
   "nx": {
     "includedScripts": []
   }
-}
+}
diff --git a/packages/pieces/community/mysql/package.json b/packages/pieces/community/mysql/package.json
@@ -1,4 +1,4 @@
 {
   "name": "@activepieces/piece-mysql",
-  "version": "0.1.4"
+  "version": "0.1.5"
 }
diff --git a/packages/pieces/community/mysql/src/lib/actions/delete-row.ts b/packages/pieces/community/mysql/src/lib/actions/delete-row.ts
@@ -1,13 +1,15 @@
 import { createAction, Property } from '@activepieces/pieces-framework';
-import { mysqlCommon, mysqlConnect, sanitizeColumnName } from '../common';
+import { mysqlCommon, mysqlConnect, sanitizeColumnName, warningMarkdown } from '../common';
 import { mysqlAuth } from '../..';
+import sqlstring from 'sqlstring';
 
 export default createAction({
   auth: mysqlAuth,
   name: 'delete_row',
   displayName: 'Delete Row',
   description: 'Deletes one or more rows from a table',
   props: {
+    markdown: warningMarkdown,
     timezone: mysqlCommon.timezone,
     table: mysqlCommon.table(),
     search_column: Property.ShortText({
@@ -20,18 +22,18 @@ export default createAction({
     }),
   },
   async run(context) {
-    const qs =
-      'DELETE FROM `' +
-      context.propsValue.table +
-      '` WHERE ' +
-      sanitizeColumnName(context.propsValue.search_column) +
-      '=?;';
-    const conn = await mysqlConnect(context.auth, context.propsValue);
+    const tableName = sanitizeColumnName(context.propsValue.table);
+    const searchColumn = sanitizeColumnName(context.propsValue.search_column);
+    const searchValue = context.propsValue.search_value;
+
+    const queryString = `DELETE FROM \`${tableName}\` WHERE ${searchColumn}=?;`;
+
+    const connection = await mysqlConnect(context.auth, context.propsValue);
     try {
-      const result = await conn.query(qs, [context.propsValue.search_value]);
+      const result = await connection.query(queryString, [searchValue]);
       return result;
     } finally {
-      await conn.end();
+      await connection.end();
     }
   },
 });
diff --git a/packages/pieces/community/mysql/src/lib/actions/execute-query.ts b/packages/pieces/community/mysql/src/lib/actions/execute-query.ts
@@ -1,5 +1,5 @@
 import { createAction, Property } from '@activepieces/pieces-framework';
-import { mysqlCommon, mysqlConnect } from '../common';
+import { mysqlCommon, mysqlConnect, warningMarkdown } from '../common';
 import { mysqlAuth } from '../..';
 
 export default createAction({
@@ -8,6 +8,7 @@ export default createAction({
   displayName: 'Execute Query',
   description: 'Executes a query on the mysql database and returns the results',
   props: {
+    markdown: warningMarkdown,
     timezone: mysqlCommon.timezone,
     query: Property.ShortText({
       displayName: 'Query',
@@ -16,7 +17,7 @@ export default createAction({
     }),
     args: Property.Array({
       displayName: 'Arguments',
-      description: 'Can inserted in the query string using ?',
+      description: 'Arguments to use in the query, if any. Should be in the same order as the ? in the query string..',
       required: false,
     }),
   },

diff --git a/packages/pieces/community/mysql/src/lib/actions/find-rows.ts b/packages/pieces/community/mysql/src/lib/actions/find-rows.ts
@@ -1,5 +1,5 @@
 import { createAction, Property } from '@activepieces/pieces-framework';
-import { mysqlCommon, mysqlConnect, isSpecialColumn } from '../common';
+import { mysqlCommon, mysqlConnect, sanitizeColumnName, warningMarkdown } from '../common';
 import { mysqlAuth } from '../..';
 
 export default createAction({
@@ -8,6 +8,7 @@ export default createAction({
   displayName: 'Find Rows',
   description: 'Reads rows from a table',
   props: {
+    markdown: warningMarkdown,
     timezone: mysqlCommon.timezone,
     table: mysqlCommon.table(),
     condition: Property.ShortText({
@@ -29,17 +30,13 @@ export default createAction({
   async run(context) {
     const columns = (context.propsValue.columns as string[]) || ['*'];
     const qsColumns = columns
-      .map((c) => (isSpecialColumn(c) ? c : '`' + c + '`'))
+      .map((c) => sanitizeColumnName(c))
       .join(',');
-    const qs =
-      'SELECT ' +
-      qsColumns +
-      ' FROM `' +
-      context.propsValue.table +
-      '` WHERE ' +
-      context.propsValue.condition +
-      ';';
+
+    const qs = `SELECT ${qsColumns} FROM \`${sanitizeColumnName(context.propsValue.table)}\` WHERE ${context.propsValue.condition};`;
+
     const conn = await mysqlConnect(context.auth, context.propsValue);
+
     try {
       const results = await conn.query(qs, context.propsValue.args);
       return { results };

diff --git a/packages/pieces/community/mysql/src/lib/actions/insert-row.ts b/packages/pieces/community/mysql/src/lib/actions/insert-row.ts
@@ -1,6 +1,7 @@
 import { createAction, Property } from '@activepieces/pieces-framework';
-import { mysqlCommon, mysqlConnect } from '../common';
+import { mysqlCommon, mysqlConnect, sanitizeColumnName, warningMarkdown } from '../common';
 import { mysqlAuth } from '../..';
+import sqlstring from 'sqlstring';
 
 export default createAction({
   auth: mysqlAuth,
@@ -12,25 +13,17 @@ export default createAction({
     table: mysqlCommon.table(),
     values: Property.Object({
       displayName: 'Values',
-      description: 'Values to be inserted into the row',
       required: true,
     }),
   },
   async run(context) {
     const fields = Object.keys(context.propsValue.values);
-    const values = fields.map((f) => context.propsValue.values[f]);
-    const qsFields = fields.map((f) => '`' + f + '`').join(',');
-    const qsValues = fields.map((f) => '?').join(',');
-    const qs =
-      'INSERT INTO `' +
-      context.propsValue.table +
-      '` (' +
-      qsFields +
-      ') VALUES (' +
-      qsValues +
-      ');';
+    const qsFields = fields.map((f) => sanitizeColumnName(f)).join(',');
+    const qsValues = fields.map(() => '?').join(',');
+    const qs = `INSERT INTO \`${sanitizeColumnName(context.propsValue.table)}\` (${qsFields}) VALUES (${qsValues});`;
     const conn = await mysqlConnect(context.auth, context.propsValue);
     try {
+      const values = fields.map((f) => context.propsValue.values[f]);
       const result = await conn.query(qs, values);
       return result;
     } finally {

diff --git a/packages/pieces/community/mysql/src/lib/actions/update-row.ts b/packages/pieces/community/mysql/src/lib/actions/update-row.ts
@@ -1,6 +1,7 @@
 import { createAction, Property } from '@activepieces/pieces-framework';
 import { mysqlCommon, mysqlConnect, sanitizeColumnName } from '../common';
 import { mysqlAuth } from '../..';
+import sqlstring from 'sqlstring';
 
 export default createAction({
   auth: mysqlAuth,
@@ -12,7 +13,6 @@ export default createAction({
     table: mysqlCommon.table(),
     values: Property.Object({
       displayName: 'Values',
-      description: 'Values to be updated',
       required: true,
     }),
     search_column: Property.ShortText({
@@ -26,18 +26,11 @@ export default createAction({
   },
   async run(context) {
     const fields = Object.keys(context.propsValue.values);
-    const values = fields.map((f) => context.propsValue.values[f]);
-    const qsValues = fields.map((f) => '`' + f + '`=?').join(',');
-    const qs =
-      'UPDATE `' +
-      context.propsValue.table +
-      '` SET ' +
-      qsValues +
-      ' WHERE ' +
-      sanitizeColumnName(context.propsValue.search_column) +
-      '=?;';
+    const qsValues = fields.map((f) => sanitizeColumnName(f) + '=?').join(',');
+    const qs = `UPDATE \`${sanitizeColumnName(context.propsValue.table)}\` SET ${qsValues} WHERE ${sqlstring.escape(context.propsValue.search_column)}=?;`;
     const conn = await mysqlConnect(context.auth, context.propsValue);
     try {
+    const values = fields.map((f) => context.propsValue.values[f]);
       const result = await conn.query(qs, [
         ...values,
         context.propsValue.search_value,

diff --git a/packages/pieces/community/mysql/src/lib/common/index.ts b/packages/pieces/community/mysql/src/lib/common/index.ts
@@ -5,6 +5,14 @@ import {
 } from '@activepieces/pieces-framework';
 import { Connection, createConnection } from 'promise-mysql';
 import { mysqlAuth } from '../..';
+import sqlstring from 'sqlstring';
+
+export const warningMarkdown = Property.MarkDown({
+  value: `
+  **DO NOT** use dynamic input directly in the query string or column names.
+  \n
+  Use **?** in the query and dynamic values in args/values for parameterized queries to prevent **SQL injection**.`
+});
 
 export async function mysqlConnect(
   auth: PiecePropValueSchema<typeof mysqlAuth>,
@@ -29,20 +37,19 @@ export async function mysqlGetTableNames(conn: Connection): Promise<string[]> {
 export const mysqlCommon = {
   timezone: Property.ShortText({
     displayName: 'Timezone',
-    description: 'Timezone for the mysql server to use',
+    description: 'Timezone for the MySQL server to use',
     required: false,
   }),
   table: (required = true) =>
     Property.Dropdown({
-      description: 'The name of the table',
       displayName: 'Table',
       required,
       refreshers: [],
       options: async ({ auth }) => {
         if (!auth) {
           return {
             disabled: true,
-            placeholder: 'connect to your database first',
+            placeholder: 'Connect to your database first',
             options: [],
           };
         }
@@ -65,13 +72,10 @@ export const mysqlCommon = {
     }),
 };
 
-export function isSpecialColumn(name: string): boolean {
-  return name == '*' || name.includes(' ') || name.includes('(');
-}
 
-export function sanitizeColumnName(name: string): string {
-  if (isSpecialColumn(name)) {
+export function sanitizeColumnName(name: string | undefined): string {
+  if ( name == '*') {
     return name;
   }
-  return '`' + name + '`';
+  return sqlstring.escapeId(name);
 }
diff --git a/packages/pieces/community/postgres/package.json b/packages/pieces/community/postgres/package.json
@@ -1,4 +1,4 @@
 {
   "name": "@activepieces/piece-postgres",
-  "version": "0.1.4"
+  "version": "0.1.5"
 }
diff --git a/packages/pieces/community/postgres/src/lib/actions/run-query.ts b/packages/pieces/community/postgres/src/lib/actions/run-query.ts
@@ -8,6 +8,11 @@ export const runQuery = createAction({
   displayName: 'Run Query',
   description: 'Run Query',
   props: {
+    markdown: Property.MarkDown({
+      value: `
+      **DO NOT** insert dynamic input directly into the query string. Instead, use $1, $2, $3 and add them in args for parameterized queries to prevent **SQL injection.**`
+    }),
+
     query: Property.ShortText({
       displayName: 'Query',
       description: 'Please use $1, $2, etc. for parameterized queries to avoid SQL injection.',