Fix: URL-encoded special characters in $filter and $orderby break OData parsing#3080
Fix: URL-encoded special characters in $filter and $orderby break OData parsing#3080
Conversation
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
…ocumentation Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where URL-encoded special characters in $filter and $orderby query parameters were being double-decoded, causing OData parsing failures. The issue occurred because HttpUtility.ParseQueryString() decodes parameter values before they reach the OData parser, which also expects to decode them.
Changes:
- Added
RawQueryStringproperty to preserve URL-encoded query strings before decoding - Modified
$filterand$orderbyparsing to use raw (URL-encoded) values instead of decoded values - Added comprehensive unit and integration tests across all supported database types
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Core/Models/RestRequestContexts/RestRequestContext.cs | Added RawQueryString property to store the URL-encoded query string |
| src/Core/Services/RestService.cs | Populated RawQueryString before parsing query parameters |
| src/Core/Parsers/RequestParser.cs | Added ExtractRawQueryParameter() method and modified $filter/$orderby parsing to use raw values |
| src/Service.Tests/UnitTests/RequestParserUnitTests.cs | Added 14 unit tests validating URL encoding preservation in various scenarios |
| src/Service.Tests/SqlTests/RestApiTests/Find/*.cs | Added integration tests for all database types (MsSql, PostgreSQL, MySQL, DwSql) |
| src/Service.Tests/DatabaseSchema-*.sql | Added test data with special characters for integration tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
RubenCerna2079
left a comment
There was a problem hiding this comment.
Need to test more scenarios in which we use special characters not just &
src/Core/Parsers/RequestParser.cs
Outdated
| context.FilterClauseInUrl = sqlMetadataProvider.GetODataParser().GetFilterClause(filterQueryString, $"{context.EntityName}.{context.DatabaseObject.FullName}"); | ||
| // Use raw (URL-encoded) filter value to preserve special characters like & | ||
| string? rawFilterValue = ExtractRawQueryParameter(context.RawQueryString, FILTER_URL); | ||
| if (rawFilterValue is not null) |
There was a problem hiding this comment.
Is there any situation where we would expect the rawFilterValue parameter to be null? If so then we should throw an exception for it.
There was a problem hiding this comment.
Added exception handling in commit cffedaa. Now throws DataApiBuilderException with "Unable to extract $filter parameter from query string" if the parameter exists in ParsedQueryString but cannot be extracted from RawQueryString, indicating an unexpected state.
src/Core/Parsers/RequestParser.cs
Outdated
| (context.OrderByClauseInUrl, context.OrderByClauseOfBackingColumns) = GenerateOrderByLists(context, sqlMetadataProvider, sortQueryString); | ||
| // Use raw (URL-encoded) orderby value to preserve special characters | ||
| string? rawSortValue = ExtractRawQueryParameter(context.RawQueryString, SORT_URL); | ||
| if (rawSortValue is not null) |
There was a problem hiding this comment.
Is there any situation where we would expect the rawFilterValue parameter to be null? If so then we should throw an exception for it.
There was a problem hiding this comment.
Added exception handling in commit cffedaa. Now throws DataApiBuilderException with "Unable to extract $orderby parameter from query string" if the parameter exists in ParsedQueryString but cannot be extracted from RawQueryString, indicating an unexpected state.
|
|
||
| /// <summary> | ||
| /// Tests the REST Api for Find operation with a filter containing special characters | ||
| /// like ampersand (&) that need to be URL-encoded. This validates that the fix for |
There was a problem hiding this comment.
Are there other special characters that we could test?
There was a problem hiding this comment.
Added 3 additional integration test methods in commit cffedaa testing special characters: +, = (A+B=C), ampersand in different context (Tom & Jerry), and % (100% Complete). Test data added across all database schemas.
| /// parameter values, preserving special characters like ampersand (&). | ||
| /// </summary> | ||
| [DataTestMethod] | ||
| [DataRow("?$filter=region%20eq%20%27filter%20%26%20test%27", "$filter", "region%20eq%20%27filter%20%26%20test%27", DisplayName = "Extract filter with encoded ampersand")] |
There was a problem hiding this comment.
We should test other special characters besides the %26 which is &
There was a problem hiding this comment.
Added 6 more test cases in commit cffedaa testing various special characters beyond &: encoded =, +, %, #, <, >, :, /, and ?. Total unit tests increased from 14 to 20, all passing.
| } | ||
|
|
||
| /// <summary> | ||
| /// Tests that ExtractRawQueryParameter handles edge cases correctly. |
There was a problem hiding this comment.
Give a better explanation of what are the edge cases
There was a problem hiding this comment.
Improved documentation in commit cffedaa to clarify edge cases: duplicate parameters (returns first occurrence), case-insensitive parameter name matching, and malformed query strings with unencoded ampersands.
…nd improve documentation Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com>
Expanded test coverage in commit cffedaa:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Why make this change?
HttpUtility.ParseQueryString()decodes query parameter values, causing double-decoding when DAB constructs OData filter strings. A URL like?$filter=title eq 'A %26 B'becomestitle eq 'A & B', where the literal&is interpreted as a query parameter separator, truncating the filter totitle eq 'Aand producing "unterminated string literal" errors.What is this change?
Preserve URL encoding for OData parameters by extracting raw values before
ParseQueryString()decodes them:RawQueryStringproperty to store original encoded queryRawQueryStringalongsideParsedQueryStringExtractRawQueryParameter()extracts encoded values by splitting on unencoded&separatorsDataApiBuilderExceptionif parameter extraction fails unexpectedlyDatabase-agnostic: operates at HTTP parsing layer before any DB-specific processing.
How was this tested?
&,+,=,%filter & test,A+B=C,Tom & Jerry,100% Complete&,=,+,%,#,<,>,:,/,?Sample Request(s)
REST - Before (fails):
REST - After (succeeds):
Works with any URL-encoded special character:
%26(&),%3D(=),%2B(+),%25(%),%23(#),%3C(<),%3E(>),%3A(:),%2F(/),%3F(?), etc.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.