feat: add a new rule should-specify-forbid-unknown-values

darraghoriordan · darraghoriordan · commit 267687d21e83 · 2021-10-26T13:55:12.000+11:00
diff --git a/README.md b/README.md
@@ -30,6 +30,12 @@ I have a bunch of rules here that are mostly for strict typing with decorators f
 
 These rules are somewhat opinionated, but necessary for clean model generation if using an Open Api model generator.
 
+### 2. Some security and best practices
+
+There is a CVE for class-transformer when using random javascript objects. You need to be careful about configuring the ValidationPipe in NestJs. See
+https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-18413
+https://github.com/typestack/class-validator/issues/438
+
 ## Available rule index (more details are available for each rule below)
 
 Nest Modules
@@ -83,6 +89,41 @@ Note: You can easily turn off all the swagger rules if you don't use swagger by
 
 ## Rule Details
 
+### Rule: should-specify-forbid-unknown-values
+
+This checks when if you are setting ValidationPipe parameters you set forbidUnknownValues to true.
+
+e.g. this passes
+
+```ts
+const validationPipeB = new ValidationPipe({
+    forbidNonWhitelisted: true,
+    forbidUnknownValues: true,
+});
+```
+
+this fails because property is not set
+
+```ts
+const validationPipeB = new ValidationPipe({
+    forbidNonWhitelisted: true,
+});
+```
+
+this fails because property is set to false
+
+```ts
+const validationPipeB = new ValidationPipe({
+    forbidNonWhitelisted: false,
+});
+```
+
+this passes because the default values seem to work ok
+
+```ts
+const validationPipeB = new ValidationPipe();
+```
+
 ### Rule: provided-injected-should-match-factory-parameters
 
 Checks that there are the same number of injected items in a Provider that are passed to the factory method
diff --git a/src/configs/recommended.ts b/src/configs/recommended.ts
@@ -20,5 +20,6 @@ export = {
         "@darraghor/nestjs-typed/api-enum-property-best-practices": "error",
         "@darraghor/nestjs-typed/api-property-returning-array-should-set-array":
             "error",
+        "@darraghor/nestjs-typed/should-specify-forbid-unknown-values": "error",
     },
 };
diff --git a/src/rules/apiEnumPropertyBestPractices/apiEnumPropertyBestPractices.ts b/src/rules/apiEnumPropertyBestPractices/apiEnumPropertyBestPractices.ts
@@ -106,7 +106,6 @@ const rule = createRule({
             needsEnumNameAdded: `Properties with enum should also specify an enumName property to keep generated models clean`,
             needsTypeRemoved: `Properties with enum should not specify a type property`,
             enumNameShouldMatchType: `The enumName should match the enum type provided`,
-            randomTest: `FAIL: {{msgText}}`,
         },
         schema: [],
         hasSuggestions: false,
diff --git a/src/rules/index.ts b/src/rules/index.ts
@@ -5,6 +5,7 @@ import controllerDecoratedHasApiTags from "./controllerDecoratedHasApiTags/contr
 import apiMethodsShouldSpecifyApiResponse from "./apiMethodsShouldSpecifyApiResponse/apiMethodsShouldSpecifyApiResponse";
 import apiEnumPropertyBestPractices from "./apiEnumPropertyBestPractices/apiEnumPropertyBestPractices";
 import apiPropertyReturningArrayShouldSetArray from "./apiPropertyReturningArrayShouldSetArray/apiPropertyReturningArrayShouldSetArray";
+import shouldSpecifyForbidUnknownValues from "./shouldSpecifyForbidUnknownValues/shouldSpecifyForbidUnknownValuesRule";
 
 const allRules = {
     "api-property-matches-property-optionality":
@@ -14,10 +15,11 @@ const allRules = {
         providedInjectedShouldMatchFactoryParameters,
     "controllers-should-supply-api-tags": controllerDecoratedHasApiTags,
     "api-method-should-specify-api-response":
-    apiMethodsShouldSpecifyApiResponse,
+        apiMethodsShouldSpecifyApiResponse,
     "api-enum-property-best-practices": apiEnumPropertyBestPractices,
     "api-property-returning-array-should-set-array":
         apiPropertyReturningArrayShouldSetArray,
+    "should-specify-forbid-unknown-values": shouldSpecifyForbidUnknownValues,
 };
 
 export default allRules;
diff --git a/src/rules/shouldSpecifyForbidUnknownValues/rule.testData.ts b/src/rules/shouldSpecifyForbidUnknownValues/rule.testData.ts
@@ -0,0 +1,134 @@
+/* eslint-disable unicorn/filename-case */
+export const testCases = [
+    {
+        moduleCode: `
+                const options = {
+                    forbidNonWhitelisted: true,
+                    forbidUnknownValues: true,
+                } as ValidationPipeOptions;
+        
+                const validationPipeA = new ValidationPipe(options);
+        
+                const validationPipeB = new ValidationPipe({
+                    transform: true,
+                    skipMissingProperties: false,
+                    whitelist: true,
+                    forbidNonWhitelisted: true,
+                    forbidUnknownValues: true,
+                });   
+        `,
+        isNewExpressionTriggered: false,
+        isVariableExpressionTriggered: false,
+        message: "property is present in both scenarios",
+    },
+    {
+        moduleCode: `
+                const options = {
+                    forbidNonWhitelisted: true,
+                } as ValidationPipeOptions;
+        
+                const validationPipeA = new ValidationPipe(options);
+        
+                const validationPipeB = new ValidationPipe({
+                    transform: true,
+                    skipMissingProperties: false,
+                    whitelist: true,
+                    forbidNonWhitelisted: true,
+                    forbidUnknownValues: true,
+                });   
+        `,
+        isNewExpressionTriggered: false,
+        isVariableExpressionTriggered: true,
+        message: "property is missing in options object variable",
+    },
+    {
+        moduleCode: `
+                const options = {
+                    forbidNonWhitelisted: true,
+                } as ThisIsNotAValidationPipeOptionsClass;
+        
+                const validationPipeA = new ValidationPipe(options);
+        
+                const validationPipeB = new ValidationPipe({
+                    transform: true,
+                    skipMissingProperties: false,
+                    whitelist: true,
+                    forbidNonWhitelisted: true,
+                    forbidUnknownValues: true,
+                });   
+        `,
+        isNewExpressionTriggered: false,
+        isVariableExpressionTriggered: false,
+        message: "not a validation options class",
+    },
+    {
+        moduleCode: `
+                const options = {
+                    forbidNonWhitelisted: true,
+                    forbidUnknownValues: true,
+                } as ValidationPipeOptions;
+        
+                const validationPipeA = new ValidationPipe(options);
+        
+                const validationPipeB = new ValidationPipe({
+                    transform: true,
+                    skipMissingProperties: false,
+                    whitelist: true,
+                    forbidNonWhitelisted: true
+                });   
+        `,
+        isNewExpressionTriggered: true,
+        isVariableExpressionTriggered: false,
+        message: "property is missing in parameter declaration",
+    },
+    {
+        moduleCode: `
+                const options = {
+                    forbidNonWhitelisted: true,
+                    forbidUnknownValues: true,
+                } as ValidationPipeOptions;
+        
+                const validationPipeA = new ValidationPipe(options);
+        
+                const validationPipeB = new ThisIsNotAValidationPipeClass({
+                    transform: true,
+                    skipMissingProperties: false,
+                    whitelist: true,
+                    forbidNonWhitelisted: true
+                });   
+        `,
+        isNewExpressionTriggered: false,
+        isVariableExpressionTriggered: false,
+        message: "not the right class to trigger the rule",
+    },
+    {
+        moduleCode: `
+                const options = {
+                    forbidNonWhitelisted: true,
+                    forbidUnknownValues: true,
+                } as ValidationPipeOptions;
+        
+                const validationPipeA = new ValidationPipe(options);
+        
+                const validationPipeB = new ValidationPipe({});   
+        `,
+        isNewExpressionTriggered: true,
+        isVariableExpressionTriggered: false,
+        message: "empty options object should trigger",
+    },
+    {
+        moduleCode: `
+                const options = {
+                    forbidNonWhitelisted: true,
+                    forbidUnknownValues: true,
+                } as ValidationPipeOptions;
+        
+                const validationPipeA = new ValidationPipe(options);
+        
+                const validationPipeB = new ValidationPipe();   
+        `,
+        isNewExpressionTriggered: false,
+        isVariableExpressionTriggered: false,
+        message: "empty constructor should not trigger the rule",
+    },
+];
diff --git a/src/rules/shouldSpecifyForbidUnknownValues/shouldSpecifyForbidUnknownValues.test.ts b/src/rules/shouldSpecifyForbidUnknownValues/shouldSpecifyForbidUnknownValues.test.ts
@@ -0,0 +1,49 @@
+import {testCases} from "./rule.testData";
+import {typedTokenHelpers} from "../../utils/typedTokenHelpers";
+import {
+    fakeContext,
+    fakeFilePath,
+} from "../../utils/nestModules/nestProvidedInjectableMapper.testData";
+
+import {
+    shouldTriggerForVariableDecleratorExpression,
+    shouldTriggerNewExpressionHasProperty,
+} from "./shouldSpecifyForbidUnknownValuesRule";
+
+// should probably be split up into multiple tests
+describe("shouldUseForbidUnknownRule", () => {
+    test.each(testCases)(
+        "is an expected response for %#",
+        (testCase: {
+            moduleCode: string;
+            isNewExpressionTriggered: boolean;
+            isVariableExpressionTriggered: boolean;
+            message: string;
+        }) => {
+            const ast = typedTokenHelpers.parseStringToAst(
+                testCase.moduleCode,
+                fakeFilePath,
+                // eslint-disable-next-line @typescript-eslint/no-unsafe-argument
+                fakeContext
+            );
+
+            const isNewExpressionTriggered =
+                shouldTriggerNewExpressionHasProperty(
+                    // eslint-disable-next-line @typescript-eslint/no-unsafe-argument,@typescript-eslint/no-explicit-any
+                    (ast as any).body[2].declarations[0].init
+                );
+            const isVariableExpressionTriggered =
+                shouldTriggerForVariableDecleratorExpression(
+                    // eslint-disable-next-line @typescript-eslint/no-unsafe-argument,@typescript-eslint/no-explicit-any
+                    (ast as any).body[0].declarations[0]
+                );
+
+            expect(isNewExpressionTriggered).toEqual(
+                testCase.isNewExpressionTriggered
+            );
+            expect(isVariableExpressionTriggered).toEqual(
+                testCase.isVariableExpressionTriggered
+            );
+        }
+    );
+});
diff --git a/src/rules/shouldSpecifyForbidUnknownValues/shouldSpecifyForbidUnknownValuesRule.ts b/src/rules/shouldSpecifyForbidUnknownValues/shouldSpecifyForbidUnknownValuesRule.ts
