Fix sql injection and add multiple mentions #31
Conversation
|
Hi @Alex-Vol-SV, Just wanted to drop in quickly to thank you very much for your PRs. I’m currently travelling for an extended period so I’m not going to have a chance to review these for at least the next couple of months, just didn’t want you to not hear anything at all for that long! I’ll let you know once I’ve been able to have a proper look. |
|
@tdmalone I may take a stab at fixing your |
Travis CI updated the base images so the hacky PG 10 install is no longer needed and on top of that is failing the builds.
Pull Request Test Coverage Report for Build 127
💛 - Coveralls |
The TODO markers indicating the possibility of SQL injection issues were used to guide this implementation. Fixed by applying parameterized queries. Found a unitest issue that was masked by the use of concatenation in SQL and fixed the unit tests to match the runtime code execution.
Enabled the code to process multiple occurances of the ++/-- syntax in one message iteratively thus handling score updates for multiple recipients in one message. Changed the handler for selfPlus to be processed within the handlePlusMinus handler an simply not increment the score while providing the disapproving random message to the instigator.
Alex-Vol-SV
force-pushed
the
fix-sql-injection-and-add-multiple-mentions
branch
from
df247d1 to
8d507dd
Compare
|
I was able to fix the issues with the new base images. It was simple once I realized the problem was that PG 10 is now properly supported. I created 3 PR with each building on the previous one, this one combines the commits from the other two. Feel free to merge this and ignore the other two or just merge one at a time starting with the Travis fix first, then the SQL injection and finally this one. If you prefer three separate PR for each issue I can break it up but until master has the Travis CI configuration fixes the other PR fail t build. |
There was a problem hiding this comment.
Looks good!
My only comment is that the tests in tests/events.js would get more "coverage" by being split into two sets, one that verifies one action and one that verifies two actions. However the coverage is sufficient to verify the method, so only do this if you really want to.
This combined PR adds the fix to allow multiple mentions in the same message to work for plus/minus actions. I do not find a good reason why other kinds of actions should be combined.
The handleSelfPlus code was merged into the handlePlusMinus code as it made the operation able to use with other plus/minus operations.
The usage did not change, all that happens is each ++/-- will be added as a line in a multiline message where before only the very first ++/-- would be considered.
See my other PR for some comments relating to the SQL injection fix.
This fixes issue #8