Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
SchroterQuentin opened this issue Jun 25, 2019 · 29 comments
Labels
affected-medium This issue impacts approximately half of our customers area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-model-binding severity-minor This label is used by an internal tool
Milestone

Comments

@SchroterQuentin
Copy link

The rights of my users are at /api/admin/users/{userId}/rights

And my controller is as simple as

[Route("/api/admin/users/{userId}/rights")]
public async Task<IActionResult> GetRights([FromRoute]string userId)
{
    var rights = await _someService.GetRights(userId);
    return Ok(rights);
}

or

[HttpPut("{userId}")]
public async Task<IActionResult> Update(string userId, [FromBody]UpdateUserViewModel parameters)
{
    var user = await _employeService.Update(userId, parameters);
    return Ok(user);
}

The problem I have is that, the userIds of my users may contains a / which is encoded to %2F in the Uri. But userId doesn't decode %2F so my string contains %2F. It's fine for me, I can deal with that.

But the userIds of my users may contains a + which is encoded to %2B in the Uri. And now, the userId decode the %2B to + 😲

Currently, I can't use WebUtility.UrlDecode(userId) because userId may contains a + which would be send as %2B decoded as + and finally to . My only actual solution is to replace %2F to / which is ugly and does not solve all the possibility : %252F

I saw a recommandation to use [FromQuery] instead of [FromRoute] but it's obvious that if both exist, it's because they have semantic difference.

It seems that's not the first time the problem appears : aspnet/Mvc#4599, #2655, #4445 and I'd like to know if it's on the roadmap to change this behavior or not.

Could you please this time consider this bug ? I'd be happy to help.

@analogrelay analogrelay added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jun 25, 2019
@mkArtakMSFT
Copy link
Member

Thanks for contacting us, @SchroterQuentin.
We'll provide an option so that FromRoute attribute decodes specific (by you) parameters.

@mkArtakMSFT mkArtakMSFT added this to the 3.1.0 milestone Jun 28, 2019
@mkArtakMSFT mkArtakMSFT added enhancement This issue represents an ask for new feature or an enhancement to an existing one PRI: 1 - Required labels Jun 28, 2019
@pranavkm pranavkm changed the title DataBinding from Uri path - string decode Provide an option on FromRouteAttribute that allows decoding the value being bound Jun 28, 2019
@SchroterQuentin
Copy link
Author

The problem exist on [HttpGet] [HttpPost] [HttpPut] etc. Is there a way to handle all this problems in one area ? Or do we have to add this options on all this attributes ?

I'm actually not understanding how would work this option ?

@rynowak rynowak removed their assignment Jul 4, 2019
@bachratyg
Copy link

bachratyg commented Jul 10, 2019

Also see aspnet/Mvc#6388

The hosts partially decodes the percent encoding and explicitly skips decoding %2F here:

Microsoft.AspNetCore.Server.IIS > Microsoft.AspNetCore.HttpSys.Internal.RequestUriBuilder.UnescapePercentEncoding
Microsoft.AspNetCore.Server.HttpSys > Microsoft.AspNetCore.HttpSys.Internal.RequestUriBuilder.UnescapePercentEncoding
Microsoft.AspNetCore.Server.Kestrel.Core > Microsoft.AspNetCore.Internal.UrlDecoder.UnescapePercentEncoding

The main problem is Routing or MVC model binding cannot reliably decode the partially decoded segment. Ideally the server should not do any decoding especially not partial. (Full decoding is not possible since it would introduce new segments.)

If the server behavior is not allowed to change (even behind a compatibility flag) then the router middleware could be modified to use the raw request url from IHttpRequestFetaure.RawTarget (which is not yet decoded) to get proper encoded tokens then decode as needed.

A possible workaround: inject a middleware before routing takes place that restores the original undecoded path from IHttpRequestFeature.RawTarget then in another middleware after routing decode the RouteData tokens.

@pranavkm
Copy link
Contributor

As part of doing this, ensure binding to non-string values that require decoded values work. See #11134

@ArthurHNL
Copy link

Can we get an update on this? I just spent half an hour debugging my API before I found out that Route parameters are not URL decoded by default.

@DanielBryars
Copy link

Yeah, this behaviour is pretty weird.

If you can't fix it then this FromRouteAttribute option would be a good compromise for me. Or perhaps provide an example implementation of the middleware suggested by https://github.com/Eilon in aspnet/Mvc#6388

@stefanluchian
Copy link

I see that this bug lives since at least 2017 and there is not even a workaround about it.
In .Net Core you don't allow us to do WCF anymore, so we are forced to do REST.
In REST we need to send parameters via Route, not via Query.
So we need to use [FromRoute], which should happen short before Model-Binding, but definitely after Routing.
So there is no worry about decoding %2B into /.
Then why is nothing happening here?
You closed any other Issues (at least 4 peaces counted) and now you let this open for loooong time.
How many developers do you need to comment on this issue, before you consider it worthy to look upon?

@pranavkm
Copy link
Contributor

Can we get an update on this?

We will consider fixing this in the upcoming 5.0 release. That said, the plan is to not have this decoding on by default - you explicitly would have to opt-in to this behavior. For current releases, you should be able to unblock yourselves by explicitly url decoding values in your action that you know require to be decoded.

@mkArtakMSFT
Copy link
Member

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@pranavkm pranavkm added affected-medium This issue impacts approximately half of our customers severity-minor This label is used by an internal tool labels Nov 6, 2020
@skovalyova
Copy link

I face the same issue when passing encoded URI as part of route. In this case, : is decoded automatically, but // remain encoded (http:%2F%2F). I have to use HttpUtility.UrlDecode as a workaround or use query parameters instead.

@ghost
Copy link

ghost commented Dec 28, 2021

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@Nils-Berghs
Copy link

Nils-Berghs commented Feb 16, 2022

What am I missing? I can't understand that this isn't fixable in 6 years. It is very easy:

  • Evaluate the route
  • Decode route params
  • Pass them to the controller action/page/...

A solution is even given in the workaround described by @celluj34 The soltution described by @celluj34 does NOT always work. It works for the / but if there are other 'escaped' characters in the route param they are already decoded, and decoding them again may cause issues.

Examples (input string in unencoded form):

  • 'ab/cd' => decodes ok
  • 'ab%cd' => decodes ok
  • 'ab%12' => ERROR, in the model binder the % character is already encoded and decoding again will decode %12

THERE IS NO SOLUTION FOR THIS PROBLEM POSSIBLE
An idea would be to replace %2f by /, but there is no way to know if the percentage was already decoded or not!

Is there any reason why route parameters should not be fully URL decoded, by default? I can think of none. And if there is, is it as devastating as the problem above? In short route parameter can not be used in asp .net core (unless you know all possible values in advance....)

@asprna
Copy link

asprna commented Apr 13, 2022

For me, this only happens when the controller inherits from ControllerBase class. The workaround for me is that instead of ControllerBase, I inherit the controller from the Controller class.

@Nils-Berghs
Copy link

@asprna I can not confirm this, for me it happens both with Controller and ControllerBase as base class. (Maybe this depends on the .net core version, after all this bug could have been fixed in 3.1)

@asprna
Copy link

asprna commented Apr 16, 2022

@Nils-Berghs I am currently using .net core 3.1

@asprna
Copy link

asprna commented Apr 16, 2022

@Nils-Berghs Actually you are right. The actual workaround is to get the value from the query string.

[ApiController] [Route("api/v1/[controller]")] public class TestController : ControllerBase { [HttpGet("Successful")] public async Task<IActionResult> Successful([FromQuery] string token) { return Ok(token); } }

@kammerjaeger
Copy link

What is the current state of this? It is another 3/4 of a year, when will we see a fix?
The user input "abc/cde/xyz%2Fuvw" cannot be differentiated on the server side from "abc/cde/xyz/uvw" or any other combination.
This is a serious problem.

@ghost
Copy link

ghost commented Oct 11, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@thegrahamking
Copy link

We are experiencing this issue and, running on Azure App Service, have yet to find a workaround other than switching to FromQuery which makes this one route inconsistent with all the others in our RESTful service.

This issue needs to be fixed or a code example of a valid workaround which allows us to continue using FromRoute provided.

@Nils-Berghs
Copy link

Nils-Berghs commented Dec 14, 2022

BUMP, please stop development on .net 8, and fix bugs like this first....

  • Moved to 3.1 milestone (Jun 2019)
  • Moved to 5 planning (Jan 2020)
  • Moved to 6 planning
  • Moved to 7 planning (Dec 2021)
  • Moved to 8 planning (Oct 2022)

More managers / scrum master than developers?

@davidfowl
Copy link
Member

cc @Tratcher

@Tratcher
Copy link
Member

The real fix here is much deeper: #30655. The server needs to make the path available as individual, fully decoded segments so that the the app / model binder isn't left dealing with this ambiguous escaping state. Routing would need to be updated to consume the path as individual segments. Model binding shouldn't need to be updated once routing is delivering the correct route values.

@JMonsorno
Copy link

Following the example from @celluj34 I made a sample app and library, don't judge the 5.0 references, that works for my purposes and addresses others concerns; namely @Nils-Berghs example and my own of raw "%2525" testcase. Feeling it's a bit unpolished for a nuget package but will decode the raw route - following some Angular influences I called "unsafe" - or if you want the controller to handle it uniquely it return back the raw value all by application level setup and controller level replacement of [FromRoute] with [FromRouteUnsafe] or [FromRouteRaw] respectively.

https://github.com/JMonsorno/CustomRouteBinderProvider

BigRedProf added a commit to BigRedProf/stories that referenced this issue Apr 14, 2023
@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-web-frameworks labels Jun 20, 2023
@code-bringer
Copy link

code-bringer commented Nov 29, 2023

I've just encountered this bug :( Is there any update on it? It would be nice to have some options that may change routing behavior for this specific case with slash encoding.
My use case is the following:
I have entity keys from multiple external systems, for some of them these keys look like numbers, for others, it could be guids. But of course, there is a system with keys that with slashes inside(it mimics some folder structure).
So in my case, I already have multiple endpoints with the following routing scheme:

[HttpGet("documents/for/{externalKeyId}/{documentType}")]
public async IAsyncEnumerable<Document> GetDocuments(
	[FromRoute] string externalKeyId,
	[FromRoute] DocumentType documentType,
	[FromQuery] DateTime? startDate,
	[FromQuery] DateTime? endDate,
	[EnumeratorCancellation] CancellationToken cancel = default
)

So as you can see keys with slashes inside are real pain in the neck.

@Odonno
Copy link

Odonno commented Jan 25, 2024

Having a use case for this at the moment, it is frustrating to have an incomplete decoded string. %20 works fine but not %2F.

I am gonna go with the solution provided by @JMonsorno until a definitive solution is implemented.

@rudism
Copy link

rudism commented Feb 1, 2024

Here's my slightly simplified version of @JMonsorno's code that will apply the modified decoding behavior to all [FromRoute] string parameters by default without using custom parameter attributes:

using Microsoft.AspNetCore.Http.Features;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.ModelBinding;
using Microsoft.AspNetCore.Mvc.ModelBinding.Binders;
using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;

namespace My.Project.Namespace;

public class RouteUnsafeBinder : IModelBinder {
  public Task BindModelAsync(ModelBindingContext bindingContext) {
    ArgumentNullException.ThrowIfNull(bindingContext);

    var modelName = bindingContext.ModelName;

    // make sure the param we want was extracted in the standard method
    // this will be the partially-decoded value that we're looking to avoid using here
    if (!bindingContext.ActionContext.RouteData.Values.ContainsKey(modelName))
      throw new NotSupportedException();

    // wrap the param in curly braces so we can look for it in the [Route(...)] path
    var templateToMatch = $"{{{modelName}}}";

    // if this is null then the developer forgot the [Route(...)] attribute
    var request = bindingContext.HttpContext.Request;
    var template = (bindingContext.ActionContext.ActionDescriptor.AttributeRouteInfo?.Template)
      ?? throw new NotSupportedException();

    // get the raw url that was called
    var rawTarget = bindingContext.HttpContext.Features.Get<IHttpRequestFeature>()?.RawTarget
      ?? throw new NotSupportedException();
    // strip the query string
    var path = new Uri($"{request.Scheme}://{request.Host}{rawTarget}").AbsolutePath;

    // go through route template and find which segment we need to extract by index
    var index = template
      .Split('/', StringSplitOptions.RemoveEmptyEntries)
      .Select((segment, index) => new { segment, index })
      .SingleOrDefault(iter =>
          iter.segment.Equals(templateToMatch, StringComparison.OrdinalIgnoreCase))
      ?.index;

    var segments = path.ToString().Split('/', StringSplitOptions.RemoveEmptyEntries);

    if (index.HasValue) {
      // extract and decode the target path segment
      var rawUrlSegment = segments[index.Value];
      var decoded = Uri.UnescapeDataString(rawUrlSegment);
      bindingContext.Result = ModelBindingResult.Success(decoded);
    } else {
      // can't think of any scenarios where we'd hit this
      throw new NotSupportedException();
    }

    return Task.CompletedTask;
  }
}

public class FromRouteUnsafeModelBinder : IModelBinderProvider {
  public IModelBinder? GetBinder(ModelBinderProviderContext context) {
    if (context.Metadata is not DefaultModelMetadata metadata) return null;
    var attribs = metadata.Attributes.ParameterAttributes;
    return attribs == null
      ? null
      // handle all [FromRoute] string parameters
      : attribs.Any(pa => pa.GetType() == typeof(FromRouteAttribute))
          && metadata.ModelType == typeof(string)
        ? new BinderTypeModelBinder(typeof(RouteUnsafeBinder))
        : null;
  }
}

I apply this binder at startup when I'm configuring services like this:

    services.AddControllers(opts => {
      opts.ModelBinderProviders.Insert(0, new FromRouteUnsafeModelBinder());
    })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-medium This issue impacts approximately half of our customers area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-model-binding severity-minor This label is used by an internal tool
Projects
No open projects
Status: No status
Development

No branches or pull requests