Endpoints with parameters in body requested without content-type crashes app

[krivin]

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.

[theganyo]

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.

[krivin]

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.

[tbfangel]

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.

[michael-may]

+1 @tbfangel Seeing the same thing on one of my projects right now.

[shashanktomar]

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)

[nailtonvieira]

+1

[schristley]

I'm getting this error, any workaround?

[krivin]

@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.

[krivin]

@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.

[gomezd]

+1 @krivin solution seems to work. Any chance this will get fixed soon?