diff --git a/app/client/package.json b/app/client/package.json index a75c50f541..154420153c 100644 --- a/app/client/package.json +++ b/app/client/package.json @@ -131,6 +131,7 @@ "deep-diff": "^1.0.2", "downloadjs": "^1.4.7", "echarts": "^5.4.2", + "eslint-linter-browserify": "^9.15.0", "fast-deep-equal": "^3.1.3", "fast-sort": "^3.4.0", "fastdom": "^1.0.11", diff --git a/app/client/src/components/editorComponents/CodeEditor/lintHelpers.ts b/app/client/src/components/editorComponents/CodeEditor/lintHelpers.ts index 125e41ebaf..15a6be3690 100644 --- a/app/client/src/components/editorComponents/CodeEditor/lintHelpers.ts +++ b/app/client/src/components/editorComponents/CodeEditor/lintHelpers.ts @@ -200,8 +200,10 @@ export const getLintAnnotations = ( // Jshint counts \t as two characters and codemirror counts it as 1. // So we need to subtract number of tabs to get accurate position. // This is not needed for custom lint errors, since they are not generated by JSHint + // ESLint doesn't have this issue and hence we are skipping if lint is generated via eslint const tabs = - error.code && error.code in CUSTOM_LINT_ERRORS + (error.code && error.code in CUSTOM_LINT_ERRORS) || + (lintLength && lintLength > 0) ? 0 : lineContent.slice(0, currentCh).match(/\t/g)?.length || 0; const from = { diff --git a/app/client/src/plugins/Linting/constants.ts b/app/client/src/plugins/Linting/constants.ts index 492f104f17..433fb0a3b1 100644 --- a/app/client/src/plugins/Linting/constants.ts +++ b/app/client/src/plugins/Linting/constants.ts @@ -1,38 +1,85 @@ import { ECMA_VERSION } from "@shared/ast"; import type { LintOptions } from "jshint"; import isEntityFunction from "./utils/isEntityFunction"; +import type { Linter } from "eslint-linter-browserify"; export enum LINTER_TYPE { "JSHINT" = "JSHint", "ESLINT" = "ESLint", } -export const lintOptions = (globalData: Record) => - ({ - indent: 2, - esversion: ECMA_VERSION, - eqeqeq: false, // Not necessary to use === - curly: false, // Blocks can be added without {}, eg if (x) return true - freeze: true, // Overriding inbuilt classes like Array is not allowed - undef: true, // Undefined variables should be reported as error - forin: false, // Doesn't require filtering for..in loops with obj.hasOwnProperty() - noempty: false, // Empty blocks are allowed - strict: false, // We won't force strict mode - unused: "strict", // Unused variables are not allowed - asi: true, // Tolerate Automatic Semicolon Insertion (no semicolons) - boss: true, // Tolerate assignments where comparisons would be expected - evil: false, // Use of eval not allowed - funcscope: true, // Tolerate variable definition inside control statements - sub: true, // Don't force dot notation - expr: true, // suppresses warnings about the use of expressions where normally you would expect to see assignments or function calls - // environments - browser: false, - worker: true, - mocha: false, - // global values - globals: globalData, - loopfunc: true, - }) as LintOptions; +export const lintOptions = ( + globalData: Record, + linterType: LINTER_TYPE = LINTER_TYPE.JSHINT, +) => { + if (linterType === LINTER_TYPE.JSHINT) { + return { + indent: 2, + esversion: ECMA_VERSION, + eqeqeq: false, // Not necessary to use === + curly: false, // Blocks can be added without {}, eg if (x) return true + freeze: true, // Overriding inbuilt classes like Array is not allowed + undef: true, // Undefined variables should be reported as error + forin: false, // Doesn't require filtering for..in loops with obj.hasOwnProperty() + noempty: false, // Empty blocks are allowed + strict: false, // We won't force strict mode + unused: "strict", // Unused variables are not allowed + asi: true, // Tolerate Automatic Semicolon Insertion (no semicolons) + boss: true, // Tolerate assignments where comparisons would be expected + evil: false, // Use of eval not allowed + funcscope: true, // Tolerate variable definition inside control statements + sub: true, // Don't force dot notation + expr: true, // suppresses warnings about the use of expressions where normally you would expect to see assignments or function calls + // environments + browser: false, + worker: true, + mocha: false, + // global values + globals: globalData, + loopfunc: true, + } as LintOptions; + } else { + const eslintGlobals: Record = { + setTimeout: "readonly", + clearTimeout: "readonly", + console: "readonly", + }; + + for (const key in globalData) { + if (globalData.hasOwnProperty(key)) { + eslintGlobals[key] = "readonly"; + } + } + + return { + languageOptions: { + ecmaVersion: ECMA_VERSION, + globals: eslintGlobals, + sourceType: "script", + }, + rules: { + eqeqeq: "off", + curly: "off", + "no-extend-native": "error", + "no-undef": "error", + "guard-for-in": "off", + "no-empty": "off", + strict: "off", + "no-unused-vars": [ + "warn", + { vars: "all", args: "all", ignoreRestSiblings: false }, + ], + "no-cond-assign": "off", + "no-eval": "error", + "block-scoped-var": "off", + "dot-notation": "off", + "no-unused-expressions": "off", + "no-loop-func": "off", + }, + } as Linter.Config; + } +}; + export const JS_OBJECT_START_STATEMENT = "export default"; export const INVALID_JSOBJECT_START_STATEMENT = `JSObject must start with '${JS_OBJECT_START_STATEMENT}'`; export const INVALID_JSOBJECT_START_STATEMENT_ERROR_CODE = @@ -43,6 +90,7 @@ export const IDENTIFIER_NOT_DEFINED_LINT_ERROR_CODE = "W117"; // For these error types, we want to show a warning // All messages can be found here => https://github.com/jshint/jshint/blob/2.9.5/src/messages.js export const WARNING_LINT_ERRORS = { + "no-unused-vars": "'{a}' is assigned a value but never used.", W098: "'{a}' is defined but never used.", W014: "Misleading line break before '{a}'; readers may interpret this as an expression boundary.", ASYNC_FUNCTION_BOUND_TO_SYNC_FIELD: diff --git a/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts b/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts index c92385bfd5..a488e90dd4 100644 --- a/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts +++ b/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts @@ -1,199 +1,600 @@ import { getScriptType } from "workers/Evaluation/evaluate"; -import { CustomLintErrorCode } from "../constants"; +import { CustomLintErrorCode, LINTER_TYPE } from "../constants"; import getLintingErrors from "../utils/getLintingErrors"; +import { Severity } from "entities/AppsmithConsole"; const webworkerTelemetry = {}; -describe("getLintingErrors", () => { - describe("1. Verify lint errors are not shown for supported window APIs", () => { - const data = {}; +const linterTypes = [ + { linterType: LINTER_TYPE.JSHINT, name: "JSHint" }, + { linterType: LINTER_TYPE.ESLINT, name: "ESLint" }, +]; - it("1. For fetch API", () => { - const originalBinding = "{{fetch()}}"; - const script = "fetch()"; +describe.each(linterTypes)( + "getLintingErrors with engine $name", + ({ linterType }) => { + describe("1. Verify lint errors are not shown for supported window APIs", () => { + const data = {}; - const scriptType = getScriptType(false, true); + it("1. For fetch API", () => { + const originalBinding = "{{fetch()}}"; + const script = "fetch()"; - const lintErrors = getLintingErrors({ - data, - originalBinding, - script, - scriptType, - webworkerTelemetry, + const scriptType = getScriptType(false, true); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + script, + scriptType, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + expect(lintErrors.length).toEqual(0); }); + it("2. For setTimeout", () => { + const originalBinding = "{{setTimeout()}}"; + const script = "setTimeout()"; - expect(Array.isArray(lintErrors)).toBe(true); - expect(lintErrors.length).toEqual(0); - }); - it("2. For setTimeout", () => { - const originalBinding = "{{setTimeout()}}"; - const script = "setTimeout()"; + const scriptType = getScriptType(false, true); - const scriptType = getScriptType(false, true); + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + webworkerTelemetry, + data, + originalBinding, + script, + scriptType, + }); - const lintErrors = getLintingErrors({ - webworkerTelemetry, - data, - originalBinding, - script, - scriptType, + expect(Array.isArray(lintErrors)).toBe(true); + expect(lintErrors.length).toEqual(0); }); + it("3. For console", () => { + const originalBinding = "{{console.log()}}"; + const script = "console.log()"; - expect(Array.isArray(lintErrors)).toBe(true); - expect(lintErrors.length).toEqual(0); - }); - it("3. For console", () => { - const originalBinding = "{{console.log()}}"; - const script = "console.log()"; + const scriptType = getScriptType(false, true); - const scriptType = getScriptType(false, true); + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + webworkerTelemetry, + data, + originalBinding, + script, + scriptType, + }); - const lintErrors = getLintingErrors({ - webworkerTelemetry, - data, - originalBinding, - script, - scriptType, + expect(lintErrors.length).toEqual(0); }); - - expect(lintErrors.length).toEqual(0); }); - }); - describe("2. Verify lint errors are shown for unsupported window APIs", () => { - const data = {}; + describe("2. Verify lint errors are shown for unsupported window APIs", () => { + const data = {}; - it("1. For window", () => { - const originalBinding = "{{window}}"; - const script = "window"; + it("1. For window", () => { + const originalBinding = "{{window}}"; + const script = "window"; - const scriptType = getScriptType(false, true); + const scriptType = getScriptType(false, true); - const lintErrors = getLintingErrors({ - webworkerTelemetry, - data, - originalBinding, - script, - scriptType, + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + webworkerTelemetry, + data, + originalBinding, + script, + scriptType, + }); + + expect(lintErrors.length).toEqual(1); }); + it("2. For document", () => { + const originalBinding = "{{document}}"; + const script = "document"; - expect(lintErrors.length).toEqual(1); - }); - it("2. For document", () => { - const originalBinding = "{{document}}"; - const script = "document"; + const scriptType = getScriptType(false, true); - const scriptType = getScriptType(false, true); + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + webworkerTelemetry, + data, + originalBinding, + script, + scriptType, + }); - const lintErrors = getLintingErrors({ - webworkerTelemetry, - data, - originalBinding, - script, - scriptType, + expect(lintErrors.length).toEqual(1); }); + it("3. For dom", () => { + const originalBinding = "{{dom}}"; + const script = "dom"; - expect(lintErrors.length).toEqual(1); - }); - it("3. For dom", () => { - const originalBinding = "{{dom}}"; - const script = "dom"; + const scriptType = getScriptType(false, true); - const scriptType = getScriptType(false, true); + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + webworkerTelemetry, + data, + originalBinding, + script, + scriptType, + }); - const lintErrors = getLintingErrors({ - webworkerTelemetry, - data, - originalBinding, - script, - scriptType, + expect(lintErrors.length).toEqual(1); }); - - expect(lintErrors.length).toEqual(1); }); - }); - describe("3. Verify lint errors are shown when mutations are performed on unmutable objects", () => { - const data = { - THIS_CONTEXT: {}, - Input1: { - text: "inputValue", - ENTITY_TYPE: "WIDGET", - }, - appsmith: { - store: { - test: "testValue", + describe("3. Verify lint errors are shown when mutations are performed on unmutable objects", () => { + const data = { + THIS_CONTEXT: {}, + Input1: { + text: "inputValue", + ENTITY_TYPE: "WIDGET", }, - }, - }; + appsmith: { + store: { + test: "testValue", + }, + }, + }; - it("1. Assigning values to input widget's properties", () => { - const originalBinding = "'myFun1() {\n\t\tInput1.text = \"\";\n\t}'"; - const script = - '\n function $$closedFn () {\n const $$result = {myFun1() {\n\t\tInput1.text = "";\n\t}}\n return $$result\n }\n $$closedFn.call(THIS_CONTEXT)\n '; - const options = { isJsObject: true }; + it("1. Assigning values to input widget's properties", () => { + const originalBinding = "'myFun1() {\n\t\tInput1.text = \"\";\n\t}'"; + const script = + '\n function $$closedFn () {\n const $$result = {myFun1() {\n\t\tInput1.text = "";\n\t}}\n return $$result\n }\n $$closedFn.call(THIS_CONTEXT)\n '; + const options = { isJsObject: true }; - const scriptType = getScriptType(false, false); + const scriptType = getScriptType(false, false); - const lintErrors = getLintingErrors({ - webworkerTelemetry, - data, - options, - originalBinding, - script, - scriptType, + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + webworkerTelemetry, + data, + options, + originalBinding, + script, + scriptType, + }); + + expect(lintErrors.length).toEqual(1); + expect(lintErrors[0].code).toEqual( + CustomLintErrorCode.INVALID_ENTITY_PROPERTY, + ); }); - expect(lintErrors.length).toEqual(1); - expect(lintErrors[0].code).toEqual( - CustomLintErrorCode.INVALID_ENTITY_PROPERTY, - ); + it("2. Assigning values to appsmith store variables", () => { + const originalBinding = + 'myFun1() {\n\t\tappsmith.store.test = "";\n\t}'; + const script = + '\n function $$closedFn () {\n const $$result = {myFun1() {\n\t\tappsmith.store.test = "";\n\t}}\n return $$result\n }\n $$closedFn.call(THIS_CONTEXT)\n'; + const options = { isJsObject: true }; + + const scriptType = getScriptType(false, false); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + webworkerTelemetry, + data, + options, + originalBinding, + script, + scriptType, + }); + + expect(lintErrors.length).toEqual(1); + expect(lintErrors[0].code).toEqual( + CustomLintErrorCode.INVALID_APPSMITH_STORE_PROPERTY_SETTER, + ); + }); + it("3. Mutating appsmith store values by calling any functions on it", () => { + const originalBinding = + "myFun1() {\n\t\tappsmith.store.test.push(1);\n\t}"; + const script = + "\n function $$closedFn () {\n const $$result = {myFun1() {\n\t\tappsmith.store.test.push(1);\n\t}}\n return $$result\n }\n $$closedFn.call(THIS_CONTEXT)\n"; + const options = { isJsObject: true }; + + const scriptType = getScriptType(false, false); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + webworkerTelemetry, + data, + options, + originalBinding, + script, + scriptType, + }); + + expect(lintErrors.length).toEqual(1); + expect(lintErrors[0].code).toEqual( + CustomLintErrorCode.INVALID_APPSMITH_STORE_PROPERTY_SETTER, + ); + }); }); - it("2. Assigning values to appsmith store variables", () => { - const originalBinding = 'myFun1() {\n\t\tappsmith.store.test = "";\n\t}'; - const script = - '\n function $$closedFn () {\n const $$result = {myFun1() {\n\t\tappsmith.store.test = "";\n\t}}\n return $$result\n }\n $$closedFn.call(THIS_CONTEXT)\n'; - const options = { isJsObject: true }; + describe("4. Config rule tests", () => { + // Test for 'eqeqeq: false' (Allow '==' and '!=') + it("1. Should allow '==' and '!=' without errors", () => { + const data = { x: 5, y: "5" }; + const originalBinding = "{{ x == y }}"; + const script = "x == y"; - const scriptType = getScriptType(false, false); + const scriptType = getScriptType(false, true); - const lintErrors = getLintingErrors({ - webworkerTelemetry, - data, - options, - originalBinding, - script, - scriptType, + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + script, + scriptType, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + // Should have no errors for using '==' + expect(lintErrors.length).toEqual(0); }); - expect(lintErrors.length).toEqual(1); - expect(lintErrors[0].code).toEqual( - CustomLintErrorCode.INVALID_APPSMITH_STORE_PROPERTY_SETTER, - ); - }); - it("3. Mutating appsmith store values by calling any functions on it", () => { - const originalBinding = - "myFun1() {\n\t\tappsmith.store.test.push(1);\n\t}"; - const script = - "\n function $$closedFn () {\n const $$result = {myFun1() {\n\t\tappsmith.store.test.push(1);\n\t}}\n return $$result\n }\n $$closedFn.call(THIS_CONTEXT)\n"; - const options = { isJsObject: true }; + // Test for `curly: false` (Blocks can be added without {}, eg if (x) return true + it("2. Should allow blocks without brackets", () => { + const data = {}; + const originalBinding = "{{ if (true) console.log('ok') }}"; + const script = "if (true) console.log('ok')"; - const scriptType = getScriptType(false, false); + const scriptType = getScriptType(false, true); - const lintErrors = getLintingErrors({ - webworkerTelemetry, - data, - options, - originalBinding, - script, - scriptType, + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + script, + scriptType, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + // Should have no errors + expect(lintErrors.length).toEqual(0); }); - expect(lintErrors.length).toEqual(1); - expect(lintErrors[0].code).toEqual( - CustomLintErrorCode.INVALID_APPSMITH_STORE_PROPERTY_SETTER, - ); + // Test for 'freeze: true' (Do not allow mutations of native objects) + it("3. Should error when modifying native objects", () => { + const data = {}; + const originalBinding = "{{ Array.prototype.myFunc = function() {} }}"; + const script = "Array.prototype.myFunc = function() {}"; + + const scriptType = getScriptType(false, true); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + script, + scriptType, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + // Should have at least one error for modifying native objects + expect(lintErrors.length).toBe(1); + const lintError = lintErrors[0]; + const expectedErrorMessage = + linterType === LINTER_TYPE.JSHINT + ? "Extending prototype of native object: 'Array'." + : "Array prototype is read only, properties should not be added."; + + expect(lintError.severity).toBe(Severity.ERROR); + + expect(lintError.errorMessage.message).toContain(expectedErrorMessage); + }); + + // Test for 'undef: true' (Disallow use of undeclared variables) + it("4. Should error on use of undeclared variables", () => { + const data = {}; + const originalBinding = "{{ x + 1 }}"; + const script = "x + 1"; // 'x' is undeclared + + const scriptType = getScriptType(false, true); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + script, + scriptType, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + // Should have at least one error for 'x' being undefined + expect(lintErrors.length).toBe(1); + // Check if the error code corresponds to undefined variable + const lintError = lintErrors[0]; + const expectedErrorMessage = "'x' is not defined."; + + expect(lintError.severity).toBe(Severity.ERROR); + + expect(lintError.errorMessage.message).toContain(expectedErrorMessage); + }); + + // Test for 'forin: false' (Doesn't require filtering for..in loops with obj.hasOwnProperty()) + it("5. Should allow unflitered forin loops without error", () => { + const data = { obj: { a: 1, b: 2 } }; + const originalBinding = + "{{ for (var key in obj) { console.log(key); } }}"; + const script = "for (var key in obj) { console.log(key); }"; + + const scriptType = getScriptType(false, true); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + script, + scriptType, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + // Should have no errors for unfiltered 'for-in' loops + expect(lintErrors.length).toEqual(0); + }); + + // Test for 'noempty: false' (Allow empty blocks) + it("6. Should allow empty blocks without errors", () => { + const data = {}; + const originalBinding = "{{ if (true) { } }}"; + const script = "if (true) { }"; + + const scriptType = getScriptType(false, true); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + script, + scriptType, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + // Should have no errors + expect(lintErrors.length).toEqual(0); + }); + + // Test for 'strict: false' (strict mode is not enforced) + it("7. should allow blocks without strict mode enabled", () => { + const data = { + THIS_CONTEXT: {}, + }; + const originalBinding = "myFun1() {\n\t\tconsole.log('test');\n\t};"; + const script = + "\n function $$closedFn () {\n const $$result = {myFun1() {\n\t\tconsole.log('test');\n\t}};\n return $$result;\n }\n $$closedFn.call(THIS_CONTEXT);\n"; + const options = { isJsObject: true }; + + const scriptType = getScriptType(false, true); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + options, + script, + scriptType, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + // Should have no errors for missing 'use strict' + expect(lintErrors.length).toEqual(0); + }); + + // Test for 'unused: 'strict'' (if a variable is defined, it should be used) + it("8. should throw error for unused variables", () => { + const data = {}; + const originalBinding = "{{ const x = 1; }}"; + const script = "const x = 1;"; + + const scriptType = getScriptType(false, true); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + script, + scriptType, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + expect(lintErrors.length).toEqual(1); + const lintError = lintErrors[0]; + + const expectedMessage = + linterType === LINTER_TYPE.JSHINT + ? "'x' is defined but never used." + : "'x' is assigned a value but never used."; + + expect(lintError.severity).toBe(Severity.WARNING); + expect(lintError.errorMessage.message).toBe(expectedMessage); + }); + + // Test for 'unused: 'strict'' (if a variable is defined, it should be used) + it("9. should allow expressions without trailing semicolon", () => { + const data = { + THIS_CONTEXT: {}, + }; + const originalBinding = "myFun1() {\n\t\tconsole.log('test')\n\t}"; + const script = + "\n function $$closedFn () {\n const $$result = {myFun1() {\n\t\tconsole.log('test')\n\t}}\n return $$result\n }\n $$closedFn.call(THIS_CONTEXT)\n"; + const options = { isJsObject: true }; + + const scriptType = getScriptType(false, true); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + options, + script, + scriptType, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + // Should have no errors + expect(lintErrors.length).toEqual(0); + }); + + // Test for 'boss: true' (Allow assignments in conditions) + it("10. Should allow assignments in conditions without errors", () => { + const data = { a: 0, b: 1 }; + const originalBinding = "{{ if (a = b) { console.log(a); } }}"; + const script = "if (a = b) { console.log(a); }"; + + const scriptType = getScriptType(false, true); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + script, + scriptType, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + // Should have no errors for assignments in conditions + expect(lintErrors.length).toEqual(0); + }); + + // Test for 'evil: false' (Disallow use of eval) + it("11a. Should error when 'eval' is used", () => { + const data = {}; + const originalBinding = "{{ eval('var a = 1;') }}"; + const script = "eval('var a = 1;');"; + + const scriptType = getScriptType(false, true); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + script, + scriptType, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + expect(lintErrors.length).toEqual(1); + const lintError = lintErrors[0]; + + const expectedMessage = "eval can be harmful."; + + expect(lintError.severity).toBe(Severity.ERROR); + expect(lintError.errorMessage.message).toBe(expectedMessage); + }); + + // Test for 'evil: false' (Disallow use of eval) + it("11b. should error on indirect eval", () => { + const data = {}; + const originalBinding = "{{ (0, eval)('var a = 1;') }}"; + const script = "(0, eval)('var a = 1;');"; + + const scriptType = getScriptType(false, true); + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + script, + scriptType, + webworkerTelemetry, + }); + + const expectedErrorMessage = + linterType === LINTER_TYPE.JSHINT + ? "Unorthodox function invocation." + : "eval can be harmful."; + + expect(lintErrors.length).toEqual(1); + expect(lintErrors[0].severity).toBe(Severity.ERROR); + expect(lintErrors[0].errorMessage.message).toBe(expectedErrorMessage); + }); + + // Test for 'funcscope: true' (Allow variable definitions inside control statements) + it("12. Should allow variable definitions inside control statements without errors", () => { + const data = { + THIS_CONTEXT: {}, + }; + const originalBinding = + "myFun1() {\n\t\tif (true) { var x = 1; console.log(x); } x+=1;\n\t};"; + const script = + "\n function $$closedFn () {\n const $$result = {myFun1() {\n\t\tif (true) { var x = 1; console.log(x); } x+=1;\n\t}};\n return $$result;\n }\n $$closedFn.call(THIS_CONTEXT);\n"; + const options = { isJsObject: true }; + + const scriptType = getScriptType(false, true); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + script, + scriptType, + options, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + // Should have no errors + expect(lintErrors.length).toEqual(0); + }); + + // Test for 'sub: true' (Allow all property accessors) + it("13. Should allow bracket notation property access without errors", () => { + const data = { obj: { property: 1 } }; + const originalBinding = "{{ obj['property']; }}"; + const script = "obj['property'];"; + + const scriptType = getScriptType(false, true); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + script, + scriptType, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + // Should have no errors for using bracket notation + expect(lintErrors.length).toEqual(0); + }); + + // Test for 'expr: true' (Allow expressions where statements are expected) + it("14. Should allow expressions as statements without errors", () => { + const data = { a: true, b: false }; + const originalBinding = "{{ a || b; }}"; + const script = "a || b;"; + + const scriptType = getScriptType(false, true); + + const lintErrors = getLintingErrors({ + getLinterTypeFn: () => linterType, + data, + originalBinding, + script, + scriptType, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + // Should have no errors for expressions used as statements + expect(lintErrors.length).toEqual(0); + }); }); - }); -}); + }, +); diff --git a/app/client/src/plugins/Linting/types.ts b/app/client/src/plugins/Linting/types.ts index a0de1b5a39..8c8a77443c 100644 --- a/app/client/src/plugins/Linting/types.ts +++ b/app/client/src/plugins/Linting/types.ts @@ -13,6 +13,7 @@ import type { TJSPropertiesState } from "workers/Evaluation/JSObject/jsPropertie import type { JSLibrary } from "workers/common/JSLibrary"; import type { WebworkerSpanData } from "UITelemetry/generateWebWorkerTraces"; import type { SpanAttributes } from "UITelemetry/generateTraces"; +import type { LINTER_TYPE } from "./constants"; export type WebworkerTelemetryAttribute = WebworkerSpanData | SpanAttributes; @@ -69,6 +70,7 @@ export interface getLintingErrorsProps { isJsObject: boolean; }; webworkerTelemetry: Record; + getLinterTypeFn?: () => LINTER_TYPE; } export interface getLintErrorsFromTreeProps { diff --git a/app/client/src/plugins/Linting/utils/getLintingErrors.ts b/app/client/src/plugins/Linting/utils/getLintingErrors.ts index 9a01b0b8de..ec13f42f82 100644 --- a/app/client/src/plugins/Linting/utils/getLintingErrors.ts +++ b/app/client/src/plugins/Linting/utils/getLintingErrors.ts @@ -41,6 +41,7 @@ import { objectKeys } from "@appsmith/utils"; import { profileFn } from "UITelemetry/generateWebWorkerTraces"; import { WorkerEnv } from "workers/Evaluation/handlers/workerEnv"; import { FEATURE_FLAG } from "ee/entities/FeatureFlag"; +import { Linter } from "eslint-linter-browserify"; const EvaluationScriptPositions: Record = {}; @@ -91,6 +92,60 @@ function generateLintingGlobalData(data: Record) { return globalData; } +function sanitizeESLintErrors( + lintErrors: Linter.LintMessage[], + scriptPos: Position, +): Linter.LintMessage[] { + return lintErrors.reduce((result: Linter.LintMessage[], lintError) => { + // Ignored errors should not be reported + if (IGNORED_LINT_ERRORS.includes(lintError.ruleId || "")) return result; + + /** Some error messages reference line numbers, + * Eg. Expected '{a}' to match '{b}' from line {c} and instead saw '{d}' + * these line numbers need to be re-calculated based on the binding location. + * Errors referencing line numbers outside the user's script should also be ignored + * */ + let message = lintError.message; + const matchedLines = message.match(/line \d/gi); + const lineNumbersInErrorMessage = new Set(); + let isInvalidErrorMessage = false; + + if (matchedLines) { + matchedLines.forEach((lineStatement) => { + const digitString = lineStatement.split(" ")[1]; + const digit = Number(digitString); + + if (isNumber(digit)) { + if (digit < scriptPos.line) { + // referenced line number is outside the scope of user's script + isInvalidErrorMessage = true; + } else { + lineNumbersInErrorMessage.add(digit); + } + } + }); + } + + if (isInvalidErrorMessage) return result; + + if (lineNumbersInErrorMessage.size) { + Array.from(lineNumbersInErrorMessage).forEach((lineNumber) => { + message = message.replaceAll( + `line ${lineNumber}`, + `line ${lineNumber - scriptPos.line + 1}`, + ); + }); + } + + result.push({ + ...lintError, + message, + }); + + return result; + }, []); +} + function sanitizeJSHintErrors( lintErrors: JSHintError[], scriptPos: Position, @@ -161,6 +216,40 @@ const getLintErrorMessage = ( } }; +function convertESLintErrorToAppsmithLintError( + eslintError: Linter.LintMessage, + script: string, + originalBinding: string, + scriptPos: Position, +): LintError { + const { column, endColumn = 0, line, message, ruleId } = eslintError; + + // Compute actual error position + const actualErrorLineNumber = line - scriptPos.line; + const actualErrorCh = + line === scriptPos.line + ? eslintError.column - scriptPos.ch + : eslintError.column; + + return { + errorType: PropertyEvaluationErrorType.LINT, + raw: script, + severity: getLintSeverity(ruleId || "", message), + errorMessage: { + name: "LintingError", + message: message, + }, + errorSegment: "", + originalBinding, + // By keeping track of these variables we can highlight the exact text that caused the error. + variables: [], + lintLength: Math.max(endColumn - column, 0), + code: ruleId || "", + line: actualErrorLineNumber, + ch: actualErrorCh, + }; +} + function convertJsHintErrorToAppsmithLintError( jsHintError: JSHintError, script: string, @@ -203,16 +292,21 @@ function convertJsHintErrorToAppsmithLintError( export default function getLintingErrors({ data, + // this is added to help with tests, once jshint is removed, this param can be removed + getLinterTypeFn = getLinterType, options, originalBinding, script, scriptType, webworkerTelemetry, }: getLintingErrorsProps): LintError[] { - const linterType = getLinterType(); + const linterType = getLinterTypeFn(); const scriptPos = getEvaluationScriptPosition(scriptType); const lintingGlobalData = generateLintingGlobalData(data); - const lintingOptions = lintOptions(lintingGlobalData); + const lintingOptions = lintOptions(lintingGlobalData, linterType); + + let messages: Linter.LintMessage[] = []; + let lintErrors: LintError[] = []; profileFn( "Linter", @@ -223,18 +317,45 @@ export default function getLintingErrors({ codeSizeInChars: originalBinding.length, }, webworkerTelemetry, - () => jshint(script, lintingOptions), + () => { + if (linterType === LINTER_TYPE.JSHINT) { + jshint(script, lintingOptions); + } else if (linterType === LINTER_TYPE.ESLINT) { + const linter = new Linter(); + + messages = linter.verify(script, lintingOptions); + } + }, ); - const sanitizedJSHintErrors = sanitizeJSHintErrors(jshint.errors, scriptPos); - const jshintErrors: LintError[] = sanitizedJSHintErrors.map((lintError) => - convertJsHintErrorToAppsmithLintError( - lintError, - script, - originalBinding, + + if (linterType === LINTER_TYPE.JSHINT) { + const sanitizedJSHintErrors = sanitizeJSHintErrors( + jshint.errors, scriptPos, - options?.isJsObject, - ), - ); + ); + + lintErrors = sanitizedJSHintErrors.map((lintError) => + convertJsHintErrorToAppsmithLintError( + lintError, + script, + originalBinding, + scriptPos, + options?.isJsObject, + ), + ); + } else { + const sanitizedESLintErrors = sanitizeESLintErrors(messages, scriptPos); + + lintErrors = sanitizedESLintErrors.map((lintError) => + convertESLintErrorToAppsmithLintError( + lintError, + script, + originalBinding, + scriptPos, + ), + ); + } + const customLintErrors = getCustomErrorsFromScript( script, data, @@ -243,7 +364,7 @@ export default function getLintingErrors({ options?.isJsObject, ); - return jshintErrors.concat(customLintErrors); + return lintErrors.concat(customLintErrors); } function getInvalidWidgetPropertySetterErrors({ diff --git a/app/client/test/setup.ts b/app/client/test/setup.ts index c7f3e4009f..31f5bfefc8 100644 --- a/app/client/test/setup.ts +++ b/app/client/test/setup.ts @@ -24,6 +24,14 @@ jest.mock("../src/api/Api.ts", () => ({ default: class Api { }, })); +// Polyfill for `structuredClone` if not available +// This is needed for eslint jest tests +if (typeof global.structuredClone === "undefined") { + global.structuredClone = (obj) => { + return JSON.parse(JSON.stringify(obj)); + }; +} + beforeAll(() => { window.IntersectionObserver = jest .fn() diff --git a/app/client/yarn.lock b/app/client/yarn.lock index abcbd21fa8..8ff2894d55 100644 --- a/app/client/yarn.lock +++ b/app/client/yarn.lock @@ -12999,6 +12999,7 @@ __metadata: eslint: ^8.51.0 eslint-config-prettier: ^9.0.0 eslint-import-resolver-babel-module: ^5.3.2 + eslint-linter-browserify: ^9.15.0 eslint-plugin-cypress: ^2.15.1 eslint-plugin-jest: ^27.4.2 eslint-plugin-prettier: ^5.0.0 @@ -13397,15 +13398,14 @@ __metadata: languageName: node linkType: hard -"asn1.js@npm:^5.2.0": - version: 5.4.1 - resolution: "asn1.js@npm:5.4.1" +"asn1.js@npm:^4.10.1": + version: 4.10.1 + resolution: "asn1.js@npm:4.10.1" dependencies: bn.js: ^4.0.0 inherits: ^2.0.1 minimalistic-assert: ^1.0.0 - safer-buffer: ^2.1.0 - checksum: 3786a101ac6f304bd4e9a7df79549a7561950a13d4bcaec0c7790d44c80d147c1a94ba3d4e663673406064642a40b23fcd6c82a9952468e386c1a1376d747f9a + checksum: 9289a1a55401238755e3142511d7b8f6fc32f08c86ff68bd7100da8b6c186179dd6b14234fba2f7f6099afcd6758a816708485efe44bc5b2a6ec87d9ceeddbb5 languageName: node linkType: hard @@ -14226,7 +14226,7 @@ __metadata: languageName: node linkType: hard -"browserify-aes@npm:^1.0.0, browserify-aes@npm:^1.0.4": +"browserify-aes@npm:^1.0.4, browserify-aes@npm:^1.2.0": version: 1.2.0 resolution: "browserify-aes@npm:1.2.0" dependencies: @@ -14240,7 +14240,7 @@ __metadata: languageName: node linkType: hard -"browserify-cipher@npm:^1.0.0": +"browserify-cipher@npm:^1.0.1": version: 1.0.1 resolution: "browserify-cipher@npm:1.0.1" dependencies: @@ -14273,20 +14273,21 @@ __metadata: languageName: node linkType: hard -"browserify-sign@npm:^4.0.0": - version: 4.2.2 - resolution: "browserify-sign@npm:4.2.2" +"browserify-sign@npm:^4.2.3": + version: 4.2.3 + resolution: "browserify-sign@npm:4.2.3" dependencies: bn.js: ^5.2.1 browserify-rsa: ^4.1.0 create-hash: ^1.2.0 create-hmac: ^1.1.7 - elliptic: ^6.5.4 + elliptic: ^6.5.5 + hash-base: ~3.0 inherits: ^2.0.4 - parse-asn1: ^5.1.6 - readable-stream: ^3.6.2 + parse-asn1: ^5.1.7 + readable-stream: ^2.3.8 safe-buffer: ^5.2.1 - checksum: b622730c0fc183328c3a1c9fdaaaa5118821ed6822b266fa6b0375db7e20061ebec87301d61931d79b9da9a96ada1cab317fce3c68f233e5e93ed02dbb35544c + checksum: 403a8061d229ae31266670345b4a7c00051266761d2c9bbeb68b1a9bcb05f68143b16110cf23a171a5d6716396a1f41296282b3e73eeec0a1871c77f0ff4ee6b languageName: node linkType: hard @@ -15733,7 +15734,7 @@ __metadata: languageName: node linkType: hard -"create-ecdh@npm:^4.0.0": +"create-ecdh@npm:^4.0.4": version: 4.0.4 resolution: "create-ecdh@npm:4.0.4" dependencies: @@ -15756,7 +15757,7 @@ __metadata: languageName: node linkType: hard -"create-hmac@npm:^1.1.0, create-hmac@npm:^1.1.4, create-hmac@npm:^1.1.7": +"create-hmac@npm:^1.1.4, create-hmac@npm:^1.1.7": version: 1.1.7 resolution: "create-hmac@npm:1.1.7" dependencies: @@ -15822,21 +15823,22 @@ __metadata: linkType: hard "crypto-browserify@npm:^3.0.0": - version: 3.12.0 - resolution: "crypto-browserify@npm:3.12.0" + version: 3.12.1 + resolution: "crypto-browserify@npm:3.12.1" dependencies: - browserify-cipher: ^1.0.0 - browserify-sign: ^4.0.0 - create-ecdh: ^4.0.0 - create-hash: ^1.1.0 - create-hmac: ^1.1.0 - diffie-hellman: ^5.0.0 - inherits: ^2.0.1 - pbkdf2: ^3.0.3 - public-encrypt: ^4.0.0 - randombytes: ^2.0.0 - randomfill: ^1.0.3 - checksum: c1609af82605474262f3eaa07daa0b2140026bd264ab316d4bf1170272570dbe02f0c49e29407fe0d3634f96c507c27a19a6765fb856fed854a625f9d15618e2 + browserify-cipher: ^1.0.1 + browserify-sign: ^4.2.3 + create-ecdh: ^4.0.4 + create-hash: ^1.2.0 + create-hmac: ^1.1.7 + diffie-hellman: ^5.0.3 + hash-base: ~3.0.4 + inherits: ^2.0.4 + pbkdf2: ^3.1.2 + public-encrypt: ^4.0.3 + randombytes: ^2.1.0 + randomfill: ^1.0.4 + checksum: 4e643dd5acfff80fbe2cc567feb75a22d726cc4df34772c988f326976c3c1ee1f8a611a33498dab11568cff3e134f0bd44a0e1f4c216585e5877ab5327cdb6fc languageName: node linkType: hard @@ -16997,7 +16999,7 @@ __metadata: languageName: node linkType: hard -"diffie-hellman@npm:^5.0.0": +"diffie-hellman@npm:^5.0.3": version: 5.0.3 resolution: "diffie-hellman@npm:5.0.3" dependencies: @@ -17429,9 +17431,9 @@ __metadata: languageName: node linkType: hard -"elliptic@npm:^6.5.3, elliptic@npm:^6.5.4": - version: 6.6.0 - resolution: "elliptic@npm:6.6.0" +"elliptic@npm:^6.5.3, elliptic@npm:^6.5.5": + version: 6.6.1 + resolution: "elliptic@npm:6.6.1" dependencies: bn.js: ^4.11.9 brorand: ^1.1.0 @@ -17440,7 +17442,7 @@ __metadata: inherits: ^2.0.4 minimalistic-assert: ^1.0.1 minimalistic-crypto-utils: ^1.0.1 - checksum: e912349b883e694bfe65005214237a470c9a098a6ba36fd24396d0ab07feb399920c0738aeed1aed6cf5dca9c64fd479e212faed3a75c9d81453671ef0de5157 + checksum: 27b14a52f68bbbc0720da259f712cb73e953f6d2047958cd02fb0d0ade2e83849dc39fb4af630889c67df8817e24237428cf59c4f4c07700f755b401149a7375 languageName: node linkType: hard @@ -18063,6 +18065,13 @@ __metadata: languageName: node linkType: hard +"eslint-linter-browserify@npm:^9.15.0": + version: 9.15.0 + resolution: "eslint-linter-browserify@npm:9.15.0" + checksum: 8aabba7419f8777c10e7e02175047a840f3bfd14c8aa9d9fcec781b0fb1ba108c9e7d0fa20c8158f943881f24696a99cb8aac343a951e29c31c1ce4bdbea9935 + languageName: node + linkType: hard + "eslint-module-utils@npm:^2.8.0": version: 2.8.0 resolution: "eslint-module-utils@npm:2.8.0" @@ -20263,6 +20272,16 @@ __metadata: languageName: node linkType: hard +"hash-base@npm:~3.0, hash-base@npm:~3.0.4": + version: 3.0.4 + resolution: "hash-base@npm:3.0.4" + dependencies: + inherits: ^2.0.1 + safe-buffer: ^5.0.1 + checksum: 878465a0dfcc33cce195c2804135352c590d6d10980adc91a9005fd377e77f2011256c2b7cfce472e3f2e92d561d1bf3228d2da06348a9017ce9a258b3b49764 + languageName: node + linkType: hard + "hash.js@npm:^1.0.0, hash.js@npm:^1.0.3": version: 1.1.7 resolution: "hash.js@npm:1.1.7" @@ -26366,16 +26385,17 @@ __metadata: languageName: node linkType: hard -"parse-asn1@npm:^5.0.0, parse-asn1@npm:^5.1.6": - version: 5.1.6 - resolution: "parse-asn1@npm:5.1.6" +"parse-asn1@npm:^5.0.0, parse-asn1@npm:^5.1.7": + version: 5.1.7 + resolution: "parse-asn1@npm:5.1.7" dependencies: - asn1.js: ^5.2.0 - browserify-aes: ^1.0.0 - evp_bytestokey: ^1.0.0 - pbkdf2: ^3.0.3 - safe-buffer: ^5.1.1 - checksum: 9243311d1f88089bc9f2158972aa38d1abd5452f7b7cabf84954ed766048fe574d434d82c6f5a39b988683e96fb84cd933071dda38927e03469dc8c8d14463c7 + asn1.js: ^4.10.1 + browserify-aes: ^1.2.0 + evp_bytestokey: ^1.0.3 + hash-base: ~3.0 + pbkdf2: ^3.1.2 + safe-buffer: ^5.2.1 + checksum: 93c7194c1ed63a13e0b212d854b5213ad1aca0ace41c66b311e97cca0519cf9240f79435a0306a3b412c257f0ea3f1953fd0d9549419a0952c9e995ab361fd6c languageName: node linkType: hard @@ -26622,7 +26642,7 @@ __metadata: languageName: node linkType: hard -"pbkdf2@npm:^3.0.3": +"pbkdf2@npm:^3.1.2": version: 3.1.2 resolution: "pbkdf2@npm:3.1.2" dependencies: @@ -28133,7 +28153,7 @@ __metadata: languageName: node linkType: hard -"public-encrypt@npm:^4.0.0": +"public-encrypt@npm:^4.0.3": version: 4.0.3 resolution: "public-encrypt@npm:4.0.3" dependencies: @@ -28296,7 +28316,7 @@ __metadata: languageName: node linkType: hard -"randomfill@npm:^1.0.3": +"randomfill@npm:^1.0.4": version: 1.0.4 resolution: "randomfill@npm:1.0.4" dependencies: @@ -29667,7 +29687,7 @@ __metadata: languageName: node linkType: hard -"readable-stream@npm:3, readable-stream@npm:^3.0.6, readable-stream@npm:^3.1.1, readable-stream@npm:^3.4.0, readable-stream@npm:^3.5.0, readable-stream@npm:^3.6.0, readable-stream@npm:^3.6.2": +"readable-stream@npm:3, readable-stream@npm:^3.0.6, readable-stream@npm:^3.1.1, readable-stream@npm:^3.4.0, readable-stream@npm:^3.5.0, readable-stream@npm:^3.6.0": version: 3.6.2 resolution: "readable-stream@npm:3.6.2" dependencies: @@ -29678,7 +29698,7 @@ __metadata: languageName: node linkType: hard -"readable-stream@npm:^2.0.1, readable-stream@npm:^2.0.2, readable-stream@npm:^2.0.6, readable-stream@npm:^2.2.2, readable-stream@npm:~2.3.6": +"readable-stream@npm:^2.0.1, readable-stream@npm:^2.0.2, readable-stream@npm:^2.0.6, readable-stream@npm:^2.2.2, readable-stream@npm:^2.3.8, readable-stream@npm:~2.3.6": version: 2.3.8 resolution: "readable-stream@npm:2.3.8" dependencies: