diff --git a/docs/allowed_framework_names.adoc b/docs/allowed_framework_names.adoc new file mode 100644 index 0000000000..745e83185a --- /dev/null +++ b/docs/allowed_framework_names.adoc @@ -0,0 +1,19 @@ +// C# +* ASP.NET +* Razor +// Java +* JSP +* Servlet +* Spring +* Thymeleaf +// JS +* Express.js +// PHP +* Core PHP +* Laravel +* Symfony +// Python +* Django +* Django Templates +* Flask +* Jinja diff --git a/docs/description.adoc b/docs/description.adoc index ac2fd54542..734dbc8997 100644 --- a/docs/description.adoc +++ b/docs/description.adoc @@ -12,7 +12,116 @@ The following are the only allowed second level titles for H2: include::section_names.adoc[] -Third level titles are not checked. +Most third-level titles (`+=== Title t3+`) are not checked, the only type of rule description for which some of these +titles are checked are the progressive education rules (see below). + +== Types of rule description + +The above list of allowed H2 sections is an absolute list and covers all the different types of rule descriptions that exist. +However, depending on the type of rule description, the expected order and allowed sections are not the same. +There are currently 3 types of rule descriptions, each having a specific structure. + +=== 1. Standard + +The standard rule description is the currently most widespread format. It includes all _Code Smells_, _Bugs_ and most _Vulnerabilities_. + +This is the "classical" format that starts with a description of the rule, and is usually followed by code examples of bad and good practices, +with some links to resources at the end. + +This format has the structure of the following section: + +. Description (no title) +. Noncompliant Code Example +. Compliant Solution +. Exceptions +. See +. See Also + +Note that most sections are optional, only the description is mandatory. + +=== 2. Hotspot + +Specific to the _Hotspot_ issue type, this format is mostly similar to the standard one, +with the addition of 2 new sections: `Ask Yourself Whether` and `Recommended Secure Coding Practices`. Additionally, the `Noncompliant Code Example` +section has been renamed into `Sensitive Code Example`: this is based on the fact that for _Hotspots_ there is no certainty that what was raised +is an actual vulnerability. + +The possible sections for this format are the following: + +. Description (no title) +. Ask Yourself Whether +. Recommended Secure Coding Practices +. Sensitive Code Example +. Compliant Solution +. Exceptions +. See +. See Also + +=== 3. Progressive Education + +Finally, the last format is the one related to the effort done on Progressive Education. +This new format has a completely new structure, with additional subtitles and information, +so as to be as comprehensible as possible for the users. + +This new structure is defined as follows: + +. Short description (no title) +. Why is this an issue? +.. What is the potential impact? +. How to fix it? +.. How to fix it in `{Framework Display Name}` +.. How does this work? +.. Pitfalls +.. Going the extra mile +. Resources +.. Documentation +.. Articles & Blog Posts +.. Conference Presentations +.. Standards + +Content of the section "_3. How to fix it?_" (i.e. subsections 3.a, 3.b, 3.c and 3.d) may be repeated multiple times, as +each repetition will represent the specific _How to fix it?_ section for a given framework. +For example: +.... +== How to fix it? + +=== How to fix it in Spring +... Some generic text and code examples for Spring... + +=== How does this work? +... Explanation about how the exploit works in Spring... + +=== Pitfalls +... Generic and Spring-specific pitfalls to avoid when fixing the issue... + +=== How to fix it in JSP +... Some generic text and code examples for JSP... + +=== How does this work? +... Explanation about how the exploit works in JSP... + +=== Pitfalls +... Generic and JSP-specific pitfalls to avoid when fixing the issue... +.... + +Ideally, by convention and for maintainability, each framework _How to fix it?_ section will be defined in separate files. +Ex: +.... +== How to fix it? + + include::./how-to-fix-it/framework-1.adoc[] + + include::./how-to-fix-it/framework-2.adoc[] +.... + +Note that each framework-specific _How to fix it?_ subsection must start with an H2 subtitle following the given format: +`How to fix it in (?:(?:an|a|the)\\s)?(.*)`. +This is important, as this format will be expected by the analyzers when loading the rule content to recognize the different subsections. +Furthermore, the display name of the framework has to match an allowed framework +display name, as defined in <>. + +Finally, additional information regarding each of the sections and subsections present in the educational rule description +can be found https://docs.google.com/document/d/1caqBXTO1r7vftWrNXt_aNVe4rHcB5C6u6oKMBFjbzwo[in this document]. == Code Examples diff --git a/rspec-tools/rspec_tools/cli.py b/rspec-tools/rspec_tools/cli.py index 3bf0d6055c..9fbee347b6 100644 --- a/rspec-tools/rspec_tools/cli.py +++ b/rspec-tools/rspec_tools/cli.py @@ -13,7 +13,8 @@ from rspec_tools.coverage import (update_coverage_for_all_repos, from rspec_tools.errors import RuleValidationError from rspec_tools.notify_failure_on_slack import notify_slack from rspec_tools.rules import LanguageSpecificRule, RulesRepository -from rspec_tools.validation.description import (validate_parameters, +from rspec_tools.validation.description import (validate_how_to_fix_it_subsections, + validate_parameters, validate_section_levels, validate_section_names, validate_source_language) @@ -86,7 +87,8 @@ def validate_rules_metadata(rules): _fatal_error(f"Validation failed due to {error_counter} errors out of {len(rules)} analyzed rules") -VALIDATORS = [validate_section_names, +VALIDATORS = [validate_how_to_fix_it_subsections, + validate_section_names, validate_section_levels, validate_parameters, validate_source_language, diff --git a/rspec-tools/rspec_tools/validation/description.py b/rspec-tools/rspec_tools/validation/description.py index 401a3f8dd2..136da47a05 100644 --- a/rspec-tools/rspec_tools/validation/description.py +++ b/rspec-tools/rspec_tools/validation/description.py @@ -6,6 +6,8 @@ from rspec_tools.errors import RuleValidationError from rspec_tools.rules import LanguageSpecificRule from rspec_tools.utils import LANG_TO_SOURCE +import re + # The list of all the sections currently accepted by the script. # The list includes multiple variants for each title because they all occur # in the migrated RSPECs. @@ -14,6 +16,11 @@ from rspec_tools.utils import LANG_TO_SOURCE SECTION_NAMES_PATH = Path(__file__).parent.parent.parent.parent.joinpath('docs/section_names.adoc') SECTION_NAMES_FILE = SECTION_NAMES_PATH.read_text(encoding='utf-8').split('\n') ACCEPTED_SECTION_NAMES: Final[list[str]] = [s.replace('* ', '').strip() for s in SECTION_NAMES_FILE if s.strip()] +# The list of all the framework names currently accepted by the script. +FRAMEWORK_NAMES_PATH = Path(__file__).parent.parent.parent.parent.joinpath('docs/allowed_framework_names.adoc') +FRAMEWORK_NAMES_FILE = FRAMEWORK_NAMES_PATH.read_text(encoding='utf-8').split('\n') +ACCEPTED_FRAMEWORK_NAMES: Final[list[str]] = [s.replace('* ', '').strip() for s in FRAMEWORK_NAMES_FILE if s.strip()] + def validate_section_names(rule_language: LanguageSpecificRule): descr = rule_language.description for h2 in descr.find_all('h2'): @@ -21,6 +28,29 @@ def validate_section_names(rule_language: LanguageSpecificRule): if name not in ACCEPTED_SECTION_NAMES: raise RuleValidationError(f'Rule {rule_language.id} has unconventional header "{name}"') +def validate_how_to_fix_it_subsections(rule_language: LanguageSpecificRule): + descr = rule_language.description + frameworks_counter = 0 + + for h3 in descr.find_all('h3'): + name = h3.text.strip() + # It is important that the Regex here matches the one used by the analyzers when loading the rules content + result = re.search('How to fix it in (?:(?:an|a|the)\\s)?(.*)', name) + if result is not None: + if result.group(1) not in ACCEPTED_FRAMEWORK_NAMES: + raise RuleValidationError(f'Rule {rule_language.id} has a "How to fix it" section for an unsupported framework: "{result.group(1)}"') + else: + frameworks_counter += 1 + + how_to_fix_it_section = descr.find('h2', string='How to fix it?') + if how_to_fix_it_section is not None: + if frameworks_counter == 0: + raise RuleValidationError(f'Rule {rule_language.id} has a "How to fix it" section but is missing subsections related to frameworks') + if frameworks_counter > 6: + raise RuleValidationError(f'Rule {rule_language.id} has more than 6 "How to fix it" subsections. Please ensure this limit can be increased with PM/UX teams') + elif frameworks_counter > 0: + raise RuleValidationError(f'Rule {rule_language.id} has "How to fix it" subsections for frameworks outside a defined "How to fix it?" section') + def validate_section_levels(rule_language: LanguageSpecificRule): h1 = rule_language.description.find('h1') if h1 is not None: diff --git a/rspec-tools/tests/resources/invalid-rules/S101/csharp/rule.adoc b/rspec-tools/tests/resources/invalid-rules/S101/csharp/rule.adoc new file mode 100644 index 0000000000..abe0aa0928 --- /dev/null +++ b/rspec-tools/tests/resources/invalid-rules/S101/csharp/rule.adoc @@ -0,0 +1,2 @@ +== How to fix it? +=== How to fix it in Foo Bar Framework diff --git a/rspec-tools/tests/resources/invalid-rules/S101/csharp/rule.html b/rspec-tools/tests/resources/invalid-rules/S101/csharp/rule.html new file mode 100644 index 0000000000..f9170d4b06 --- /dev/null +++ b/rspec-tools/tests/resources/invalid-rules/S101/csharp/rule.html @@ -0,0 +1,15 @@ + + +
+
+

How to fix it?

+
+
+

How to fix it in Foo Bar Framework

+ +
+
+
+
+ diff --git a/rspec-tools/tests/resources/invalid-rules/S101/java/rule.adoc b/rspec-tools/tests/resources/invalid-rules/S101/java/rule.adoc new file mode 100644 index 0000000000..f387a80c5f --- /dev/null +++ b/rspec-tools/tests/resources/invalid-rules/S101/java/rule.adoc @@ -0,0 +1 @@ +== How to fix it? diff --git a/rspec-tools/tests/resources/invalid-rules/S101/java/rule.html b/rspec-tools/tests/resources/invalid-rules/S101/java/rule.html new file mode 100644 index 0000000000..0ea44510c8 --- /dev/null +++ b/rspec-tools/tests/resources/invalid-rules/S101/java/rule.html @@ -0,0 +1,11 @@ + + +
+
+

How to fix it?

+
+
+
+
+ diff --git a/rspec-tools/tests/resources/invalid-rules/S101/javascript/rule.adoc b/rspec-tools/tests/resources/invalid-rules/S101/javascript/rule.adoc new file mode 100644 index 0000000000..0c0e0e59b0 --- /dev/null +++ b/rspec-tools/tests/resources/invalid-rules/S101/javascript/rule.adoc @@ -0,0 +1,8 @@ +== How to fix it? +=== How to fix it in Express.js +=== How to fix it in Express.js +=== How to fix it in Express.js +=== How to fix it in Express.js +=== How to fix it in Express.js +=== How to fix it in Express.js +=== How to fix it in Express.js diff --git a/rspec-tools/tests/resources/invalid-rules/S101/javascript/rule.html b/rspec-tools/tests/resources/invalid-rules/S101/javascript/rule.html new file mode 100644 index 0000000000..b21b4496dd --- /dev/null +++ b/rspec-tools/tests/resources/invalid-rules/S101/javascript/rule.html @@ -0,0 +1,39 @@ + + +
+
+

How to fix it?

+
+
+

How to fix it in Express.js

+ +
+
+

How to fix it in Express.js

+ +
+
+

How to fix it in Express.js

+ +
+
+

How to fix it in Express.js

+ +
+
+

How to fix it in Express.js

+ +
+
+

How to fix it in Express.js

+ +
+
+

How to fix it in Express.js

+ +
+
+
+
+ diff --git a/rspec-tools/tests/resources/invalid-rules/S101/python/rule.adoc b/rspec-tools/tests/resources/invalid-rules/S101/python/rule.adoc new file mode 100644 index 0000000000..3c04959ee7 --- /dev/null +++ b/rspec-tools/tests/resources/invalid-rules/S101/python/rule.adoc @@ -0,0 +1 @@ +=== How to fix it in Flask diff --git a/rspec-tools/tests/resources/invalid-rules/S101/python/rule.html b/rspec-tools/tests/resources/invalid-rules/S101/python/rule.html new file mode 100644 index 0000000000..a8ea7819cd --- /dev/null +++ b/rspec-tools/tests/resources/invalid-rules/S101/python/rule.html @@ -0,0 +1,10 @@ + + +
+
+

How to fix it in Flask

+ +
+
+ diff --git a/rspec-tools/tests/resources/rules/S101/csharp/rule.adoc b/rspec-tools/tests/resources/rules/S101/csharp/rule.adoc new file mode 100644 index 0000000000..a46e1ae933 --- /dev/null +++ b/rspec-tools/tests/resources/rules/S101/csharp/rule.adoc @@ -0,0 +1,3 @@ +== How to fix it? +=== How to fix it in ASP.NET +=== How to fix it in Razor diff --git a/rspec-tools/tests/resources/rules/S101/csharp/rule.html b/rspec-tools/tests/resources/rules/S101/csharp/rule.html new file mode 100644 index 0000000000..3ffcf8f4ca --- /dev/null +++ b/rspec-tools/tests/resources/rules/S101/csharp/rule.html @@ -0,0 +1,19 @@ + + +
+
+

How to fix it?

+
+
+

How to fix it in ASP.NET

+ +
+
+

How to fix it in Razor

+ +
+
+
+
+ diff --git a/rspec-tools/tests/resources/rules/S101/metadata.json b/rspec-tools/tests/resources/rules/S101/metadata.json new file mode 100644 index 0000000000..d30e97792c --- /dev/null +++ b/rspec-tools/tests/resources/rules/S101/metadata.json @@ -0,0 +1,28 @@ +{ + "title": "Class names should comply with a naming convention", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "convention" + ], + "extra": { + "replacementRules": [ + + ], + "legacyKeys": [ + "ClassName" + ] + }, + "defaultSeverity": "Minor", + "ruleSpecification": "RSPEC-101", + "sqKey": "S101", + "scope": "All", + "defaultQualityProfiles": [ + "Sonar way" + ], + "quickfix": "unknown" +} diff --git a/rspec-tools/tests/test_create_rule.py b/rspec-tools/tests/test_create_rule.py index 70db10f1c2..adcb65e269 100644 --- a/rspec-tools/tests/test_create_rule.py +++ b/rspec-tools/tests/test_create_rule.py @@ -217,7 +217,7 @@ def test_add_language_the_rule_is_already_defined_for(rule_creator: RuleCreator) def test_add_language_to_nonexistent_rule(rule_creator: RuleCreator): '''Test add_language_branch correctly fails when invoked for a non-existent rule.''' with pytest.raises(InvalidArgumentError): - rule_creator.add_language_branch(101, 'cfamily') + rule_creator.add_language_branch(105, 'cfamily') def test_add_language_new_pull_request(rule_creator: RuleCreator): diff --git a/rspec-tools/tests/test_rules_repository.py b/rspec-tools/tests/test_rules_repository.py index 414e488b9f..3f89464ad5 100644 --- a/rspec-tools/tests/test_rules_repository.py +++ b/rspec-tools/tests/test_rules_repository.py @@ -7,7 +7,7 @@ from rspec_tools.errors import RuleNotFoundError def test_list_rules(mockrules: Path): '''Check that rules are all listed.''' rules = {rule.id for rule in RulesRepository(rules_path=mockrules).rules} - assert rules == {'S100', 'S120', 'S4727', 'S1033'} + assert rules == {'S100', 'S101', 'S120', 'S4727', 'S1033'} def test_list_languages(mockrules: Path): diff --git a/rspec-tools/tests/validation/test_description_validation.py b/rspec-tools/tests/validation/test_description_validation.py index 90b6fb2d89..bd2708a3f9 100644 --- a/rspec-tools/tests/validation/test_description_validation.py +++ b/rspec-tools/tests/validation/test_description_validation.py @@ -1,13 +1,12 @@ +import re from pathlib import Path -from unittest.mock import patch, PropertyMock import pytest -import re from rspec_tools.errors import RuleValidationError -from copy import deepcopy +from rspec_tools.rules import RulesRepository +from rspec_tools.validation.description import validate_how_to_fix_it_subsections, validate_section_names, \ + validate_section_levels, validate_parameters, validate_source_language -from rspec_tools.rules import LanguageSpecificRule, RulesRepository -from rspec_tools.validation.description import validate_section_names, validate_section_levels, validate_parameters, validate_source_language @pytest.fixture def rule_language(mockrules: Path): @@ -87,3 +86,32 @@ def test_wrong_source_language_fails_validation(invalid_rule): rule = invalid_rule('S100', 'csharp') with pytest.raises(RuleValidationError, match=re.escape(f'Rule {rule.id} has unknown language "unknown" in code example in section "Noncompliant Code Example".\nAre you looking for "csharp"?')): validate_source_language(rule) + +def test_unsupported_framework_name_in_how_to_fix_it_subsection_validation(invalid_rule): + '''Check that having "How to fix it" subsections using framework names that are not inside the "allowed_framework_names.adoc" file breaks validation''' + rule = invalid_rule('S101', 'csharp') + with pytest.raises(RuleValidationError, match=f'Rule csharp:S101 has a "How to fix it" section for an unsupported framework: "Foo Bar Framework"'): + validate_how_to_fix_it_subsections(rule) + +def test_missing_subsections_in_how_to_fix_it_validation(invalid_rule): + '''Check that having a "How to fix it?" section without any subsection breaks validation''' + rule = invalid_rule('S101', 'java') + with pytest.raises(RuleValidationError, match=f'Rule java:S101 has a "How to fix it" section but is missing subsections related to frameworks'): + validate_how_to_fix_it_subsections(rule) + +def test_too_many_subsections_in_how_to_fix_it_validation(invalid_rule): + '''Check that having more than the current hard limit (6) "How to fix it" subsections breaks validation''' + rule = invalid_rule('S101', 'javascript') + with pytest.raises(RuleValidationError, match=f'Rule javascript:S101 has more than 6 "How to fix it" subsections. Please ensure this limit can be increased with PM/UX teams'): + validate_how_to_fix_it_subsections(rule) + +def test_subsections_without_parent_section_in_how_to_fix_it_validation(invalid_rule): + '''Check that having "How to fix it" subsections without the parent "How to fix it?" section breaks validation''' + rule = invalid_rule('S101', 'python') + with pytest.raises(RuleValidationError, match=f'Rule python:S101 has "How to fix it" subsections for frameworks outside a defined "How to fix it\\?" section'): + validate_how_to_fix_it_subsections(rule) + +def test_valid_how_to_fix_it_subsections_validation(rule_language): + '''Check that expected format is considered valid''' + rule = rule_language('S101', 'csharp') + validate_how_to_fix_it_subsections(rule) diff --git a/rules/S5131/python/how-to-fix-it/dtl.adoc b/rules/S5131/python/how-to-fix-it/dtl.adoc index 9102b61a4d..60cf7748c7 100644 --- a/rules/S5131/python/how-to-fix-it/dtl.adoc +++ b/rules/S5131/python/how-to-fix-it/dtl.adoc @@ -1,4 +1,4 @@ -=== How to fix it in DTL +=== How to fix it in Django Templates The following code is vulnerable to cross-site scripting because auto-escaping of special HTML characters has been disabled. The recommended way to fix this code is to move the HTML content to the template and to only inject the dynamic value. Therefore, it is not necessary to disable auto-escaping.