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

🇨🇦 Use UNDICI fetch in Canada because of their badly configured servers #275

Merged
merged 9 commits into from
Apr 1, 2024

Conversation

djensenius
Copy link
Contributor

@djensenius djensenius commented Mar 29, 2024

This pull request includes changes to enhance the codebase's compatibility with newer versions of Node.js, does some linting, and update dependencies. The most important change is an update to Canadian code to use the fetch function from the undici library instead of got for HTTP requests if the Node.js version is 21 or higher.

This addresses #254

@Hacksore
Copy link
Owner

Thanks so much for this! Is NODE_TLS_REJECT_UNAUTHORIZED=0 required to make the call work?

@djensenius
Copy link
Contributor Author

It does not require any extra env variables!

The one downside is I haven't had time to write the mocks for using fetch instead of got when using Node 21+. Should be noted that Node 21+ is required for this to work as that's when fetch was turned on by default.

@@ -111,6 +113,40 @@ export class CanadianController extends SessionController<CanadianBlueLinkyConfi
/* eslint-disable @typescript-eslint/no-explicit-any */
private async request(endpoint, body: any, headers: any = {}): Promise<any | null> {
logger.debug(`[${endpoint}] ${JSON.stringify(headers)} ${JSON.stringify(body)}`);
const [major,,] = process.versions.node.split('.').map(Number);
if (major >= 21) {
process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw we are setting NODE_TLS_REJECT_UNAUTHORIZED to disable TLS validation, I thought we only need to override via SSL_OP_LEGACY_SERVER_CONNECT?

@Hacksore
Copy link
Owner

Lets make this becomes 9.x version target and we update the package.json engines field to require node >= 21.x.

https://docs.npmjs.com/cli/v10/configuring-npm/package-json#engines

If you can do that we can shipit :shipit:

❤️ ❤️ ❤️

@Hacksore Hacksore mentioned this pull request Mar 30, 2024
@djensenius
Copy link
Contributor Author

Hmm, are you sure the engine should be set to Node > 21? It's using got if under Node 21 in Canada, and got everywhere else. So it's a weird place where using the build in fetch, turned on by default in Node 21, is only used for Canada to get around a misconfigured server.

Happy to do it! Or I could rewrite the rest to use fetch and get rid of the got requirement. I'd want some help mocking tests though :)

@Hacksore
Copy link
Owner

Hacksore commented Apr 1, 2024

@djensenius you're right this should be good then and can stay 8.x.

Merging 😅.

@Hacksore Hacksore merged commit 8e1ddde into Hacksore:master Apr 1, 2024
2 checks passed
@carlosgamezvillegas
Copy link

@Hacksore,
Is there going to be a new release in npm with these changes?

@Hacksore
Copy link
Owner

Hacksore commented Apr 2, 2024

Published v8.3.1, give that a try.

@carlosgamezvillegas
Copy link

Hello @djensenius,

I tried to use this new version but I got the following error:


Client Error GotError [RequestError]: write EPROTO 405CCDDA01000000:error:0A000152:SSL routines:final_renegotiate:unsafe legacy renegotiation disabled:../deps/openssl/openssl/ssl/statem/extensions.c:922:

at ClientRequest.<anonymous> (/usr/local/lib/node_modules/homebridge-hyundai-bluelink/node_modules/got/source/request-as-event-emitter.js:178:14)
at Object.onceWrapper (node:events:634:26)
at ClientRequest.emit (node:events:531:35)
at ClientRequest.origin.emit (/usr/local/lib/node_modules/homebridge-hyundai-bluelink/node_modules/@szmarczak/http-timer/source/index.js:37:11)
at TLSSocket.socketErrorListener (node:_http_client:500:9)
at TLSSocket.emit (node:events:519:28)
at emitErrorNT (node:internal/streams/destroy:169:8)
at emitErrorCloseNT (node:internal/streams/destroy:128:3)
at processTicksAndRejections (node:internal/process/task_queues:82:21) {

code: 'EPROTO',
host: 'api.telematics.hyundaiusa.com',
hostname: 'api.telematics.hyundaiusa.com',
method: 'POST',
path: '/v2/ac/oauth/token',
socketPath: undefined,
protocol: 'https:',
url: 'https://api.telematics.hyundaiusa.com/v2/ac/oauth/token',
gotOptions: {
path: '/v2/ac/oauth/token',
protocol: 'https:',
slashes: true,
auth: null,
host: 'api.telematics.hyundaiusa.com',
port: null,
hostname: 'api.telematics.hyundaiusa.com',
hash: null,
search: null,
query: null,
pathname: '/v2/ac/oauth/token',
href: 'https://api.telematics.hyundaiusa.com/v2/ac/oauth/token',
retry: {
retries: [Function (anonymous)],
methods: [Set],
statusCodes: [Set],
errorCodes: [Set]
},
headers: {
'user-agent': 'PostmanRuntime/7.26.10',
client_id: 'm66129Bb-em93-SPAHYN-bZ91-am4540zp19920',
client_secret: 'v558o935-6nne-423i-baa8',
accept: 'application/json',
'accept-encoding': 'gzip, deflate',
'content-type': 'application/json',
'content-length': 68
},
hooks: {
beforeRequest: [],
beforeRedirect: [],
beforeRetry: [],
afterResponse: [],
beforeError: [],
init: []
},
decompress: true,
throwHttpErrors: true,
followRedirect: true,
stream: false,
form: false,
json: true,
cache: false,
useElectronNet: false,
method: 'POST',


Just for giggles I changed my Country to Canada and I I get "car was not found" for obvious reasons. Was the change you made only applied to Canada? if so, do you think it can be applied to the United States?

Environment:
Raspberry Pi - Raspbian
Node: 21.7.1
I am using bluelinky in a hombridge plugin (Homebridge-bluelinky)

Kind regards,

@djensenius
Copy link
Contributor Author

Yes, only Canada and only Node 21+.

Which country are you?

@carlosgamezvillegas
Copy link

@djensenius,

I am in the USA and to work around the problem I use node 16, but since it is not supported anymore by Homebridge and other programs. Is there something you can do to help us?

@djensenius
Copy link
Contributor Author

@carlosgamezvillegas if you (or someone) wants to update the automated tests to mock fetch in addition to the existing got tests, I'd be happy to make the changes to the rest of the countries.

I'm a little stretched thin for time and I'm a little unhappy about not having tests merged with this change for Canada.

@carlosgamezvillegas
Copy link

@djensenius,
Unfortunately I am knowledgeable enough to carry on with that task. In any case, thank you for all your contributions.

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

Successfully merging this pull request may close these issues.

3 participants