From 8ce1b471fdabee9ea0634068da2594bd3242fce8 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 2 Mar 2022 17:40:50 +0000 Subject: [PATCH] Fixes for SQL issues raised by Marty. --- .../scripts/integrations/oracle/oracle.md | 4 +- packages/server/src/integrations/oracle.ts | 56 +++++++------------ packages/server/src/threads/query.js | 49 +++++++++++++++- packages/string-templates/src/index.cjs | 1 + packages/string-templates/src/index.js | 21 ++++++- packages/string-templates/src/index.mjs | 1 + packages/string-templates/src/utilities.js | 1 + packages/string-templates/test/basic.spec.js | 11 ++++ 8 files changed, 103 insertions(+), 41 deletions(-) diff --git a/packages/server/scripts/integrations/oracle/oracle.md b/packages/server/scripts/integrations/oracle/oracle.md index 6c2d7a9252..58d2a7dfbf 100644 --- a/packages/server/scripts/integrations/oracle/oracle.md +++ b/packages/server/scripts/integrations/oracle/oracle.md @@ -8,9 +8,9 @@ To install oracle express edition simply run `docker-compose up` -- A single instance pluggable database (PDB) will be created named `xepdb` +- A single instance pluggable database (PDB) will be created named `xepdb1` - The default password is configured in the compose file as `oracle` - - The `system`, `sys` and `pdbadmin` users all share this password + - The `system` and `pdbadmin` users share this password ## Instant Client diff --git a/packages/server/src/integrations/oracle.ts b/packages/server/src/integrations/oracle.ts index f602a97591..d25f1077f6 100644 --- a/packages/server/src/integrations/oracle.ts +++ b/packages/server/src/integrations/oracle.ts @@ -1,24 +1,24 @@ import { - Integration, DatasourceFieldTypes, + Integration, + Operation, + QueryJson, QueryTypes, SqlQuery, - QueryJson, - Operation, } from "../definitions/datasource" import { - finaliseExternalTables, - getSqlQuery, buildExternalTableId, convertSqlType, + finaliseExternalTables, + getSqlQuery, SqlClients, } from "./utils" import oracledb, { - ExecuteOptions, - Result, + BindParameters, Connection, ConnectionAttributes, - BindParameters, + ExecuteOptions, + Result, } from "oracledb" import Sql from "./base/sql" import { Table } from "../definitions/common" @@ -233,20 +233,14 @@ module OracleModule { return oracleTables } - private isSupportedColumn(column: OracleColumn) { - if (UNSUPPORTED_TYPES.includes(column.type)) { - return false - } - - return true + private static isSupportedColumn(column: OracleColumn) { + return !UNSUPPORTED_TYPES.includes(column.type) } - private isAutoColumn(column: OracleColumn) { - if (column.default && column.default.toLowerCase().includes("nextval")) { - return true - } - - return false + private static isAutoColumn(column: OracleColumn) { + return !!( + column.default && column.default.toLowerCase().includes("nextval") + ) } /** @@ -254,7 +248,7 @@ module OracleModule { * This matches the default behaviour for generating DDL used in knex. */ private isBooleanType(column: OracleColumn): boolean { - if ( + return ( column.type.toLowerCase() === "number" && Object.values(column.constraints).filter(c => { if ( @@ -273,11 +267,7 @@ module OracleModule { } return false }).length > 0 - ) { - return true - } - - return false + ) } private internalConvertType(column: OracleColumn): string { @@ -317,7 +307,9 @@ module OracleModule { // iterate each column on the table Object.values(oracleTable.columns) // remove columns that we can't read / save - .filter(oracleColumn => this.isSupportedColumn(oracleColumn)) + .filter(oracleColumn => + OracleIntegration.isSupportedColumn(oracleColumn) + ) // match the order of the columns in the db .sort((c1, c2) => c1.id - c2.id) .forEach(oracleColumn => { @@ -325,7 +317,7 @@ module OracleModule { let fieldSchema = table.schema[columnName] if (!fieldSchema) { fieldSchema = { - autocolumn: this.isAutoColumn(oracleColumn), + autocolumn: OracleIntegration.isAutoColumn(oracleColumn), name: columnName, type: this.internalConvertType(oracleColumn), } @@ -356,13 +348,7 @@ module OracleModule { const options: ExecuteOptions = { autoCommit: true } const bindings: BindParameters = query.bindings || [] - const result: Result = await connection.execute( - query.sql, - bindings, - options - ) - - return result + return await connection.execute(query.sql, bindings, options) } finally { if (connection) { try { diff --git a/packages/server/src/threads/query.js b/packages/server/src/threads/query.js index 5b1a30b57d..de5ac03930 100644 --- a/packages/server/src/threads/query.js +++ b/packages/server/src/threads/query.js @@ -2,8 +2,13 @@ const threadUtils = require("./utils") threadUtils.threadSetup() const ScriptRunner = require("../utilities/scriptRunner") const { integrations } = require("../integrations") -const { processStringSync } = require("@budibase/string-templates") +const { SourceNames } = require("../definitions/datasource") +const { + processStringSync, + findHBSBlocks, +} = require("@budibase/string-templates") const { doInAppContext, getAppDB } = require("@budibase/backend-core/context") +const { isSQL } = require("../integrations/utils") class QueryRunner { constructor(input, flags = { noRecursiveQuery: false }) { @@ -23,11 +28,49 @@ class QueryRunner { this.hasRerun = false } + interpolateSQL(fields, parameters) { + let { datasource } = this + let sql = fields.sql + const bindings = findHBSBlocks(sql) + let index = 1 + let variables = [] + for (let binding of bindings) { + let variable + switch (datasource.source) { + case SourceNames.POSTGRES: + variable = `$${index}` + break + case SourceNames.SQL_SERVER: + variable = `(@p${index - 1})` + break + case SourceNames.MYSQL: + variable = "?" + break + case SourceNames.ORACLE: + variable = `:${index}` + break + } + index++ + variables.push(binding) + sql = sql.replace(binding, variable) + } + // replicate the knex structure + fields.sql = sql + fields.bindings = this.enrichQueryFields(variables, parameters) + return fields + } + async execute() { let { datasource, fields, queryVerb, transformer } = this // pre-query, make sure datasource variables are added to parameters const parameters = await this.addDatasourceVariables() - let query = this.enrichQueryFields(fields, parameters) + let query + // handle SQL injections by interpolating the variables + if (isSQL(datasource)) { + query = this.interpolateSQL(fields, parameters) + } else { + query = this.enrichQueryFields(fields, parameters) + } // Add pagination values for REST queries if (this.pagination) { @@ -179,7 +222,7 @@ class QueryRunner { } enrichQueryFields(fields, parameters = {}) { - const enrichedQuery = {} + const enrichedQuery = Array.isArray(fields) ? [] : {} // enrich the fields with dynamic parameters for (let key of Object.keys(fields)) { diff --git a/packages/string-templates/src/index.cjs b/packages/string-templates/src/index.cjs index 5a84c45d78..d0de680aca 100644 --- a/packages/string-templates/src/index.cjs +++ b/packages/string-templates/src/index.cjs @@ -18,6 +18,7 @@ module.exports.processObject = templates.processObject module.exports.doesContainStrings = templates.doesContainStrings module.exports.doesContainString = templates.doesContainString module.exports.disableEscaping = templates.disableEscaping +module.exports.findHBSBlocks = templates.findHBSBlocks /** * Use vm2 to run JS scripts in a node env diff --git a/packages/string-templates/src/index.js b/packages/string-templates/src/index.js index eedd9423ee..6e464c27c4 100644 --- a/packages/string-templates/src/index.js +++ b/packages/string-templates/src/index.js @@ -3,7 +3,11 @@ const { registerAll, registerMinimum } = require("./helpers/index") const processors = require("./processors") const { atob, btoa } = require("./utilities") const manifest = require("../manifest.json") -const { FIND_HBS_REGEX, findDoubleHbsInstances } = require("./utilities") +const { + FIND_HBS_REGEX, + FIND_ANY_HBS_REGEX, + findDoubleHbsInstances, +} = require("./utilities") const hbsInstance = handlebars.create() registerAll(hbsInstance) @@ -310,6 +314,21 @@ module.exports.doesContainStrings = (template, strings) => { return false } +/** + * Given a string, this will return any {{ binding }} or {{{ binding }}} type + * statements. + * @param {string} string The string to search within. + * @return {string[]} The found HBS blocks. + */ +module.exports.findHBSBlocks = string => { + let regexp = new RegExp(FIND_ANY_HBS_REGEX) + let matches = string.match(regexp) + if (matches == null) { + return [] + } + return matches +} + /** * This function looks in the supplied template for handlebars instances, if they contain * JS the JS will be decoded and then the supplied string will be looked for. For example diff --git a/packages/string-templates/src/index.mjs b/packages/string-templates/src/index.mjs index f2cffdf378..3d115cdec1 100644 --- a/packages/string-templates/src/index.mjs +++ b/packages/string-templates/src/index.mjs @@ -18,6 +18,7 @@ export const processObject = templates.processObject export const doesContainStrings = templates.doesContainStrings export const doesContainString = templates.doesContainString export const disableEscaping = templates.disableEscaping +export const findHBSBlocks = templates.findHBSBlocks /** * Use polyfilled vm to run JS scripts in a browser Env diff --git a/packages/string-templates/src/utilities.js b/packages/string-templates/src/utilities.js index 0572350d62..775c150e1b 100644 --- a/packages/string-templates/src/utilities.js +++ b/packages/string-templates/src/utilities.js @@ -1,6 +1,7 @@ const ALPHA_NUMERIC_REGEX = /^[A-Za-z0-9]+$/g module.exports.FIND_HBS_REGEX = /{{([^{].*?)}}/g +module.exports.FIND_ANY_HBS_REGEX = /{?{{([^{].*?)}}}?/g module.exports.FIND_TRIPLE_HBS_REGEX = /{{{([^{].*?)}}}/g // originally this could be done with a single regex using look behinds diff --git a/packages/string-templates/test/basic.spec.js b/packages/string-templates/test/basic.spec.js index fbd1c5f440..6c85aa5fa1 100644 --- a/packages/string-templates/test/basic.spec.js +++ b/packages/string-templates/test/basic.spec.js @@ -7,6 +7,7 @@ const { encodeJSBinding, doesContainString, disableEscaping, + findHBSBlocks, } = require("../src/index.cjs") describe("Test that the string processing works correctly", () => { @@ -200,3 +201,13 @@ describe("check that disabling escaping function works", () => { }) }) +describe("check find hbs blocks function", () => { + it("should find none", () => { + expect(findHBSBlocks("hello there")).toEqual([]) + }) + + it("should find two", () => { + expect(findHBSBlocks("{{ hello }} there {{{ name }}}")).toEqual(["{{ hello }}", "{{{ name }}}"]) + }) +}) +