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

[BUG] TypeError: expirationTimer.unref is not a function #917

Closed
maxime-ducoroy opened this issue May 21, 2024 · 3 comments
Closed

[BUG] TypeError: expirationTimer.unref is not a function #917

maxime-ducoroy opened this issue May 21, 2024 · 3 comments
Labels

Comments

@maxime-ducoroy
Copy link

Hello, find my bug below

To Reproduce

My Code

import * as metadata from 'passport-saml-metadata'
import passport from 'passport'
import { Strategy as SamlStrategy, type SamlConfig } from 'passport-saml'

const { SSO_URL, SSO_METADATA_URL, SSO_ENTITY_ID } = useRuntimeConfig().public
const reader = await metadata.fetch({
  url: SSO_METADATA_URL
})
const config = metadata.toPassportConfig(reader) as unknown as SamlConfig
config.entryPoint = SSO_URL
config.issuer = SSO_ENTITY_ID
config.callbackUrl = 'http://localhost:3000/api/login/callback'
config.cert = reader.signingCert
config.requestIdExpirationPeriodMs = 3600
const samlStrategy = new SamlStrategy(config, (done, profile) => {
  console.log('Maurice aime mauricette')
})
passport.use('saml', samlStrategy)

When i run my code i have this error:
TypeError: expirationTimer.unref is not a function
at new CacheProvider (inmemory-cache-provider.ts:52:21)

It's because setInterval return an ID and number haven't got unref function.

Expected behavior

don't call expirationTimer.unref() function.

Environment

  • Node.js version: 20.12.2
  • passport-saml version: 3.2.4
@maxime-ducoroy
Copy link
Author

maxime-ducoroy commented May 21, 2024

It's in node_modules/passport-saml/lib/node-saml/inmemory-cache-provider.js

@srd90
Copy link

srd90 commented May 22, 2024

First of all passport-saml 3.2.4 is deprecated as reported by npm (npm install and website) https://www.npmjs.com/package/passport-saml I assume no-one is going to spend time investigating that version's problems.

Having said that and out of curiosity:

nodejs API documentation for version 20.12.2 is very clear about possible return values of setInterval: https://nodejs.org/docs/latest-v20.x/api/timers.html#setintervalcallback-delay-args

It throws TypeError or returns Timeout object ( https://nodejs.org/docs/latest-v20.x/api/timers.html#class-timeout )

Source code verifies this:
https://github.com/nodejs/node/blob/v20.12.2/lib/timers.js#L212-L240

IMHO this case "smells" like case of usage of some incorrectly implemented test library which mocks setInterval incorrectly or (based on quick consultation with internet search engine) you might be trying to execute your code after all in browser https://stackoverflow.com/questions/56646815/setinterval-is-different-between-browser-and-node

don't call expirationTimer.unref() function.

There seems to be reason why unref is called: #68


Sidenote (not related to this issue):
I wonder how many unreference repeated interval timers there might be at end of the day e.g. in case of multisaml strategy because SAML ctor (which creates inmemory cache provide implicitily if none is provided) is invoked multiple times (at least once per new authentication attempt/logout/saml responce callback) (links point to 3.2.4 version)

authenticate(req: RequestWithUser, options: AuthenticateOptions): void {
this._options.getSamlOptions(req, (err, samlOptions) => {
if (err) {
return this.error(err);
}
const samlService = new SAML({ ...this._options, ...samlOptions });

constructor(ctorOptions: SamlConfig) {
this.options = this.initialize(ctorOptions);

cacheProvider:
ctorOptions.cacheProvider ??
new InMemoryCacheProvider({
keyExpirationPeriodMs: ctorOptions.requestIdExpirationPeriodMs,
}),

constructor(options: Partial<CacheProviderOptions>) {
this.cacheKeys = {};
this.options = {
...options,
keyExpirationPeriodMs: options?.keyExpirationPeriodMs ?? 28800000, // 8 hours,
};
// Expire old cache keys
const expirationTimer = setInterval(() => {
const nowMs = new Date().getTime();
const keys = Object.keys(this.cacheKeys);
keys.forEach((key) => {
if (
nowMs >=
new Date(this.cacheKeys[key].createdAt).getTime() + this.options.keyExpirationPeriodMs
) {
this.removeAsync(key);
}
});
}, this.options.keyExpirationPeriodMs);
// we only want this to run if the process is still open; it shouldn't hold the process open (issue #68)
expirationTimer.unref();
}

I.e. is there perhaps "memory leakish" issue. I did not spend time to investigate code flow too well.

@srd90
Copy link

srd90 commented May 23, 2024

Seems that usage of setInterval was dropped at

which was released at version 4.0.0 (Oct 10th 2022) of @node-saml/node-saml:
https://github.com/node-saml/node-saml/blob/v5.0.0/CHANGELOG.md#v400-2022-10-28

Note: @node-saml/passport-saml >=4.0.0 uses internally aforementioned @node-saml/node-saml. Development of core saml functionality was splitted to separated repository to the library which does not have any dependencies to passportjs or to any specific web framework and @node-saml/passport-saml became "shell" that provides just passportjs module functionality/implementation of passportjs strategy for SAML 2.0

@cjbarth cjbarth closed this as completed Jun 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants