Server-side network applications rarely get the chance to join the ranks of our reviews of errors found in open source software. This is probably due to their popularity. After all, we try to pay attention to the projects that readers themselves offer us. At the same time, servers often perform very important functions, but their performance and benefits remain invisible to most users. So, by chance, the code of ONLYOFFICE Community Server was checked. It turned out to be a very fun review.

Introduction

ONLYOFFICE Community Server — free open-source collaborative system developed to manage documents, projects, customer relationship and email correspondence, all in one place. On its website, the company emphasizes the security of its solutions with phrases such as "Run your private office with the ONLYOFFICE " and "Secure office and productivity apps". However, no tools for code quality control are apparently used in the development process.

It all started with the fact that I was looking through the source code of several network applications in search of inspiration for implementing one of my application ideas. The PVS-Studio analyzer was working in the background, and I was sending funny errors into the general corporate chat.

This resulted in several posts of error examples on Twitter:


Later, representatives commented on the tweet, and even later posted a denial of the problem:


Most likely, this is true. Anyway, this does not add points to the quality of the project. Let's see what else I managed to find there.

"Wizard" of checking input data

I am flabbergasted by the singularity of some developers' approaches to checking input data.

Warning 1

V3022 Expression 'string.IsNullOrEmpty("password")' is always false. SmtpSettings.cs 104

public void SetCredentials(string userName, string password, string domain)
{
    if (string.IsNullOrEmpty(userName))
    {
        throw new ArgumentException("Empty user name.", "userName");
    }
    if (string.IsNullOrEmpty("password"))
    {
        throw new ArgumentException("Empty password.", "password");
    }
    CredentialsUserName = userName;
    CredentialsUserPassword = password;
    CredentialsDomain = domain;
}

As you might have noticed, this code fragment sets the tone for the entire article. It can be described with the phrase "The code is funny, but the situation is terrible". One must be very tired to confuse the password variable with the string "password". This error allows code execution to continue with an empty password. According to the author of the code, the password is additionally checked in the program's interface. However, the programming process is designed so that previously written functions are often reused. Therefore, this error can manifest itself anywhere in the future. Always remember the importance of well-timed detecting errors in your code.

Warning 2

V3022 Expression 'String.IsNullOrEmpty("name")' is always false. SendInterceptorSkeleton.cs 36

V3022 Expression 'String.IsNullOrEmpty("sendInterceptor")' is always false. SendInterceptorSkeleton.cs 37

public SendInterceptorSkeleton(
  string name,
  ....,
  Func<NotifyRequest, InterceptorPlace, bool> sendInterceptor)
{
    if (String.IsNullOrEmpty("name"))                           // <=
        throw new ArgumentNullException("name");
    if (String.IsNullOrEmpty("sendInterceptor"))                // <=
        throw new ArgumentNullException("sendInterceptor");

    method = sendInterceptor;
    Name = name;
    PreventPlace = preventPlace;
    Lifetime = lifetime;
}

Suddenly, several similar errors were found in the code. It is funny at first, but one should think about the reasons for writing such code. Maybe this is a habit left after switching from another programming language. From our experience of checking C++ projects, when it comes to C++, errors are often brought by former Python programmers.

Warning 3

V3022 Expression 'id < 0' is always false. Unsigned type value is always >= 0. UserFolderEngine.cs 173

public MailUserFolderData Update(uint id, string name, uint? parentId = null)
{
    if (id < 0)
        throw new ArgumentException("id");
    ....
}

The id variable has an unsigned uint type. Therefore, the check is pointless here. Call of this function has to be given special attention. I wonder what is passed to this function. Most likely, the signed int type was used everywhere before, but after refactoring, the check remained.

Copy-Paste code

Warning 1

V3001 There are identical sub-expressions 'searchFilterData.WithCalendar == WithCalendar' to the left and to the right of the '&&' operator. MailSearchFilterData.cs 131


This code fragment had to be rendered as an image to convey the scale of the written conditional expression. It has a problem area. Specifying a place in the analyzer's message can hardly help a user find 2 identical checks. So let's use a red marker:


And here are the conditional expressions that the analyzer warned about. In addition to fixing this place, I would recommend that the author formatted the code better to avoid such errors in the future.

Warning 2

V3030 Recurring check. The '!String.IsNullOrEmpty(user)' condition was already verified in line 173. CommonLinkUtility.cs 176

public static string GetUserProfile(string user, bool absolute)
{
  var queryParams = "";

  if (!String.IsNullOrEmpty(user))
  {
      var guid = Guid.Empty;
      if (!String.IsNullOrEmpty(user) && 32 <= user.Length && user[8] == '-')
      {
        ....
}

The user string is checked 2 times in a row in the same way. Perhaps, this code can be slightly refactored. Although on the other hand, maybe in one of the cases the programmer wanted to check the absolute Boolean variable.

Warning 3

V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless WikiEngine.cs 688

private static LinkType CheckTheLink(string str, out string sLink)
{
    sLink = string.Empty;

    if (string.IsNullOrEmpty(str))
        return LinkType.None;

    if (str[0] == '[')
    {
        sLink = str.Trim("[]".ToCharArray()).Split('|')[0].Trim();
    }
    else if (....)
    {
        sLink = str.Split('|')[0].Trim();
    }
    sLink = sLink.Split('#')[0].Trim();    // <=
    if (string.IsNullOrEmpty(str))         // <=
        return LinkType.None;

    if (sLink.Contains(":"))
    {
      ....
    }
    ....
}

I'm sure you couldn't find the error here just by reviewing the fragment. The analyzer detected a useless check, which turned out to be a copy of the code from above. The sLink variable must be checked instead of the str variable.

Warning 4

V3004 The 'then' statement is equivalent to the 'else' statement. SelectelStorage.cs 461

public override string[] ListFilesRelative(....)
{
    var paths = new List<String>();
    var client = GetClient().Result;

    if (recursive)
    {
        paths = client.GetContainerFilesAsync(_private_container, int.MaxValue,
            null, MakePath(domain, path)).Result.Select(x => x.Name).ToList();
    }
    else
    {
        paths = client.GetContainerFilesAsync(_private_container, int.MaxValue,
            null, MakePath(domain, path)).Result.Select(x => x.Name).ToList();
    }
    ....
}

The analyzer found a very clear Copy-Paste code. Perhaps, the paths variable has to be evaluated recursively, but this wasn't done.

Warning 5

V3009 It's odd that this method always returns one and the same value of 'true'. MessageEngine.cs 318

//TODO: Simplify
public bool SetUnread(List<int> ids, bool unread, bool allChain = false)
{
    ....
    if (!chainedMessages.Any())
        return true;

    var listIds = allChain
        ? chainedMessages.Where(x => x.IsNew == !unread).Select(....).ToList()
        : ids;

    if (!listIds.Any())
        return true;
    ....
    return true;
}

The size of this function is 135 lines. Even the developers themselves left a comment that it should be simplified. The function code definitely needs some tweaks, because it also returns the same value in all cases.

Useless function calls

Warning 1

V3010 The return value of function 'Distinct' is required to be utilized. DbTenantService.cs 132

public IEnumerable<Tenant> GetTenants(string login, string passwordHash)
{
  //new password
  result = result.Concat(ExecList(q).ConvertAll(ToTenant)).ToList();
  result.Distinct();
  ....
}

The Distinct method removes duplicates from the collection. But in C#, most of these extension methods do not change the object, but create a copy. So in this example, the result list remains the same as it was before the method was called. You can also see the names login and passwordHash. This may be another security issue.

Warning 2

V3010 The return value of function 'ToString' is required to be utilized. UserPhotoManager.cs 678

private static void ResizeImage(ResizeWorkerItem item)
{
  ....
  using (var stream2 = new MemoryStream(data))
  {
      item.DataStore.Save(fileName, stream2).ToString();

      AddToCache(item.UserId, item.Size, fileName);
  }
  ....
}

The ToString method is standard here. It returns a text representation of the object, but the return value is not used.

Warning 3

V3010 The return value of function 'Replace' is required to be utilized. TextFileUserImporter.cs 252

private int GetFieldsMapping(....)
{
  ....
  if (NameMapping != null && NameMapping.ContainsKey(propertyField))
  {
      propertyField = NameMapping[propertyField];
  }

  propertyField.Replace(" ", "");
  ....
}

Someone made a serious mistake. It was necessary to remove all spaces from the propertyField property, but this did not happen, because the Replace function did not change the source object.

Warning 4

V3038 The '"yy"' argument was passed to 'Replace' method several times. It is possible that other argument should be passed instead. MasterLocalizationResources.cs 38

private static string GetDatepikerDateFormat(string s)
{
    return s
        .Replace("yyyy", "yy")
        .Replace("yy", "yy")   // <=
        .Replace("MMMM", "MM")
        .Replace("MMM", "M")
        .Replace("MM", "mm")
        .Replace("M", "mm")
        .Replace("dddd", "DD")
        .Replace("ddd", "D")
        .Replace("dd", "11")
        .Replace("d", "dd")
        .Replace("11", "dd")
        .Replace("'", "")
        ;
}

Here, calls to Replace functions are written correctly, but in one place it is done with strange identical arguments.

Potential NullReferenceException

Warning 1

V3022 Expression 'portalUser.BirthDate.ToString()' is always not null. The operator '??' is excessive. LdapUserManager.cs 436

public DateTime? BirthDate { get; set; }

private bool NeedUpdateUser(UserInfo portalUser, UserInfo ldapUser)
{
  ....
  _log.DebugFormat("NeedUpdateUser by BirthDate -> portal: '{0}', ldap: '{1}'",
      portalUser.BirthDate.ToString() ?? "NULL",  // <=
      ldapUser.BirthDate.ToString() ?? "NULL");   // <=
  needUpdate = true;
  ....
}

ToString won't have the null value. The check was made here in order to output the "NULL" value to the debug log if the date is not set. However, since the ToString method returns an empty string if there is no value, the error in the algorithm may be less noticeable in the logs.

The entire list of questionable logging places looks like this:

• V3022 Expression 'ldapUser.BirthDate.ToString()' is always not null. The operator '??' is excessive. LdapUserManager.cs 437
• V3022 Expression 'portalUser.Sex.ToString()' is always not null. The operator '??' is excessive. LdapUserManager.cs 444
• V3022 Expression 'ldapUser.Sex.ToString()' is always not null. The operator '??' is excessive. LdapUserManager.cs 445

Warning 2

V3095 The 'r.Attributes["href"]' object was used before it was verified against null. Check lines: 86, 87. HelpCenterStorage.cs 86

public override void Init(string html, string helpLinkBlock, string baseUrl)
{
    ....
    foreach (var href in hrefs.Where(r =>
    {
        var value = r.Attributes["href"].Value;
        return r.Attributes["href"] != null
               && !string.IsNullOrEmpty(value)
               && !value.StartsWith("mailto:")
               && !value.StartsWith("http");
    }))
    {
      ....
    }
    ....
}

When parsing Html or Xml, it is very dangerous to access attributes by name without checking. This error is particularly stunning because the value of the href attribute is first extracted and then checked to see if it is present at all.

Warning 3

V3146 Possible null dereference. The 'listTags.FirstOrDefault' can return default null value. FileMarker.cs 299

public static void RemoveMarkAsNew(....)
{
  ....
  var listTags = tagDao.GetNewTags(userID, (Folder)fileEntry, true).ToList();
  valueNew = listTags.FirstOrDefault(tag => tag.EntryId.Equals(....)).Count;
  ....
}

The analyzer detected unsafe use of the result of calling the FirstOrDefault method. This method returns the default value if there is no object in the list that matches the search predicate. The default value for reference types is an empty reference (null). Accordingly, before using the resulting reference, one must check it, and not call the property immediately, as opposed to as it is here.

Warning 4

V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. ResCulture.cs 28

public class ResCulture
{
    public string Title { get; set; }
    public string Value { get; set; }
    public bool Available { get; set; }

    public override bool Equals(object obj)
    {
        return Title.Equals(((ResCulture) obj).Title);
    }
    ....
}

Object references in C# are often compared with null. Therefore, when overloading comparison methods, it is very important to anticipate such situations and add the appropriate check to the beginning of the function. In this case the authors did not do it here.

Other errors

Warning 1

V3022 Expression is always true. Probably the '&&' operator should be used here. ListItemHistoryDao.cs 140

public virtual int CreateItem(ListItemHistory item)
{
    if (item.EntityType != EntityType.Opportunity ||   // <=
        item.EntityType != EntityType.Contact)
        throw new ArgumentException();

    if (item.EntityType == EntityType.Opportunity &&
        (DaoFactory.DealDao.GetByID(item.EntityID) == null ||
         DaoFactory.DealMilestoneDao.GetByID(item.StatusID) == null))
        throw new ArgumentException();

    if (item.EntityType == EntityType.Contact &&
        (DaoFactory.ContactDao.GetByID(item.EntityID) == null ||
         DaoFactory.ListItemDao.GetByID(item.StatusID) == null))
        throw new ArgumentException();
    ....
}

The call of the CreateItem method will result in an ArgumentException. The fact is that the very first conditional expression contains an error. The condition always has the result true. The error is in choosing a logical operator. One should have used the && operator.

Most likely, this method has never been called before, as it is virtual and has always been overridden in derived classes until now.

To avoid such errors in future, I recommend reading my article: "Logical Expressions in C, C++, C#, and Java. Mistakes Made by Professionals". Don't forget to save the link to it. You can find review of all erroneous combinations from logical operators there.

Warning 2

V3052 The original exception object 'ex' was swallowed. Stack of original exception could be lost. GoogleDriveStorage.cs 267

public DriveFile CopyEntry(string toFolderId, string originEntryId)
{
    var body = FileConstructor(folderId: toFolderId);
    try
    {
        var request = _driveService.Files.Copy(body, originEntryId);
        request.Fields = GoogleLoginProvider.FilesFields;
        return request.Execute();
    }
    catch (GoogleApiException ex)
    {
        if (ex.HttpStatusCode == HttpStatusCode.Forbidden)
        {
            throw new SecurityException(ex.Error.Message);
        }
        throw;
    }
}

The GoogleApiException exception was converted to SecurityException while losing information from the original exception that might be useful.

This small change will make the generated warning more informative:

throw new SecurityException(ex.Error.Message, ex);

Although it may well be possible that the GoogleApiException exception was hidden intentionally.

Warning 3

V3118 Minutes component of TimeSpan is used, which does not represent full time interval. Possibly 'TotalMinutes' value was intended instead. NotifyClient.cs 281

public static void SendAutoReminderAboutTask(DateTime scheduleDate)
{
    ....
    var deadlineReminderDate = deadline.AddMinutes(-alertValue);

    if (deadlineReminderDate.Subtract(scheduleDate).Minutes > 1) continue;
    ....
}

I used to think that diagnostics are of the precautionary nature. As for code of my projects, it always gave false warnings. In this case, I'm almost sure there was an error. Most likely, one should have used the TotalMinutes property instead of Minutes.

Warning 4

V3008 The 'key' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 244, 240. Metadata.cs 244

private byte[] GenerateKey()
{
    var key = new byte[keyLength];

    using (var deriveBytes = new Rfc2898DeriveBytes(Password, Salt, ....))
    {
        key = deriveBytes.GetBytes(keyLength);
    }

    return key;
}

The problem with this fragment is that when you enter a function, an array of bytes is always created, and then gets immediately reassigned. In other words, there is constant allocation of memory, which does not make sense.

The best way would be to switch to C#8 instead of C#5 and write shorter code:

private byte[] GenerateKey()
{
  using var deriveBytes = new Rfc2898DeriveBytes(Password, Salt, ....);
  return deriveBytes.GetBytes(keyLength);
}

I can't say whether the project can be upgraded or not, but there are quite a few such places. It is best to rewrite them somehow:

• V3008 The 'hmacKey' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 256, 252. Metadata.cs 256
• V3008 The 'hmacHash' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 270, 264. Metadata.cs 270
• V3008 The 'paths' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 512, 508. RackspaceCloudStorage.cs 512
• V3008 The 'b' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 265, 264. BookmarkingUserControl.ascx.cs 265
• V3008 The 'taskIds' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 412, 391. TaskDao.cs 412

As a last resort, one can refuse from allocating memory when declaring a variable.

Bug in PVS-Studio

You might think we only write about other people's mistakes. No, our team is self-critical, admits its mistakes and doesn't hesitate to write about them too. Everyone makes mistakes.

While working on the article, we found a rather stupid bug. We recognize it and would like to share it with you.

Code from the same Community Server:

private bool IsPhrase(string searchText)
{
    return searchText.Contains(" ") || searchText.Contains("\r\n") ||
                                       searchText.Contains("\n");
}

Normally, I would cite a full analyzer warning before the code, as is done in the entire article, but that's the problem. The warning looks like this:


The \r and \n control characters are not escaped before being output to the table.

Conclusion

It's been a long time since I came across such an interesting project to check. Thanks to the authors of ONLYOFFCE. We contacted you, but there was no feedback.

We regularly write such articles. This genre is more than ten years old. Therefore, developers should not take criticism to heart. We will be happy to share a full version of the report to improve the project or provide a temporary license to review the project. This refers not only to the developers of CommunityServer project, but to everyone who wants to use the #onlyoffice promo code to be able to use the analyzer for ONE MONTH for free.


Security experts will also be interested to know that we are actively supporting the OWASP standard. Some diagnostics are already available. The analyzer interface will soon be updated to make it even more convenient to enable a particular standard for code analysis.