Don't send POST params on query string also

[jabbera]

token_url takes post data, don't also send the same data on the query string.

[consideRatio]

Not confident on theory here, and lacking live integration tests, i think there it would be good to either review the behavior in 15.1.0 about the behavior used then or try validate what we do against a oauth2 standard document.

I'm on vacation and wont look into it now, but wanted especially to say: thank you @jabbera for your help with these patches!!! ❤️ 🎉 🌻

[consideRatio]

My intuition agrees with that this makes sense btw, just hoping its not required by some authenticator for a weird reason.

[jabbera]

Full disclosure: only tested on AzureAD.

By doing this you we are putting the server side secret and the refresh token in the query string. This is explicitly against OWASP guidance and the oauth spec*. Any auth provider that requires this would not be an auth provider worth trusting IMO and should have provider specific overrides to support this broken behavior so other providers don't leak secrets.

In our case we redirect all traffic out a corporate firewall. By putting these secrets in the query string anyone with access to read firewall traffic history can now impersonate any user who was logged into our jupyterhub instance. (We keep track of what URIs people visit) To make matters worse (for us) we use the on behalf of oauth flow to generate user tokens that allow impersonated access to storage and sql resources. This is all really bad stuff and a legitimate auth provider would never require post data to be sent on the query string.

At a minimum I'd request the removal of the refresh token and the client secret.

*The client MUST use the HTTP "POST" method when making access token requests.

[jabbera]

I spot checked some of the 15.1.0 providers and there is a mix of ones that use the query string (GitHub) and those that don't (AzureAD, auth0, generic). That said, I can't imagine any require a secret to be passed this way for the reasons mentioned above. I'm happy to live off my fork but this is a default insecure configuration that's being provided and my sec team won't allow it. I can't imagine I'm alone here.

[consideRatio]

Thanks for clarifying this is also to be considered a security aspect and looking into this further @jabbera!

I understand that both query strings and post body is encrypted, but query strings may get stored in logs and such far more often and that is against OWASP recommendations.

To me, that is a good enough background to dare go for it.

@GeorgianaElena or @manics wdyt, lets go for a merge?

[manics]

Including POST parameters in the URL originates from this comment:
#526 (comment)
which references the GitLab authenticator.

We've got two options here:

1. Merge this PR as is, and hope that GitLab no longer requires parameters in the URL (ideally someone who uses GitLab would test this)
2. Modify GitLab only to override this change

As you can probably guess 1. is a better option.

[manics]

Based on a quick skim of the latest version of the referenced GitLab doc I can't see anything about needing URL parameters:
https://github.com/gitlabhq/gitlabhq/blob/v15.11.1/doc/api/oauth2.md

Edit: actually I'm not sure, the example Ruby code is

parameters = 'client_id=APP_ID&code=RETURNED_CODE&grant_type=authorization_code&redirect_uri=REDIRECT_URI&code_verifier=CODE_VERIFIER'
RestClient.post 'https://gitlab.example.com/oauth/token', parameters

Does this mean parameters is the body, or is it added as the URL query string?

[consideRatio]

I think the post body is used when Ruby's REST client is used like in their example.

https://www.rubydoc.info/gems/rest-client/2.1.0/RestClient

The format of the body makes me wonder still, but i think we can work that separately. This PR doesnt change the post body anyhow

[consideRatio]

Ah the example post body is now something that i recognize as a x-www-form-urlencoded string like: Name=John+Smith&Age=23

So, i think we should be fine in gitlab as well without fixes.

👍 For merge

[jabbera]

Thanks folks.