Fix/detect windows excluded ports

[VioletXF]

Add an extra step to check if a port is in excluded port range on Windows

[laggingreflex]

So it basically returns "reserved" instead of "closed" for those ports, right?

Can you give a use case where such a distinction would be important?

I'm also concerned, since this project is so old, about projects that might depend on it being "closed" and not be able to handle an entirely new keyword. Basically could this change affect users negatively?

Great catch BTW!

[VioletXF]

First, I apologize for leaving this PR inactive for so long. I've been quite busy recently.

I've been using this package to search for an available port within a preferred range to bind a new server. While it generally works well, I've encountered occasional instances where I couldn't bind a server even though the package indicated that the port was not in use (i.e., 'closed').

After some research, I discovered that Windows might 'exclude' certain ports. This means that these ports are reserved by other software or the OS, making them unavailable for binding even though they appear to be free.

I understand that this could be a breaking change, so it might be better to introduce a new function to address this issue.

I'm figuring out how to write reliable test cases. However, testing 'excluded port ranges' can be challenging since they vary randomly, and some environments may not have them at all.

(And apologies for my bad English - I used ChatGPT to correct the grammar.)

[laggingreflex]

Thanks for the detailed explanation!

Ok so it seems the keyword 'reserved' is not really necessary in your use case, right? You only just wanna know if it's 'open' or 'closed', right?

So why not just have it return 'open'?

It's the breaking away from these two keywords that we currently use: 'open' and 'closed', and introducing a new 'reserved' that might be (more of) a breaking change, than simply just amending the functionality to check whether a port (for whatever reason - reserved or otherwise) is open or closed.

since they vary randomly

As for tests, you can just use the command above to find out those ports during tests.

• tests.js

  function findExcludedPorts () {
    if (!isWindows) return [] // nothing to check if not windows, right?
    const output = execSync('... net sh command to find excluded ports')
    const excludedPorts = output.something
    // return excludedPorts
    const sample = output[1]
    test.cb('...', t => {
      portScanner.find(sample) //... test here
    })
  }
  
  

[VioletXF]

Yes, the keyword 'reserved' is not necessary to me. However, I think returning 'open' for excluded ports may be a breaking change too, since originally it returned 'closed'.

And the test code you provided will work in most cases, but we have some edge cases where there are no excluded ports; in this case we cannot test anything (the test will pass though).

[laggingreflex]

However, I think returning 'open' for excluded ports may be a breaking change too, since originally it returned 'closed'.

Good point, but I would consider that a fix rather than a breaking change. Since despite returning "closed" it was actually "open" in reality (right? at least in the sense that it cannot be used, which is the main point of this project - to tell users whether a port can be used to listen to in their node apps or not), so it was a bug anyway which will be fixed by this PR.

Also there are internal workings that currently only expect "open" and "closed", so that's another reason to stick to those keywords. (unless we wanna update that as well)

but we have some edge cases where there are no excluded ports

That's most probably fine. We can just write a test specifically for Windows for now that has this issue (excluded ports).

[VioletXF]

Since despite returning "closed" it was actually "open" in reality (right? at least in the sense that it cannot be used, which is the main point of this project - to tell users whether a port can be used to listen to in their node apps or not)

The terminology 'open' is not strictly correct (since it is technically 'closed'). If the main objective is just finding a free, bindable port, it would be fine.

We can just write a test specifically for Windows for now that has this issue

That's a good idea. However, be aware that some Windows systems might not have any excluded ports in certain situations. While I haven’t encountered such cases personally, it’s worth noting that the test could produce false positives in rare circumstances.

[laggingreflex]

If the main objective is just finding a free, bindable port, it would be fine.

Hmm, what other use cases are you thinking might be there?

As far as I can tell, that's probably the main purpose:

Portscanner can check a port, or range of ports, for 'open' or 'closed' statuses.

---

it’s worth noting that the test could produce false positives in rare circumstances.

probably fine, tests don't need to be exhaustive, just covering the basics is a good start, we can refine later if we need to.

[VioletXF]

Hmm, what other use cases are you thinking might be there?

Maybe someone can use this package to detect whether some server program is running (they can do this check without this package by sending arbitrary request... but just in case). So I can't say this is 100% safe from breaking something.

The only way I can come up with not to break anything is, just adding an option to let users decide whether it should check excluded ports or not. (Or we can add new methods - e.g. isPortExcluded / findBindablePort). But it will make API a little complicated to use.

[laggingreflex]

Maybe someone can use this package to detect whether some server program is running (they can do this check without this package by sending arbitrary request... but just in case). So I can't say this is 100% safe from breaking something.

I'm not sure I understand, could you elaborate as an example?

Right now, if a user tests a reserved port (say 5357) it would currently return "closed", right?

And if we fix this by changing it to "open", how would it break that user's use case (if anything it'd help them)? Like what could such a user be doing with "closed" return value for this reserved port?

isPortExcluded

Great idea, but also fully agree on it complicating things further. This might go into scope creep (original package intent is to just find out open ports).

---

Personally I'm still leaning towards changing it to "open" and calling it a day. That would be the least of a breaking change than all other ideas IMO.

[VioletXF]

I'll give you an another example since I also don't think the example I gave you is appropriate.

Consider a port scanner, which scans for literally "open" ports, to check if we can "communicate" with those ports (not to check if we can "bind" a new server).

A hacking tool that checks vulnerability of all the open servers can be an application example. For this case, if we mark excluded ports as "open", the communication will fail since there are no servers bound to those ports.

[laggingreflex]

I'm convinced, thanks for persisting! Let's go forward with this.

I tried adding some tests but it seems not all ports given by the netsh command are returning the "reserved" keyword. Could you improve on this? We don't need to be exhaustive but we do need it to be reproducible.

ports: [
  [ '2869', '2869' ],
  [ '36189', '36288' ],
  [ '36289', '36388' ],
  [ '50000', '50059' ]
]
portsToCheck: [
   2869, 36189, 36190,
  36191, 36289, 36290,
  36291, 50000, 50001,
  50002
]
testing: 2869
testing: 36189
testing: 36190
testing: 36191
testing: 36289
testing: 36290
testing: 36291
testing: 50000
testing: 50001
testing: 50002
50002 { error: null, status: 'closed' }
50001 { error: null, status: 'closed' }
50000 { error: null, status: 'closed' }
36291 { error: null, status: 'reserved' }
36290 { error: null, status: 'reserved' }
36289 { error: null, status: 'reserved' }
36191 { error: null, status: 'reserved' }
36190 { error: null, status: 'reserved' }
36189 { error: null, status: 'reserved' }
2869 { error: null, status: 'open' }

[VioletXF]

As you said before, adding a new keyword "reserved" can also break something as returning "open" for excluded ports can. There are several options we can try to keep backward compatibility:

1. Add option
2. Introduce a new method
3. To make things easier, just bump the major version and document about the new keyword.

And thanks for working on the tests! As I know, the range 50000~50059 is 'administered port exclusions' (you can see '*' on the output), which means the user can freely bind a new server since the user reserved them.

~~I have no idea about 2869 BTW. Could you try running the tests multiple times? It could be a timing issue.~~ Nevermind, getting "open" from excluded port is totally possible since the program which reserved the port would be using it.

[laggingreflex]

I was planning to but I did not get a chance to improve the tests yet, let me know if you're able to. They just need to be reproducible (even if we test just a few ports). Like is there a way to know for sure which ones would return the "reserved" status? Also probably worth considering not creating any false positives (a "closed" returning "reserved")?

Also yeah releasing a new major was my plan as well.