Create rule S6932: Use model binding instead of reading raw request data #3668
Conversation
martin-strecker-sonarsource
changed the title
|
This can be extended to Razor Pages (despite not being in the scope of the sprint, the concept is the same). public class HomePageModel: PageModel
{
public IActionResult OnPost()
{
var name = Request.Form["name"]; // Noncompliant
...
}
} |
antonioaversa
left a comment
antonioaversa
left a comment
There was a problem hiding this comment.
I have done a first round mainly analyzing code examples, for both ASP.NET Core and Framework.
Once all the comments on those are addressed, and the necessary information are included into the RSPEC, I will move to review the v2 of the RSPEC.
|
@martin-strecker-sonarsource Please check the CI: The framework mentioned in the "How to fix it" section should be included in the list of frameworks here. |
antonioaversa
left a comment
antonioaversa
left a comment
There was a problem hiding this comment.
We are close to be done on code examples. I will then move the resulting RSPEC.
Please have a look at the message that @zsolt-kolbay-sonarsource has written here.
antonioaversa
left a comment
antonioaversa
left a comment
There was a problem hiding this comment.
I have reviewed the table in the "How to fix it for ASP.NET Core".
It's great work: the table looks very nice and comprehensive!
I think it will help a lot the user to get basic understanding of what to do, when faced with this issue.
I think you can proceed to make something similar for ASP.NET Framework.
I wouldn't worry if you don't get to have the same level of detail for ASP.NET Framework. What is important to me is that we have at least the Replace column and a second column with the [FromX] attribute or the ISomething type. The rest is nice but not strictly necessary IMO.
rules/S6932/csharp/rule.adoc
Outdated
|
|
||
| == How to fix it in ASP.NET Core | ||
|
|
||
| `Request.Form`, `Request.Form.Files`, `Request.Headers`, `Request.Query` and `Request.RouteValues` are key/value collections that expose data from the incoming HTPP request: |
There was a problem hiding this comment.
Strictly speaking, Request.Form.Files is not a key/value collection. It is an IEnumerable of IFormFile, that is not a KeyValuePair.
However, IFormFile has content, as well as Name, that can be considered as a "key" of sort.
So you either:
- remove "key/value" and only talk about "collections"
- or keep it as is, assuming a broader definition of "key/value collection"
Both are fine with me.
There was a problem hiding this comment.
what about "keyed collection"? This is a more generic term and a little bit further away from KeyValuePair<,> than "key/value collection".
There was a problem hiding this comment.
Good for me. I don't consider this a blocking change. I let you make the change, if you prefer so.
|
|
||
| To replace the key/value collection access, you can: | ||
|
|
||
| [options="header"] |
There was a problem hiding this comment.
Remark for the implementation phase
The table looks incorrectly rendered on https://sonarsource.github.io/rspec and correctly rendered by the Visual Studio Code plugin:
The rendering of the table will need to be checked on Peach, once the rule is embedded into the analyzer.
| "status": "ready", | ||
| "remediation": { | ||
| "func": "Constant\/Issue", | ||
| "constantCost": "5min" |
There was a problem hiding this comment.
As discussed offline, 5 minutes may not be far off the actual time.
Let's keep this for the time being. We may change it later based on Denis' opinion.
I keep this conversation open for Denis.
antonioaversa
left a comment
antonioaversa
left a comment
There was a problem hiding this comment.
Looks very close to completion. Remaining discussions are not blocking.
Can you please check Zsolt's comment here and provide an answer?
I think it makes sense to also support Razor pages.
If you would prefer to have a dedicated rule for that, creating a new Rule Idea and linking here would be enough.
rules/S6932/S6932.AspNet4x.cs
Outdated
|
|
||
| protected override void Initialize(RequestContext requestContext) | ||
| { | ||
| _ = requestContext.HttpContext.Request.Form["id"]; // Compliant. Model binding is not supported here |
There was a problem hiding this comment.
In all other compliant/non-compliant examples you use : as a separator (that is also in line with the results of this poll), but here, and in all following examples . is used. I would be coherent throughout the entire file, and use : everywhere.
martin-strecker-sonarsource
left a comment
martin-strecker-sonarsource
left a comment
There was a problem hiding this comment.
I missed sending some pending comments. They might be outdated by now.
rules/S6932/S6932.AspNet4x.cs
Outdated
| _ = Request.Form.Get(@"key"); // Noncompliant | ||
| _ = Request.Form.GetValues(@"key"); // Noncompliant | ||
|
|
||
| const string key = "id"; |
There was a problem hiding this comment.
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. |
I went through all resolved conversations and checked whether there was something new. |
|
|
|
@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 2015820. Such an addition needs more tests and should result in an update of the RSpec. |
4 participants
You can preview this rule here (updated a few minutes after each push).
Review
A dedicated reviewer checked the rule description successfully for: