-
Notifications
You must be signed in to change notification settings - Fork 10
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
Request-target should not be URL decoded #12
Comments
Well, it's a little more complicated than that. The .NET Uri class has a tendency to encode and decode, depending which property or method you use. The decoding is there to get a common baseline: If the signer and the receiver both agree to decode before starting, there is no issue with encoding. But it's definitely valid to retain the original string when creating the composed string to hash. I will investigate further. |
…d signature string + add some covering tests. Still need to validate when using in a running web application.
…codes the uri, in the way that a server would receive it, no matter if the client encodes it or not.
As a suggestion, I switched it: Request URIs are now encoded for the signature, instead of decoded. Why? Servers have no way of knowing the original string that was used to sign. Whether it was encoded or not. I also aligned it to use the same URL encoding that System.Net.HttpClient uses (the System.Uri class), when sending requests: Let me know if this meets your needs. |
Created #13 to make this behavior configurable. |
Released in v5.0.1. |
Also see here: |
There should be an additional (small) fix. Why is the requestUri URL decoded? It's not necessary, if the URL is encoded it should be taken this way otherwise the verification process fails.
So this is the change:
var requestForSigning = new HttpRequestForSigning { Method = httpRequestMessage.Method, RequestUri = httpRequestMessage.RequestUri.IsAbsoluteUri ? httpRequestMessage.RequestUri.PathAndQuery : httpRequestMessage.RequestUri.OriginalString.Split('#')[0] };
Originally posted by @dieterde in #10 (comment)
The text was updated successfully, but these errors were encountered: