diff --git a/its/ruling/src/test/expected/custom-jsts/javascript-S1451.json b/its/ruling/src/test/expected/custom-jsts/javascript-S1451.json index ff67a6631be..76f7b8467e3 100644 --- a/its/ruling/src/test/expected/custom-jsts/javascript-S1451.json +++ b/its/ruling/src/test/expected/custom-jsts/javascript-S1451.json @@ -215,6 +215,9 @@ "custom-jsts:S9114.js": [ 0 ], +"custom-jsts:S9135.js": [ +0 +], "custom-jsts:boundOrAssignedEvalOrArguments.js": [ 0 ] diff --git a/its/ruling/src/test/expected/custom-jsts/javascript-S9135.json b/its/ruling/src/test/expected/custom-jsts/javascript-S9135.json new file mode 100644 index 00000000000..bc80b600bbb --- /dev/null +++ b/its/ruling/src/test/expected/custom-jsts/javascript-S9135.json @@ -0,0 +1,6 @@ +{ +"custom-jsts:S9135.js": [ +24, +27 +] +} diff --git a/its/sources/custom/jsts/S9135.js b/its/sources/custom/jsts/S9135.js new file mode 100644 index 00000000000..6b7c600146a --- /dev/null +++ b/its/sources/custom/jsts/S9135.js @@ -0,0 +1,33 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute this program and/or modify it under the terms of + * the Sonar Source-Available License as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE, either express or implied. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +import lodash from 'lodash'; +import underscore from 'underscore'; + +const user = { address: { city: 'Bern' } }; +const address = user.address; + +const lodashCopy = lodash.clone(user); +lodashCopy.address.city = 'Geneva'; + +const underscoreCopy = underscore.clone(user); +underscoreCopy.address.city = 'Geneva'; + +const deepCopy = structuredClone(user); +deepCopy.address.city = 'Geneva'; + +const topLevelCopy = lodash.clone(user); +topLevelCopy.address = address; diff --git a/packages/analysis/src/jsts/rules/S9135/index.ts b/packages/analysis/src/jsts/rules/S9135/index.ts new file mode 100644 index 00000000000..6abe8e6e2e6 --- /dev/null +++ b/packages/analysis/src/jsts/rules/S9135/index.ts @@ -0,0 +1,17 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +export { rule } from './rule.js'; diff --git a/packages/analysis/src/jsts/rules/S9135/meta.ts b/packages/analysis/src/jsts/rules/S9135/meta.ts new file mode 100644 index 00000000000..10ff2e0edee --- /dev/null +++ b/packages/analysis/src/jsts/rules/S9135/meta.ts @@ -0,0 +1,19 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +export const implementation = 'original'; +export const eslintId = 'avoid-mutating-nested-properties-of-shallow-clones'; +export const hasSecondaries = true; diff --git a/packages/analysis/src/jsts/rules/S9135/rule.ts b/packages/analysis/src/jsts/rules/S9135/rule.ts new file mode 100644 index 00000000000..942061d1a03 --- /dev/null +++ b/packages/analysis/src/jsts/rules/S9135/rule.ts @@ -0,0 +1,220 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +// https://sonarsource.github.io/rspec/#/rspec/S9135/javascript + +import type { Rule } from 'eslint'; +import type estree from 'estree'; +import { + getUniqueWriteReference, + getVariableFromName, + unwrapTypeScriptExpression, +} from '../helpers/ast.js'; +import { getParent } from '../helpers/ancestor.js'; +import { report, toSecondaryLocation } from '../helpers/location.js'; +import { getFullyQualifiedName } from '../helpers/module.js'; +import { generateMeta } from '../helpers/generate-meta.js'; +import * as meta from './generated-meta.js'; + +const LODASH_MESSAGE = + 'Mutating a nested property of this shallow clone changes the original value; use structuredClone() or _.cloneDeep() when isolation is required.'; +const UNDERSCORE_MESSAGE = + 'Mutating a nested property of this shallow clone changes the original value; use structuredClone() when isolation is required.'; +const SECONDARY_MESSAGE = 'Shallow clone created here.'; +const NOSONAR_COMMENT = ' // NOSONAR: shared nested state is intentional'; + +const CLONE_LIBRARIES = new Map([ + ['lodash.clone', 'lodash'], + ['lodash-es.clone', 'lodash'], + ['underscore.clone', 'underscore'], +]); + +type MutationNode = estree.AssignmentExpression | estree.UpdateExpression | estree.UnaryExpression; + +type StaticMemberChain = { + root: estree.Identifier; + depth: number; +}; + +export const rule: Rule.RuleModule = { + meta: generateMeta(meta, { hasSuggestions: true }), + create(context: Rule.RuleContext) { + return { + AssignmentExpression: (node: estree.Node): void => { + const assignment = node as estree.AssignmentExpression; + checkMutation(context, assignment.left, assignment); + }, + UpdateExpression: (node: estree.Node): void => { + const update = node as estree.UpdateExpression; + checkMutation(context, update.argument, update); + }, + UnaryExpression: (node: estree.Node): void => { + const unary = node as estree.UnaryExpression; + if (unary.operator === 'delete') { + checkMutation(context, unary.argument, unary); + } + }, + }; + }, +}; + +function checkMutation( + context: Rule.RuleContext, + mutatedNode: estree.Node, + mutation: MutationNode, +): void { + const statement = getMutationStatement(context, mutation); + if (statement === undefined) { + return; + } + + const memberChain = getStaticMemberChain(mutatedNode); + if (memberChain === undefined) { + return; + } + + const cloneCall = getCloneCall(context, memberChain.root); + if (cloneCall === undefined) { + return; + } + + const library = getCloneLibrary(context, cloneCall); + if (library === undefined) { + return; + } + + const argument = cloneCall.arguments[0]; + if (argument === undefined || argument.type === 'SpreadElement') { + return; + } + + const nosonarFix = getNosonarFix(context, statement, mutation); + report( + context, + { + node: mutation, + message: library === 'underscore' ? UNDERSCORE_MESSAGE : LODASH_MESSAGE, + suggest: [ + { + desc: 'Replace the shallow clone with structuredClone()', + fix: replaceWithStructuredClone(cloneCall, argument, context), + }, + ...(nosonarFix === undefined + ? [] + : [ + { + desc: 'Add // NOSONAR: shared nested state is intentional', + fix: nosonarFix, + }, + ]), + ], + }, + [toSecondaryLocation(cloneCall, SECONDARY_MESSAGE)], + ); +} + +function getMutationStatement( + context: Rule.RuleContext, + node: MutationNode, +): estree.Node | undefined { + let parent = getParent(context, node); + while (parent !== undefined) { + if (parent.type.endsWith('Statement') || parent.type === 'VariableDeclaration') { + return parent; + } + parent = getParent(context, parent); + } + return undefined; +} + +function getStaticMemberChain(node: estree.Node): StaticMemberChain | undefined { + let current: estree.Node = unwrapTypeScriptExpression(node); + let depth = 0; + + while (current.type === 'MemberExpression') { + if (!isStaticMember(current)) { + return undefined; + } + depth += 1; + current = unwrapTypeScriptExpression(current.object); + } + + return current.type === 'Identifier' && depth >= 2 ? { root: current, depth } : undefined; +} + +function isStaticMember(member: estree.MemberExpression): boolean { + if (!member.computed) { + return member.property.type === 'Identifier'; + } + return ( + member.property.type === 'Literal' && + (typeof member.property.value === 'string' || typeof member.property.value === 'number') + ); +} + +function getCloneCall( + context: Rule.RuleContext, + root: estree.Identifier, +): estree.CallExpression | undefined { + const variable = getVariableFromName(context, root.name, root); + const writeExpression = getUniqueWriteReference(variable); + if (writeExpression?.type !== 'CallExpression' || writeExpression.arguments.length !== 1) { + return undefined; + } + return writeExpression; +} + +function getCloneLibrary( + context: Rule.RuleContext, + cloneCall: estree.CallExpression, +): 'lodash' | 'underscore' | undefined { + return CLONE_LIBRARIES.get(getFullyQualifiedName(context, cloneCall.callee) ?? ''); +} + +function replaceWithStructuredClone( + cloneCall: estree.CallExpression, + argument: estree.Node, + context: Rule.RuleContext, +): Rule.ReportFixer { + const argumentText = context.sourceCode.getText(argument); + return (fixer: Rule.RuleFixer): Rule.Fix => + fixer.replaceText(cloneCall, `structuredClone(${argumentText})`); +} + +function getNosonarFix( + context: Rule.RuleContext, + statement: estree.Node, + mutation: MutationNode, +): Rule.ReportFixer | undefined { + if ( + statement.loc?.start.line !== mutation.loc?.start.line || + statement.loc?.end.line !== mutation.loc?.start.line || + !hasNoTokensAfter(context, statement) + ) { + return undefined; + } + return (fixer: Rule.RuleFixer): Rule.Fix => fixer.insertTextAfter(statement, NOSONAR_COMMENT); +} + +function hasNoTokensAfter(context: Rule.RuleContext, node: estree.Node): boolean { + const range = node.range; + if (range === undefined) { + return false; + } + const source = context.sourceCode.getText(); + const lineEnd = source.indexOf('\n', range[1]); + return source.slice(range[1], lineEnd === -1 ? source.length : lineEnd).trim() === ''; +} diff --git a/packages/analysis/src/jsts/rules/S9135/unit.test.ts b/packages/analysis/src/jsts/rules/S9135/unit.test.ts new file mode 100644 index 00000000000..becf12c7a35 --- /dev/null +++ b/packages/analysis/src/jsts/rules/S9135/unit.test.ts @@ -0,0 +1,511 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +import { describe, it } from 'node:test'; +import { DefaultParserRuleTester } from '../../../../tests/jsts/tools/testers/rule-tester.js'; +import { RuleTester as ESLintRuleTester } from 'eslint'; +import tsParser from '@typescript-eslint/parser'; +import { rule } from './rule.js'; + +const LODASH_MESSAGE = + 'Mutating a nested property of this shallow clone changes the original value; use structuredClone() or _.cloneDeep() when isolation is required.'; +const UNDERSCORE_MESSAGE = + 'Mutating a nested property of this shallow clone changes the original value; use structuredClone() when isolation is required.'; + +describe('S9135', () => { + it('reports nested mutations of shallow clones and offers both suggestions', () => { + const ruleTester = new DefaultParserRuleTester(); + + ruleTester.run('avoid-mutating-nested-properties-of-shallow-clones', rule, { + valid: [ + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy.address.city; +`, + }, + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +const alias = copy; +alias.address.city = 'Geneva'; +`, + }, + { + code: ` +import _ from 'lodash'; +let copy = _.clone(user); +copy = other; +copy.address.city = 'Geneva'; +`, + }, + { + code: ` +import _ from 'lodash'; +let copy = _.clone(user); +copy.address.city = 'Geneva'; // Compliant: later reassignment +copy = other; +`, + }, + { + code: ` +const copy = clone(user); +copy.address.city = 'Geneva'; +`, + }, + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy[address].city = 'Geneva'; +`, + }, + ], + invalid: [ + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy.address.city = 'Geneva'; +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'lodash'; +const copy = structuredClone(user); +copy.address.city = 'Geneva'; +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy.address.city = 'Geneva'; // NOSONAR: shared nested state is intentional +`, + }, + ], + }, + ], + }, + { + code: ` +import _ from 'lodash'; +function process() { + const copy = _.clone(user); + items.forEach(() => { + copy.address.city = 'Geneva'; + }); +} +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'lodash'; +function process() { + const copy = structuredClone(user); + items.forEach(() => { + copy.address.city = 'Geneva'; + }); +} +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import _ from 'lodash'; +function process() { + const copy = _.clone(user); + items.forEach(() => { + copy.address.city = 'Geneva'; // NOSONAR: shared nested state is intentional + }); +} +`, + }, + ], + }, + ], + }, + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +function update() { + copy.address.city = 'Geneva'; +} +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'lodash'; +const copy = structuredClone(user); +function update() { + copy.address.city = 'Geneva'; +} +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import _ from 'lodash'; +const copy = _.clone(user); +function update() { + copy.address.city = 'Geneva'; // NOSONAR: shared nested state is intentional +} +`, + }, + ], + }, + ], + }, + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy.items[0].name = 'x'; +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'lodash'; +const copy = structuredClone(user); +copy.items[0].name = 'x'; +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy.items[0].name = 'x'; // NOSONAR: shared nested state is intentional +`, + }, + ], + }, + ], + }, + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy['address'].city = 'Geneva'; +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'lodash'; +const copy = structuredClone(user); +copy['address'].city = 'Geneva'; +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy['address'].city = 'Geneva'; // NOSONAR: shared nested state is intentional +`, + }, + ], + }, + ], + }, + { + code: ` +import clone from 'lodash/clone'; +const copy = clone(user); +copy.settings.retryCount++; +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import clone from 'lodash/clone'; +const copy = structuredClone(user); +copy.settings.retryCount++; +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import clone from 'lodash/clone'; +const copy = clone(user); +copy.settings.retryCount++; // NOSONAR: shared nested state is intentional +`, + }, + ], + }, + ], + }, + { + code: ` +import { clone } from 'lodash-es'; +const copy = clone(user); +delete copy.address.city; +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import { clone } from 'lodash-es'; +const copy = structuredClone(user); +delete copy.address.city; +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import { clone } from 'lodash-es'; +const copy = clone(user); +delete copy.address.city; // NOSONAR: shared nested state is intentional +`, + }, + ], + }, + ], + }, + { + code: ` +import _ from 'underscore'; +const copy = _.clone(user); +copy.address.city = 'Geneva'; +`, + errors: [ + { + message: UNDERSCORE_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'underscore'; +const copy = structuredClone(user); +copy.address.city = 'Geneva'; +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import _ from 'underscore'; +const copy = _.clone(user); +copy.address.city = 'Geneva'; // NOSONAR: shared nested state is intentional +`, + }, + ], + }, + ], + }, + { + code: ` +import _ from 'lodash'; +for (const user of users) { + const copy = _.clone(user); + copy.address.city = 'Geneva'; +} +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'lodash'; +for (const user of users) { + const copy = structuredClone(user); + copy.address.city = 'Geneva'; +} +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import _ from 'lodash'; +for (const user of users) { + const copy = _.clone(user); + copy.address.city = 'Geneva'; // NOSONAR: shared nested state is intentional +} +`, + }, + ], + }, + ], + }, + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +if (enabled) copy.address.city = 'Geneva'; else reset(); +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'lodash'; +const copy = structuredClone(user); +if (enabled) copy.address.city = 'Geneva'; else reset(); +`, + }, + ], + }, + ], + }, + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +const result = (copy.address.city = 'Geneva'); +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'lodash'; +const copy = structuredClone(user); +const result = (copy.address.city = 'Geneva'); +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import _ from 'lodash'; +const copy = _.clone(user); +const result = (copy.address.city = 'Geneva'); // NOSONAR: shared nested state is intentional +`, + }, + ], + }, + ], + }, + ], + }); + }); + + it('reports nested mutations through TypeScript non-null assertions', () => { + const ruleTester = new ESLintRuleTester({ + languageOptions: { parser: tsParser }, + }); + + ruleTester.run('avoid-mutating-nested-properties-of-shallow-clones', rule, { + valid: [], + invalid: [ + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy.address!.city = 'Geneva'; +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'lodash'; +const copy = structuredClone(user); +copy.address!.city = 'Geneva'; +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy.address!.city = 'Geneva'; // NOSONAR: shared nested state is intentional +`, + }, + ], + }, + ], + }, + ], + }); + }); + + it('reports the clone call as a secondary location in Sonar runtime mode', () => { + const ruleTester = new DefaultParserRuleTester(); + + ruleTester.run('avoid-mutating-nested-properties-of-shallow-clones', rule, { + valid: [], + invalid: [ + { + code: `import _ from 'lodash';\nconst copy = _.clone(user);\ncopy.address.city = 'Geneva';`, + settings: { sonarRuntime: true }, + errors: [ + { + message: JSON.stringify({ + message: LODASH_MESSAGE, + secondaryLocations: [ + { + message: 'Shallow clone created here.', + column: 13, + line: 2, + endColumn: 26, + endLine: 2, + }, + ], + }), + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: `import _ from 'lodash';\nconst copy = structuredClone(user);\ncopy.address.city = 'Geneva';`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: `import _ from 'lodash';\nconst copy = _.clone(user);\ncopy.address.city = 'Geneva'; // NOSONAR: shared nested state is intentional`, + }, + ], + }, + ], + }, + ], + }); + }); +}); diff --git a/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9135.json b/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9135.json new file mode 100644 index 00000000000..61598f34b17 --- /dev/null +++ b/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9135.json @@ -0,0 +1,31 @@ +{ + "title": "Avoid mutating nested properties of shallow clones", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant/Issue", + "constantCost": "5min" + }, + "tags": [ + "lodash", + "underscore.js" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-9135", + "sqKey": "S9135", + "scope": "Main", + "quickfix": "covered", + "code": { + "impacts": { + "RELIABILITY": "MEDIUM" + }, + "attribute": "LOGICAL" + }, + "compatibleLanguages": [ + "js", + "ts" + ], + "defaultQualityProfiles": [ + "Sonar way" + ] +}