Skip to content
This repository has been archived by the owner on Sep 18, 2021. It is now read-only.

Callback URI for Flows.Code with query parameters fails #414

Closed
kking-envelope opened this issue Oct 5, 2014 · 14 comments
Closed

Callback URI for Flows.Code with query parameters fails #414

kking-envelope opened this issue Oct 5, 2014 · 14 comments
Assignees

Comments

@kking-envelope
Copy link

a url such as: http://localhost/?callback becomes http://localhost/?callback?code=abcdefg.

add a check into Thinktecture.IdentityServer.Core.Connect.Results.AuthorizeCodeResult (Line 37)

Original:

var url = string.Format("{0}?code={2}", _response.RedirectUri.AbsoluteUri, _response.Code);

Modified:

string join = _response.RedirectUri.Query != String.Empty ? "&" : "?";
var url = string.Format("{0}{1}code={2}", _response.RedirectUri.AbsoluteUri, join, _response.Code);
@leastprivilege
Copy link
Member

Thanks. We will discuss that.

@abrunyee
Copy link

abrunyee commented Oct 9, 2014

I've hit this same issue when configuring a Wordpress site to use a plugin to link it to my identity server instance (the redirect URL needs to include ?action=...). The fix I was considering was in "AuthorizeRedirectResult.cs", line 51:

character = url.Contains("?") ? "&" : "?";

I've not tested this yet, but that should fix my specific issue.

@kking-envelope
Copy link
Author

It will (as in fixing the double ? in the URI will make it work with wordpress) - that was the problem I was trying to solve.

@abrunyee
Copy link

abrunyee commented Oct 9, 2014

Just tested it using the MVC CodeFlowClient Manual test. Authorisation works, but then the 'Get Token' operation fails, and I haven't had chance to dig into the code to find out why.

It looks like allowing query strings in the redirect_uri could have a few knock-on effects, but I think it's definitely worth implementing.

@kking-envelope
Copy link
Author

It passed everything in Core.Tests and works with the ImplicitFlow javascript client; maybe there is some other issue.

@abrunyee
Copy link

abrunyee commented Oct 9, 2014

Not at my machine right now, but my repo didn't seem to have that class in it (hence me using a slightly different fix). I'll have another look in a bit. Thanks :)

@kking-envelope
Copy link
Author

I was running an older version. I just updated and implemented your fix. I don't have any issues with CodeFlow or Implicit Clients getting tokens.

@kking-envelope
Copy link
Author

I actually used

character = _response.RedirectUri.Query == String.Empty ? "?" : "&";

@abrunyee
Copy link

abrunyee commented Oct 9, 2014

Ah, that would explain it I guess. I'll use your (more correct looking) fix and see if I can reproduce the error I got with freshly cloned repos - I've probably hacked my CodeFlow client around a bit while playing with stuff :)

@leastprivilege
Copy link
Member

We made some changes to the URL handling in the dev branch - give it a try and let us know if this works for you.

@abrunyee
Copy link

Thanks for making those changes. I'm grabbing the dev branch now. Will let you know.

@abrunyee
Copy link

I've been struggling to plug the dev branch into my applications, so I've grabbed a fresh dev repo, and the client samples, but the "MVC CodeFlowClient" fails with an unexpected error. I'm trying to track down exactly where it's breaking. Do you have any tips on making the samples work against the latest code?

@brockallen
Copy link
Member

Yes, right now I can imagine many of the samples aren't working -- several interface and API changes on dev. Watch for checkins or check back in 2-3 days.

@abrunyee
Copy link

Ah, OK, thanks. I've tracked the exception down to HostDataProtector.Unprotect, called from AntiForgeryTokenValidator.GetCookieToken, but I'll leave it for now and keep an eye on the samples repo. Cheers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants