Stop passing raw API query options into the expression parser - #766
Merged
Conversation
$filter, $orderby and $select went from the query string straight into System.Linq.Dynamic.Core with no parsing restrictions, no field whitelist and no length limit, and with no try/catch - so a parse failure left OnActionExecuted as a 500 rather than a 400. The only guard was MaxLimit on $top. $orderby and $select no longer reach the parser as client text at all. They are read as field lists, checked against the projected model and rebuilt here, which removes the class of problem instead of filtering it. $filter still needs a real expression, so it is fenced on three sides: a ParsingConfig that resolves no types, disables the it/root context keywords, bans new, refuses to probe assemblies and denies Equals/ToString on object, backed by an empty custom type provider; a 512 character limit; and a whitelist that accepts only members of the model, seven cheap text methods and the parser's own keywords. String literals are stripped before the whitelist runs, so a product named "Password" stays searchable. $skip and $top now reject non-numeric and negative input instead of silently ignoring it. 13 tests cover the rejections, including reflection via it.GetType(), a fully qualified type, object construction and a field the model does not expose. One test exercises the ParsingConfig on its own, so a future change to the whitelist cannot quietly re-open type access. Store scoping (2.2 in the audit) is deliberately not part of this: the API requires ManageAccessAdminPanel, which no store or vendor group holds by default, the DTOs carry no store fields to filter on, and scoping the existing endpoints would break every global-admin integration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DataTestMethod is obsolete in MSTest 4; TestMethod carries DataRow on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type: bugfix
Issue
EnableQueryAttributetook$filter,$orderbyand$selectstraight from the query string and handed them toSystem.Linq.Dynamic.Core— with no parsing restrictions, no whitelist of queryable fields, and no limit on expression size:The only guard anywhere was
Configurations.MaxLimiton$top. With the defaultParsingConfigthe parser resolves types, honours theit/rootcontext keywords and acceptsnew, so a caller could reach well past the model being queried. There was also notry/catch: because the filter runs inOnActionExecuted, a parse failure left the pipeline as a 500 instead of a 400, and$skip=abcwas silently ignored rather than rejected.Reproduce (module enabled, token with admin panel access):
GET /api/Product?$filter=it.GetType().Assembly != nullor$select=with anything that is not a field of the model.Mitigating, and why this was not first in the queue: the module ships disabled (
"Grand.Module.Api": false) and every controller sits behindAuthorizeApiAdmin.Solution
$orderbyand$selectno longer reach the parser as client text at all. They are read as field lists, checked against the projected model and rebuilt inApiQueryOptions. That removes the class of problem rather than filtering it, and matches what those two options mean in OData anyway.$filterstill needs a real expression, so it is fenced on three sides:ParsingConfigthat resolves no types, disables theit/rootcontext keywords, bansnew, refuses to probe assemblies for additional types and deniesEquals/ToStringonobject, backed by an emptyIDynamicLinqCustomTypeProvider.Contains,StartsWith,EndsWith,ToLower,ToUpper,Trim,Length,Equals) and the parser's own keywords. String literals are stripped before the whitelist runs, so a product named"Password"stays searchable.$skipand$topnow reject non-numeric and negative input.ApiQueryOptionExceptionandParseExceptionmap to 400 with a message naming the offending field.Store scoping (item 2.2 of the same audit) is deliberately not in this PR. Verified in code: the API requires
StandardPermission.ManageAccessAdminPanel, which no store-manager or vendor group holds by default (they getManageAccessStoreManagerPanel/ManageAccessVendorPanel), so there is no narrower principal that could read another store through it. The DTOs carry noStores/LimitedToStoresto filter on —TableCollection<C>()deserializes entity documents into the DTO — and the only "current store" available to an API request comes from theHostheader, so scoping the existing endpoints would break every global-admin integration. A store-scoped API belongs behind its own permission and an explicit, default-off setting.Breaking changes
None to any published type. Behaviour changes for callers that were relying on the unrestricted parser:
$filternaming anything outside the queried model, or using type/reflection syntax, now returns 400 instead of executing.$selectaccepts a plain field list only; expressions such asName as Aliasnow return 400.$orderbyacceptsField [asc|desc]only.$skip/$topreturn 400 instead of being ignored.Every one of these was previously either an error waiting to happen or an unintended capability; no documented usage changes.
Testing
dotnet build ./GrandNode.slndotnet test src/Tests/Grand.Module.Api.Tests— 40 pass, 13 of them new."Grand.Module.Api": trueinFeatureManagementandBackendAPI.Enabledtotrue, then obtain a token viaPOST /token/create.GET /api/Product?$filter=Name.Contains("shirt")→ 200, filtered.GET /api/Product?$filter=it.GetType().Assembly != null→ 400, message names the rejected identifier. Same for$filter=PasswordHash != nulland$select=it.GetType().GET /api/Product?$select=Id,Name→ 200, projection contains those two fields only.GET /api/Product?$orderby=Name desc→ 200, sorted;?$orderby=Name sideways→ 400.GET /api/Product?$skip=-1→ 400;?$top=99999→ capped atConfigurations.MaxLimit.🤖 Generated with Claude Code