Security when performing expansions to related entities.

[dotnetero]

Greetings community.

I am working on a project that has several tables with sensitive information, such as personal data, financial information or credentials. The goal is to be able to omit these properties while keeping the metadata intact. To do this, I came up with the idea of adding an attribute that inherits from EnableQuery, adding a property that allows me to parameterize possible related entities and their properties that can be read, but trying to prevent the end user from incorporating sensitive properties, as long as they request an expansion, otherwise everything above is omitted.
My question is whether I am doing the right thing, because I don't want to affect the performance that ASPNETCOREODATA offers us.

Attribute:

public class CustomEnableQuery : EnableQueryAttribute
{
    private string? _entitiesToProtect;
    /// <summary>
    /// It is required to write the entity and properties you want to display from the expansion, as follows: "Entity1| Property1.Property2.Property3:Entity2|Property1.Property2"
    /// </summary>
    public string? EntitiesToProtect
    {
        get => _entitiesToProtect;
        set => _entitiesToProtect = value;
    }

    public override void OnActionExecuting(ActionExecutingContext actionExecutingContext)
    {
        try
        {
            IQueryCollection queryCollection = actionExecutingContext.HttpContext.Request.Query;
            Dictionary<string, StringValues> newQuery = new();

            foreach (var e in queryCollection)
            {
                newQuery[e.Key] = ProcessQueryString(e.Value);
            }

            actionExecutingContext.HttpContext.Request.Query = new QueryCollection(newQuery);

            base.OnActionExecuting(actionExecutingContext);
        }
        catch (ODataException e)
        {
            throw new ODataException(e.Message);
        }
    }

    private string ProcessQueryString(string value)
    {
        Dictionary<string, string> entityProperties = new();
        string[] words = value.Split(",");

        foreach (string entities in _entitiesToProtect.Split(":"))
        {
            string[] entity = entities.Split("|");
            string properties = entity[1].Replace(".", ",");
            entityProperties[entity[0]] = properties;
        }

        for (int i = 0; i < words.Length; i++)
        {
            int lastLenght = 0;
            foreach (string entity in entityProperties.Keys)
            {
                if (words[i].Contains(entity))
                {
                    lastLenght = words[i].IndexOf(entity, lastLenght) + entity.Length;
                    if (words[i].Length <= lastLenght)
                    {
                        words[i] = words[i].Insert(lastLenght, $"($select={entityProperties[entity]})");
                    }
                    else if (words[i].ElementAt(lastLenght) == '(')
                    {
                        if (words[i].Substring(lastLenght + 1, 8) == "$select=")
                        {
                            int semicolonIndex = words[i].IndexOf(";", lastLenght + 1);
                            if (semicolonIndex > 0)
                            {
                                words[i] = words[i].Remove(lastLenght + 1, (semicolonIndex - 1) - lastLenght);
                                words[i] = words[i].Insert(lastLenght + 1, $"$select={entityProperties[entity]}");
                                continue;
                            }
                            int bracketsIndex = words[i].IndexOf(")", lastLenght + 1);
                            if (bracketsIndex > 0)
                            {
                                words[i] = words[i].Remove(lastLenght + 1, (bracketsIndex - 1) - lastLenght);
                                words[i] = words[i].Insert(lastLenght + 1, $"$select={entityProperties[entity]}");
                                continue;
                            }
                        }
                        words[i] = words[i].Insert(lastLenght + 1, $"$select={entityProperties[entity]};");
                    }
                    else
                    {
                        words[i] = words[i].Insert(lastLenght, $"($select={entityProperties[entity]})");
                    }
                }
            }
        }
        return string.Join(",", words);
    }
}

Usage:

    [CustomEnableQuery(EntitiesToProtect = "Usuario|Id.Nombre.Sobrenombre")]
    public IQueryable<SegUsuarioRol> Get()
    {
        return Ctx.SegUsuarioRol;
    }

Original query: https://localhost:7219/sapcore/SegUsuarioRol?$expand=Usuario($select=Password)
Result query: https://localhost:7219/sapcore/SegUsuarioRol?$expand=Usuario($select=Id,Nombre,Sobrenombre)

    [CustomEnableQuery(MaxExpansionDepth = 4, 
        EntitiesToProtect = "Modulo|Id.Concepto:Grupo|Id.Concepto:Categoria|Id.Concepto:CategoriaPrincipal|Id.Concepto")]
    public IQueryable<SegPermiso> Get()
    {
        return Ctx.SegPermiso;
    }

Original query: https://localhost:7219/sapcore/SegPermiso?$expand=Rol,Modulo($select=Small;$expand=Grupo($expand=Categoria($expand=CategoriaPrincipal)))
Result query: https://localhost:7219/sapcore/SegPermiso?$expand=Rol,Modulo($select=Id,Concepto;$expand=Grupo($select=Id,Concepto;$expand=Categoria($select=Id,Concepto;$expand=CategoriaPrincipal($select=Id,Concepto))))

Thank you very much for any comments you can provide me, as well as if there is already something that I am ignoring, I ask that you share a link or your knowledge with me.

[habbes]

The best way to assess the performance impact on your service is to measure. You can benchmark your service and see how much CPU time is spent on your ProcessQueryString method and whether that's significant for your service based on your performance targets.

From a high-level view, the method does seem like it has a lot of room for optimizations. It performs a lot nested O(n) operations that could be expensive if the query string is large and if this occurs on a hot path, e.g. IndexOf(), Remove(), Contains(). It also has a lot of string allocations from methods like string.Replace(), string.Split().

Finally, I think this approach may work well for simple cases but fail for slightly more complex scenarios or edge cases. For a more robust solution, you may consider using a parsed representation of the OData query options (e.g. ODataQueryOptions, or ODataUriParser's SelectExpandClause).

But if the solution works for your use cases, I don't think this will present the biggest performance bottleneck. But if it does, consider avoiding nested O(n) operations and consider using spans (ReadOnlySpan<T>) instead of string allocations where possible.

[dotnetero]

Thanks for the comments, I will work on implementing your recommendations.

[habbes]

There are recurring questions and attempts at restricting access to fields and nested properties. I think the library should provide an easier and flexible way for users to control what can be selected and expanded in which contexts.