Create Service API mutates function arguments

[petermikitsh]

Steps to reproduce

const app = express(feathers());

// app setup abridged

const credentials = {email: '[email protected]', password: 'supersecret'};
app.service('api/users').create(credentials).then((user) => {
  console.log('credentials is', credentials);
  console.log('user is', user);
});

Console output:

credentials is { email: '[email protected]',
  password: '$2a$12$GJa/B3lOwPHhfJaGO4GNmOZA3NgXDEBqbhG1pWMRThm78mMnLUR/S' }
user is { id: '5642db35-f993-4c9c-bf86-a83268347225',
  admin: false,
  notifyAll: false,
  notifySigned: true,
  email: '[email protected]',
  password: '$2a$12$GJa/B3lOwPHhfJaGO4GNmOZA3NgXDEBqbhG1pWMRThm78mMnLUR/S',
  updatedAt: 2018-01-17T03:45:57.864Z,
  createdAt: 2018-01-17T03:45:57.864Z }

Expected behavior

The credentials object shouldn't have mutated. That is, it should equal {email: '[email protected]', password: 'supersecret'};.

Actual behavior

The password property on the credentials object was mutated.

System configuration

Tell us about the applicable parts of your setup.

Module versions (especially the part that's not working):

NodeJS version: 8.2.1

Operating System: OS X 10.12

Browser Version: N/A

React Native Version: N/A

Module Loader: Webpack

I'm using all of the latest feathers modules:

"@feathersjs/feathers": "3.0.5",
"feathers-sequelize": "3.0.0",

[daffl]

Feathers does not make a copy of your data so if you modify them (e.g. in a hook) it is a reference to the same data that you originally passed. Currently it is up to the developer to make a copy if that is not the behaviour you intend.

[daffl]

Mind you, I totally agree that hashPassword should do exactly that (which it seems it does not)!

[petermikitsh]

So the object passed in to the create API's data argument will be mutated by the hooks?

[petermikitsh]

Maybe I could write a global before create hook to augment the behavior:

function makeACopy(context) {
  context.data = Object.assign({}, ...context.data);
  return context;
}

app.hooks({
  before: {
   create: [makeACopy]
  }
});

[daffl]

That would totally work. But yes, any object you pass will stay the same. There is a similar discussion in https://github.com/feathersjs/feathers/issues/675. I think you are right and making a copy would be the expected behaviour but it feels like it might be a potentially pretty breaking change.

[petermikitsh]

I agree that it could be a fairly significant breaking change, but it'd be a worthwhile one to make. At least in the example I showed, it could be a real security issue if we aren't expecting objects to be modified containing things like hashed passwords.

I only wrote this for an integration test (to confirm local auth works as expected), so I suppose the concern might be more academic than practical. But if it's an unexpected behavior, it could result in data leakage and security issues.

[Parnswir]

The find method can mutate the params as well if you use hooks, which was quite surprising when I tried something like this:

const params = { foo: 'bar', uuid: uuid() };
const existingDocuments = await service.find({query: params});
if (existingDocuments.length === 0) {
	// [...]
	await service.create(params); // Mongoose schema complains about stuff mutated by find-hooks
}

[daffl]

I'm going to close this since in current Feathers all built-ins (hooks, dispatchers, database adapters etc.) have been made sure to no longer mutate any existing objects. It is still possible to mutate them yourself but we also tried to make sure to only show non-mutable code examples in the new documentation.