-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
client_secret being passed in post data in Basic auth #950
Comments
Hi there! Just out of curiosity, since I'm not familiar with Signicat, have you actually tried to just provide a secret and see if sending it in post data would work? Is this the documentation of what you are referring to?: https://developer.signicat.com/documentation/authentication/protocols/openid-connect/endpoints/ I think we have to see if we want to/need to expand the public facing API as you suggested above, but I have to discuss this further first. I'll get back to you with more information, Thanks! |
Hi @balazsorban44. Thanks for your response. Yes when sending the I was able to get around this by monkeypatching querystring to filter out empty keys: const _stringify = querystring.stringify;
querystring.stringify = (...args) => {
args[0] = Object.keys(args[0]).reduce(
(obj, x) => (args[0][x] ? { ...obj, [x]: args[0][x] } : obj),
{}
);
return _stringify.call(null, ...args);
}; However, I got stuck again when trying to parse the profileData: next-auth/src/server/lib/oauth/callback.js Line 144 in 9dbd372
Since we're using encryption Signicat is sending back an encrypted JWT which obviously fails to This is unfortunate because if I could only get past that line, I would be able to decode it in the next-auth/src/server/lib/oauth/callback.js Line 153 in 8827950
I really wish to use this excellent library but for now I don't see how to get it working without doing a fork and rewrite parts of:
|
Hmm, if you have the time and interest, I think you could try create the changes and propose it in a PR, and we can discuss further what we could incorporate into the core package. I think if it is a general enough solution that we can back up with links to official OAuth and/or OIDC specs and doesn't alter the user facing api much (or at all), I am pretty sure we could (and should) accept such a change. |
After playing around with the code base and with panva/node-openid-client, I think perhaps this could be a solution:
Here is the relevant code in panva/node-openid Basically there needs to be a way to decrypt tokens before they get decoded if keys are present. |
@balazsorban44 I've just come across this issue with an OAuth service that doesn't accept via POST. As per the most up-to-date IETF OAuth 2.0 Authorization Framework spec, providing the client credentials via the request-body (POST) is explicitly called out as NOT RECOMMENDED and only to be used in situations where clients are unable to directly utilize the HTTP Basic authentication scheme, which doesn't cover this library's primary use case. Here is the snippet in question in the RFC:
Reference: https://tools.ietf.org/html/rfc6749#section-2.3.1 Not having the ability to choose between Basic Auth and POST by easily stripping out the As per your previous comments with regards to links, does this cover that requirement? |
@carlbarrdahl We have the exact same issue using Signicat's OpenID-connect solution. Did you manage to get it working with the modifications? |
A hacky way is to set next-auth/src/server/lib/oauth/client.js Lines 144 to 146 in 0fae0c7
#1846 would hopefully help to deal with this more elegantly |
Yeap @balazsorban44 that gets us halfway there, the only other thing that's needed is removing next-auth/src/server/lib/oauth/client.js Line 156 in 0fae0c7
Otherwise, the OAuth vendor complains that you're sending through the client credentials in an invalid format and rejects the request. |
There is another way to get you there using patch-package, modify the // Added as a fix for Reddit Authentication
if (provider.id === 'reddit' || provider.id === 'mycustomprovider') {
headers.Authorization = 'Basic ' + Buffer.from((provider.clientId + ':' + provider.clientSecret)).toString('base64')
}
if(provider.id === 'mycustomprovider') {
delete params.client_secret;
} This is how we solved it for now. |
Hi there! It looks like this issue hasn't had any activity for a while. It will be closed if no further activity occurs. If you think your issue is still relevant, feel free to comment on it to keep it open. (Read more at #912) Thanks! |
I Will close this in favor of #1846 |
In case anyone wanders in here trying to use Signicat Express with
|
Describe the bug
We're trying to integrate Signicat OpenID with Basic auth and get this message:
This is because
client_secret
is sent as post data here, even if itsnull
orundefined
:next-auth/src/server/lib/oauth/callback.js
Line 225 in 9dbd372
With empty client secret (note the:
&client_secret=
):Here are some possible solutions:
params.client_secret
at all.Steps to reproduce
Use custom oauth2 config:
Expected behavior
client_secret
should be omitted from postData ifheaders.Authorization.includes("Basic")
Feedback
Documentation refers to searching through online documentation, code comments and issue history. The example project refers to next-auth-example.
The text was updated successfully, but these errors were encountered: