BugFixed: prevent toLowerCase crash the App. #2993
Conversation
|
Thanks! A few thoughts: I would rather change this to throw a TypeError when not given a string instead of skipping lower casing, or perhaps coerce to argument to a string. Would like thoughts on this from other members. Also, please add a test that tests this condition, which is a prereq to accept the pull request. |
|
All right @dougwilson , i'll do that. So, in my opinion is better throw a TypeError. |
|
Sounds good, @ScalaSoft, please ping me once you've updated the pull request :) |
|
@dougwilson I made the necessary changes. Please check. |
|
|
||
| app.use(function(req, res){ | ||
| var name = ['something']; | ||
| assert.throws(typeThrowError(name), TypeError, "Error thrown"); |
There was a problem hiding this comment.
This test is just testing the function typeThrowError in this file, and is not actually testing the new if statement you added in lib/request.js. This is the reason your pull request is in a failure state, because you haven't actually tested your change yet :) To test your change, you'd want to do something like req.header(42) and test that it threw.
|
|
||
| // If name wasn't a string, it will crash | ||
| if(typeof name !== 'string') { | ||
| return new TypeError('Parameter "name" needs to be a String but got a ' + (typeof name)); |
There was a problem hiding this comment.
The error should be thrown, not returned. If you are going to go along the pattern of "but go a..." I would suggest using the type detection routing from the rest of the case base, which better handles all the various things that simply return object, like for example, null.
I caught this bug in development. When a user accidentaly ran a non string parameter to get on request, the App crashes.
I fixed this.