From c8f1947cb382052faeec93ba505874ad968769af Mon Sep 17 00:00:00 2001 From: Caspian Date: Mon, 12 Aug 2024 17:21:15 -0400 Subject: [PATCH] Database client name fix, and more rules for malformed transactions (#17) - Fix the name of a database client (the linter was failing silently on TypeORM before) - Add some TODOs - Add some rules for checking if a transaction is malformed - Now not running SQL injection over every method, only over transactions --- dbos-rules.test.ts | 41 +++++++++++++++---- dbos-rules.ts | 100 +++++++++++++++++++++++++++++++-------------- package-lock.json | 4 +- package.json | 2 +- 4 files changed, 105 insertions(+), 42 deletions(-) diff --git a/dbos-rules.test.ts b/dbos-rules.test.ts index b2e5696..7fb2da3 100644 --- a/dbos-rules.test.ts +++ b/dbos-rules.test.ts @@ -82,12 +82,12 @@ function makeSqlInjectionCode(code: string, sqlClient: string): string { $executeRawUnsafe(query: string, ...values: any[]) {} } - class PoolClient { - query(query: string, ...values: any[]) {} + class EntityManager { + query(query: string, parameters?: T) {} } - class TypeORMEntityManager { - query(query: string, parameters?: T) {} + class PoolClient { + query(query: string, ...values: any[]) {} } function Transaction(target?: any, key?: any, descriptor?: any): any { @@ -348,15 +348,15 @@ const testSet: TestSet = [ "PoolClient" ), - // Failure test #8 (testing `TypeORMEntityManager`) + // Failure test #8 (testing `EntityManager`) makeSqlInjectionFailureTest(` ctxt.client.query("foo" + (5).toString()); `, Array(1).fill("sqlInjection"), - "TypeORMEntityManager" + "EntityManager" ), - // Failure test #9 (testing not using `TransactionContext`) + // Failure test #9 (testing not using `TransactionContext`, and malformed transactions) makeSqlInjectionFailureTest(` ctxt; @@ -387,8 +387,33 @@ const testSet: TestSet = [ // But this one does ctxt.client.raw(bob.baz2); + + ////////// + + // Testing transactions without params + class Other2 { + @Transaction() + myInvalidTransactionWithoutParams() {} + } + + // Testing transactions without a specified client type + class Other3 { + @Transaction() + myInvalidTransactionWithoutTypeParam(ctxt: TransactionContext) {} + } + + class InvalidDatabaseClient {} + + // Testing transactions with an invalid client type + class Other4 { + @Transaction() + myInvalidTransactionWithoutTypeParam(ctxt: TransactionContext) {} + } `, - Array(1).fill("transactionDoesntUseTheDatabase") + [ + "transactionDoesntUseTheDatabase", "transactionHasNoParameters", + "transactionContextHasNoTypeArguments", "transactionContextHasInvalidClientType" + ] ), // Failure test #10 (a simpler object test) diff --git a/dbos-rules.ts b/dbos-rules.ts index e3041dd..1fde3e1 100644 --- a/dbos-rules.ts +++ b/dbos-rules.ts @@ -53,10 +53,11 @@ const awaitableTypes = new Set(["WorkflowContext"]); // Awaitable in determinist // This maps the ORM client name to a list of raw SQL query calls to check const ormClientInfoForRawSqlQueries: Map = new Map([ - ["PoolClient", ["query"]], - ["PrismaClient", ["$queryRawUnsafe", "$executeRawUnsafe"]], - ["TypeORMEntityManager", ["query"]], - ["Knex", ["raw"]] + ["Knex", ["raw"]], // For Knex + ["PrismaClient", ["$queryRawUnsafe", "$executeRawUnsafe"]], // For Prisma + ["EntityManager", ["query"]], // For TypeORM + ["PoolClient", ["query"]], // This is supported in `dbos-transact` (see `user_database.ts`, but not sure what ORM this corresponds to) + // ["PgDatabase", []], // For Drizzle (TODO: add full support for this) ]); const assignmentTokenKinds = new Set([ @@ -107,8 +108,12 @@ and accesses via brackets (e.g. \`a["b"]\`) only succeed when every field in the // The keys are the ids, and the values are the messages themselves return new Map([ - ["sqlInjection", `Possible SQL injection detected. The parameter to the query call site traces back to the nonliteral on line {{ lineNumber }}: '{{ theExpression }}'\n${sqlInjectionNotes}`], + ["transactionHasNoParameters", "This transaction has no parameters; add a `TransactionContext` parameter"], + ["transactionContextHasNoTypeArguments", "The context passed to this transaction has no type arguments; add one to specify the database client"], + ["transactionContextHasInvalidClientType", "The database client type `{{ clientType }}` used here is not recognized by the linter; consult the DBOS docs to find a supported one"], ["transactionDoesntUseTheDatabase", "This transaction does not use the database (via its `client` field). Consider using a communicator or a normal function"], + + ["sqlInjection", `Possible SQL injection detected. The parameter to the query call site traces back to the nonliteral on line {{ lineNumber }}: \`{{ theExpression }}\`\n${sqlInjectionNotes}`], ["globalMutation", "Deterministic DBOS operations (e.g. workflow code) should not mutate global variables; it can lead to non-reproducible behavior"], ["awaitingOnNotAllowedType", awaitMessage], ["Date", makeDateMessage("`Date()` or `new Date()`")], @@ -156,10 +161,13 @@ TODO (requests from others, and general things for me to do): - Chuck gave a suggestion to allow some function calls for LR-values; and do this by finding a way to mark them as constant +- Alex gave me this suggestion from a user: resolve many promises in parallel (with `Promise.all`) + From me: - Run this over `dbos-transact` - Maybe track type and variable aliasing somewhere, somehow (if needed) - Mark some simple function calls as being constant (this could quickly spiral in terms of complexity) +- Add full Drizzle support! */ ////////// These are some utility functions @@ -187,6 +195,18 @@ function getSymbol(nodeOrType: Node | Type): Maybe { return symbol; } +function getTypeName(nodeOrType: Node | Type): string { + if (nodeOrType instanceof Node) { + // If it's a literal type, it'll get the base type; otherwise, nothing happens + const type = nodeOrType.getType().getBaseTypeOfLiteralType(); + const maybeSymbol = getSymbol(type); + return maybeSymbol?.getName() ?? type.getText(nodeOrType); + } + else { + return getSymbol(nodeOrType)?.getName() ?? nodeOrType.getText(); + } +} + function getRefsToNodeOrSymbol(nodeOrSymbol: Node | Symbol): Node[] { let maybeSymbol = nodeOrSymbol instanceof Node ? getSymbol(nodeOrSymbol) : nodeOrSymbol; @@ -288,11 +308,10 @@ const callsBannedFunction: ErrorChecker = (node, _fnDecl, _isLocal) => { // TODO: match against `.then` as well (with a promise object preceding it) const awaitsOnNotAllowedType: ErrorChecker = (node, _fnDecl, _isLocal) => { - // If the valid type set and arg type set intersect, then there's a valid type in the args. function validTypeExistsInFunctionCallParams(functionCall: CallExpression, validTypes: Set): boolean { // I'd like to use `isDisjointFrom` here, but it doesn't seem to be available, for some reason - const argTypes = functionCall.getArguments().map(getTypeNameForTsMorphNode); + const argTypes = functionCall.getArguments().map(getTypeName); return argTypes.some((argType) => validTypes.has(argType)); } @@ -316,8 +335,7 @@ const awaitsOnNotAllowedType: ErrorChecker = (node, _fnDecl, _isLocal) => { ////////// - const typeName = getTypeNameForTsMorphNode(lhs); - const awaitingOnAllowedType = awaitableTypes.has(typeName); + const awaitingOnAllowedType = awaitableTypes.has(getTypeName(lhs)); if (!awaitingOnAllowedType) { /* We should be allowed to await if we call a function that passes @@ -567,14 +585,15 @@ function checkCallForInjection(callParam: Node): Maybe { const callExprWithoutParams = callExpr.getExpression(); const args = callExpr.getArguments(); @@ -629,12 +648,19 @@ function maybeGetArgFromRawSqlCallSite(callExpr: CallExpression): Maybe { // `client.`, or `ctxt.client.`, and so on with the prefixes const identifiers = callExprWithoutParams.getDescendantsOfKind(SyntaxKind.Identifier); - const identifierTypeNames = identifiers.map(getTypeNameForTsMorphNode); - if (identifierTypeNames.length < 2) return; // Can't recognize a raw SQL call for 0 or 1 identifiers + if (identifiers.length <= 1) { + debugLog(`Cannot recognize a raw SQL call from this here: '${callExpr.getText()}'`); + return; + } + const identifierTypeNames = identifiers.map(getTypeName); const expectedClient = identifierTypeNames[identifierTypeNames.length - 2]; const callNames = ormClientInfoForRawSqlQueries.get(expectedClient); - if (callNames === Nothing) return; + + if (callNames === Nothing) { + debugLog(`Unrecognized database client: '${expectedClient}'`); + return; + } const expectedRawQueryCall = identifiers[identifiers.length - 1].getText(); @@ -651,21 +677,42 @@ const isSqlInjection: ErrorChecker = (node, _fnDecl, _isLocal) => { return checkCallForInjection(maybeArg); } } -}; +} -////////// This code is for detecting useless transactions +////////// This code is for detecting useless/malformed transactions /* Note: this may result in false negatives for nested closures that capture the transaction context's client, and when you call helper functions that you pass the context object to, but that helper function does nothing. */ -const transactionDoesntUseTheDatabase: ErrorChecker = (node, fnDecl, _isLocal) => { +const transactionIsMalformed: ErrorChecker = (node, fnDecl, _isLocal) => { if (node !== fnDecl) return; // Only analyze the whole function + ////////// Step 1: check if the transaction has no parameters + const params = fnDecl.getParameters(); - if (params.length === 0) return; // In this case, not a valid transaction + if (params.length === 0) return "transactionHasNoParameters"; + + ////////// Step 2: check if the transaction context has no type arguments const transactionContext = params[0]; - const transactionContextSymbol = getSymbol(transactionContext); // The first param should be the transaction context - if (transactionContextSymbol === Nothing) return; // No symbol for the first param -> should not analyze + const typeArgs = transactionContext.getType().getTypeArguments(); + if (typeArgs.length === 0) return "transactionContextHasNoTypeArguments"; + + ////////// Step 3: check if the database client used is unrecognized + + const clientType = getTypeName(typeArgs[0]); + + if (!ormClientInfoForRawSqlQueries.has(clientType)) { + return ["transactionContextHasInvalidClientType", {clientType: clientType}]; + } + + ////////// Step 4: check if the transaction context is never used + + const transactionContextSymbol = getSymbol(transactionContext); + + if (transactionContextSymbol === Nothing) { + debugLog("No symbol was ever found for the transaction context!"); + return; // No symbol for the first param -> should not analyze + } let foundDatabaseUsage = false; @@ -703,8 +750,7 @@ First field: a set of method decorators to match on (if `Nothing`, then match on Second field: a list of error checkers to run. */ const decoratorSetErrorCheckerMapping: [Maybe>, ErrorChecker[]][] = [ - [Nothing, [isSqlInjection]], // Checking for SQL injection here (all functions) - [new Set(["Transaction"]), [transactionDoesntUseTheDatabase]], // Checking for useless transactions here + [new Set(["Transaction"]), [isSqlInjection, transactionIsMalformed]], // Checking for SQL injection and malformed transactions here [new Set(["Workflow"]), [mutatesGlobalVariable, callsBannedFunction, awaitsOnNotAllowedType]] // Checking for nondeterminism here ]; @@ -816,14 +862,6 @@ function makeEslintNode(tsMorphNode: Node): EslintNode { return eslintNode; } -function getTypeNameForTsMorphNode(tsMorphNode: Node): string { - // If it's a literal type, it'll get the base type; otherwise, nothing happens - const type = tsMorphNode.getType().getBaseTypeOfLiteralType(); - - const maybeSymbol = getSymbol(type); - return maybeSymbol?.getName() ?? type.getText(); -} - // This is just for making sure that the unit tests are well constructed (not used when deployed) function checkDiagnostics(node: Node) { const project = new Project({}); diff --git a/package-lock.json b/package-lock.json index 89ad2a3..b6f9d85 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@dbos-inc/eslint-plugin", - "version": "3.2.0", + "version": "3.3.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@dbos-inc/eslint-plugin", - "version": "3.2.0", + "version": "3.3.0", "license": "MIT", "dependencies": { "@types/node": "^20.14.12", diff --git a/package.json b/package.json index 4ac267b..2f8b2b5 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@dbos-inc/eslint-plugin", - "version": "3.2.0", + "version": "3.3.0", "description": "eslint plugin for DBOS SDK", "license": "MIT", "repository": {