Prevents multiple types of crashes from invalid data; adds related tests #18
Conversation
This prevents a possible crashing attack. Resolves test failure.
…ections from a flooding address. Also allows tests to modify the flood window size. The current tests are relying on implementation details, and wouldn't pass if our anti-flooding logic were more aggressive, because we're expecting responses to messages from the same IP which does the flooding. This resolves that by ensuring that they won't also be disconnected.
|
If there are any other known ways of making the server crash, please mention them so we can create test cases (like @siracusa suggested at 1:20:39 of ATP 70). |
…cking were not. I don't think this is the way afterEach() should ideally be used -- assert() will abort the test suite rather than just fail the current test -- but it works.
banksJeremy
changed the title
reliably trigger crashes if flood prevention is commented-out. Rollback some unnecessary changes to accidentalbot.js. Commenting out messages logged when responding to attacks -- letting them flood your log wouldn't be great either.
banksJeremy
changed the title
|
@jeremyBanks, thanks so much for all the work on this. You're awesome. I'd like to add the tests to the master, but I'd also like to spend a few minutes wrapping my head around automated testing using Mocha and Travis. I've done plenty of NUnit/Jenkins in my day, but never Mocha/Travis. I have zero doubt that you've done everything exactly as you should; I'd just like to spend a few minutes making sure I understand it all. I probably won't have time for that until tomorrow, but please don't consider that a passive/aggressive statement about this request! I'll get to it! :) |
|
@cliss sure, that makes sense. It's your project, after all, so it's important that you know what's going on. :) I should disclaim that I don't have a ton of experience with JavaScript testing frameworks in general, and have none at all with Mocha (but it seemed the most-recommended when I took a look around), so note that this probably doesn't represent an ideal test structure. I was partially just figuring out how to make it work as I went along. It would benefit from some reorganization in the future. As a user, Travis is the simpler, I think. A few lines of configuration tell it what files to run, and in what environment. You sign up for free at travis-ci.org and enable it for your repository. Then it just runs the specified test script every time there's a new change to the repository, and sends email notifications. |
|
Although particularly now that I've posted these working attacks, it remains unlikely that the bot will make it through another episode without these changes. Poor bot. 😢 |
|
Haha, I plan to spend my getting-situated-before-recording-time tomorrow taking a look at this and getting it all integrated. :) |
…etection. Fix a mistake in a test (the invalid JSON wasn't being sent!). Adds a bit more logging so we're aware of all messages and errors -- if users are going to send something to bring it down, we should hope to have at least some related log messages (even if still not very detailed). Keeps track of which addresses have already been disconnected in the current window, so that they can be more efficiently ignored (without logging) for subsequent messages in the same window.
…ebSocket level.
Error from socket for 127.0.5.31: control frames cannot have more than 125 bytes of data
Conflicts: accidentalbot.js
|
It looks like request flooding isn't an actual issue, and the known crashes were dealt with by #25, so I'm going to close this. I'll probably submit another PR with tests if I get something together that looks like less of a mess. edit: D'oh, the flood blocking was already merged as #10. I guess that would mean this was even more pointless. |
Protects against another way users could currently cause the server to crash: text messages which are not valid JSON.
This adds a few basic tests which confirm that the server can still respond to a PING from one user, after being "attacked" (sent binary messages or invalid JSON text messages) by a different user
This also adds a Travis CI configuration file, which automatically runs these tests against Node 0.11 and 0.11. You can see the relevant logs for my branch here:
. If @cliss also enabled Travis for his master repository, this would result in the tests automatically being run against any submitted pull requests.
This doesn't include a DOS test, but I'd be surprised if a single regular user could bring down this very simple Node server with a DOS. I think the crash was more likely due to a crashing bug -- hopefully one of these two we now prevent.