Chi 1991 parameterized queries

hrm-domain/hrm-service/package.json

@@ -45,6 +45,7 @@
     "@tech-matters/twilio-client": "^1.0.0",
     "@tech-matters/http": "^1.0.0",
     "@tech-matters/resources-service": "^1.0.0",
+    "@tech-matters/sql": "^1.0.0",
     "@tech-matters/twilio-worker-auth": "^1.0.0",
     "@tech-matters/types": "^1.0.0",
     "aws-sdk": "^2.1231.0",

hrm-domain/hrm-service/service-tests/case-search.test.ts

@@ -1284,6 +1284,20 @@ describe('/cases route', () => {
         await caseDb.deleteById(createdCase.id, accountSid);
         useOpenRules();
       });
+
+      test('Should throw error promptly with malformed input', async () => {
+        const start = Date.now();
+        const response = await request
+          .post(`${route}/search`)
+          .query({ limit: 20, offset: 0 })
+          .set(headers)
+          .send({
+            filters: { createdAt: { from: '${contactNumber}' } },
+            contactNumber: "=')) and (select pg_sleep(5)) is null AND ((1=1) --",
+          });
+        expect(response.ok).toBeFalsy();
+        expect(Date.now() - start).toBeLessThan(2000);
+      }, 10000);
     });
   });
 });

hrm-domain/hrm-service/src/case/case-data-access.ts

@@ -17,11 +17,13 @@
 import { db, pgp } from '../connection-pool';
 import { getPaginationElements } from '../search';
 import { updateByIdSql } from './sql/case-update-sql';
-import { OrderByColumnType, OrderByDirectionType, selectCaseSearch } from './sql/case-search-sql';
+import { OrderByColumnType, selectCaseSearch } from './sql/case-search-sql';
 import { caseSectionUpsertSql, deleteMissingCaseSectionsSql } from './sql/case-sections-sql';
 import { DELETE_BY_ID } from './sql/case-delete-sql';
 import { selectSingleCaseByIdSql } from './sql/case-get-sql';
 import { Contact } from '../contact/contact-data-access';
+import { parameterizedQuery, OrderByDirectionType } from '@tech-matters/sql';
+import { ParameterizedQuery } from 'pg-promise';
 
 export type CaseRecordCommon = {
   info: any;
@@ -114,11 +116,12 @@ export const create = async (
       let inserted: CaseRecord = await transaction.one(statement);
       if ((caseSections ?? []).length) {
         const allSections = caseSections.map(s => ({ ...s, caseId: inserted.id, accountSid }));
-        const sectionStatement = `${caseSectionUpsertSql(allSections)};${selectSingleCaseByIdSql(
-          'Cases',
-        )}`;
+
         const queryValues = { accountSid, caseId: inserted.id };
-        inserted = await transaction.one(sectionStatement, queryValues);
+        await transaction.none(caseSectionUpsertSql(allSections));
+        inserted = await transaction.one(
+          parameterizedQuery(selectSingleCaseByIdSql('Cases'), queryValues),
+        );
       }
 
       return inserted;
@@ -133,7 +136,7 @@ export const getById = async (
   return db.task(async connection => {
     const statement = selectSingleCaseByIdSql('Cases');
     const queryValues = { accountSid, caseId };
-    return connection.oneOrNone<CaseRecord>(statement, queryValues);
+    return connection.oneOrNone<CaseRecord>(parameterizedQuery(statement, queryValues));
   });
 };
 
@@ -159,7 +162,9 @@ export const search = async (
       limit: limit,
       offset: offset,
     };
-    const result: CaseWithCount[] = await connection.any<CaseWithCount>(statement, queryValues);
+    const result: CaseWithCount[] = await connection.manyOrNone<CaseWithCount>(
+      parameterizedQuery(statement, queryValues),
+    );
     const totalCount: number = result.length ? result[0].totalCount : 0;
     return { rows: result, count: totalCount };
   });
@@ -168,7 +173,7 @@ export const search = async (
 };
 
 export const deleteById = async (id, accountSid) => {
-  return db.oneOrNone(DELETE_BY_ID, [accountSid, id]);
+  return db.oneOrNone(new ParameterizedQuery({ text: DELETE_BY_ID, values: [accountSid, id] }));
 };
 
 export const update = async (
@@ -177,6 +182,7 @@ export const update = async (
   accountSid: string,
 ): Promise<CaseRecord> => {
   return db.tx(async transaction => {
+    const caseUpdateSqlStatements = [];
     const statementValues = {
       accountSid,
       caseId: id,
@@ -196,8 +202,22 @@ export const update = async (
       Object.assign(statementValues, values);
       await transaction.none(sql, statementValues);
     }
-    await transaction.none(updateByIdSql(caseRecordUpdates, accountSid, id), statementValues);
-
-    return transaction.oneOrNone(selectSingleCaseByIdSql('Cases'), statementValues);
+    const caseUpdateQuery = updateByIdSql(caseRecordUpdates);
+    // If there are preceding statements, put them in a CTE so we can run a single prepared statement
+    const fullUpdateQuery = caseUpdateSqlStatements.length
+      ? `WITH
+      ${caseUpdateSqlStatements.map((statement, i) => `q${i} AS (${statement})`).join(`,
+      `)}
+      ${caseUpdateQuery}`
+      : caseUpdateQuery;
+    await transaction.none(parameterizedQuery(fullUpdateQuery, statementValues));
+
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
+    return transaction.oneOrNone(
+      parameterizedQuery(selectSingleCaseByIdSql('Cases'), {
+        accountSid,
+        caseId: id,
+      }),
+    );
   });
 };

hrm-domain/hrm-service/src/case/sql/case-search-sql.ts

@@ -19,15 +19,8 @@ import { SELECT_CASE_SECTIONS } from './case-sections-sql';
 import { CaseListFilters, DateExistsCondition, DateFilter } from '../case-data-access';
 import { leftJoinCsamReportsOnFK } from '../../csam-report/sql/csam-report-get-sql';
 import { leftJoinReferralsOnFK } from '../../referral/sql/referral-get-sql';
-
-export const OrderByDirection = {
-  ascendingNullsLast: 'ASC NULLS LAST',
-  descendingNullsLast: 'DESC NULLS LAST',
-  ascending: 'ASC',
-  descending: 'DESC',
-} as const;
-
-export type OrderByDirectionType = typeof OrderByDirection[keyof typeof OrderByDirection];
+import { OrderByDirection } from '@tech-matters/sql';
+import { OrderByDirectionType } from '@tech-matters/sql/dist/ordering';
 
 export const OrderByColumn = {
   ID: 'id',
@@ -95,8 +88,8 @@ const dateFilterCondition = (
   if (filter.to || filter.from) {
     filter.to = filter.to ?? null;
     filter.from = filter.from ?? null;
-    return `(($<${filterName}.from> IS NULL OR ${field} >= $<${filterName}.from>::TIMESTAMP WITH TIME ZONE) 
-            AND ($<${filterName}.to> IS NULL OR ${field} <= $<${filterName}.to>::TIMESTAMP WITH TIME ZONE)
+    return `(($<${filterName}.from>::TIMESTAMP WITH TIME ZONE IS NULL OR ${field} >= $<${filterName}.from>::TIMESTAMP WITH TIME ZONE) 
+            AND ($<${filterName}.to>::TIMESTAMP WITH TIME ZONE IS NULL OR ${field} <= $<${filterName}.to>::TIMESTAMP WITH TIME ZONE)
             ${existsCondition ? ` AND ${existsCondition}` : ''})`;
   }
   return existsCondition;
@@ -160,15 +153,15 @@ const nameAndPhoneNumberSearchSql = (
   lastNameSources: string[],
   phoneNumberColumns: string[],
 ) =>
-  `CASE WHEN $<firstName> IS NULL THEN TRUE
+  `CASE WHEN $<firstName>::text IS NULL THEN TRUE
         ELSE ${firstNameSources.map(fns => `${fns} ILIKE $<firstName>`).join('\n OR ')}
         END
       AND
-        CASE WHEN $<lastName> IS NULL THEN TRUE
+        CASE WHEN $<lastName>::text IS NULL THEN TRUE
         ELSE ${lastNameSources.map(lns => `${lns} ILIKE $<lastName>`).join('\n OR ')}
         END
       AND
-        CASE WHEN $<phoneNumber> IS NULL THEN TRUE
+        CASE WHEN $<phoneNumber>::text IS NULL THEN TRUE
         ELSE (
           ${phoneNumberColumns
             .map(pn => `regexp_replace(${pn}, '\\D', '', 'g') ILIKE $<phoneNumber>`)
@@ -178,7 +171,7 @@ const nameAndPhoneNumberSearchSql = (
 
 const SEARCH_WHERE_CLAUSE = `(
       -- search on childInformation of connectedContacts
-      ($<firstName> IS NULL AND $<lastName> IS NULL AND $<phoneNumber> IS NULL) OR
+      ($<firstName>::text IS NULL AND $<lastName>::text IS NULL AND $<phoneNumber>::text IS NULL) OR
       EXISTS (
         SELECT 1 FROM "Contacts" c WHERE c."caseId" = cases.id  AND c."accountSid" = cases."accountSid"
           AND (
@@ -236,7 +229,7 @@ const SEARCH_WHERE_CLAUSE = `(
       )
     )
     AND (
-      $<contactNumber> IS NULL OR
+      $<contactNumber>::text IS NULL OR
       EXISTS (
         SELECT 1 FROM "Contacts" c WHERE c."caseId" = cases.id AND c."accountSid" = cases."accountSid" AND c.number = $<contactNumber>
       )
@@ -277,7 +270,7 @@ export const selectCaseSearch = (
     `WHERE
     (info IS NULL OR jsonb_typeof(info) = 'object')
     AND
-      $<accountSid> IS NOT NULL AND cases."accountSid" = $<accountSid>`,
+      $<accountSid>::text IS NOT NULL AND cases."accountSid" = $<accountSid>`,
     SEARCH_WHERE_CLAUSE,
     filterSql(filters),
   ].filter(sql => sql).join(`

hrm-domain/hrm-service/src/case/sql/case-update-sql.ts

@@ -26,14 +26,7 @@ const updateCaseColumnSet = new pgp.helpers.ColumnSet(
   { table: 'Cases' },
 );
 
-export const updateByIdSql = (
-  updatedValues: Record<string, unknown>,
-  accountSid: string,
-  caseId: string,
-) => `
+export const updateByIdSql = (updatedValues: Record<string, unknown>) => `
         ${pgp.helpers.update(updatedValues, updateCaseColumnSet)} 
-        ${pgp.as.format(`WHERE "Cases"."accountSid" = $<accountSid> AND "Cases"."id" = $<caseId>`, {
-          accountSid,
-          caseId,
-        })} 
+          WHERE "Cases"."accountSid" = $<accountSid> AND "Cases"."id" = $<caseId> 
 `;

hrm-domain/hrm-service/src/connection-pool.ts

@@ -14,7 +14,7 @@
  * along with this program.  If not, see https://www.gnu.org/licenses/.
  */
 
-import pgPromise from 'pg-promise';
+import pgPromise, { ITask } from 'pg-promise';
 import config from './config/db';
 
 export const pgp = pgPromise({});
@@ -24,3 +24,13 @@ export const db = pgp(
     config.host
   }:${config.port}/${encodeURIComponent(config.database)}?&application_name=hrm-service`,
 );
+
+export const txIfNotInOne = async <T>(
+  task: ITask<T> | undefined,
+  work: (y: ITask<T>) => Promise<T>,
+): Promise<T> => {
+  if (task) {
+    return task.txIf(work);
+  }
+  return db.tx(work);
+};

hrm-domain/hrm-service/src/contact-job/contact-job-data-access.ts

@@ -14,7 +14,7 @@
  * along with this program.  If not, see https://www.gnu.org/licenses/.
  */
 
-import { db, pgp } from '../connection-pool';
+import { db, pgp, txIfNotInOne } from '../connection-pool';
 // eslint-disable-next-line prettier/prettier
 import type { Contact } from '../contact/contact-data-access';
 import {
@@ -29,7 +29,6 @@ import {
   UPDATE_JOB_CLEANUP_ACTIVE_SQL,
   UPDATE_JOB_CLEANUP_PENDING_SQL,
 } from './sql/contact-job-sql';
-import { txIfNotInOne } from '../sql';
 
 import { ContactJobType } from '@tech-matters/types';
 

hrm-domain/hrm-service/src/contact/contact-data-access.ts

@@ -13,7 +13,7 @@
  * You should have received a copy of the GNU Affero General Public License
  * along with this program.  If not, see https://www.gnu.org/licenses/.
  */
-
+import { txIfNotInOne } from '../connection-pool';
 import { db } from '../connection-pool';
 import {
   UPDATE_CASEID_BY_ID,
@@ -31,7 +31,6 @@ import {
 } from './contact-json';
 // eslint-disable-next-line prettier/prettier
 import type { ITask } from 'pg-promise';
-import { txIfNotInOne } from '../sql';
 
 type ExistingContactRecord = {
   id: number;

hrm-domain/hrm-service/src/csam-report/csam-report-data-access.ts

@@ -14,7 +14,7 @@
  * along with this program.  If not, see https://www.gnu.org/licenses/.
  */
 
-import { db } from '../connection-pool';
+import { db, txIfNotInOne } from '../connection-pool';
 import { insertCSAMReportSql } from './sql/csam-report-insert-sql';
 import {
   selectSingleCsamReportByIdSql,
@@ -24,7 +24,6 @@ import {
   updateContactIdByCsamReportIdsSql,
   updateAcknowledgedByCsamReportIdSql,
 } from './sql/csam-report-update-sql';
-import { txIfNotInOne } from '../sql';
 
 export type NewCSAMReport = {
   reportType: 'counsellor-generated' | 'self-generated';

hrm-domain/hrm-service/src/referral/referral-data-access.ts

@@ -20,8 +20,8 @@ import {
   DatabaseForeignKeyViolationError,
   DatabaseUniqueConstraintViolationError,
   inferPostgresError,
-  txIfNotInOne,
-} from '../sql';
+} from '@tech-matters/sql';
+import { txIfNotInOne } from '../connection-pool';
 
 // Working around the lack of a 'cause' property in the Error class for ES2020 - can be removed when we upgrade to ES2022
 export class DuplicateReferralError extends Error {

hrm-domain/hrm-service/src/search.ts

@@ -14,7 +14,7 @@
  * along with this program.  If not, see https://www.gnu.org/licenses/.
  */
 
-import { OrderByDirection } from './sql';
+import { OrderByDirection } from '@tech-matters/sql';
 
 export type PaginationQuery = {
   limit?: string;