Skip to content

Add UrlDecode option to FromRouteAttribute for full percent-decoding#65434

Open
mikekistler wants to merge 2 commits intodotnet:mainfrom
mikekistler:fromroute-urldecode
Open

Add UrlDecode option to FromRouteAttribute for full percent-decoding#65434
mikekistler wants to merge 2 commits intodotnet:mainfrom
mikekistler:fromroute-urldecode

Conversation

@mikekistler
Copy link
Contributor

Adds a UrlDecode property to IFromRouteMetadata and FromRouteAttribute that enables full RFC 3986 percent-decoding of route parameter values using Uri.UnescapeDataString(). This addresses the inconsistent decoding behavior where some characters (like %2F) are preserved while others (like %2B) are decoded by the server's path decoder.

The feature is opt-in (defaults to false) to maintain backward compatibility. Users enable it with [FromRoute(UrlDecode = true)].

Implementation covers:

  • IFromRouteMetadata interface (default interface method)
  • FromRouteAttribute (new UrlDecode property)
  • Minimal APIs runtime path (RequestDelegateFactory)
  • Minimal APIs source generator path (RDG EndpointParameterEmitter)
  • MVC model binding (SimpleTypeModelBinder)

Tests added: 10 new tests across Minimal APIs (runtime + RDG) and MVC covering decode-on, decode-off, null handling, and multi-character encoding.

Fixes #11544

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mikekistler mikekistler requested a review from a team as a code owner February 15, 2026 14:57
Copilot AI review requested due to automatic review settings February 15, 2026 14:57
@github-actions github-actions bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Feb 15, 2026
@mikekistler mikekistler added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions labels Feb 15, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds an opt-in UrlDecode switch to [FromRoute] / IFromRouteMetadata to enable full percent-decoding of route values (e.g., decoding %2F to /) across MVC model binding, Minimal APIs runtime binding, and the Request Delegate Generator path—addressing current inconsistent decoding behavior.

Changes:

  • Adds UrlDecode to IFromRouteMetadata (defaulting to false) and to FromRouteAttribute.
  • Implements full percent-decoding in Minimal APIs (runtime RequestDelegateFactory + RDG emitter) and MVC (SimpleTypeModelBinder) when UrlDecode = true.
  • Adds new test coverage for decode-on/decode-off and multi-encoding scenarios.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/Http/Http.Abstractions/src/Metadata/IFromRouteMetadata.cs Adds UrlDecode (default interface implementation) to the route metadata contract.
src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt Public API annotation for IFromRouteMetadata.UrlDecode.
src/Mvc/Mvc.Core/src/FromRouteAttribute.cs Adds UrlDecode to [FromRoute] for MVC + Minimal API attribute usage.
src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt Public API annotation for FromRouteAttribute.UrlDecode.
src/Http/Http.Extensions/src/RequestDelegateFactory.cs Applies URL decoding during Minimal API runtime route binding when enabled.
src/Http/Http.Extensions/gen/.../EndpointParameter.cs Carries UrlDecode through RDG’s static model and equality logic.
src/Http/Http.Extensions/gen/.../EndpointParameterEmitter.cs Emits Uri.UnescapeDataString for route params when UrlDecode is enabled.
src/Mvc/Mvc.Core/src/ModelBinding/Binders/SimpleTypeModelBinder.cs Applies URL decoding during MVC simple-type route binding when enabled.
src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs Adds Minimal API runtime tests for UrlDecode on/off and multi-character decoding.
src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateCreationTests.RouteParameter.cs Adds RDG tests for UrlDecode route decoding behavior.
src/Mvc/Mvc.Core/test/ModelBinding/Binders/SimpleTypeModelBinderTest.cs Adds MVC binder tests verifying decode-on/decode-off behavior.
Comments suppressed due to low confidence (1)

src/Http/Http.Extensions/gen/Microsoft.AspNetCore.Http.RequestDelegateGenerator/StaticRouteHandlerModel/EndpointParameter.cs:611

  • UrlDecode affects the emitted binding code, but it isn’t included in SignatureEquals. Since endpoint delegate de-duplication uses SignatureEquals, two otherwise-identical endpoints that differ only by UrlDecode could be treated as the same signature and reuse the wrong generated delegate. Include UrlDecode (and update the related signature hash if needed) in the signature comparison.
    public bool SignatureEquals(object obj) =>
        obj is EndpointParameter other &&
        SymbolEqualityComparer.IncludeNullability.Equals(other.Type, Type) &&
        // The name of the parameter matters when we are querying for a specific parameter using
        // an indexer, like `context.Request.RouteValues["id"]` or `context.Request.Query["id"]`

Comment on lines +1576 to +1583
return Expression.Block(
typeof(string),
new[] { tempVar },
Expression.Assign(tempVar, valueExpression),
Expression.Condition(
Expression.Equal(tempVar, Expression.Constant(null, typeof(string))),
Expression.Constant(null, typeof(string)),
Expression.Call(UriUnescapeDataStringMethod, tempVar)));
Copy link

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uri.UnescapeDataString can throw on malformed percent-encoding (e.g. trailing % or non-hex). In the minimal-API runtime path this would currently bubble out as a 500 instead of being treated as a binding failure (400 / ThrowOnBadRequest). Consider switching to Uri.TryUnescapeDataString (or wrapping in try/catch) and integrating failures into the existing parameter-binding failure flow (set wasParamCheckFailure, log/throw consistently).

Suggested change
return Expression.Block(
typeof(string),
new[] { tempVar },
Expression.Assign(tempVar, valueExpression),
Expression.Condition(
Expression.Equal(tempVar, Expression.Constant(null, typeof(string))),
Expression.Constant(null, typeof(string)),
Expression.Call(UriUnescapeDataStringMethod, tempVar)));
var unescapedVar = Expression.Variable(typeof(string), "unescapedRouteValue");
var tryUnescape = Expression.TryCatch(
Expression.Block(
typeof(string),
Expression.Assign(unescapedVar, Expression.Call(UriUnescapeDataStringMethod, tempVar)),
unescapedVar),
Expression.Catch(
typeof(FormatException),
tempVar));
return Expression.Block(
typeof(string),
new[] { tempVar, unescapedVar },
Expression.Assign(tempVar, valueExpression),
Expression.Condition(
Expression.Equal(tempVar, Expression.Constant(null, typeof(string))),
Expression.Constant(null, typeof(string)),
tryUnescape));

Copilot uses AI. Check for mistakes.
: GetValueFromProperty(property, itemProperty, key, GetExpressionType(parameter.ParameterType));

// Apply Uri.UnescapeDataString to fully decode percent-encoded route values (e.g. %2F → /)
if (urlDecode)
Copy link

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ApplyUrlDecode assumes the value expression is a string, but BindParameterFromProperty can produce non-string expressions (e.g. string[], StringValues) via GetExpressionType. If someone uses [FromRoute(UrlDecode = true)] on those parameter types, expression construction will throw due to the type mismatch. Restrict URL-decoding to cases where valueExpression.Type == typeof(string) (and/or only for supported route-binding target types).

Suggested change
if (urlDecode)
if (urlDecode && valueExpression.Type == typeof(string))

Copilot uses AI. Check for mistakes.
Comment on lines +211 to +213
codeWriter.StartBlock();
codeWriter.WriteLine($"{endpointParameter.EmitAssigningCodeResult()} = Uri.UnescapeDataString({endpointParameter.EmitAssigningCodeResult()});");
codeWriter.EndBlock();
Copy link

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated route handler code calls Uri.UnescapeDataString without handling malformed percent-encoding. With UrlDecode enabled this can turn an otherwise-valid request into an unhandled exception/500. Consider emitting a try/catch (or using Uri.TryUnescapeDataString) that marks binding as failed and follows the existing wasParamCheckFailure + logOrThrowExceptionHelper pattern.

Suggested change
codeWriter.StartBlock();
codeWriter.WriteLine($"{endpointParameter.EmitAssigningCodeResult()} = Uri.UnescapeDataString({endpointParameter.EmitAssigningCodeResult()});");
codeWriter.EndBlock();
codeWriter.StartBlock();
codeWriter.WriteLine("try");
codeWriter.StartBlock();
codeWriter.WriteLine($"{endpointParameter.EmitAssigningCodeResult()} = Uri.UnescapeDataString({endpointParameter.EmitAssigningCodeResult()});");
codeWriter.EndBlock();
codeWriter.WriteLine("catch (UriFormatException)");
codeWriter.StartBlock();
codeWriter.WriteLine("wasParamCheckFailure = true;");
codeWriter.EndBlock();
codeWriter.EndBlock();

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that Uri.TryUnescapeDataString() should be used for safety

Assert.Equal("domain/user", body);
}

[Fact]
Copy link

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new UrlDecode behavior has no coverage for malformed percent-encoding (e.g. "abc%", "%2", "%ZZ"). Adding tests for these cases would help lock down the intended behavior (binding failure vs exception) for the runtime factory path when UrlDecode = true.

Suggested change
[Fact]
[Fact]
public async Task FromRouteWithUrlDecodeTrue_AllowsIncompleteEscapeSequenceAtEnd()
{
var httpContext = CreateHttpContext();
var responseBodyStream = new MemoryStream();
httpContext.Response.Body = responseBodyStream;
httpContext.Request.RouteValues["userId"] = "abc%";
var factoryResult = RequestDelegateFactory.Create(
([FromRoute(UrlDecode = true)] string userId) => userId,
new() { RouteParameterNames = new[] { "userId" } });
await factoryResult.RequestDelegate(httpContext);
Assert.Equal(200, httpContext.Response.StatusCode);
var body = Encoding.UTF8.GetString(responseBodyStream.ToArray());
Assert.Equal("abc%", body);
}
[Fact]
public async Task FromRouteWithUrlDecodeTrue_AllowsSingleHexDigitEscapeSequence()
{
var httpContext = CreateHttpContext();
var responseBodyStream = new MemoryStream();
httpContext.Response.Body = responseBodyStream;
httpContext.Request.RouteValues["userId"] = "%2";
var factoryResult = RequestDelegateFactory.Create(
([FromRoute(UrlDecode = true)] string userId) => userId,
new() { RouteParameterNames = new[] { "userId" } });
await factoryResult.RequestDelegate(httpContext);
Assert.Equal(200, httpContext.Response.StatusCode);
var body = Encoding.UTF8.GetString(responseBodyStream.ToArray());
Assert.Equal("%2", body);
}
[Fact]
public async Task FromRouteWithUrlDecodeTrue_AllowsInvalidHexDigitsInEscapeSequence()
{
var httpContext = CreateHttpContext();
var responseBodyStream = new MemoryStream();
httpContext.Response.Body = responseBodyStream;
httpContext.Request.RouteValues["userId"] = "%ZZ";
var factoryResult = RequestDelegateFactory.Create(
([FromRoute(UrlDecode = true)] string userId) => userId,
new() { RouteParameterNames = new[] { "userId" } });
await factoryResult.RequestDelegate(httpContext);
Assert.Equal(200, httpContext.Response.StatusCode);
var body = Encoding.UTF8.GetString(responseBodyStream.ToArray());
Assert.Equal("%ZZ", body);
}
[Fact]

Copilot uses AI. Check for mistakes.
@dotnet-policy-service
Copy link
Contributor

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 23, 2026
@DeagleGross
Copy link
Member

Looks good, but:

  1. there is a public API change at IFromRouteMetadata.cs‎ - we should probably file the api-review meeting to agree on this change (even though it looks minor)

  2. I agree with changing to Uri.TryUnescapeDataString -> this is a secure option which will not throw and cause exceptions in the generated-code layer

@halter73
Copy link
Member

halter73 commented Mar 2, 2026

Wouldn't this just double-decode leading to potential security vulnerabilities? Kestrel an IIS should already be decoding the path aside from %2F.

// URI was encoded, unescape and then parse as UTF-8
pathLength = UrlDecoder.DecodeInPlace(path, isFormEncoding: false);

It looks like left a comment on an issue describing this behavior last year #30655 (comment).

We could try looking into using RawTarget to prevent the double decoding, but that would risk accidently parsing the route differently. It seems like the easier thing would be just to special case %2F and decode that and only that post-routing. But even that seems risky from a security perspective, because we couldn't distinguish between %252F vs %2F, and even if the app consistently treats those the same, front ends and proxies might not.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide an option on FromRouteAttribute that allows decoding the value being bound

4 participants