# The Orchard Core threequel\. Rechecking the project with PVS\-Studio

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\! 

![0982_Orchard/image1.png](https://import.viva64.com/docx/blog/0982_Orchard/image1.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/) or [here](https://pvs-studio.com/en/blog/posts/csharp/0681/) 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/) 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/) 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/)\!**

**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/) 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/) 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/) 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/) 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/) and [here](https://pvs-studio.com/en/blog/posts/csharp/0681/)\)\. 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/) 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/) 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/), 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/) 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/) 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/) 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/), 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/)\. 

[Check](https://pvs-studio.com/en/pvs-studio/try-free/) your project with our analyzer\! Perhaps you'll find really interesting warnings\.