Fix for URL parsing from doeringp requires UrlDecode #11
Conversation
…arser: Parameter wtrealm is always null #1
… less error prone. BUT! We still need to DECODE!
| [Fact] | ||
| public void GetSignInRequestMessage_parses_url_query_correctly() | ||
| { | ||
| var encodedUrl = "%2Fwsfederation%3Fwtrealm%3Drealm%26wa%3Dwsignin1.0%26wreply%3Dhttps%253A%252F%252Flocalhost%253A44310%252Fsignin-wsfed%26wctx%3DCfDJ8N3n"; |
There was a problem hiding this comment.
Your tests did not pass because the URL you pass in your tests is not like it would be passed when you would use the library. It would be rather like this "/WsFederation?wtrealm=urn%3Aaspnetcorerp&wa=wsignin1.0&wreply=http%3A%2F%2Flocalhost%3A10314%2F&wctx=CfDJ8H09lVj2W7VEs5pRor8gYi8GmcbmE1K-5wiTeL1pozxPOJ3rkb-SeD5X-YzLYNtr1UrkAAHIkMyZaGNDJ2B_OVdO9KixFMcmiY2UGm7JXMbYM47UBM2-XgZFoSsEJGPO8eXVPGxiNBINF1fC2qjJ9MI9oYmHcLY79wz3lG0XcVFq5lR8CNiX6BAIRBWRZC7Wj3Ur1YmsH3DZXvtcRzxRKQN6_QytWYwRGA9_BEc7ezHjVae05RN4NdA165pd8IzYeg". You can try it yourself when you run the samples and step in with a debugger.
There was a problem hiding this comment.
The strange thing is that I got the values directly from my debugger. I did "copy value".
There was a problem hiding this comment.
I think your IDE somehow URL encode the value, that's why it is wrong. If you think about it the "/WsFederation" is the path for the server to interpret, why should it be URL encoded?
| { | ||
| public static WsFederationMessage GetSignInRequestMessage(string encodedUrl) | ||
| { | ||
| var decodedUrl = WebUtility.UrlDecode(encodedUrl); |
There was a problem hiding this comment.
this is not necessary WsFederationMessage.FromUri() takes care of things that need to be decoded.
| public static WsFederationMessage GetSignInRequestMessage(string encodedUrl) | ||
| { | ||
| var decodedUrl = WebUtility.UrlDecode(encodedUrl); | ||
| // Fix: Add dummy.com host so Uri can properly parse out the query string. |
|
Thank you for your PR, I think your tests could be a good addition to our unit tests, here are really not much of them at the moment and I would be happy to merge if you resolve my concerns in comments. |
|
No problem Alexej. I will try again. |
|
Haha. Hi again @616b2f . For some reason I thought we were speaking different languages, but now I figured out why I get encoded URLs. It is because my IdentityServer implementation never decodes the URL before it is passed to IsValidReturnUrl. (I did not implement IdentityServer myself. I have inherited this project.) In this picture you can see the encoded URL and it is passed as encoded all the way down the call stack.
I am surprised because the code for
Also I added a I'm not sure what to do now. I am finishing my day, so I will sleep on it. Cordially, |
|
@jesslilly if you could remove the "WebUtility.UrlDecode" and change the URL in the test. I would love to merge your PR. |
|
Hi Alexej, I'm sorry it has been so long, I have forgotten what this is
about.
…On Wed, Feb 9, 2022 at 3:07 AM Alexej Kowalew ***@***.***> wrote:
@jesslilly <https://github.com/jesslilly> if you could remove the
"WebUtility.UrlDecode" and change the URL in the test. I would love to
merge your PR.
—
Reply to this email directly, view it on GitHub
<#11 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKXMGTV5QTWIIZH3VBKHJ3U2IOD5ANCNFSM4YMUXGZA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Kind Regards,
Jess
|
|
@jesslilly me too so don't worry :) |
2 participants
Hi Alexej,
I think I did the pull request correctly this time. The fix you merged in from doeringp does not pass the unit tests I made. We must still call UrlDecode so that "new Uri" will work correctly. I did use doeringp's good idea to use a dummy.com host since it just makes the URI parsing easier and uses less code. Does not rely on parsing for a question mark.
Also I like having these tests. I the other tests in the solution do not cover this issue.
When I add decode back, the new tests pass.
Also, there should be a comment why we use a dummy URL.
Thank you. I hope this PR is merged. 😄