Disable IsRedirectURISecure

[mvrhov]

Hi,

I'm porting an old application to a go and there is a multitude of clients for which we allowed to have custom redirect URIs and most of them won't get fixed so the first hurdle I found is that there is a URL check performed.

Now I've fixed this locally with the following code:

flow_authorize_code_auth.go

	check, _ := ctx.Value(fosite.CtxDisableRedirectURICheck).(bool)
	if !check && !fosite.IsRedirectURISecure(ar.GetRedirectURI()) {
		return errors.WithStack(fosite.ErrInvalidRequest.WithDebug("Redirect URL is using an insecure protocol, http is only allowed for hosts with suffix `localhost`, for example: http://myapp.localhost/"))
	}

and
context.go

type ContextKey int

const (
	CtxDisableRedirectURICheck ContextKey = iota
)

Before I do aa PR do you have another suggestion?

[aeneasr]

Custom URI schemes (as in `myapp://`) should be supported. Disabling redirect checking is a security vulnerability so it’s not going to be merged. If custom URI schemes are not supported (although I’m fairly sure they are), we can take another look at that.

…

On 22. May 2018, at 08:52, Miha Vrhovnik ***@***.***> wrote: Hi, I'm porting an old application to a go and there is a multitude of clients for which we allowed to have custom redirect URIs and most of them won't get fixed so the first hurdle I found is that there is a URL check performed. Now I've fixed this locally with the following code: flow_authorize_code_auth.go check, _ := ctx.Value(fosite.CtxDisableRedirectURICheck).(bool) if !check && !fosite.IsRedirectURISecure(ar.GetRedirectURI()) { return errors.WithStack(fosite.ErrInvalidRequest.WithDebug("Redirect URL is using an insecure protocol, http is only allowed for hosts with suffix `localhost`, for example: http://myapp.localhost/")) } and context.go type ContextKey int const ( CtxDisableRedirectURICheck ContextKey = iota ) Before I do aa PR do you have another suggestion? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[mvrhov]

I don't see this as a vulnerability. Whatever is in the list inside database or wherever the clients are read from is still respected. This just disables the https enforcement check. Which is also a pin if doing local dev. all 30 of my dev projects are on http and each of them has a custom domain name usually just http://projectname .

edit: I know from where the "vulnerability" comes from The specs says that tokens should be protected by https. But still changing the configuration of each of the project on each developers machine is a pain so still looking for a suggestion..

[aeneasr]

I see, I thought you were talking about custom redirect URI schemas (only saw the issue in my mail inbox) but it's really about enabling HTTP on non-localhost domains.

Doing so is indeed a weakening of the security standards and best practices enforced by this library, and - although I understand your set up - will not make it into the library. I've seen too many applications were an initial "debug" or "local dev" approach was deployed to production and made the application inherently insecure. This library was written with the intent to prevent this and that is why we enforce strict security policies. This is also why such a feature will not be merged.

One possibility is to append every projectname with the localhost suffix which - by the way - also clearly indicates to the developer/user/browser that you're working on a local domain. If you don't want to do that, you could probably hack your way around it by having https in the redirect URI values but rewrite it - just before the redirect happens (WriteAuthorizeResponse) - to http. I definitely do not recommend doing that and want to explicitly warn you of doing so. It will be a hack, and it will feel like a hack, and it should be like that, so when you go to production this hack will be caught by code review. To be a bit more secure, you could enforce some check that checks if the domain is whitelisted before doing the rewrite.

My recommendation however is to stick to clear and transparent security guidelines. Don't weaken or hack security just to make it work on your dev machine when there are, in fact, ways to solve this issue by sticking to clear and obvious naming and best practices.

[mvrhov]

For now I've added this in a local fork. Later when the initial development is done this will get removed. For now this hack gets a BUG status in ticketing system.

I'm going to close this and later if I come to the problems with custom schemes, I'll open a new ticket. I hope I don't have to register each and any one of them as there is literally 100s of them

[aeneasr]

Feel free to reopen or discuss this if you encounter any hurdles. Good luck! :)