Святослав Размыслов

17 Май 2023

Теги:

#CSharp

BTCPay Server: топ-10 ошибок в коде финансового приложения для Bitcoin

17 Май 2023

Автор: Святослав Размыслов

Введение
Результаты анализа
Заключение

Наша компания пишет много материалов на тему качества кода. Некоторые проекты, выбранные для аудита кода, не очень близки всем читателям, но все из вас точно пользуются финансовыми приложениями. Может, не конкретно этим, но статья о том, что разработка программ в этой сфере без применения подходов контроля качества несёт потенциальные риски для всех пользователей.

Введение

BTCPay Server — это бесплатный биткойн-платежный процессор с открытым исходным кодом, который позволяет принимать биткойны без комиссий и посредников.

Приложение активно разрабатывается и доступно на GitHub.

Проект было очень легко проверить с помощью статического анализатора PVS-Studio, т. к. он написан на C# и открывается в Visual Studio. Для этой IDE у нас есть плагин, позволяющий запускать анализ и визуализировать результаты.

В статью вошли 10 самых интересных примеров кода, которые анализатор посчитал ошибочными.

Результаты анализа

Пример 1

V3022 Expression 'request.PaymentTolerance < 0 && request.PaymentTolerance > 100' is always false. Probably the '||' operator should be used here. BTCPayServer\Controllers\GreenField\GreenfieldStoresController.cs 241

private IActionResult Validate(StoreBaseData request)
{
  ....
  if (request.PaymentTolerance < 0 && request.PaymentTolerance > 100)
    ModelState.AddModelError(nameof(request.PaymentTolerance),
      "PaymentTolerance can only be between 0 and 100 percent");
  ....
}

С отменой операций из-за неверного ввода в финансовых приложениях сталкивался каждый. Иногда приложения не очень удачно уведомляют пользователя о неверных данных, но иногда в коде просто есть глупая ошибка.

Так, в этом фрагменте кода перепутали операторы '&&' и '||'. Код предназначался для проверки значения в диапазоне от 0 до 100, но в итоге предупреждение пользователю не будет выдано.

Как результат, неверно введённое значение пройдёт дальше по коду и может привести к ошибке в логике приложения.

Пример 2

V3001 There are identical sub-expressions 'e.Name == InvoiceEvent.FailedToConfirm' to the left and to the right of the '||' operator. BTCPayServer\HostedServices\BitpayIPNSender.cs 264

public Task StartAsync(CancellationToken cancellationToken)
{
  ....
  if (invoice.FullNotifications)
  {
    if (e.Name == InvoiceEvent.Expired ||
        e.Name == InvoiceEvent.PaidInFull ||
        e.Name == InvoiceEvent.FailedToConfirm || // <=
        e.Name == InvoiceEvent.MarkedInvalid ||
        e.Name == InvoiceEvent.MarkedCompleted ||
        e.Name == InvoiceEvent.FailedToConfirm || // <=
        e.Name == InvoiceEvent.Completed ||
        e.Name == InvoiceEvent.ExpiredPaidPartial
    )
    {
      await Notify(invoice, e, false, sendMail);
      sendMail = false;
    }
  }

  if (e.Name == InvoiceEvent.Confirmed)
  {
    await Notify(invoice, e, false, sendMail);
    sendMail = false;
  }
  ....
}

Одинаковые сравнения свидетельствуют о том, что этот фрагмент кода писался методом copy-paste. Как результат, в сравнении участвуют две одинаковые константы.

Существует две трактовки найденного предупреждения:

Одна из констант лишняя, надо удалить.
После копирования забыли переименовать константу.

Я полагаю, что здесь именно второй случай, который является ошибкой.

Давайте взглянем на все константы:

public class InvoiceEvent : IHasInvoiceId
{
  public const string Created = "invoice_created";
  public const string ReceivedPayment = "invoice_receivedPayment";
  public const string PaymentSettled = "invoice_paymentSettled";
  public const string MarkedCompleted = "invoice_markedComplete";
  public const string MarkedInvalid = "invoice_markedInvalid";
  public const string Expired = "invoice_expired";
  public const string ExpiredPaidPartial = "invoice_expiredPaidPartial";
  public const string PaidInFull = "invoice_paidInFull";
  public const string PaidAfterExpiration = "invoice_paidAfterExpiration";
  public const string FailedToConfirm = "invoice_failedToConfirm";
  public const string Confirmed = "invoice_confirmed";
  public const string Completed = "invoice_completed";
  ....
}

Все константы можно условно разделить на 3 группы:

Создание платежей.
Подтверждение платежей.
Завершение платежей.

Тогда методом исключения можно предположить, что одна из копий должна быть константой PaidAfterExpiration, т. к. дублирующийся код и неиспользованная константа похожи на записи из группы про завершение платежей.

Пример 3

V3061 Parameter 'storeId' is always rewritten in method body before being used. BTCPayServer\Controllers\UIStoresController.cs 890

[HttpPost("{storeId}/tokens/create")]
public async Task<IActionResult> CreateToken(string storeId, ....)
{
  if (!ModelState.IsValid)
  {
    return View(nameof(CreateToken), model);
  }
  model.Label = model.Label ?? String.Empty;
  var userId = GetUserId();
  if (userId == null)
    return Challenge(AuthenticationSchemes.Cookie);
  storeId = model.StoreId; // <=
  ....
}

Значение параметра storeId перезаписывается до его использования. В большинстве случаев это свидетельствует об ошибке, т. к. код функции уже не соответствует её интерфейсу.

Пример 4

V3095 The 'request' object was used before it was verified against null. Check lines: 355, 364. BTCPayServer\Controllers\GreenField\GreenfieldPullPaymentController.cs 355

public async Task<IActionResult> CreatePayoutThroughStore(
  string storeId, CreatePayoutThroughStoreRequest request)
{
  if (request.Approved is true)
  {
    if (!(await _authorizationService.AuthorizeAsync(....)).Succeeded)
    {
      return this.CreateAPIPermissionError(....);
    }
  }

  if (request is null ||
    !PaymentMethodId.TryParse(request?.PaymentMethod, ....))
  {
    ModelState.AddModelError(nameof(request.PaymentMethod),
      "Invalid payment method");
    return this.CreateValidationError(ModelState);
  }
  ....
}

Обращение к переменной request сначала происходит без проверок, а потом с ними. С этим кодом точно что-то не так.

Пример 5

V3008 The 'model.StoreName' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 166, 165. BTCPayServer\Controllers\GreenField\GreenfieldStoresController.cs 166

private void ToModel(StoreBaseData restModel, StoreData model, ....)
{
  var blob = model.GetStoreBlob();
  model.StoreName = restModel.Name;
  model.StoreName = restModel.Name;
  model.StoreWebsite = restModel.Website;
  model.SpeedPolicy = restModel.SpeedPolicy;
  model.SetDefaultPaymentId(defaultPaymentMethod);
  ....
}

Анализатор обнаружил два одинаковых присваивания. Как и в примере номер 2, тут очевидный copy-paste стиль написания кода. Возможно, здесь присутствует ошибка.

Пример 6

V3029 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 59, 64. BTCPayServer\Hosting\BTCpayMiddleware.cs 59

public async Task Invoke(HttpContext httpContext)
{
  CultureInfo.CurrentCulture = CultureInfo.InvariantCulture;
  CultureInfo.CurrentUICulture = CultureInfo.InvariantCulture;
  try
  {
    var bitpayAuth = GetBitpayAuth(httpContext, out bool isBitpayAuth);
    var isBitpayAPI = IsBitpayAPI(httpContext, isBitpayAuth);
    if (isBitpayAPI && httpContext.Request.Method == "OPTIONS")
    {
      httpContext.Response.StatusCode = 200;
      httpContext.Response.SetHeader("Access-Control-Allow-Origin", "*");
      if (httpContext.Request.Headers.ContainsKey("...."))
      {
        httpContext.Response.SetHeader("Access-Control-Allow-Headers",
        httpContext.Request.Headers["...."].FirstOrDefault());
      }
      return; // We bypass MVC completely
    }
    httpContext.SetIsBitpayAPI(isBitpayAPI);
    if (isBitpayAPI) // <=
    {
      httpContext.Response.SetHeader("Access-Control-Allow-Origin", "*");
      httpContext.SetBitpayAuth(bitpayAuth);
    }
    if (isBitpayAPI) // <=
    {
      await _Next(httpContext);
      return;
    }
    ....
  }
  ....
}

Переменная isBitpayAPI проверяется два раза подряд. При этом она не изменяется, а блоки условных выражений сильно отличаются.

Можно предположить, что в блоке первого условия надо обновить состояние этой переменной таким образом:

isBitpayAPI = IsBitpayAPI(httpContext, isBitpayAuth);

Пример 7

V3125 The 'request' object was used after it was verified against null. Check lines: 136, 130. BTCPayServer\Controllers\GreenField\GreenfieldLightningNodeApiController.cs 136

public virtual async Task<IActionResult> OpenChannel(....)
{
  var lightningClient = await GetLightningClient(cryptoCode, true);
  if (request?.NodeURI is null)
  {
    ModelState.AddModelError(nameof(request.NodeURI),
      "A valid node info was not provided to open a channel with");
  }

  if (request.ChannelAmount == null)
  {
    ModelState.AddModelError(nameof(request.ChannelAmount), "....");
  }
  ....
}

Этот фрагмент кода похож на пример 4: тоже переменная request то проверяется на валидность, то нет. Но тут ошибка имеет другой характер.

В проекте присутствуют похожие участки кода, но содержащие дополнительный код между приведёнными условиями:

if (!ModelState.IsValid)
{
  return this.CreateValidationError(ModelState);
}

Т. е. во многих местах присутствует косвенная проверка, а в этом месте забыли её добавить.

Пример 8

V3168 Awaiting on expression with potential null value can lead to NullReferenceException. BTCPayServer\HostedServices\InvoiceWatcher.cs 383

private async Task<bool> UpdateConfirmationCount(InvoiceEntity invoice)
{
  ....
  var transactionResult = await _explorerClientProvider.GetExplorerClient(
    payment.GetCryptoCode())?.GetTransactionAsync(....);
  ....
}

Мы видим оператор '?.', наличие которого предполагает возращение значения null. В этом случае await null приведёт к исключению NullReferenceException.

Пример 9

V3022 Expression 'items == null' is always false. BTCPayServer\Services\Invoices\InvoiceRepository.cs 427

public async Task MassArchive(string[] invoiceIds, bool archive = true)
{
  using var context = _applicationDbContextFactory.CreateContext();
  var items = context.Invoices.Where(a => invoiceIds.Contains(a.Id));
  if (items == null)
  {
    return;
  }

  foreach (InvoiceData invoice in items)
  {
    invoice.Archived = archive;
  }

  await context.SaveChangesAsync();
}

Метод расширения Where возвращает не null, а пустую коллекцию, если в ней не оказалось подходящих условию элементов. Хотя в данном случае метод самописный, его реализация всё равно обращается к стандартному методу:

public static IQueryable<TEntity> Where<TEntity>(....) where TEntity : class
{
  return System.Linq.Queryable.Where(obj, predicate);
}

Можно предложить, что автор кода хотел не звать метод сохранения изменений для пустой коллекции, но из-за неправильного сравнения это всё равно происходит.

Пример 10

V3108 It is not recommended to return 'null' from 'ToString()' method. BTCPayServer\Controllers\UILNURLController.cs 364

public class LightningAddressSettings
{
  ....
  public override string ToString()
  {
    return null;
  }
  ....
}

Может не ошибка, но возвращать значение null из перегруженного метода ToString не очень хорошая идея, в том числе по рекомендациям Microsoft. Он вызывается в очень многих ситуациях, иногда даже неочевидным для разработчика образом, что может привести к потенциальным исключениям в приложении.

Заключение

Всего анализатор выдал 239 предупреждений, что вполне нормально для такого небольшого проекта. Там ещё есть что поисследовать. Но 10 предупреждений, как в примерах, могут снизить доверие к приложению, работающему с финансовой информацией.

Можно предположить, что методология статического анализа в проект никогда не внедрялась, даже с использованием бесплатных инструментов. Будем рады, если разработчики рассмотрят наше решение первым для контроля качества кода. Готовы предоставить временную лицензию.

Чтобы не повторить ошибок авторов проекта BTCPay Server, скачайте PVS-Studio. Он поддерживает анализ таких языков, как C, C++, C# и Java.

#CSharp