Sails: Data leak between requests

[bobthebuilderio]

Node version: node:8.12-alpine
Sails version _(sails)_: "^1.2.3"
ORM hook version _(sails-hook-orm)_: "^2.1.1"
Sockets hook version _(sails-hook-sockets)_: "^1.4.0"

---

Unfortunately we faced two major issues on 2 important api built with sailsjs.

ISSUE 1: Somehow response data from one request say to '/user/login' end up being swapped with someone's else request response. So users on our apps got swapped and saw the other's info,.

ISSUE 2: Similarly, two parallel requests to an api say 'list-items' end up mixing items from both users together in the resonse. So one user gets some of his items and some of the other user's items & same with the other user.

What we did to solve the problem, was to refactor the code a bit:
1- We stopped using things like promise.all() and did some refactoring which solved ISSUE 1
2- We had a function receives items list as input goes into a for loop ( for x of y ) to call two awaits db calls for each of the items, create some additional properly for the item, push into a new array and return this array. This array was getting mixed up.

Regardless of our code and whether it could be written in a better way or not, how on earth is data being swapped like this ? is there anyone who knows or faced something like this with saisjs before ?

Thank you !

[sailsbot]

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

[bobthebuilderio]

As an additional piece of information, it is worth noting that we do use DynamoDb Waterline Adapter. We added some extra feature support ourselves internally as it was a bit behind. Yet I still don't see how the cases above could occur regardless of anything else. There seem to be a Major / fundamental issue somewhere.

[bobthebuilderio]

I do not think the issue has to do with the ORM itself, as the data if logged before the array handling function described in issue 2 is correct, but get's mixed after the function gets called.

[nahanil]

Can you share any actual code or create a minimal github repo that can reproduce these issues?

[bobthebuilderio]

This is the main controller that fetches the data - THIS IS THE CODE THAT IS GENERATING THE ISSUE. I will follow up with another comment of the file where issue was fixed.

`module.exports = {

friendlyName: sails.__('controllers.orderItem.arango.listByOrderId.friendlyName'),
description: sails.__('controllers.orderItem.arango.listByOrderId.description'),

inputs: {
orderId: {
description: sails.__('controllers.orderItem.arango.listByOrderId.inputs.orderId.description'),
type: 'string',
required: true,
},
},

exits: {
orderItemNotFound: { responseType: 'orderItemNotFound' },
universalItemNotFound: { responseType: 'universalItemNotFound' },
orderNotFound: { responseType: 'orderNotFound' },
serverError: { responseType: 'serverError' },
success: { description: 'order with itemslist' },
},

fn: async function (inputs, exits) {
try {

  const { orderId } = inputs;

  // Get item lists
  const orderWithItemsList = await sails.helpers.models.arangodb.orderItem.listByOrderId.with({ orderId });
  const { order, itemsList } = orderWithItemsList;

  // Validation added to investigate bug
  _validateItemsList2({ itemsList });
 // !!!!!!! DATA IS OK HERE STILL !!!!!!  //
  // Prepare return data
  const newItemsList = await _addMissingDetailsList({ itemsList });
  console.log('TAKE 2');
  // Validation added to investigate bug
  _validateItemsList2({ itemsList: newItemsList });
 //!!!!!!!!! BUG APPEARS IN THIS CHECK !!!!! DATA MIXED WITH ANOTHER USER REQUEST THAT HAPPENED AT THE SAME TIME //

  const newOrder = await _addMissingDetailsToOrder({ order });
  console.log('TAKE 3');
  // Validation added to investigate bug
  _validateItemsList2({ itemsList: newItemsList });

  orderWithItems = {
    ...sails.dtos.arango.OrderDTO({ order: newOrder }),
    itemsList: sails.dtos.arango.ItemListDTO({ items: newItemsList }),
  };
  console.log('TAKE 4');
  // Validation added to investigate bug
  _validateItemsList2({ itemsList: orderWithItems.itemsList });
  // Done!
  exits.success({ orderWithItems });

} catch (error) {
  switch (_.get(error, 'raw.code') || _.get(error, 'code')) {

    case 'orderItemNotFound':
      exits.orderItemNotFound({ message: sails.__('controllers.orderItem.arango.listByOrderId.exits.orderItemNotFound') });
      break;

    case 'universalItemNotFound':
      exits.universalItemNotFound({ message: sails.__('controllers.orderItem.arango.listByOrderId.exits.universalItemNotFound') });
      break;

    case 'orderNotFound':
      exits.orderNotFound({ message: sails.__('controllers.orderItem.arango.listByOrderId.exits.orderNotFound') });
      break;

    default:
      exits.serverError(error);
  }
} finally {
  return exits;
}

},
};

/**

• _addMissingDetailsList
  *
• @param {object} inputs
• @param {array} inputs.itemsList
  *
  */
  async function _addMissingDetailsList ({ itemsList }) {

const itemsListToReturn = [];

for (item of itemsList) {

const orderItem = await OrderItem.getByCodeIfExists.with({
  code:    item.itemCode,
  orderId: item.orderId,
});

const universalItem = await UniversalItem.getByCodeIfExists.with({
  code: item.itemCode,
});

item.stainDetails = orderItem.stainDetails;
item.frontImage = universalItem.frontImage;
itemsListToReturn.push(item);

}

return itemsListToReturn;
}

/**

• _addMissingDetailsList
  *
• @param {object} inputs
• @param {object} inputs.order
  *
  */
  async function _addMissingDetailsToOrder ({ order }) {
  const { pickupImageBags } = await sails.hooks.srvOrder.order.getByIdIfExists({ orderId: order.orderId });

order.pickupImageBags = !_.isArray(pickupImageBags)
? [pickupImageBags]
: pickupImageBags;

return order;
}

/**

• _validateItemsList2
  *
• @param {object} inputs
• @param {array} inputs.itemsList
  *
  */
  function _validateItemsList2 ({ itemsList }) {

const itemsList2 = _.groupBy(itemsList, 'orderAlphaId');

if (Object.keys(itemsList2).length > 1) {
console.log('CUSTOMER APPROVAL MIXED ORDERS AT BACKEND MAPPING LEVEL');

}
}

`

[bobthebuilderio]

THIS IS THE CODE THAT FIXED THE ISSUE. AS YOU CAN SEE SIMPLE CHANGE TO THE CODE. NOTHING THAT IS OBVIOUSLY WRONG OR COULD CAUSE AN ISSUE LIKE THIS. SOMETHING HAS TO DO WITH ASYNC/AWAIT / LOOPS. As the issue arises when we have promise.all or awaits in for loops. Might be just talking gibberish :) !

`module.exports = {

friendlyName: sails.__('controllers.orderItem.arango.listByOrderId.friendlyName'),
description: sails.__('controllers.orderItem.arango.listByOrderId.description'),

inputs: {
orderId: {
description: sails.__('controllers.orderItem.arango.listByOrderId.inputs.orderId.description'),
type: 'string',
required: true,
},
},

exits: {
orderItemNotFound: { responseType: 'orderItemNotFound' },
universalItemNotFound: { responseType: 'universalItemNotFound' },
orderNotFound: { responseType: 'orderNotFound' },
serverError: { responseType: 'serverError' },
success: { description: 'order with itemslist' },
},

fn: async function (inputs, exits) {
try {

  const { orderId } = inputs;

  // Get item lists
  const orderWithItemsList = await sails.helpers.models.arangodb.orderItem.listByOrderId.with({ orderId });
  const { order, itemsList } = orderWithItemsList;

  // Prepare return data
  await _addMissingDetailsList({ itemsList });

  const newOrder = await _addMissingDetailsToOrder({ order });

  orderWithItems = {
    ...sails.dtos.arango.OrderDTO({ order: newOrder }),
    itemsList: sails.dtos.arango.ItemListDTO({ items: itemsList }),
  };

  // Done!
  exits.success({ orderWithItems });

} catch (error) {
  switch (_.get(error, 'raw.code') || _.get(error, 'code')) {

    case 'orderItemNotFound':
      exits.orderItemNotFound({ message: sails.__('controllers.orderItem.arango.listByOrderId.exits.orderItemNotFound') });
      break;

    case 'universalItemNotFound':
      exits.universalItemNotFound({ message: sails.__('controllers.orderItem.arango.listByOrderId.exits.universalItemNotFound') });
      break;

    case 'orderNotFound':
      exits.orderNotFound({ message: sails.__('controllers.orderItem.arango.listByOrderId.exits.orderNotFound') });
      break;

    default:
      exits.serverError(error);
  }
} finally {
  return exits;
}

},
};

/**

• _addMissingDetailsList
  *
• @param {object} inputs
• @param {array} inputs.itemsList
  *
  */
  async function _addMissingDetailsList ({ itemsList }) {

for (let i = 0; i < itemsList.length; i++) {

const orderItem = await OrderItem.getByCodeIfExists.with({
  code:    itemsList[i].itemCode,
  orderId: itemsList[i].orderId,
});

const universalItem = await UniversalItem.getByCodeIfExists.with({
  code: itemsList[i].itemCode,
});

itemsList[i].stainDetails = orderItem.stainDetails;
itemsList[i].frontImage = universalItem.frontImage;

}

}

/**

• _addMissingDetailsList
  *
• @param {object} inputs
• @param {object} inputs.order
  *
  */
  async function _addMissingDetailsToOrder ({ order }) {
  const { pickupImageBags } = await sails.hooks.srvOrder.order.getByIdIfExists({ orderId: order.orderId });

order.pickupImageBags = !_.isArray(pickupImageBags)
? [pickupImageBags]
: pickupImageBags;

return order;
}
`

[bobthebuilderio]

@johnabrams7 I think this should not be labeled as a Question, rather should be something more serious and needs immediate attention. What do you think ?

[johnabrams7]

@bobthebuilderio Yes, good point - went ahead and labeled it Bug instead 👍

Huge thanks for all the effort and dedication with this, along with a workaround! 🎉
Team will give this a look. In the meantime, can you provide us a minimal repo that reproduces this issue? PR welcome as well!

To confirm, which specific DynamoDb Waterline Adapter are you using with Sails? There appears to be multiple.

[bobthebuilderio]

@johnabrams7 sorry for the delay, was overwhelmed. I will ask someone from the team to provide a clean repo that could reproduce the issue. Hopefully will be able to demostrate it as this appears on a production environment, so we might have to create a script that simulate simultaneous hits. Will update you soon.

[johnabrams7]

@bobthebuilderio Now worries, that sounds great! We'll look forward to checking it out.