SonarSource / sonar-dotnet

Code analyzer for C# and VB.NET projects
https://redirect.sonarsource.com/plugins/csharp.html
GNU Lesser General Public License v3.0
761 stars 223 forks source link

New Rule Idea: Use Data Transfer Objects instead of Database Entities in Web Services #7762

Open zsolt-kolbay-sonarsource opened 11 months ago

zsolt-kolbay-sonarsource commented 11 months ago

TITLE Use Data Transfer Objects instead of Database Entities in Web Service input and output values

WHY IS THIS AN ISSUE Exposing database entities directly in web services can lead to various issues, such as tight coupling between the database schema and the service interface, potential data leakage, and unnecessary network traffic. In the context of microservices or high data volume applications, these issues can significantly impact the performance, security, and maintainability of your software. Moreover, changes in your database schema can directly affect clients of your service and may necessitate more frequent and potentially breaking updates.

NONCOMPLIANT CODE EXAMPLE

[HttpGet]
public async Task<ActionResult<IEnumerable<User>>> GetUsers()  // Noncompliant - return value is a database entity
{
    return await _context.Users.ToListAsync();
}

[HttpPost]
public async Task<ActionResult<User>> CreateUser(User user)  // Noncompliant - input parameter and return value are database entities
{
    _context.Users.Add(user);
    await _context.SaveChangesAsync();
    return Ok();
}

And the User entity looks like this:

[Table("Users")]
public class User
{
    [Key]
    public int UserId { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public bool IsAdmin { get; set; }
    ...
}

In this noncompliant example, the web service method GetUsers() is directly returning User entities from the database. This exposes the entire User schema to clients and tightly couples the service interface with the database schema. For example, this can unintentionally expose the IsAdmin field from the database. A malicious user can alter the HTTP request by setting that field to true, and thereby gain administrator privileges.

COMPLIANT SOLUTION

In this example, a UserDto is used instead of the User entity, which hides the internal database structure and allows only the necessary fields to be set by the client. Mapping between the DTO and the entity should be performed inside the method. Note: _mapper represents an object that would be responsible for converting between the DTO and the entity, usually created using a library like AutoMapper.

This approach ensures that clients are only able to manipulate data in the way intended by the API, and it also provides a clean separation between the public-facing interface of the API and the internal database schema.

[HttpGet]
public async Task<ActionResult<IEnumerable<UserDto>>> GetUsers()  // Compliant
{
    var users = await _context.Users.ToListAsync();
    return _mapper.Map<List<UserDto>>(users);
}

[HttpPost]
public async Task<ActionResult<UserDto>> CreateUser(UserDto userDto) // Compliant
{
    var user = _mapper.Map<User>(userDto);
    _context.Users.Add(user);
    await _context.SaveChangesAsync();
    return Ok();
}

RECOMMENDED CODING PRACTICES

To avoid the issues associated with directly exposing database entities in web services, consider adopting the following practices:

While using DTOs can add some complexity to your code, they offer significant benefits in terms of security, performance, and maintainability. Consider your specific project requirements and make an informed decision accordingly.

IMPLEMENTATION

The analyzer will validate every method declaration:

denis-troller commented 3 months ago

You can add that properly selecting your fields to map to a DTO can lead to increased performance for your storage engine because you avoid materializing unused fields.