Skip to content

prevent body format errors and wasted json parsing on correct but "non json" repsonses #95

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lucidNTR wants to merge 6 commits into
base: master
Choose a base branch
from
Open

prevent body format errors and wasted json parsing on correct but "non json" repsonses #95

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4490374
prevent body format errors and wasted json parsing on correct but "no…
lucidNTR Sep 25, 2017
0aeca63
fix: linting
lucidNTR Nov 8, 2017
a67d697
fix: peer dep hawk 6.0.2 no longer supports node 4.2
lucidNTR Nov 8, 2017
ede8cb8
remove yarn file
lucidNTR Nov 28, 2017
2df3285
Try parsing json as fallback for undefined content-type
lucidNTR Nov 28, 2017
35e1a79
fix latest linting rules
lucidNTR Nov 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .eslintignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
test/unit/**/node_modules
test/unit/**/node_modules
examples/**/node_modules
4 changes: 2 additions & 2 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
},
"extends": "eslint:recommended",
"rules": {
// 4-space indentation
// 2-space indentation
"indent": 2,
// Require semicolons
"semi": [2, "always"],
Expand Down Expand Up @@ -44,4 +44,4 @@
"no-console": 0,
"max-len": [2, { "code": 120 }]
}
}
}
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ tests-report.xml
*.swp
*.swo
*.log
.DS_Store
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
language: node_js
node_js:
- "4.2"
- "4.5"
- "4"
- "5"
- "6"
Expand Down
59 changes: 30 additions & 29 deletions handlers/_swagger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ exports.init = function (app, auth, config, logger, serviceLoader, swagger, call
responseModelValidationLevel = swagger.getResponseModelValidationLevel();
polymorphicValidation = swagger.isPolymorphicValidationEnabled();
httpMethods = swagger.getValidHttpMethods();
rejectRequestAfterFirstValidationError = !!cfg.rejectRequestAfterFirstValidationError;
rejectRequestAfterFirstValidationError = !!cfg.rejectRequestAfterFirstValidationError;

var useBasePath = cfg.useBasePath || (cfg.useBasePath === undefined); //default to true
var serveSpec = cfg.serve;
Expand Down Expand Up @@ -319,31 +319,32 @@ function registerRoute(app, auth, additionalMiddleware, method, path, data, allo
setDefaultQueryParams(req, data, logger);
setDefaultHeaders(req, data, logger);

//Wrap the set function, which is responsible for setting headers
if (allowedTypes) {
//Validate that the content-type is correct per the swagger definition
wrapCall(res, 'set', function (name, value) {
if (name === 'Content-Type') {
var type = value.split(';')[0]; //parse off the optional encoding
if (!_.contains(allowedTypes, type)) {
logger.warn('Invalid content type specified: %s. Expecting one of %s', type, allowedTypes);
}
// Wrap the set function, which is responsible for setting headers
var type = '';
Copy link
Copy Markdown
Member

@seanpk seanpk Nov 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for clarity, I think it would be good to call the variable something like responseContentType

// Validate that the content-type is correct per the swagger definition
wrapCall(res, 'set', function (name, value) {
if (name === 'Content-Type') {
type = value.split(';')[0]; //parse off the optional encoding
if (allowedTypes && !_.contains(allowedTypes, type)) {
logger.warn('Invalid content type specified: %s. Expecting one of %s', type, allowedTypes);
}
});
}
}
});

if (responseModelValidationLevel) {
var responseSender = res.send;
res.send = function (body) {
var isBodyValid = isValidDataType(body);
Copy link
Copy Markdown
Member

@seanpk seanpk Nov 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like it might be good to refactor this function's name, maybe to isValidJsonData

With that change, it may make sense to move the check for 'application/json' to the outermost extent to protect the subsequent code. However, I am not sure how that will affect users who just call res.send without explicitly setting the Content-Type ...

if (!isBodyValid) {
try { //body can come in as JSON, we want it unJSONified
body = JSON.parse(body);
} catch (err) {
logger.error('Unexpected format when attempting to validate response');
res.send = responseSender;
responseSender.call(res, body);
return;
if ( type === 'application/json') {
Copy link
Copy Markdown
Member

@seanpk seanpk Nov 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an extra space between the bracket and type ...

Also, how does this affect users who are not explicitly setting their response content type?
Has express already set that header by this time?
Above, should the default value for type be 'application/json'?
Or should this check be more like: if (!type || type === 'application/json')?

try { //body can come in as JSON, we want it unJSONified
body = JSON.parse(body);
} catch (err) {
logger.info('Unexpected format when attempting to validate response');
res.send = responseSender;
responseSender.call(res, body);
return;
}
}
} else if (body) {
// if the response object has a property which is an object that implements toJSON() ...
Expand Down Expand Up @@ -377,7 +378,7 @@ function registerRoute(app, auth, additionalMiddleware, method, path, data, allo
alteredBody._response_validation_errors = invalidBodyDetails;
}
}

// 'warn'
// response is sent to the caller unmodified, errors (this model) are only logged (see below)
validationErrors.invalidResponse = {
Expand All @@ -386,7 +387,7 @@ function registerRoute(app, auth, additionalMiddleware, method, path, data, allo
statusCode: res.statusCode,
body: body || null
};

// 'fail'
// changes the http response code to 522 and separates the validation errors and response body.
// Will break client code; should only be used when developing/testing an API in stand alone
Expand All @@ -396,7 +397,7 @@ function registerRoute(app, auth, additionalMiddleware, method, path, data, allo
} else if (alteredBody) {
body = alteredBody;
}

// in all cases, log the validation errors
logger[responseModelValidationLevel === 'warn' ? 'warn' : 'error'](
'Response validation error:', JSON.stringify(validationErrors, null, 2)
Expand Down Expand Up @@ -457,7 +458,7 @@ function validateRequestParameters(req, data, swaggerDoc, logger, callback) {
}
}
}

if (validationErrors.length > 1) {
var superValidationError = _createRequestValidationError('Multiple validation errors for this request',
{ in: 'request' }, []);
Expand All @@ -483,10 +484,10 @@ function validateRequestParameters(req, data, swaggerDoc, logger, callback) {
return callback(validationErrors[0]);
}
return callback();

/**
* @param {Object} parameter the swagger-defined parameter to validate
*
*
* @returns {Object} the validation error for the parameter, or null if there's no problem
*/
function _validateParameter(parameter) {
Expand Down Expand Up @@ -586,7 +587,7 @@ function validateRequestParameters(req, data, swaggerDoc, logger, callback) {
}
if (!result.valid || polymorphicValidationErrors.length > 0) {
result.errors = result.errors || [];
error = _createRequestValidationError('Error validating request body', parameter,
error = _createRequestValidationError('Error validating request body', parameter,
result.errors.concat(polymorphicValidationErrors));
}
break;
Expand All @@ -604,7 +605,7 @@ function validateRequestParameters(req, data, swaggerDoc, logger, callback) {
* @param {string} [parameterConfig.name] the name of the parameter that failed validation
* (only used when parameterConfig.in is header, path, query, or form)
* @param {Object[]} subErrors the array of validation errors from the swaggerUtil validation function
*
*
* @returns {Object} a VError representing the validation errors detected for the request
*/
function _createRequestValidationError(message, parameterConfig, subErrors) {
Expand All @@ -622,7 +623,7 @@ function _createRequestValidationError(message, parameterConfig, subErrors) {
subError.field = _.get(subError, 'source.name',
path.join((subError.dataPath || '/'), _.get(subError, 'params.key', '')));
subError.in = _.get(subError, 'source.type');

});
return error;
}
Expand Down Expand Up @@ -682,7 +683,7 @@ function validateResponseModels(res, body, data, swaggerDoc, logger) {
* res.req.method, res.req.path, res.statusCode
* @param {Object} res the express.js response object
* @param {Object[]} subErrors the array of validation errors from the swaggerUtil validation function
*
*
* @returns {Object} a VError representing the validation errors detected for the response
*/
function _createResponseValidationError(messageFormat, res, subErrors) {
Expand Down
Loading