Wrong port selection - Githubissues

m66g commented 3 years ago

When you set a port on the options, this port is ignored in the first login, and is recognized in the following logins.

Example: const options = { password: 'secret', // Password can also be passed during login host: '192.168.1.1', // Autodiscovery will be performed when left out port: 80, // SOAP port. Autodiscovery will be performed when left out tls: false // TLS/SSL (HTTPS) is only supported on certain router types }

Port 80 is a wrong port, the router uses 5000.

Critical lines of code in netgear.js // discover soap port and login method supported by router if (!this.loginMethod || !this.port) { const currentSetting = await this.getCurrentSetting(); this.port = currentSetting.port; }

For the first login, async login does the following:

loginMethod is unknown, port is set to 80 -> condition is true
currentSetting is obtained
this.port becomes 5000

For the second login, the same code does the following

loginMethod is known (2), port is set to 80 -> condition is false
this.port stays 80

This results in successful login for the first login, and a login error for all following tries. It's difficult to troubleshoot, as the error is shown for the second try to login, while the real problem is in the first try to login.

Example code to verify behaviour:

// create a router session, login to router, fetch attached devices
const Netgear = require('/usr/lib/node_modules/netgear');

const router = new Netgear();

const options = {
   password: 'secret',  // Password can also be passed during login
   host: '192.168.1.1', // Autodiscovery will be performed when left out
   port: 80,    // SOAP port. Autodiscovery will be performed when left out
   tls: false   // TLS/SSL (HTTPS) is only supported on certain router types
}

const getDevices = async() => {
    return new Promise (async (resolve, reject) => {
    try {
        console.log (Date.now()/1000 + ' start');
        console.log (await router.login(options));
    console.log (Date.now()/1000 + ' Logged in');
    const deviceArray = await router.getAttachedDevices();
    console.log (Date.now()/1000 + ' got it');
    // console.log(deviceArray);
    console.log('Lenght of result: ' + deviceArray.length);
    console.log (await router.logout());
    console.log (Date.now()/1000 + ' Logged out');
    resolve (deviceArray);
    } catch (error) {
        reject(error);
    }
    });
}

var http = require('http');

http.createServer(function(request, response) {

   if ('/' == request.url) {
      if ('GET' == request.method) {
         response.writeHead(200, {'Content-Type': 'application/json; charset=utf-8'});
         const gd = getDevices();
         console.log (Date.now()/1000 + ' Result: ');
         console.log (gd);
         response.end();
      }

      if ('POST' == request.method) {
         console.log('POST')
         var body = '';
         request.on('data', function(data) {
            body += data
            console.log('Partial body: ' + body)
         })
         request.on('end', function() {
            console.log('Body: ' + body)
            response.writeHead(200, {'Content-Type': 'text/html'})
            response.end('post received')
         })
      }
   }   
})
.listen(5001);
console.log('Listening to port 5001...');

To reproduce, send two get request GET localhost:5001/ The first login is successful, all following requests fail

This is how the code should be improved:

Replace // discover soap port and login method supported by router if (!this.loginMethod || !this.port) { const currentSetting = await this.getCurrentSetting(); this.port = currentSetting.port; }

By // discover soap port and login method supported by router if (!this.loginMethod || !this.port) { const currentSetting = await this.getCurrentSetting(); if (!this.loginMethod) { this.loginMethod = Number(currentSetting.LoginMethod) || 1; } if (!this.port) { this.port = currentSetting.port; } }

Sorry for my bad formatting, I'm new to github.

Great project, by the way, thanks a lot!!!

Kind regards

gruijter commented 2 years ago

Can you try to run the test again with the new version 4.4.2?

m66g commented 2 years ago

Hi gruiter,

we all spend a valueable part of our free time here. I really appreciate what you are providing here. Likewise, a little more information could help me (or anyone who tests the bugfix) to keep efforts efficient and effective. That is: Have you done anything in particular to fix the problem described, and if so, what did you correct? [I hope you didn't ask without doing anything about it... but I simply don't know.]

Moreover, honestely, I think I am the least person who can do the test efficiently, as I already corrected the bugfix I described above in the code I run locally. I'd have to remove the software, download the updated version, try it, and possibly implement the fix again is at least the double effort than anyone else has, who has not fixed the bug yet. The bug can much easier be confirmed as "still existing" or "corrected" by anyone using the software "as provided", setting the wrong port in the options (eg. 80), and try two consecutive logins (even that is described in the original ticket)!

Please don't get me wrong, but as much as a value your time, we should have an eye on keeping the overall effort low and reasonable for everyone.

Kind regards

gruijter commented 2 years ago

Hi @m66g ,

Whenever I take time to update my code I check if there are open issues. So during my last update I took some extra time to check your post. Your use case it not typical. You manually set a wrong port and then find that the login / auto-port selection is not working properly. Normally I would say that if you do not know the port, you also should not try to set it manually (unless you are doing manual testing).

But based on your descriptions I definitely did make several changes to the discovery process, and to how manual settings are handled. The last changes can be seen in the commit: https://github.com/gruijter/netgear.js/commit/a0d6c92d56301aef012dbd9e45d0eaa4df70d2e1

Since you posted the issue I assumed you would want to see if the new version works for your specific use case. But if you have no need for that, then it's ok. For my personal use cases all is working good, and I have not received issues from anyone else that could be related to what you experienced.

So I do think your post did help to make the netgear module more robust for edge cases.

Cheers! Robin

gruijter / netgear.js

Wrong port selection #13