dommilosz / minecraft-auth

29 stars 3 forks source link

Microsoft Auth not working on NodeJS v20 #24

Closed CordonZeus22 closed 6 months ago

CordonZeus22 commented 6 months ago

On NodeJS >= 20.0.0, the error "Cannot write headers after they are sent to the client" is throw when sending a request to the MicrosoftAuth server. It's because these two lines are executed after each requests.

https://github.com/dommilosz/minecraft-auth/blob/84b3a6405fc1cf4b4b8e22b7a070d0caefa68ea3/src/MicrosoftAuth/MicrosoftAuth.ts#L127-L128

For fixing just remove these lines, or move them in the default block instead of

https://github.com/dommilosz/minecraft-auth/blob/84b3a6405fc1cf4b4b8e22b7a070d0caefa68ea3/src/MicrosoftAuth/MicrosoftAuth.ts#L120-L125

It may be useful to setup the CI for running the tests on the supported versions of NodeJS

dommilosz commented 6 months ago

Yeah. I'll consider setting up ci in github actions. I'll look into it when back at home.

CordonZeus22 commented 6 months ago

I have seen you have added the ci and the tests doesn't pass on node 20, a fix for this may be to remove the server.closeAllConnections and instead use res.setHeader("Connection, "close") on all connections, this tell to the browser to not use keep-alive connections. By doing that, all the tests pass on node >= 14.6.0 (The problem come from jest that doesn't support node 14, and the atob module need to be imported in MojandAPI)

CordonZeus22 commented 6 months ago

I think you have forgot to edit the package.json engines fields for adding the node 16 version

dommilosz commented 6 months ago

Yeah fixed it.

CordonZeus22 commented 6 months ago

You have replaced the engines by node >= 16. This will doesn't warn on node version 17, 19 and 21 that haven't been tested.

dommilosz commented 6 months ago

I've added the versions into the tests. Do you have maybe any idea why sometimes this test fails but sometimes not? https://github.com/dommilosz/minecraft-auth/actions/runs/7341469648/job/19989210959. It's happening randomly on I think all versions. Probably some timing issues.

CordonZeus22 commented 6 months ago

I don't know why the tests failed, I have the same error in local, but it's not timeout issue as it throw Timeout error not undefined when the timeout expires.

CordonZeus22 commented 6 months ago

In the tests, when the listenForCode function is called, the error is catched and ignored, and the server is never close on some test, when we log the catched error we can see on some tests that the Timeout error is thrown, maybe this is the reason for the random errors

For see this, I have englobed all the test in a describe block with a each at 200

describe.each(new Array(200).fill(""))("Microsoft Server", function() {
// All the tests
})

and keep only the Microsoft Auth: /url endpoint test with the test.only

CordonZeus22 commented 6 months ago

This has been fixed with #25 and #26