swagger-node-runner
swagger-node-runner copied to clipboard
apigee-127
Endpoints with parameters in body requested without content-type crashes app
On https://github.com/theganyo/swagger-node-runner/blob/master/fittings/swagger_params_parser.js#L40, it says 'we do not check for errors here' Why is that?
There's a lot of nice validation in https://github.com/apigee-127/sway/blob/master/lib/types/parameter.js#L118 that results in exceptions. Since they are not caught in the parser, they will typically take down the process.
Say you have a swagger spec with an endpoint with a parameter "in: formData". Everything is working fine and dandy. However, if a (malicious?) client requests the endpoint and omits the Content-Type header, then that causes req.body to be undefined and an exception in to be thrown (https://github.com/apigee-127/sway/blob/master/lib/types/parameter.js#L147). Since that is not caught, gone is your service.
Since the pipe itself has an error handler attached, these errors will be caught and dealt with. The service will not crash. If you find otherwise, please let me know.
The parameter.getValue() is called in a callback from async.series(), it can't be caught by the pipe error handler.
See for your self with this swagger:
swagger: '2.0'
info:
title: Crash example
version: 0.0.0
paths:
'/':
post:
operationId: crash
x-swagger-router-controller: crashController
parameters:
- name: bla
in: formData
required: true
type: string
responses:
'504':
description: Got ìt
Just POST to / without a Content-Type header. No body needed.
In general, I think it's dangerous to assume that exceptions thrown from functions which are passed a callback that takes an err will be caught is a dangerous assumption.
What's happening on this issue? We see exactly the same error on a standard file upload using multipart/form-data. If you omit the file parameter, the server crashes due to the uncaught exception:
/goals/node_modules/pipeworks/pipeworks.js:208
throw err; // rethrow;
^
Error: req.files must be provided for 'formData' parameters of type 'file'
at Parameter.getValue (/goals/node_modules/sway/lib/types/parameter.js:141:15)
at /goals/node_modules/swagger-node-runner/fittings/swagger_params_parser.js:40:44
at Array.forEach (native)
at /goals/node_modules/swagger-node-runner/fittings/swagger_params_parser.js:39:46
at finishedParseBody (/goals/node_modules/swagger-node-runner/fittings/swagger_params_parser.js:128:12)
at /goals/node_modules/swagger-node-runner/node_modules/async/lib/async.js:726:13
at /goals/node_modules/swagger-node-runner/node_modules/async/lib/async.js:52:16
at /goals/node_modules/swagger-node-runner/node_modules/async/lib/async.js:269:32
at /goals/node_modules/swagger-node-runner/node_modules/async/lib/async.js:44:16
at /goals/node_modules/swagger-node-runner/node_modules/async/lib/async.js:723:17
at /goals/node_modules/swagger-node-runner/node_modules/async/lib/async.js:167:37
at parseText (/goals/node_modules/swagger-node-runner/fittings/swagger_params_parser.js:119:16)
at /goals/node_modules/swagger-node-runner/node_modules/async/lib/async.js:718:13
at Immediate.iterate (/goals/node_modules/swagger-node-runner/node_modules/async/lib/async.js:262:13)
at runCallback (timers.js:574:20)
at tryOnImmediate (timers.js:554:5)
at processImmediate [as _immediateCallback] (timers.js:533:5)
In pipeworks.js, there's a conditional only doing this if there is no faultPipe, however I have not been able to figure out how too add such a faultpipe handling the error.
In order to work around this issue, we have had to add an uncaught exception handler to our server, but this is not a solution we want to keep.
Still happening after more than 1 year.
Error: req.body must be provided for 'formData' parameters
at Parameter.getValue (/usr/src/app/node_modules/sway/lib/types/parameter.js:147:15)
at /usr/src/app/node_modules/swagger-node-runner/fittings/swagger_params_parser.js:40:44
at Array.forEach (native)
at /usr/src/app/node_modules/swagger-node-runner/fittings/swagger_params_parser.js:39:46
at finishedParseBody (/usr/src/app/node_modules/swagger-node-runner/fittings/swagger_params_parser.js:128:12)
at /usr/src/app/node_modules/swagger-node-runner/node_modules/async/lib/async.js:726:13
at /usr/src/app/node_modules/swagger-node-runner/node_modules/async/lib/async.js:52:16
at /usr/src/app/node_modules/swagger-node-runner/node_modules/async/lib/async.js:269:32
at /usr/src/app/node_modules/swagger-node-runner/node_modules/async/lib/async.js:44:16
at /usr/src/app/node_modules/swagger-node-runner/node_modules/async/lib/async.js:723:17
at /usr/src/app/node_modules/swagger-node-runner/node_modules/async/lib/async.js:167:37
at parseText (/usr/src/app/node_modules/swagger-node-runner/fittings/swagger_params_parser.js:121:60)
at /usr/src/app/node_modules/swagger-node-runner/node_modules/async/lib/async.js:718:13
at Immediate.iterate (/usr/src/app/node_modules/swagger-node-runner/node_modules/async/lib/async.js:262:13)
at runCallback (timers.js:672:20)
at tryOnImmediate (timers.js:645:5)
at processImmediate [as _immediateCallback] (timers.js:617:5)
@whitlockjc: thanks, https://github.com/apigee-127/sway/pull/148 seems to also fix the specific example in this issue, with required: false. However, the server crash issue remains with required: true.
@theganyo: just played a bit with swagger_param_parser.js - this seems to be what's needed to keep the server from crashing:
try {
var params = req.swagger.params = {};
req.swagger.operation.parameterObjects.forEach(function (parameter) {
params[parameter.name] = parameter.getValue(req);
});
}
catch (e) {
e.statusCode = 400; // most of the errors thrown from parameter.getValue(req) are caused by a bad request
next(e);
return;
}
at https://github.com/theganyo/swagger-node-runner/blob/master/fittings/swagger_params_parser.js#L38.
