restrictToOwner does not work
Closed this issue · 1 comments
Steps to reproduce
restrictToOwner as in the documentatio does not appear to be working. For find and get requests, the example works because it is substituting user supplied data owner values with the user ID used to authenticate. So if a user A tries to access a record belonging to B by substituting User A's ID for that of user B, it is set back to User A's ID before it is further processed (i.e. it is sanitised).
However, I have found that this doesn't work for editing of data. For the editing of data, it would require either (1) a round-trip to find the user ID of the record being edited or (2) it would need to be done in an after hook so that User B's ownership is available for comparison to the authenticated User (A). In the example within the documentation for restrictToOwner, the hook does nothing other than set the data owner to be the authenticated user. Thus if user A tries to illegally take ownership of a record belonging to user B by substitution, there is nothing from stopping him.
Expected behavior
Based on the way it is shown in the example, setField should prevent User A from substituting himself as owner on a record belonging to User B.
Actual behavior
What this tool is actually doing, based on inspection of the hooks, is substituting the value of the owner field in the record being edited with the authenticated user information.
So in the case of this record:
{
"owner": 0, <- User A
"createdAt": "1967-10-03T13:18:24.107Z",
"updatedAt": "1956-02-18T06:10:15.869Z",
"name": "random jhdsfksfdfgfdgdfgfdgdfgsdf",
"uuid": "cca26dc1-c714-4b2b-81e9-ffb1d88c0f21",
"permissions": ["change"],
"description": "eos quae dolores,v:WSA42];",
"author": "Carmel Maggio",
"sink": "https://dortha.net",
"secret": "xgpwNRG5mMu8NdP",
"data": {}
}
The return if this is PUT by user of ID 1 (User B) is as follows:
{
"owner": 1,
"createdAt": "1967-10-03T13:18:24.107Z",
"updatedAt": "1956-02-18T06:10:15.869Z",
"name": "random jhdsfksfdfgfdgdfgfdgdfgsdf",
"uuid": "cca26dc1-c714-4b2b-81e9-ffb1d88c0f21",
"permissions": ["change"],
"description": "eos quae dolores,v:WSA42];",
"author": "Carmel Maggio",
"sink": "https://dortha.net",
"secret": "xgpwNRG5mMu8NdP",
"data": {}
}
The tool doesn't actually appear to be doing anything at all when it comes to editing and so the use of this particular tool for restricting edits to the owner is incorrect. If less experienced users were to use this as part of their application without fully testing it as I have, this would result in a security vulnerability in their application. As such, this is a security vulnerability because the tool gives the impression it can do something which it cannot do. setField is not an appropriate hook for this type of requirement and it should be removed from the documentation for any write activities.
It is however, useful for filtering records to only those belonging to the user in question.
This is the code:
const { disallow, iff, validateSchema } = require('feathers-hooks-common');
const Ajv = require('ajv');
const ajv = new Ajv({useDefaults: true});
const checkpointSchema = require('./checkpoints.schema');
const { authenticate } = require('@feathersjs/authentication').hooks;
const { setField } = require('feathers-authentication-hooks');
const permissions = require('feathers-permissions');
const checkPermissions = permissions({
roles: ['admin'],
field: 'roles',
error: false
});
const checkOwner = iff(context => !context.params.permitted,
setField({
from: 'params.user.id',
as: 'params.query.owner'
})
);
const setUserId = setField({
from: 'params.user.id',
as: 'data.owner'
});
const limitToUser = setField({
from: 'params.user.id',
as: 'params.query.owner'
});
const recordContext = context => {
console.log(context);
};
module.exports = {
before: {
all: [],
find: [ authenticate('jwt'), checkPermissions, checkOwner ],
get: [ authenticate('jwt'), checkPermissions, checkOwner ],
create: [ authenticate('jwt'), setUserId, validateSchema(checkpointSchema, ajv) ],
update: [ authenticate('jwt'), checkPermissions, recordContext, checkOwner, validateSchema(checkpointSchema, ajv) ],
patch: [ authenticate('jwt'), checkPermissions, checkOwner ],
remove: [ authenticate('jwt'), checkPermissions, checkOwner ]
},
after: {
all: [],
find: [],
get: [],
create: [],
update: [],
patch: [],
remove: []
},
error: {
all: [],
find: [],
get: [],
create: [],
update: [],
patch: [],
remove: []
}
};
System configuration
Tell us about the applicable parts of your setup.
Module versions (especially the part that's not working):
"dependencies": {
"@feathersjs/authentication": "^4.5.1",
"@feathersjs/authentication-local": "^4.5.1",
"@feathersjs/authentication-oauth": "^4.5.1",
"@feathersjs/configuration": "^4.5.1",
"@feathersjs/errors": "^4.5.1",
"@feathersjs/express": "^4.5.1",
"@feathersjs/feathers": "^4.5.1",
"@feathersjs/socketio": "^4.5.1",
"@feathersjs/transport-commons": "^4.5.1",
"ajv": "^6.11.0",
"compression": "^1.7.4",
"cors": "^2.8.5",
"faker": "^4.1.0",
"feathers-authentication-hooks": "^1.0.1",
"feathers-hooks-common": "^5.0.2",
"feathers-memory": "^4.1.0",
"feathers-nedb": "^5.1.0",
"feathers-permissions": "^2.0.1",
"feathers-swagger": "^1.1.0",
"helmet": "^3.21.2",
"json-schema-faker": "^0.5.0-rc23",
"json-schema-ref-parser": "^7.1.3",
"json-schema-to-openapi-schema": "^0.4.0",
"nedb": "^1.8.0",
"serve-favicon": "^2.5.0",
"winston": "^3.2.1"
},
"devDependencies": {
"axios": "^0.19.0",
"eslint": "^6.8.0",
"mocha": "^7.0.1",
"nodemon": "^2.0.2"
}
}
NodeJS version:
v10.15.0
I managed to get this to work in the end. I was getting a very similar problem to another user. When the data owner is updated, it makes it appear as though the operation has failed. I believed that this meant failure and misunderstood what was actually going on. I made changes to try and get that to work which made it appear as though it was just replacing the ID, when in fact if taken out it does appear to be validating the authenticated user against the ownership property.