Skip to content
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

Arity check does not work. Therefore Id token will never work. #168

Open
arturohernandez10 opened this issue Oct 10, 2022 · 4 comments
Open

Comments

@arturohernandez10
Copy link

The google client code, returns an accessToken, refreshToken, and the result itself. Which now returns the idToken, or at least in my case. The way to get that back id to read the "params".

For that this library attempts to check the arity but because of the many layers of passport it proves to be a bad interface choice. Passport can apply a mixin, in which case the callback has zero parameters during runtime. Effectively breaking this code:

Mixin code

var arity = self._verify.length;
if (arity == 5) {
  self._verify(accessToken, refreshToken, params, profile, verified);
} else { // arity == 4
  self._verify(accessToken, refreshToken, profile, verified);
}

the solution really is to do away with the arity and leave the optional parameters to the end. To avoid breaking changes you could do something like this:

var arity = self._verify.length;
if (legacyCheck && arity == 5) {
  self._verify(accessToken, refreshToken, params, profile, verified);
} else (legacyCheck) { // arity == 4
  self._verify(accessToken, refreshToken, profile, verified);
} else { 
  self._verify(accessToken, refreshToken, profile, verified, params);
}

Ideally legacyCheck is a configuration variable so that users can keep the old behavior. The biggest issue is the typescript types.

@jaredhanson
Copy link
Owner

This seems to be related to how Nest is wrapping Passport, correct? If so, I think the issue would be best addressed in the wrapper rather than here. Passport is expecting a function as an argument, in which case the arity checking works as intended. It's unclear to me how that is impacted by mixins (which seem to be a Nest construct?).

@xenia-lang
Copy link

In order to work with as many passport strategies as possible, the callback the Nest wrapper uses, is a variadic function. The arguments passed to the callback are forwarded to the user implementation and the wrapper takes care of calling the callback it was passed with the result or the thrown exception.

The problem arises due to the fact that the rest parameter does not contribute to the arity of a function. This means that the callback used by Nest has an arity of 0. Therefore, as a Nest user, it is impossible to aquire the params argument the oauth strategy passes to _verify without redoing the work the Nest people already did.

@xenia-lang
Copy link

xenia-lang commented Feb 9, 2023

Unfortunately, the solution suggested by @arturohernandez10 does not work, since nest expects the callback it is passed (verified in the oauth2 strategy code) to be the last argument.

Can you (or another Developer) elaborate on why the arity check was implemented? Is it for legacy reasons? If so, what where the reasons to not choose a feature flag?

@perf2711
Copy link

perf2711 commented Feb 27, 2023

A quick fix for that is to override the function Nest provides:

// Typescript
const verify: (...args: any[]) => any = (this as any)._verify;
(this as any)._verify = (arg1: any, arg2: any, arg3: any, arg4: any, arg5: any) =>
    verify(arg1, arg2, arg3, arg4, arg5);
// Javascript
const verify = this._verify;
this._verify = (arg1, arg2, arg3, arg4, arg5) => verify(arg1, arg2, arg3, arg4, arg5);

However, would be nice to see a fix for this issue. Maybe one idea is to keep the arity check as it is, but add another parameter to options like passParamsToVerify?

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

No branches or pull requests

4 participants