Aspnetcore: "Equal" sign optional in "application/x-www-form-urlencoded" results in unparseable query

[odinnou]

First of all, I create question on stack overflow for this, before submit this issue : post

Hello in my project built on ASP.NET Core 2.2, I need to implement webhook to handle "notification" from a third-party API.

We deploy it, then we test it but every requests fall in 400 bad request.
I investigate, then I detect that the body of sended requests (application/x-www-form-urlencoded) contains a "missing" equal sign in the query &data[subject_name]&data[user_uid], ex:

webhook_type=create&network_name=test&data[id]=389&data[action_name]=action&data[target_name]=target&data[subject_name]&data[user_uid]=b6643dc6-946b-490a-86b8-eb5c67f82bca&data[type]=Comment

data[subject_name] may be null or empty but with this query data[user_uid] is not parsed (default guid) ! Because ASP.NET framework can't correctly parse the query

I propose to the third-party API developers two solutions :

1. force add "equal" sign when the field is null or empty

webhook_type=create&network_name=test&data[id]=389&data[action_name]=action&data[target_name]=target&data[subject_name]=&data[user_uid]=b6643dc6-946b-490a-86b8-eb5c67f82bca&data[type]=Comment

1. remove null or empty fields from the query

webhook_type=create&network_name=test&data[id]=389&data[action_name]=action&data[target_name]=target&data[user_uid]=b6643dc6-946b-490a-86b8-eb5c67f82bca&data[type]=Comment

The answer was: "no, it's a standard we do not change anything"

Here the model

public class WebHookDto<T> where T : ThirdPartyNotificationDto
{
    [Required]
    [EnumDataType(typeof(WebHookType))]
    [FromForm(Name = "webhook_type")]
    public WebHookType WebHookType { get; set; }

    [Required]
    [MinLength(1)]
    [FromForm(Name = "network_name")]
    public string NetworkName { get; set; }

    [Required]
    [FromForm(Name = "data")]
    public T Data { get; set; }
}

public class ThirdPartyNotificationDto
{
    [Required]
    [FromForm(Name = "id")]
    public long Id { get; set; }
}

public class UserNotificationDto : ThirdPartyNotificationDto
{
    [Required]
    [FromForm(Name = "user_uid")]
    public Guid UserId { get; set; }

    [FromForm(Name = "action_name")]
    public string ActionName { get; set; }

    [FromForm(Name = "target_name")]
    public string TargetName { get; set; }

    [FromForm(Name = "subject_name")]
    public string SubjectName { get; set; }

    [Required]
    [EnumDataType(typeof(NotificationTargetType))]
    [FromForm(Name = "type")]
    public NotificationTargetType TargetType { get; set; }
}

Here the controller/action

[HttpPost("user")]
public AcceptedResult UserNotificationListener([FromForm]WebHookDto<UserNotificationDto> request)
{
    // some code that validate the query or throw exception
}

Here the full query

POST /api/v1/notification/user HTTP/1.1
Host: localhost:44300
Content-Type: application/x-www-form-urlencoded

webhook_type=create&network_name=test&data[id]=389&data[action_name]=action&data[target_name]=target&data[subject_name]&data[user_uid]=b6643dc6-946b-490a-86b8-eb5c67f82bca&data[type]=Comment

My questions are:

The optional "equal" sign is it a standard like third-party developers said ?

What is the best way to fix this issue, if third-party developers stuck on their position ?

Thanks in advance
Kind regards

Sorry for my poor english :(
Rémi

[odinnou]

Another precision, if you don't add the 'equal' sign for the last parameter of the query, example :

webhook_type=create&network_name=test&data[id]

this last parameter will not be present in Request.Forms

It's less important because this parameter should be null in every case

Thanks in advance

[odinnou]

Hello, I found a workaround for thie issue with the use of a middleware :

app.UseMiddleware<RemoveInvalidFormKeysMiddleware>();
app.UseMvc();

This middleware will rewrite "invalid" keys in Request.Forms

public class RemoveInvalidFormKeysMiddleware
{
    private readonly RequestDelegate next;

    public RemoveInvalidFormKeysMiddleware(RequestDelegate next)
    {
        this.next = next;
    }

    public async Task Invoke(HttpContext context)
    {
        Dictionary<string, StringValues> validForm = new Dictionary<string, StringValues>();

        foreach (var form in context.Request.Form)
        {
            if (!form.Key.Contains('&'))
            {
                validForm.Add(form.Key, form.Value);
                continue;
            }

            string key = form.Key.Substring(form.Key.LastIndexOf('&') + 1);

            if (!string.IsNullOrWhiteSpace(key))
            {
                validForm.Add(key, form.Value);
            }
        }

        context.Request.Form = new FormCollection(validForm);

        await next(context);
    }
}

Then we can keep the rest of the logic without change anything else

[HttpPost("user")]
public AcceptedResult UserNotificationListener([FromForm]WebHookDto<UserNotificationDto> request)
{
    // some code that validate the query or throw exception
}

Hope it helps, Rémi

[pranavkm]

/cc @Tratcher

[Tratcher]

You're using the terms "form" and "query" interchangeably here, but you're always talking about a form post and never a url query string, correct? The formats are similar.

[odinnou]

hello, yep always a form post, sorry for the disturbing term "query"

[odinnou]

For information, I have test the same form post on a Spring Boot project and the missing "equal" sign is ignored and properties are correctly binded.
I don't know if it's because it's a real standard or if spring boot is more "permissive" on this point

Thanks in advance

[davidfowl]

I don't know if it's because it's a real standard or if spring boot is more "permissive" on this point

It's certainly a data point worth considering. We should do some research to see what is supported in the various places:

• IIS
• ASP.NET (non core)
• Spring boot (we know now)
• nodejs

[anurse]

Seems like a reasonable bit of investigation. As a workaround, you can always read the form body manually off HttpContext.Request.

[anurse]

We think there might be a regression here since the new parser throws. Considering for 3.1.

[anurse]

The optional "equal" sign is it a standard like third-party developers said ?

We should confirm this, though it doesn't necessarily mean we shouldn't fix the issue.

[davidfowl]

@anurse isn;t this also busted in 2.2?

Hello in my project built on ASP.NET Core 2.2, I need to implement webhook to handle "notification" from a third-party API.

It's 2.2, what did I miss?

[Tratcher]

@davidfowl we've had 2.2 and 3.0 reports but the symptoms are different. On 2.2 it returns bad results, on 3.0 it throws.

[anurse]

Yeah, sorry. That’s the “regression” we covered in triage. Wasn’t clear in my comment :)

[davidfowl]

@davidfowl we've had 2.2 and 3.0 reports but the symptoms are different. On 2.2 it returns bad results, on 3.0 it throws.

Ah, we should add a test here to make sure the behavior is the same.

[Tratcher]

https://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.1 gives the most formal definition I can find for x-www-form-urlencoded:

This is the default content type. Forms submitted with this content type must be encoded as follows:
Control names and values are escaped. Space characters are replaced by '+', and then reserved characters are escaped as described in [RFC1738], section 2.2: Non-alphanumeric characters are replaced by '%HH', a percent sign and two hexadecimal digits representing the ASCII code of the character. Line breaks are represented as "CR LF" pairs (i.e., '%0D%0A').
The control names/values are listed in the order they appear in the document. The name is separated from the value by '=' and name/value pairs are separated from each other by `&'.

It does not specify if the '=' and value are optional. Claiming this is spec behavior is a reach. At best it's common practice.

[Tratcher]

I've confirmed that
A) 2.2 allows "key=" but not "key" as the last entry in a form. The trailing key without a value is discarded.
B) 2.2 allows "key1=&key2=value", but misreads "key1&key2=value" as "key1&key2" = "value".
C) A has regressed further in 3.0 by throwing rather than discarding the last value. System.InvalidOperationException : End of body before form was fully parsed.
D) B persists in 3.0.

[davidfowl]

The code as written in 3.0 doesn’t check for the end of stream when parsing the key.