Prevent security scanner from killing wrapper with ECONNRESET #277
Conversation
|
@ChrisMiami - I don't personally come across this scenario, but the fix seems fine/logical. My one concern is cluttering the logs with ignored log messages. In particular, if someone is retroactively scanning logs, will they be scanning through alot of noise? If it's more of a signal of a resolvable problem in the underlying app, then I'm fine with it as-is. If not, it seems prudent to have a configuration flag to toggle the messaging. |
|
@coreybutler - this makes eminent sense, but although the event has been trapped to make it a non-issue, I'd guess it's something that more people would want to see than not. In my case it was a proactive internal security scanner, but in other cases it could signal a breach or DoS attack because what else would be connecting to that socket? Still, I'll add a separate log-level filter option (different PR?) to only log events at or above the listed level, with doc update. Is that a good solution? |
|
@ChrisMiami I'm not sure if this is still something you're interested in, given the age. I've been going through old PR's in a number of projects and came across this. I'd still be open to merging this one, and yes, having a separate log-level filter option would be a good solution. |
|
@coreybutler We're running into this issue as well and would love to have this PR merged. Could we create the logging level feature as a separate issue? |
|
@coreybutler We're also hitting this same issue in production FYI |
|
@coreybutler still interested in this, FWIW. |
|
|
||
| server.on('error', function (err) { | ||
| launch('warn', err.message); | ||
| server = net.createServer().listen(); | ||
| server.close(); // don't leak a trail of unclosed servers |
There was a problem hiding this comment.
Hi @ChrisMiami,
I have a question. Does the subsequent servers also close?
The first server will close the socket because we are listening the event, but after the first error, we are not listening the error events for the new created servers.
Thank you very much for your contribution
|
@ChrisMiami may you please allow me to push changes to your branch? I've made the logging change and we want to merge this ASAP. Otherwise, I'll create another PR |
wrapper.js is continually killed by a security penetration test that causes ECONNRESET errors on the socket. Handling the socket error prevents a server error which forestalls cycling the app. Since we don't care about the connections anyway, I log the error but ignore it.
In server.on('error'), I added a server.close() because server errors do not automatically close the server, so it was probably leaking servers.