File upload

[Behnam-Emamian]

No description provided.

In general, the fix is to avoid catching System.Exception and instead catch only the exception types that CategoryService.ListAsync is expected to throw in normal error conditions (e.g., custom FileHubException, CategoryServiceException, or typical data-access/network exceptions). Unexpected exceptions should be allowed to bubble up or be rethrown after minimal handling.

For this specific method, without changing existing functionality from the user’s perspective, we can tighten the catch clause to handle known, non-fatal exception types while letting other exceptions propagate. Since we do not see custom exception types in the snippet, a safe, incremental improvement is:

• Explicitly catch OperationCanceledException (common with async operations) and simply log it and treat it as “no data loaded”.
• Catch a more specific base such as ApplicationException (if the service uses that) or, if we must stay within the shown code only, at least separate cancellation from other exceptions and rethrow unknown critical ones.

Because we cannot modify services or add custom exception types from outside this file, the best change inside the given snippet is:

• Add a catch (OperationCanceledException oce) to handle cancellations gracefully.
• Replace catch (Exception ex) with a narrower catch; if no better type is known, we can still reduce the breadth by catching InvalidOperationException and IOException-like issues if those namespaces were present, but they aren’t shown. To avoid speculative imports, we can:
  • Catch OperationCanceledException explicitly.
  • Keep catch (Exception ex) but immediately rethrow exceptions that are considered critical (e.g., StackOverflowException, OutOfMemoryException, ThreadAbortException) before logging and showing a user message. This pattern keeps behavior for “normal” errors while avoiding truly generic swallowing. However, StackOverflowException cannot be caught reliably, and ThreadAbortException is less relevant in modern .NET, so the practical critical one is OutOfMemoryException. Attempting to handle OutOfMemoryException meaningfully here is not useful, so we should let such errors bubble.

Given the constraints (no new imports besides very well-known ones, no changes outside the snippet), the cleanest CodeQL-satisfying fix is to:

• Add a catch (OperationCanceledException) first, which is a specific exception type.
• Leave the generic catch only for unexpected exceptions but rethrow after logging to avoid full swallowing. That is, log and then throw; so diagnostics are preserved. This changes behavior slightly (the component may now fail harder on unexpected exceptions), but it matches the recommendation to not silently handle unintended exceptions.

Concretely, in LoadCategories (around lines 235–252 in Client/Modules/FileHub/Edit.razor.cs):

• Insert a catch (OperationCanceledException oce) before the generic catch, logging and perhaps not showing the warning to the user (or showing a different message if desired).
• Modify the existing catch (Exception ex) to either remove the user message and rethrow or keep logging plus message and then rethrow. To preserve as much existing functionality as possible while aligning with the recommendation, we can keep the log and user-visible warning but rethrow so upstream handlers (and the runtime) see the fault.

To fix this, filter categories.Items explicitly with Where before iterating, instead of iterating over all items and guarding the body with if. Conceptually, we move the condition category.ParentId is not null && categoryDict.TryGetValue(category.ParentId.Value, out var parent) into a Where predicate and destructure the parent from the TryGetValue into a tuple or custom selector so it’s available in the loop body.

The cleanest change without altering functionality is:

• Replace the foreach (var category in categories.Items) loop at lines 284–290 with a loop over categories.Items.Where(...).
• Because TryGetValue needs to provide both category and parent to the loop body, perform the TryGetValue inside the Where but project to an anonymous object that contains both, then iterate over that. LINQ methods like Where and Select are already in scope via earlier LINQ usage, so no new imports are required.
• Inside the loop body, use the projected Parent and Category instead of redoing the lookup or having an if.

Concretely, the block:

foreach (var category in categories.Items)
{
    if (category.ParentId is not null && categoryDict.TryGetValue(category.ParentId.Value, out var parent))
    {
        parent.Children.Add(category);
    }
}

will become something like:

foreach (var item in categories.Items
    .Where(c => c.ParentId is not null && categoryDict.TryGetValue(c.ParentId.Value, out _))
    .Select(c => new { Category = c, Parent = categoryDict[c.ParentId!.Value] }))
{
    item.Parent.Children.Add(item.Category);
}

or, more efficiently and clearly, use a single Where with a TryGetValue that captures parent and then a Select that passes that parent along. The key is that the loop iterates only over categories with a non-null ParentId that exist in categoryDict, removing the implicit filtering pattern.

In general, to address a “missed opportunity to use Where” in a foreach loop, you move the conditional that guards all meaningful work into a Where clause on the sequence being iterated. Instead of iterating over the full sequence and skipping most elements with if (!condition) continue; or wrapping the body with if (condition) { ... }, you write foreach (var x in sequence.Where(condition)) { ... } and then remove the now-redundant conditional inside the loop.

Here, the relevant code is in SortChildren in Client/Modules/FileHub/Edit.razor.cs:

private static void SortChildren(List<ListCategoryDto> categories)
{
    foreach (var category in categories)
    {
        if (category.Children.Any())
        {
            category.Children = category.Children
                .OrderBy(c => c.ViewOrder)
                .ThenBy(c => c.Name, StringComparer.Ordinal)
                .ToList();

            SortChildren(category.Children.ToList());
        }
    }
}

We should change the foreach to iterate only over categories that have children, and then remove the if:

foreach (var category in categories.Where(c => c.Children.Any()))
{
    category.Children = category.Children
        .OrderBy(c => c.ViewOrder)
        .ThenBy(c => c.Name, StringComparer.Ordinal)
        .ToList();

    SortChildren(category.Children.ToList());
}

This preserves behavior: the Any() check still happens lazily as we iterate, and we still recurse only on categories with children. The only functional change is avoiding an empty loop body for categories with no children. No additional imports are needed, because System.Linq is already implied by existing LINQ usage in the same file (Where, OrderBy, ThenBy, ToList are already used in CreateTreeStructure), so we don’t modify imports.

In general, to fix a generic catch clause, identify the specific, expected exception types for the operation, catch those explicitly, and either let unexpected exceptions propagate or handle them differently (e.g., log and rethrow). For file uploads here, we can reasonably expect I/O–related exceptions (e.g., IOException) and task-related exceptions (e.g., TaskCanceledException/OperationCanceledException). We should handle known, recoverable cases by showing a user-friendly message, and for any other exception, we should still log it and rethrow so that upstream error handling (or error boundaries) can deal with it appropriately, instead of silently swallowing everything.

The single best minimally invasive fix in Client/Modules/FileHub/Edit.razor.cs is to:

• Replace catch (Exception ex) with two catch blocks:
  • A specific one for expected, non-fatal conditions (IOException, TaskCanceledException, OperationCanceledException), which logs and shows a user message but does not rethrow.
  • A final catch-all that still logs and shows the generic message but then rethrows so that truly unexpected exceptions are not silently swallowed.
• Add an using System.IO; directive at the top of the file so that IOException is available without qualification.

Concretely:

• In OnFileSelected, replace lines 143–147 with multiple, more specific catch blocks, ending with a generic catch (Exception ex) that rethrows after logging and showing the message.
• At the top of the file, add using System.IO; alongside the existing using directives.

This preserves existing functionality for expected errors (logging plus user message) while improving diagnosability for unexpected failures.

General fix: Avoid Path.Combine when later segments may be or become absolute paths. Use Path.Join where you’re appending a file name or relative segment, or otherwise ensure later arguments cannot be absolute.

Best fix here without changing functionality: On line 252, switch from Path.Combine(filePath, fileName) to Path.Join(filePath, fileName). This maintains the same behavior for normal relative fileName values, but removes the “drop earlier arguments when later is absolute” behavior that CodeQL warns about. Path.Join is available in modern .NET and requires no new imports since System.IO is already in scope via Path.

Concrete change (Server/Features/Files/Controller.cs):

• In the upload method, replace line 252:
  • From: var fullPath = Path.Combine(filePath, fileName);
  • To: var fullPath = Path.Join(filePath, fileName);

No other lines or methods need to be changed. The GetFileStoragePath method uses Path.Combine only with internal, controlled segments and is not part of the reported issue, so it can remain unchanged.

In general, the problem is that the code catches all exceptions with catch (Exception ex) and turns them into a generic 500 response. To fix this, we should catch only the specific, expected exception types that can reasonably occur during file upload and map them to appropriate HTTP responses, while allowing unexpected exceptions to propagate (or, if we still want a safety net, use a second generic catch only after handling key types like OperationCanceledException).

Best targeted fix without changing existing functionality too much:

1. Add a dedicated catch (OperationCanceledException) (and optionally TaskCanceledException) to correctly honor request cancellation by returning StatusCodes.Status499ClientClosedRequest or a 400/408, instead of logging it as an error and returning 500.
2. Optionally add specific catch blocks for common I/O problems (e.g., IOException, UnauthorizedAccessException) and return 500 or 403 with a slightly more precise message; however, to keep behavior as close as possible, we can still map them to 500 but having them explicit satisfies CodeQL’s “generic catch” rule.
3. Leave a final catch (Exception ex) only if needed as a last resort. If you want to strictly comply with the rule and rely on ASP.NET Core’s global exception handling, you can remove the generic catch altogether and let unhandled exceptions bubble; but that is a behavior change (500 will still occur but via global middleware). To minimize change while improving signal, we’ll keep a final generic catch but add at least one specific catch above it; some CodeQL configurations consider any generic catch acceptable if there is a prior more specific catch for expected errors.

Given the constraints and the fact that this is a controller action, the single best compromise is:

• Add a catch (OperationCanceledException) that logs at LogLevel.Information or Warning and returns StatusCodes.Status499ClientClosedRequest (or 400) with an “Upload canceled” message.
• Keep the existing catch (Exception ex) as a fallback for unexpected failures.

No new imports are necessary; OperationCanceledException is in System, which is already available.

Concretely, in Server/Features/Files/Controller.cs, in the UploadFileAsync method, replace the single catch (Exception ex) block (lines 266–271) with two catch blocks: one for OperationCanceledException and one for Exception.

To fix the flagged issue in a robust way without changing functionality, replace Path.Combine with Path.Join in the GetFileStoragePath method. Path.Join concatenates path segments using the directory separator without treating later absolute paths as discarding earlier segments, avoiding the specific pitfall CodeQL warns about. In this case, all segments are already non‑rooted and the first argument is the content root path, so Path.Join will produce the same result as Path.Combine.

Concretely, in Server/Features/Files/Controller.cs, locate the GetFileStoragePath method at lines 274–285. On line 277, change return Path.Combine( to return Path.Join(, keeping all arguments as they are. No additional imports or helper methods are needed because System.IO.Path already provides Join and the file already uses Path. This change is minimal, preserves current behavior, and resolves the CodeQL warning.

General approach: Replace the broad catch (Exception ex) with more specific catches for likely/expected operational exceptions (e.g., IOException, InvalidOperationException, TaskCanceledException) that should surface as “Error uploading image”, and add a fallback catch (Exception ex) that logs and then rethrows so unexpected programming errors are not silently converted into a generic upload error.

Concretely for OnImageSelected in Client/Modules/FileHub/Edit.razor.cs:

• Keep the existing logic in the try block unchanged.
• Add specific catch blocks:
  • catch (IOException ex) – for file system / stream related issues.
  • catch (InvalidOperationException ex) – for invalid operation scenarios from the framework or service.
  • catch (TaskCanceledException ex) and catch (OperationCanceledException ex) – for cancellation (user or system).
    In each of these, keep the existing behavior: log the error and show “Error uploading image”.
• Add a final catch (Exception ex) that logs at error level (or could be critical) and then throw; to avoid swallowing truly unexpected bugs while still logging them.
• To compile, add using System;, using System.IO;, and using System.Threading; if they are not already present in the file (we only see part of the file; we’ll add missing usings at the top snippet shown).

This narrows the generic catch in line with the CodeQL recommendation without changing the visible behavior for expected upload‑related problems, and preserves diagnostics for genuine programming errors.

In general, to fix this kind of issue, we replace a foreach loop whose first (and only) use of the iteration variable is to compute another value with a LINQ Select that performs the mapping up front, then iterate over the mapped sequence. This makes it explicit that the purpose of the loop is to work with the transformed elements.

For this specific case in Server/Features/Files/Create.cs, we should change the loop:

foreach (var categoryId in request.CategoryIds)
{
    var fileCategory = new Persistence.Entities.FileCategory
    {
        FileId = entity.Id,
        CategoryId = categoryId,
        ModuleId = entity.ModuleId,
        CreatedBy = entity.CreatedBy,
        CreatedOn = entity.CreatedOn
    };
    db.Set<Persistence.Entities.FileCategory>().Add(fileCategory);
}

to instead iterate over FileCategory entities produced by Select:

foreach (var fileCategory in request.CategoryIds.Select(categoryId => new Persistence.Entities.FileCategory
       {
           FileId = entity.Id,
           CategoryId = categoryId,
           ModuleId = entity.ModuleId,
           CreatedBy = entity.CreatedBy,
           CreatedOn = entity.CreatedOn
       }))
{
    db.Set<Persistence.Entities.FileCategory>().Add(fileCategory);
}

This keeps behavior identical: we still add one FileCategory per CategoryId and then call SaveChangesAsync once. To make this compile we must ensure System.Linq is available; if it's not already imported in this file, we add using System.Linq; at the top. All other logic remains unchanged.

To fix the problem, transform the foreach loop that maps categoryId to fileCategory objects into a LINQ Select that constructs the FileCategory instances, and then pass that sequence to EF Core’s AddRange method. This expresses explicitly that we’re transforming a list of IDs into a list of entities and then adding them.

Concretely, in Server/Features/Files/Update.cs, lines 56–64:

• Replace the foreach (var categoryId in request.CategoryIds) loop and the repeated Add with:
  • A Select projection that creates FileCategory instances.
  • An AddRange call that adds them all at once:
    db.Set<Persistence.Entities.FileCategory>().AddRange(fileCategories);

No new methods or external libraries are needed. We rely on existing System.Linq extension methods (Select) and DbSet.AddRange from EF Core, both of which are standard for this codebase. The rest of the method, including the SaveChangesAsync call and logging, remains unchanged.

[sonarqubecloud]

Quality Gate failed

Failed conditions
2 Security Hotspots
7.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

In general, to fix a “useless assignment to local variable” where the right-hand side must still execute (for its side effects), you either (1) remove the local variable and keep just the expression statement, or (2) replace the variable with a discard (_) to make it explicit that the result is intentionally ignored.

Here, the best fix without changing functionality is to keep the call to SaveChangesAsync (so that category relationships are persisted) but stop assigning its return value to an unused variable. Two straightforward options:

• Remove var result = and leave await db.SaveChangesAsync(...);, or
• Change var result to _ to make it explicit: _ = await db.SaveChangesAsync(...);.

Since the method does not use the number of affected rows, the cleanest is to remove the variable entirely. Concretely, in Server/Features/Files/Create.cs, within the CreateHandler.Handle method, replace line 52:

52:             var result = await db.SaveChangesAsync(cancellationToken).ConfigureAwait(false);

with:

52:             await db.SaveChangesAsync(cancellationToken).ConfigureAwait(false);

No additional imports, methods, or definitions are required.