rspec icon indicating copy to clipboard operation
rspec copied to clipboard

Create rule S6932: Use model binding instead of reading raw request data

Open github-actions[bot] opened this issue 1 year ago • 7 comments

You can preview this rule here (updated a few minutes after each push).

Review

A dedicated reviewer checked the rule description successfully for:

  • [ ] logical errors and incorrect information
  • [ ] information gaps and missing content
  • [ ] text style and tone
  • [ ] PR summary and labels follow the guidelines

github-actions[bot] avatar Feb 21 '24 13:02 github-actions[bot]

This can be extended to Razor Pages (despite not being in the scope of the sprint, the concept is the same). The PageModel class also has a Request property, the same type as the Request property in MVC Controllers. So the description will be pretty much the same. The later implementation as well. The only difference will be that instead of Controllers the rule will look through PageModel classes.

public class HomePageModel: PageModel
{
    public IActionResult OnPost()
    {
        var name = Request.Form["name"];   // Noncompliant
        ...
    }
}

@martin-strecker-sonarsource Please check the CI:

Rule csharp:S6932 has a "How to fix it" section for an unsupported framework: "ASP.NET Core"

The framework mentioned in the "How to fix it" section should be included in the list of frameworks here.

antonioaversa avatar Feb 23 '24 15:02 antonioaversa

We will not handle the VariableDeclarationSyntax. We will call semanticModel.GetConstantValue(argument.Expression) instead. Do you want me to handle the whole constant expressions section of the spec?

No, knowing a bit how the implementation will look like, I think cover all constant expressions would be an overkill that doesn't bring much value. It's good as it is IMO.

antonioaversa avatar Feb 29 '24 09:02 antonioaversa

@martin-strecker-sonarsource

I missed sending some pending comments. They might be outdated by now.

I went through all resolved conversations and checked whether there was something new. It should be all good now.

antonioaversa avatar Feb 29 '24 09:02 antonioaversa

Quality Gate passed Quality Gate passed for 'rspec-frontend'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

sonarqube-next[bot] avatar Feb 29 '24 10:02 sonarqube-next[bot]

Quality Gate passed Quality Gate passed for 'rspec-tools'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

sonarqube-next[bot] avatar Feb 29 '24 10:02 sonarqube-next[bot]

@zsolt-kolbay-sonarsource Thank you for bringing this up. I did not have the time to look into this in more detail, but you are right that this looks like a low-hanging fruit. I added comments next to the parameterized tests, that we can extend these to PageModel 201582063b49d5e04bf2d7659ffbe9ee097a066f. Such an addition needs more tests and should result in an update of the RSpec.