Sails: Machine leaks API information in production

[alxndrsn]

In production, when required param(s) are omitted from e.g. a POST request, machine's E_MISSING_OR_INVALID_PARAMS response will leak information about the API.

E.g.:

$ curl http://localhost:1337/some_endpoint

{
  "code": "E_MISSING_OR_INVALID_PARAMS",
  "problems": [
    "\"some_var\" is required, but it was not defined."
  ],
  "message": "The server could not fulfill this request (`POST /some_endpoint`) due to 1 missing or invalid parameter."
}

This error message is generated in the machine-as-action module, and there does not appear to be an option to disable this.

These detailed messages are very helpful in development, and likely if building a production API to be consumed by third parties.

However, for some projects they represent an unacceptable level of data leakage.

It would be great if there were an option to disable these detailed error messages in production, and instead return a straight 400 error with content simply Bad Request (text/plain).

[sailsbot]

@alxndrsn Thanks for posting! We'll take a look as soon as possible.

In the mean time, there are a few ways you can help speed things along:

• look for a workaround. _(Even if it's just temporary, sharing your solution can save someone else a lot of time and effort.)_
• tell us why this issue is important to you and your team. What are you trying to accomplish? _(Submissions with a little bit of human context tend to be easier to understand and faster to resolve.)_
• make sure you've provided clear instructions on how to reproduce the bug from a clean install.
• double-check that you've provided all of the requested version and dependency information. _(Some of this info might seem irrelevant at first, like which database adapter you're using, but we ask that you include it anyway. Oftentimes an issue is caused by a confluence of unexpected factors, and it can save everybody a ton of time to know all the details up front.)_
• read the code of conduct.
• if appropriate, ask your business to sponsor your issue. _(Open source is our passion, and our core maintainers volunteer many of their nights and weekends working on Sails. But you only get so many nights and weekends in life, and stuff gets done a lot faster when you can work on it during normal daylight hours.)_
• let us know if you are using a 3rd party plugin; whether that's a database adapter, a non-standard view engine, or any other dependency maintained by someone other than our core team. _(Besides the name of the 3rd party package, it helps to include the exact version you're using. If you're unsure, check out this list of all the core packages we maintain.)_
  

  ---

Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

[rachaelshaw]

@alxndrsn this totally make sense, maybe some kind of function that would run in this scenario and return the desired response body (leaving the headers & status code the same).

If anyone else looking at this issue has anything to add, open to more suggestions/feedback!

[crh3675]

Although the framework has great power with its automation, you can provide a workaround with your own controller I presume.

[alxndrsn]

@crh3675 this is true, but inconvenient if it has to be done in many controllers simultaneously.

[crh3675]

Policies are processed before controllers, perhaps in there? You can apply a global policy across all controllers which might be another temporary solution

[alxndrsn]

As far as I could work out, you can only have one policy per controller. Also sounds a bit hacky... maybe more intuitive would be an error-handling middleware which filters out the messages I don't like.

[crh3675]

You can have multiple policies per controller - we do it all the time- just use an array instead. Or set a wildcard catchall.

module.exports.policies = {

  // Default policy is to require authentication
  '*': ['isAuthenticated'],

  /**
   * Main auth controller
   */
  AuthController: {
    '*': ['isPublic'],
    'logout' : ['isPublic'],
    'switch': ['isService', 'isAdmin'],
    'change': ['isService'],
    'validate': ['isPublic']
  },

  AdminController: ['isPublic'],
  HealthController: ['isPublic'],

Not that with multiple policies, you need to enfore the usage of next in the polict

module.exports = function(req, res, next) {
   if (//something) {
      res.send('bah');
   } else {
      next();
   }
}

[alxndrsn]

You can have multiple policies per controller

@crh3675 this is true. I got confused, because I've tried in the past to enforce policies like "is admin OR manager", but this doesn't seem simple to achieve via policies alone - the combined policies such as

    'some-endpoint': ['isAdmin', 'isManager'],

are a conjunction rather than disjunction, so I've ended up with policies like isAdminOrManager. This gets tedious when there are many different combinations implemented.

[alxndrsn]

@raqem I think there are workarounds implied, but not clearly defined. Options seem to be:

1. rewriting the controller to not use machine
2. adding middleware to filter out these errors for particular endpoints and/or in particular environments

I'd guess that 2 is the less risky option, but I can't confirm that it either works or is reliable. I'll share info here when I've implemented a workaround.

[alxndrsn]

@crh3675 going back to your original point:

Policies are processed before controllers, perhaps in there? You can apply a global policy across all controllers which might be another temporary solution

As there's no setting for machine to disable these error messages, it's not clear to me how a policy would be able to help in this situation.

[crh3675]

I guess it would be easier if the machine actually threw an error that was catchable. I understand a bit more after looking into the machine modularization. Looks like you might be able to create a custom api/responses/badRequest.js with custom code and catch the E_MISSING_OR_INVALID_PARAMS error code . Seems to not be the most desirable but may provide a workaround.

[karmamaster]

I am finding a solution for this too, These messages can be leak our api params to outside.

[crh3675]

Super simple fix in in responses/badRequest

  if (_.isError(data)) {
    if (data.code === 'E_MISSING_OR_INVALID_PARAMS') {
      if (data.hasOwnProperty('problems')) {
        delete data.problems;
      }
    }

[alxndrsn]

@crh3675 neat. Do you mean in ./node_modules/sails/lib/hooks/responses/defaults/badRequest.js?

[crh3675]

When you create a new Sails application you should have a folder under api/responses of which you can change

[karmamaster]

When you create a new Sails application you should have a folder under api/responses of which you can change

No, sails will not create response folder automatically. You will need to create the folder also the customResponse file by yourself

[crh3675]

So it seems that the solution is to copy /node_modules/sails/lib/hooks/responses/defaults/badRequest.js into api/responses and customize. It seems my earlier version of Sails 0.12 did auto-create those but 1.0 does not.

[karmamaster]

Yeah, the solution I choose like this:

1. Create a custom response file in api/response folder.
2. define exits property like this:

{
        successCreate: {
          statusCode: 201,
          description: 'Success to create',
          responseType: 'responseToClient',
        },

        invalidInputParam: {
          statusCode: 400,
          description: 'Bad param input',
          responseType: 'responseToClient',
        },

        notAuthorize: {
          statusCode: 401 ,
          description: 'Not authorize to access',
          responseType: 'responseToClient',
        },

        notUnique: {
          statusCode: 409,
          description: 'Not unique',
          responseType: 'responseToClient',
        },

      }

1. After that, whenever you call exit.[ exit function name ]() then the data will automatically send direct to responseToClient function. Just do whatever you want in that file