Conversation
assmass13
force-pushed
the
port-sniffer
branch
from
dfb6959 to
518e887
Compare
OleksiyRudenko
left a comment
OleksiyRudenko
left a comment
There was a problem hiding this comment.
@assmass13 good job!
However your commit affects demo files added by @AMashoshyna :
- removes
submissions/amashoshyna/port-sniffer/index.js - renames
submissions/amashoshyna/port-sniffer/README.md
The above means these files will be no longer available for future contributors which is not really nice.
Please fix the issue.
OleksiyRudenko
left a comment
OleksiyRudenko
left a comment
There was a problem hiding this comment.
The above is not a code review, however needs to be fixed before merge.
assmass13
force-pushed
the
port-sniffer
branch
from
518e887 to
6870a94
Compare
assmass13
force-pushed
the
port-sniffer
branch
from
6870a94 to
e4d1c0a
Compare
|
@assmass13 thank you for updating the code base. Please avoid force pushing branches as this destroys the history. |
lempiy
left a comment
lempiy
left a comment
There was a problem hiding this comment.
Very cool. See small requests bellow.
| exit(0); | ||
| } else if ( | ||
| args.indexOf('--host') === -1 || | ||
| !args[args.indexOf('--host') + 1] |
There was a problem hiding this comment.
What if we have
node sniffer.js --host --ports '80-90'| } | ||
|
|
||
| return args.reduce((result, item, index, arr) => { | ||
| if (item.indexOf('--') === 0 && arr[index + 1]) { |
There was a problem hiding this comment.
item.indexOf('--') === 0=>
item.startsWith('--')| const parsedArgs = parseArgs(processArgs); | ||
| const address = await getAddress(parsedArgs['--host']); | ||
| const portLimits = | ||
| '--ports' in parsedArgs |
There was a problem hiding this comment.
be careful with that:
'length' in []
// true
'includes' in []
// true|
|
||
| return args.reduce((result, item, index, arr) => { | ||
| if (item.indexOf('--') === 0 && arr[index + 1]) { | ||
| return { ...result, [item]: arr[index + 1] }; |
There was a problem hiding this comment.
maybe write to result object without useless symbols ?
{ ...result, [item.slice(2)]: arr[index + 1] }
|
|
||
| function getPortLimits(portsRange) { | ||
| const arePortsValid = ports => | ||
| ports.length === 2 && |
There was a problem hiding this comment.
Its better to move it to module scope, cause this function doesn't use closure variables.
| for (let port = startPort; port < endPort; port += 1) { | ||
| openPorts.push(scanAddressPort(address, port)); | ||
| } | ||
| return Promise.all(openPorts).then(values => values.filter(Number.isFinite)); |
There was a problem hiding this comment.
Bad idea, you may get a lot opened sockets at once, which, believe me, is not what we want to have. It may consume a lot of system resources.
There was a problem hiding this comment.
Resolve it with batches or sequentially
| }); | ||
|
|
||
| socket.on('timeout', () => { | ||
| resolve(false); |
There was a problem hiding this comment.
Its a bad practice to return different datatypes from one function. In this case boolean and number. Use null for that.
| async function getAddress(host) { | ||
| return new Promise((resolve, reject) => { | ||
| dns.lookup(host, (err, address) => { | ||
| if (err) reject(err); |
|
|
||
| function exit(code) { | ||
| process.exit(code); | ||
| } |
There was a problem hiding this comment.
Maybe if you have helpers already you can do following ?
function writeFatal(str) {
rocess.stdout.write(str);
process.exit(1);
}
function writeSuccess(str) {
rocess.stdout.write(str);
process.exit(0);
}
| function getPortLimits(portsRange) { | ||
| const arePortsValid = ports => | ||
| ports.length === 2 && | ||
| ports[0] >= defaultPortLimits.startPort && |
There was a problem hiding this comment.
btw, ports[0] never be lower then 0 cause you got ports.split('-')
| const parsedArgs = parseArgs(processArgs); | ||
| const address = await getAddress(parsedArgs['--host']); | ||
| const portLimits = | ||
| '--ports' in parsedArgs |
There was a problem hiding this comment.
be careful with that:
'length' in []
// true
'includes' in []
// true4 participants
Port sniffer is developed.