Allow RedirectURI to have a custom behaviour

[blatyo]

At the company I work at, we spin up custom environments for each branch. This change would allow us to define a version of RedirectURI that allows wildcards so that we don't need to define a redirect_uri everytime we spin up an environment with a different subdomain.

[danschultzer]

Thanks for the PR, looks good, but I would strongly advice against permitting wildcard URL’s, as it’s in general not safe: https://security.stackexchange.com/questions/180505/why-is-a-wildcard-subdomain-callback-url-in-oauth-considered-unsafe

The best approach is if you can somehow automatically update the redirect uris for the app(s) each time you spin up a new subdomain app.

The PR is good as it is, but I’ll have to read up on the rfc specs to check what has to be hard requirements. I prefer that even what developers customize has to adhere to the specs as much as possible, and that may mean that the RedirectURI macro should be rewritten a bit to ensure that.

[blatyo]

I understand the security concerns. This is only intended for test environments and the overridden version also ensures that wildcards are only allowed on a specific domain that we own.

Providing an API to add and remove redirect URI's to an application also introduces a potential security vulnerability. It's also a larger attack vector, because if an attacker can take advantage of this, they can add any domain or cause a denial of service.

The only part of the interface I'm interested in modifying is match?/2 and validate/2. The first allowing the wildcard match and the second to ensure other domains aren't allowed. So, I could create a PR that only allowed customization of that.

[danschultzer]

I understand the security concerns. This is only intended for test environments and the overridden version also ensures that wildcards are only allowed on a specific domain that we own.

If you're using MIX_ENV=dev for your environment, maybe something similar to how forced SSL works could be added:

ex_oauth2_provider/lib/ex_oauth2_provider/config.ex

Lines 110 to 116 in d602d8e

	# Forces the usage of the HTTPS protocol in non-native redirect uris
	# (enabled by default in non-development environments). OAuth2
	# delegates security in communication to the HTTPS protocol so it is
	# wise to keep this enabled.
	@spec force_ssl_in_redirect_uri?(keyword()) :: boolean()
	def force_ssl_in_redirect_uri?(config),
	do: get(config, :force_ssl_in_redirect_uri, Mix.env != :dev)

So these things can only be overridden if it's not production environment.

Providing an API to add and remove redirect URI's to an application also introduces a potential security vulnerability. It's also a larger attack vector, because if an attacker can take advantage of this, they can add any domain or cause a denial of service.

The only part of the interface I'm interested in modifying is match?/2 and validate/2. The first allowing the wildcard match and the second to ensure other domains aren't allowed. So, I could create a PR that only allowed customization of that.

Yeah, limiting the scope of what can be changed would be great. In regards to automatically updating the URI list, I was thinking of just a release task that updates the relevant app(s) with an ecto query. No idea how your actual setup is so this may not be a good idea in your case 😄

I read up on the specs, and here's the relevant sections in the RFC:

https://tools.ietf.org/html/rfc6749#section-3.1.2
https://tools.ietf.org/html/rfc3986#section-4.3

The URI must be absolute, wildcard is against the specs. This PR doesn't make ExOauth2Provider less secure by default, but there may be a way to make it clearer that specs may be broken by overriding this module/methods and that it's not meant for public facing API's.

Let me try a few things and see what makes sense to do here.

[danschultzer]

I've pushed a commit that limits the scope of this greatly. I found out that wildcard domains are valid in the current release of ExOauth2Provider so the security discussion is moot 😞

All you need to change is the matches?/2. The commit adds a :redirect_uri_match_fun config option that you can set to do your own custom matching. The tests shows an example with wildcard matching.

I love the macro setup you had, but I think this is the better way for now since this way all the current validations are kept in place (fragments, https, etc) and it's not necessary to customize anything else than the matching. The tests I have added also makes sure that if/when I redo validations, I'll keep in mind that wildcard domains in current releases are valid.

Wdyt? Will this work for you?

[blatyo]

Yea, this works for me. I can do the part I intended to do in the validation in the match.

Thanks

[danschultzer]

I'll release a new version ASAP.

[danschultzer]

v0.5.2 released