The Orchard Core threequel. Rechecking the project with PVS-Studio
Tags: csharp, dotnet, opensource, codequality
Published: 2022-08-26 08:07:25

In this article, we check the Orchard Core project with the help of the PVS\-Studio static analyzer\. We are going to find out if the platform code is as good as the sites created on its basis\. May the force of static analysis be with us\! 

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/53k0sws6jcx88jqfffur.png)

## Introduction

Orchard Core is a modular, multi\-tenant, open\-source application framework and CMS for ASP\.NET Core\. We've already checked this project twice and found interesting warnings\. We even wrote articles about these warnings — click [here](https://pvs-studio.com/en/blog/posts/csharp/0456/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) or [here](https://pvs-studio.com/en/blog/posts/csharp/0681/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) if you want to learn more\. Let's see if we are going to find something wonderful this time =\)

The project code is available in the [repository on GitHub](https://github.com/OrchardCMS/OrchardCore/tree/v1.4.0)\. We check the code with the PVS\-Studio static code analyzer\.

The analyzer issued 281 warnings for 3791 files with the \.cs extension\. 54 warnings had a high level of certainty, 143 — medium level and 84 — low level\. Now, let's look at the most interesting of them\.

## The analysis results

**Issue 1**

```cpp
public async Task<IActionResult> LinkExternalLogin(
                   LinkExternalLoginViewModel model,
                   string returnUrl = null)
{
  ....
  var info = await _signInManager.GetExternalLoginInfoAsync();

  var email = info.Principal.FindFirstValue(ClaimTypes.Email)
           ?? info.Principal.FindFirstValue("email");
  ....

  if (info == null)
  {
    _logger.LogWarning("Error loading external login info.");
    return NotFound();
  }
  ....
}
```

PVS\-Studio warning:[ V3095](https://pvs-studio.com/en/docs/warnings/v3095/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) The 'info' object was used before it was verified against null\. Check lines: 637, 641\. AccountController\.cs 637

Let's start our review with a potential dereference of a *null* reference — "beloved" by many developers\. Take a look at the *Principal* property of the *info* object which was accessed twice in a row and a *null* check right in the next line\. Looks elegant, doesn't it? Actually, it's easy to overlook such errors during code review\. Most likely, a check for *null* should be performed before *info* is dereferenced\. In this case, there would be no problems\.

**Issue 2**

```cpp
public async ValueTask<Completion> WriteToAsync(
             List<FilterArgument> argumentsList,
             IReadOnlyList<Statement> statements,
             TextWriter writer,
             TextEncoder encoder,
             LiquidTemplateContext context)
{
  if (displayFor != null)
  {
    ....
  }
  else if (removeFor != null)
  {
    ....

    if (metadata.RemoveRouteValues != null)
    {  
      if (routeValues != null)
      {
        foreach (var attribute in routeValues)
        {
          metadata.RemoveRouteValues.Add(attribute.Key, attribute.Value);
        }
      } 

        ....
    }
  }
  else if (createFor != null)
  {
    ....
    var metadata = await contentManager
                   .PopulateAspectAsync<ContentItemMetadata>(createFor);

    if (metadata.CreateRouteValues == null)                       // <=
    {
      if (routeValues != null)
      {
        foreach (var attribute in routeValues)
        {
          metadata.CreateRouteValues.Add(attribute.Key,           // <=
                                         attribute.Value);     
        }
      }
      ....
    }
  }
  ....
}
```

PVS\-Studio warning: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) Possible null dereference\. Consider inspecting 'metadata\.CreateRouteValues'\. ContentAnchorTag\.cs 188

I would be remiss if I didn't mention typos in repeated nested conditionals\. Here, the *CreateRouteValues* property *of* the *metadata* object is dereferenced right in the *then* block, which explicitly indicates *null*\. 

To make sure it's just an unfortunate typo, just look at the similar *else if* conditional given above\. A proper comparison operator is used there and therefore the *metadata* object's properties are dereferenced without any errors\.

By the way, this error ranked first in our [top of errors on ASP\.NET Core](https://habr.com/en/company/pvs-studio/blog/654395/)\.

**Tip: During code review, check the last block of nested conditionals twice\. This block may hide an insidious [last line effect](https://pvs-studio.com/en/blog/posts/cpp/0260/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core)\!**

**Issue 3**

```cpp
public async Task<IActionResult> DeleteMediaList(string[] paths)
{
  foreach (var path in paths)
  {
    ....
  }  

  if (paths == null)
  {
    return NotFound();
  }
  ....
}
```

PVS\-Studio warning: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) The 'paths' object was used before it was verified against null\. Check lines: 304, 312\. AdminController\.cs 304

This error seems more exciting\. At first glance, the code looks right\. Although *paths* is used before a *null* check, the code doesn't dereference a reference to this object explicitly\. It's not really that simple though\. During the *foreach* loop iteration through the collection, the loop calls the *GetEnumerator* method\. This leads to a *NullReferenceException*, and the program crashes\.

**Tip: Always be aware of the way different language constructs function, or use a reliable code\-checking software solution\.**

**Issue 4**

```cpp
private async Task EnsureConfigurationAsync()
{
  ....
  var lastProviders = (_applicationConfiguration as IConfigurationRoot)
                        ?.Providers.Where(p => 
                        p is EnvironmentVariablesConfigurationProvider ||
                        p is CommandLineConfigurationProvider).ToArray();
  ....
  if (lastProviders.Count() > 0)
  {
    ....
  }
  ....
}
```

PVS\-Studio warning: [V3105](https://pvs-studio.com/en/docs/warnings/v3105/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) The 'lastProviders' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. ShellSettingsManager\.cs 242

Although the above code fragment contains only the *lastProviders* object's assignment and a conditional, the error is inconspicuous\. The analyzer informs us that the reference to an object assigned via a null\-conditional operator is dereferenced\. Indeed, *lastProviders* is derived from the result of *\_applicationConfiguration* cast to *IConfigurationRoot* executed via *as*\. In this case*,* *lastProviders* can take *null* if the cast is not possible\. The developers execute the function via the '\.?' operator on purpose\. But they failed to add any checks for *null* in the conditional that contains a call to *lastProviders*\.*Count*\. 

This code fragment shows a common pattern of errors found by PVS\-Studio\. Many developers prefer the use of null\-conditional operators, instead of explicit checks for *null*\. This approach makes the code less cumbersome and more readable\. But null\-conditional operators may get lost in a large code base\. In this case, ominous *NullReferenceException* may be thrown\. 

**Tip: Pay attention to the use of null\-conditional operators\. Try not to lose the sight of null**

**Issue 5**

```cpp
private async Task<string> GenerateUsername(ExternalLoginInfo info)
{
  ....
  var externalClaims = info?.Principal.GetSerializableClaims();
  ....
  foreach (var item in _externalLoginHandlers)
  {
    try
    {
      var userName = await item.GenerateUserName(
                      info.LoginProvider, externalClaims.ToArray());
      ....
    }
    ....
  }
  ....
}
```

PVS\-Studio warning: [V3105](https://pvs-studio.com/en/docs/warnings/v3105/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) The 'externalClaims' variable was used after it was assigned through a null\-conditional operator\. NullReferenceException is possible\. AccountController\.cs 786

The analyzer warns about the potentially dangerous use of the *externalClaims* variable assigned through a null\-conditional operator\. As in the previous case, there is no protection against dereference of the null reference\.

**Issue 6**

```cpp
public async Task ShouldDiscardDraftThenCreateNewPublishedContentItemVersion()
{
  using (var context = new BlogPostDeploymentContext())
  {
    ....
    await shellScope.UsingAsync(async scope =>
    {
      ....
      var originalVersion = blogPosts.FirstOrDefault(x => 
           x.ContentItemVersionId == context.OriginalBlogPostVersionId);
      Assert.False(originalVersion?.Latest);
      Assert.False(originalVersion?.Published);

      var draftVersion = blogPosts.FirstOrDefault(x => 
           x.ContentItemVersionId == draftContentItemVersionId);
      Assert.False(draftVersion?.Latest);
      Assert.False(draftVersion?.Published);

      var newVersion = blogPosts.FirstOrDefault(x => 
           x.ContentItemVersionId == "newversion");
      Assert.Equal("new version", newVersion.DisplayText);           // <=
      Assert.True(newVersion?.Latest);                               // <=
      Assert.True(newVersion?.Published);                            // <=
    });
  }
}
```

PVS\-Studio warning: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) The 'newVersion' object was used before it was verified against null\. Check lines: 94, 95\. BlogPostCreateDeploymentPlanTests\.cs 94

This piece of code shows what all developers are so afraid of — copy\-paste errors\. Here a developer forgot to use the null\-conditional operator when a program accessed the *newVersion* object\. Therefore, when a program accesses the *DisplayText* property, *NullReferenceException* can be thrown\.

This happened, most likely, when a developer copied similar blocks of code contained the '?\.' operator\. However, when a new line with the *newVersion* object had appeared, the null\-conditional operator magically disappeared\.

**Tip: When you copy code, it is worth to spend more time on code review\. You can also use a static analyzer to ease the process of code review\.**

## Worthy of mention

As I said earlier, we have checked the Orchard project twice \([here](https://pvs-studio.com/en/blog/posts/csharp/0456/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) and [here](https://pvs-studio.com/en/blog/posts/csharp/0681/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core)\)\. Great that developers fixed all the errors found during the first check\. However, after the second check some errors left unfixed\. The PVS\-Studio team feels obligated to point out these potential errors again\.

Let's start with the following interesting example:

```cpp
public async Task<IActionResult> Import(ImportViewModel model)
{
  ....
  var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(
                             x => x.ClientName == model.ClientName);
  var apiKey = Encoding.UTF8.GetString( _dataProtector.Unprotect(   
                                     remoteClient.ProtectedApiKey)); // <=

  if (remoteClient == null ||                                        // <=
      model.ApiKey != apiKey || 
      model.ClientName != remoteClient.ClientName)
  {
    return StatusCode((int)HttpStatusCode.BadRequest, 
                        "The Api Key was not recognized");
  }
  ....
}
```

PVS\-Studio warning: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) The 'remoteClient' object was used before it was verified against null\. Check lines: 46, 48\. ImportRemoteInstanceController\.cs 46

The analyzer informs us that *remoteClient* is dereferenced before its check for null\. Most likely, the check should be performed before the dereference\. Otherwise, *NullReferenceException* will be thrown\.

Also, in the previous check, we supposed that a check for *null* was unnecessary, and instead of the *FirstOrDefault* method, it is better to use just *First*\. This solution seemed reasonable\. Although three years later the analyzer issued a warning on this code fragment again\.\.\. 

In the project code, the *FirstOrDefault* method is used without any checks for *null* \(more on that later\)\. Here we have an explicit check for *null*\. So, you just need to replace the conditional and the *apiKey* assignment here\.

Now let's take a look not at the warning itself but at the recommendation:

```cpp
private async Task ExecuteAsync(HttpContext context, ....)
{
  ....
  GraphQLRequest request = null;
  ....
  if (HttpMethods.IsPost(context.Request.Method))
  {
    ....
    request = ....;
    ....
  }
  else if (HttpMethods.IsGet(context.Request.Method))
  {
    ....
    request = new GraphQLRequest();
    ....
  }
  var queryToExecute = request.Query;
  ....
}
```

PVS\-Studio warning: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) Possible null dereference\. Consider inspecting 'request'\. GraphQLMiddleware\.cs 157

The *request* object is initialized in each of nested conditionals\. You can find the full code [here](https://github.com/OrchardCMS/OrchardCore/blob/v1.4.0/src/OrchardCore.Modules/OrchardCore.Apis.GraphQL/GraphQLMiddleware.cs)\. Let's take a look at the first two conditions that check the *request* type for compliance with *IsPost* and *IsGet*\. As mentioned in the [previous article](https://pvs-studio.com/en/blog/posts/csharp/0681/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core), the *Microsoft\.AspNetCore\.HttpMethods* class has nine static methods to check the *request* type\. Thus, when an unknown *request* is passed, *NullReferenceException* will be thrown\.

Of course, this is not a mistake but a decision to cover only those features of the program that the developers use\. However, we still would like to draw developers' attention to such cases\. In the future, this may save them from the exhausting search for the crashed place in program\.

Moreover, a *null* check and an exception throw take only a few lines =\)\.

Let's look at the last, but not the least amusing error in this chapter:

```cpp
public async Task ConfigureOAuthAsync(HttpRequestMessage request)
{
  ....
  if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret))
      settings.ConsumerSecret = protrector.Unprotect(
                                 settings.ConsumerSecret);
  if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret))
      settings.AccessTokenSecret = protrector.Unprotect(
                              settings.AccessTokenSecret);
  ....
}
```

PVS\-Studio warning: [V3127](https://pvs-studio.com/en/docs/warnings/v3127/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) Two similar code fragments were found\. Perhaps, this is a typo and 'AccessTokenSecret' variable should be used instead of 'ConsumerSecret' TwitterClientMessageHandler\.cs 51

It would seem just another copy paste mistake, but what an annoying one\! Instead of the *consumerSecret* check in the second condition, it's better to check *AccessTokenSecret*, because *AccessTokenSecret* was not checked at all\. However, the *then* block clearly indicates — the check should be here\. The fixed version might look as follows:

```cpp
public async Task ConfigureOAuthAsync(HttpRequestMessage request)
{
  ....
  if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret))
      settings.ConsumerSecret = 
            protrector.Unprotect(settings.ConsumerSecret);
  if (!string.IsNullOrWhiteSpace(settings.AccessTokenSecret))
      settings.AccessTokenSecret = 
         protrector.Unprotect(settings.AccessTokenSecret);
  ....
}
```

To conclude this section, I would like to note that the above code fragments have not been fixed for a long time\. Perhaps these pieces of code contain not errors but only poorly written fragments in fully functional and secure code\. 

Whether the code fragments described in this article contain errors or not \- it would be great if developers reviewed the code again\. If you think that the analyzer would bombard you with false positives for such extraordinary code fragment, then we hurry to reassure you\. PVS\-Studio has a reliable mechanism for [suppressing false positives](https://pvs-studio.com/en/docs/manual/0032/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) that will not let you suffer=\)\.

## FirstOrDefault — love at first sight

Well, we should consider one more analyzer's warning\. The analyzer mentioned a dereference of the value returned by the *FirstOrDefault* method without any checks for *null* in 39 code fragments\. Take a look at the following code fragment:

```cpp
public async Task<IActionResult> AddContentItem(int deploymentPlanId,
                                                string returnUrl, 
                                                string contentItemId)
{
  var step = (ContentItemDeploymentStep)_factories.FirstOrDefault(x => 
              x.Name == nameof(ContentItemDeploymentStep)).Create();
  ....
}
```

PVS\-Studio warning: [V3146](https://pvs-studio.com/en/docs/warnings/v3146/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) Possible null dereference\. The '\_factories\.FirstOrDefault' can return default null value\. AddToDeploymentPlanController\.cs 77

The analyzer warns us that the *FirstOrDefault* method may return *null*\. This would lead to a *NullReferenceException*\. Most likely, developers do not expect *null* to appear during execution, so they thought no checks were required\. But why not *First*? Because the default value may still appear? Then, where is a check for *null*? In fact, the analyzer found 39 such cases\!

**Tip: Use *First* instead of *FirstOrDefault* where the sequence contains at least one element\. This approach will make code more readable\. Make your code as attractive as the websites created with Orchard\! =\)**

## Conclusion 

As in the [previous article](https://pvs-studio.com/en/blog/posts/csharp/0681/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core), I would like to mention the high quality of the Orchard project code base\! It's been three years\. And this time we found some warnings that we had already described in our previous articles\. However, the developers did a really great job over these years\.

Of course, we will continue to check open\-source projects and watch how developers find and fix errors after a long period of time\. But the inspection of code every three years is not enough\. If you want to get the maximum from using a static code analyzer, use it [regularly](https://pvs-studio.com/en/blog/posts/a0067/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core)\. 

[Check](https://pvs-studio.com/en/pvs-studio/download/?utm_source=dev_to&utm_medium=articles&utm_content=Orchard_Core) your project with our analyzer\! Perhaps you'll find really interesting warnings\.