S6967 - ModelState.IsValid not needed for JsonResult

Rules and Languages Report False-positive / False-negative...
1

C#
S6967 ModelState,IsValid should be checked in controller actions.
When returning JsonResult (called from via ajax within javascript), there isn’t a Model to check. For me, I like to keep the JsonResult within the same controller as the Page’s IActionResult without creating special classes for the returns.

SonarLint - 8.0.0.92083 - Not connected

To recreate, add a procedure like public JsonResult Testing() with a return Json(“”) and it should be flagged. Interesting enough, its not always, but I’ve not found a trigger why it does sometimes and not others.

2

Hey there.’

Can you please provide a complete code sample?

3

I think its easier to see with the screenshots. I grabbed an IActionResult that is from a view that may have a model on it. I found a procedure that returns decimal and it was also flagged for the S6967.

Screenshot 2024-07-02 103206
Screenshot 2024-07-02 1032061139×435 30.7 KB

Screenshot 2024-07-02 103105
Screenshot 2024-07-02 103105693×288 14.3 KB

4

I appreciate it, but a self-contained snippet (or two) of code that a developer can paste into their IDE while investigating an FP is really invaluable.

5

This should work. I noticed if there are passed variables, this it triggers the S6967. If no passed variables, it does not.

public class TestController : Controller
{
    public IActionResult Index(string id)
    {
        return View();
    }

    [AllowAnonymous]
    [HttpGet]
    public JsonResult GetDelaysJson(DateTime startDate, DateTime endDate, double TotalHeats, double TotalTons,int area, string groupby = "")
    {
        var delaysWithOutageMins = new
        {
            Error = ""
        };

        return Json(delaysWithOutageMins);
    }

    /// <summary>
    /// Get the last 50 heats
    /// </summary>
    /// <returns></returns>
    [AllowAnonymous]
    public  JsonResult GetLast50HeatsJson()
    {
        
        var last50HeatsData =  new SelectListItem
        {
            Text = "Test",
            Value = "Test"
        };

        return Json(last50HeatsData);
    }

    /// <summary>
    /// Check if the user is valid to edit the webpage
    /// </summary>
    /// <param name="user"></param>
    /// <param name="webpage"></param>
    /// <returns></returns>
    [AllowAnonymous]
    [HttpGet]
    public IActionResult IsUserValidToEdit(string user, string webpage)
    {
        try
        {
            var result = "await _externalData.GetValidUser(user, webpage)";

            return Ok(result);
        }
        catch (Exception ex)
        {
            return BadRequest(new StatusCodeError { Error = "Error", ErrorMsg = ex.Message, ErrorDetails = ex.Message, StatusCode = 400 });
        }
    }
}
8

Hi @BartNucor. Sorry for the late reply. I agree, the rule is too strict in certain cases.
We have an open issue for this on Github, which aims to ignore Controller methods that only have string, object, or dynamic parameters. After that fix is released, the rule will only raise a warning for the GetDelaysJson method from your code sample (because DateTime, double, and int parameters can still be passed in an invalid manner, and they should be validated).

9

Thanks for getting back to me. I agree they should checked, I mainly was concerned with the wording since it specifically mentions ModelState and with an Ajax/WebApi call, there is no Model. Maybe, in my instance, my code demotes its a web call (has HTTPGet or WebRequest Methods), it wouldn’t show a warning. Also, since the controller has [ApiController], I don’t think there would be a model to valid. I’ve never tested don’t ModelState.IsValid on one of the calls, I can’t say it work return true or false. If it was false, I’m not sure what to make it true.

10

Let’s see if I can address what you mentioned:

  1. If the Controller class is decorated with [ApiController] then the rule will ignore it completely. That’s because the Action methods in those Controller classes automatically return HTTP 404 when ModelState.IsValid is false.
  2. On the notion of AJAX/WebApi methods: you mentioned there’s no Model to validate. AFAIK a Model in ASP.NET MVC is essentially all the input that the Action method receives from the client. So even if the method has a single int parameter, that’s considered having a Model. Have you tried if the ModelState is validated in these methods? e.g. have an AJAX method with an int parameter and the client passes “XYZ” as an input value?
  3. If you simply don’t want to use ModelState.IsValid (e.g. you know for sure the input will always be valid or it’s not worth checking for any reason) then you can disable the rule for a file or the whole project.
11

I finally did some additional testing and I apologize, I was getting 2 different things mixed up. My web APIs don’t have the S6967 warning which is valid since They typically test the data prior to getting to the procedure. I did find in some occasions, bad data was allowed so the ModelState should be checked.

On MVC type controllers, I was wrong about the detection and the reasoning behind it.

This item can be closed.

2 Likes