Upgrade to Node 18 + qs dependency fixes for Snyk #249
Conversation
| @@ -14,6 +15,10 @@ const HEARTHBEAT_QUERY_TIMEOUT_MS = 5000; | |||
| const HEARTBEAT_DELAY_MS = nconf.get('LDAP_HEARTBEAT_SECONDS') * 1000 | |||
|
|
|||
| function createConnection () { | |||
| // Resolve using IPv4 first. This lets us continue to use localhost | |||
| "engines": { | ||
| "node": "^12.19" | ||
| "node": "^18.16.0" |
There was a problem hiding this comment.
I feel like I've noted this before, but I think still allowing older versions would allow for an easier update path. Does this work with 12 / 16, etc? If so, maybe leaving this line alone for the time being would avoid a major version.
…previous versions of Node as well
| nconf.set('LAST_SENT_THUMBPRINT', getCurrentThumbprint(workingPath)); | ||
| nconf.set('TENANT_SIGNING_KEY', response.data.signingKey || ''); | ||
|
|
||
| console.log( |
There was a problem hiding this comment.
is this a new console log thats added?
There was a problem hiding this comment.
No this is from below https://github.com/auth0/ad-ldap-connector/pull/249/files#diff-ddc0f03695a62f6303846c3dd87809c9035df116b5d81e7f143472b0067fe901L65 -- some of this stuff was moved around because of how error handling works in axios
| @@ -1,12 +1,15 @@ | |||
| { | |||
There was a problem hiding this comment.
Do we also need to update https://github.com/auth0/ad-ldap-connector/blob/master/.nvmrc to reflect node 18?
| "test": "NODE_TLS_REJECT_UNAUTHORIZED=0 mocha --timeout 50000 --reporter spec --exit", | ||
| "snyk": "snyk test", | ||
| "start": "node server.js" | ||
| "start": "node server.js", | ||
| "start-local": "source .env && npm run start" |
There was a problem hiding this comment.
This is at least an unexpected pattern to me, I've typically used the language's dotenv project to load a .env
package.json
Outdated
| @@ -1,12 +1,15 @@ | |||
| { | |||
| "name": "ad-ldap-connector", | |||
| "version": "6.1.8", | |||
| "version": "6.1.9", | |||
There was a problem hiding this comment.
If we are forcing Node18, this should be a major version bump imo. I'd personally like to avoid that if possible for ease of customer updating.
Seems like this would at least be a minor version bump as support for a new runtime is being added.
There was a problem hiding this comment.
We are no longer forcing Node 18 -- just making sure it runs on Node 18.
There was a problem hiding this comment.
I would still say that this is a minor version bump, multiple packages are being updated.
|
Minor nit: Would be nice to have formatting changes in a separate PR for ease of review. |
lostinauth0
dismissed stale reviews from pnkrishnamurthy-okta and bernardwolff
via
0d2267c
package.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "ad-ldap-connector", | |||
| "version": "6.1.9", | |||
There was a problem hiding this comment.
would be nice to have this happen automatically
By submitting a PR to this repository, you agree to the terms within the Auth0 Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.
Description
This PR upgrades AD LDAP Connector to run on Node 18.
It also fixes some dependencies to take care of Snyk reported issues with the
qspackageReferences
https://app.snyk.io/org/auth0-dir-services/project/db89091f-ee88-4ba3-9cb9-7723b9ad6fdf (search for
qs)https://auth0team.atlassian.net/browse/IUM-2445
Testing
Connector was run and tested against openldap in Vivaldi.
https://oktawiki.atlassian.net/wiki/spaces/~604ed65660d39e006f457fc5/pages/2847250039/IUM-2445+-Upgrading+AD-LDAP+connector+to+Node+18+and+fixing+SEC+issues
Checklist