Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New rule S6932: Use model binding instead of reading raw request data #8953

Merged
merged 19 commits into from
May 23, 2024
Merged

New rule S6932: Use model binding instead of reading raw request data #8953

merged 19 commits into from
May 23, 2024

Conversation

martin-strecker-sonarsource
Copy link
Contributor

Fixes #8871
Based on #8950
Replaces #8930

@martin-strecker-sonarsource martin-strecker-sonarsource mentioned this pull request Mar 18, 2024
Closed
@martin-strecker-sonarsource martin-strecker-sonarsource changed the title Copy rule implementation Create rule S6932: Use model binding instead of reading raw request data Mar 18, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Mar 18, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Mar 18, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
6 New issues
90.4% Coverage on New Code (required ≥ 95%)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@martin-strecker-sonarsource martin-strecker-sonarsource linked an issue Mar 19, 2024 that may be closed by this pull request
New rule S6932: Use model binding instead of reading raw request data #8871
Closed
@CristianAmbrosini CristianAmbrosini changed the title Create rule S6932: Use model binding instead of reading raw request data New rule S6932: Use model binding instead of reading raw request data Apr 24, 2024
@martin-strecker-sonarsource martin-strecker-sonarsource force-pushed the Martin/ArgumentDescriptor branch 2 times, most recently from f798a43 to f71058e Compare May 6, 2024 08:54
Base automatically changed from Martin/ArgumentDescriptor to feature/New_S6932 May 14, 2024 15:40
Base automatically changed from feature/New_S6932 to master May 15, 2024 07:49
@martin-strecker-sonarsource martin-strecker-sonarsource marked this pull request as ready for review May 21, 2024 13:07
@martin-strecker-sonarsource martin-strecker-sonarsource added the Sprint: Hardening Fix FPs/FNs/improvements label May 21, 2024
@martin-strecker-sonarsource martin-strecker-sonarsource added this to Review in progress in Best Kanban May 21, 2024
@martin-strecker-sonarsource martin-strecker-sonarsource removed this from Review in progress in Best Kanban May 21, 2024
@martin-strecker-sonarsource martin-strecker-sonarsource removed the Sprint: Hardening Fix FPs/FNs/improvements label May 21, 2024
zsolt-kolbay-sonarsource
zsolt-kolbay-sonarsource approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Very well documented and optimized! Left a couple of polishing comments.

analyzers/tests/SonarAnalyzer.Test/TestCases/AspNet/UseAspNetModelBinding_AspNetCore.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/UseAspNetModelBinding.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/UseAspNetModelBinding.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/UseAspNetModelBinding.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Rules/AspNet/UseAspNetModelBindingTest.cs Outdated Show resolved Hide resolved
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 23, 2024

Quality Gate Passed Quality Gate passed for 'Sonar .NET Java Plugin'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 23, 2024

Quality Gate Passed Quality Gate passed for 'SonarAnalyzer for .NET'

Issues
0 New issues
2 Accepted issues

Measures
0 Security Hotspots
90.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@martin-strecker-sonarsource martin-strecker-sonarsource merged commit 26f7c71 into master May 23, 2024
26 checks passed
@martin-strecker-sonarsource martin-strecker-sonarsource deleted the Martin/S6932 branch May 23, 2024 11:21
@martin-strecker-sonarsource
Copy link
Contributor Author

Peach validation:

Project Issues
bitwarden-core 3
cloudscribe 8
das-apim-endpoints 2
jellyfin 4
oqtane-framewok 1
orchard-core 8
piranha-core 3
piranha-core-templates 2
Radarr 2
simplcommerce 3
umbraco 5
Grand Total 41

The issues are raised for

Most of the issues are TPs. A lot of FPs are in helper methods of controllers where data is extracted conditionally. It is debatable whether we need to fix these:

Some accesses are problematic for other reasons as well, like using IHttpContextAccessor here.

Some FPs are undetectable like here or here, where FileFormCollection binding is insufficient for the use case.

One FP should be fixed:

Access to IFormCollection.Files should be allowed, as we recommend binding to IFormCollection in the RSpec. See #9344

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New rule S6932: Use model binding instead of reading raw request data
2 participants
@martin-strecker-sonarsource @zsolt-kolbay-sonarsource