Server hardening with socket message validation.

[marshallswain]

Working on #112 & #96, I realized that it is really easy to take the server down by sending a malformed message. It's not very resilient. (and we don't want to test the limits of the forever module )

Really, Feathers shouldn't be responsible for a lot of validation, but it should do anything it can to keep itself running. It seems to me like we ought to at least validate the number of arguments as well as the presence (& maybe lightly the type) of each argument.

// Service methods arranged by number of arguments
var myService = {
  find:   function(params, callback) {},
  get:    function(id, params, callback) {},
  remove: function(id, params, callback) {},
  create: function(data, params, callback) {},
  update: function(id, data, params, callback) {},
  patch:  function(id, data, params, callback) {}
};

If we do validate the type of each argument, is there any case where id would be anything other than a string or number? MongoDB usually uses ObjectIDs, but that's something that the MongoDB service would need to validate. I can't imagine a need for using an object as an id, but maybe I need a better imagination. I think we're already checking for the presence of at least id in other places. We do it in utils.js feathers-hooks and it seems like I've seen something like this elsewhere.

The right place to be handling this is probably in commons.js. We are already creating a params object if it's not provided. Combined with #112, which does the same for the callback, I think all that's left is validating id and data as well as the number of arguments.

I think that, ideally, if the request is malformed, we would immediately send back an error 400 / Bad Request. Developers making services wouldn't have to worry about handling the same basic validation, but can focus on validation unique to the module they're developing.

@feathersjs/contributors While we're on the topic, maybe you've run into other places that the server could be hardened a bit. Let me know what you think.

[daffl]

That is true, you just have to emit some invalid data to make the server crash. The best place is probably in the socket argument validation at https://github.com/feathersjs/feathers/blob/master/lib/providers/socket/commons.js#L31. We want to make sure that there always is the minimum of the required parameters for Socket calls.

I don't think this is a problem for REST calls since they'll always look the same.

[marshallswain]

It seems from reading the paragraph directly under Section 2.1 Resource in the RestDoc spec that REST only allows for updating a single resource at a time. So id shouldn't be an array, ever, right? If that's the case, then do we want to hold people to the spec and validate the id like that? Or should we let every service handle that?

[marshallswain]

Here's a crack at minimum validation.

// Validate number of args.
      // 'find' doesn't require any.
      if (method !== 'find') {
        if (!args[0]) {
          // ERROR
        }
      }

      // 'update' and 'patch' have data at args[1].
      if(method === 'update' || method === 'patch'){
        if (!args[1]) {
          // ERROR
        }
      }

      // Validate params, create if not provided
      if(!args[position] || typeof args[position] === 'function') {
        args.splice(position, 0, {});
      }

      // Validate callback, create if not provided
      if (typeof args[position + 1] !== 'function') {
        // ... Send any errors back to the developer.
        var callback = function(err){
          if (err) {
            emitter.emit('error', {
              path:name,
              err:err
            });
          }
        };
        args.push(callback);
      }
      args[position] = _.extend({ query: args[position] }, params);
      if (error) {
        // SEND BACK AN ERROR.
        emitter.emit(name', err);
      } else {
        service[method].apply(service, args);
      }

I'm not sure it's enough.

[daffl]

I'm not sure either. Here are my thoughts:

• The default parameter object is empty (but needs to be extended for external calls)
• The last parameter always has to be a function. Otherwise use an empty callback
• For patch, update and remove the first argument has to be a string or number
• If create, patch and update only have one argument of type object it is the data parameter

The other question is: Should this be a service mixin or part of the socket handlers? Technically it would be possible to create a mixin that does that normalization and we won't have to worry about it anymore anywhere. The problem is that it would make things like hooks way more complicated. Or should the mixin just validate the parameter list to throw a reasonable error?

[ekryski]

@daffl It might make sense for it to be a service mixin as I think we should also be normalizing params on the REST side as well. I know less about the socket code currently, so your call.

Not sure if it was the intention, but I don't like the idea of forcing the user to pass a callback. I agree with the empty argument params, but the last param should be a callback, or they could pass nothing and internally we assign an empty callback to it. Maybe that is still too much overhead on our end but I think it will make a nicer API.

[daffl]

I don't think there is a need to normalize anything on the REST side. It'll always have a callback and parameters. I was thinking for 1.1 to provide Feathers utility functions that normalize the parameters that plugins can use as well though, e.g. var args = app.feathers.getParameters(methodName, arguments). That could be used in the Socket method handler as well as plugins like feathers-hooks (they hook in before a core mixin would so we'll need some external argument normalization anyway).

[lock]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue with a link to this issue for related bugs.