It is checking of Querystring key null instead of its value

[alex-piccione]

https://github.com/tsibelman/aws-signer-v4-dot-net/blob/464a21cbea33390500f52323a179f1ba5c760c4d/Aws4Signer/AWS4RequestSigner.cs#L159

Base on the comment

//Handles keys without values

it seems to look for the key instead of its value.

Example:

var requestUri = "https://aaa?colorA=&colorB=red";
# create a request with this requestUri 

var querystring = HttpUtility.ParseQueryString(request.RequestUri.Query);
foreach (var key in querystring.AllKeys)

querystring is {colorA=&colorB=red}
querystring.AllKeys is an array with "ColorA" and "ColorB"

I don't think it can ever be key == null.
Probably the check should be on the value instead, querystring[key] == null.

Also, it seems to mixed up key and value when it creates the escaped value,
maybe the code was meant to be like this:

var escapedKey = Uri.EscapeDataString(key);
//if (key == null)
if (querystring[key] == null)//Handles keys without values
{
    //values.Add(Uri.EscapeDataString(querystring[key]), new[] { $"{Uri.EscapeDataString(querystring[key])}=" });
    values.Add(escapedKey, new[] { $"{escapedKey}=" });
}

[tsibelman]

Hi, thanks for the contribution but that was not a mistake. It was a real case I encountered.

Consider URLs in the form of https://www.test.com/?param

Folowing code will print "Key is:null value is:param" var paramCollection=HttpUtility.ParseQueryString(new Uri("https://www.test.com/?param").Query); Console.WriteLine("Key is:"+(paramCollection.AllKeys[0]??"null")+" value is:"+paramCollection.GetValues(0)[0]);

[alex-piccione]

I see the difference now, thanks for the example.
The "Handles keys without values" didn't make me think about that scenario.
For the benefit of other readers:
?param is parsed as a null key with "param" value
?param= is parsed as "param" key with null value

So, the querystring parser is consideringnull the key and "param" the value, but the code (in the key == null branch) is doing the other way (using the value "param" as key name and setting null/empty the value):
values.Add(Uri.EscapeDataString(querystring[key]), new[] { $"{Uri.EscapeDataString(querystring[key])}=" }); that is "param" the key and null the value.

A quick test:
string url = "https://www.test.com/?aaa=1&aaa=2&value1&value2";
result in:
querystring[0] (key = "aaa"): "1,2"
querystring[1] (key is null) : "value1,value2"

I'm not saying it is wrong or not working, just an analysis to understand the behavior.

[tsibelman]

I think it very weird way of handling such cases if I would design NameValueCollection I would handling key and key= cases as the same one instead of treating one as key without value and the other as value without the key, but this an option we have right now.