Database adapter security and usability improvements

daffl commented 6 years ago

Feathers database adapters cover a wide range of standard CRUD functionality. Although the standard behaviour is documented, their currently very open nature can make it easy to miss adding certain restrictions in order to secure your application properly. To improve this, I am planning to implement the following breaking changes:

Single items and `params.query`

get, remove, update and patch will respect the query object additionally to the id. This will make it much easier to limit access for users by setting the query with limitations (e.g. an owner userId) much like it is already done for normal find calls.

// Will throw 404 if `userId` is not the same for the entry with `accountid`
app.service('accounts').get('accountid', {
  query: { userId: 'authenticated user id' }
});

Multiple updates

create with arrays and multiple updates (patch, update and remove with id null) will be disabled by default. They can be enabled explicitly by setting the multi options to true (or the list or methods to allow). It is then up to the developer to make sure that those calls are secured properly and all affected hooks are also taking result arrays into consideration.

const memory = require('feathers-memory');

app.use('/messages', memory({
  multi: true,
  paginate: {
    default: 2,
    max: 4
  }
}));

app.use('/messages', memory({
  // Only allow multiple `create` and `patch`
  multi: [ 'create', 'patch' ],
  paginate: {
    default: 2,
    max: 4
  }
}));

Non Feathers query syntax

All adapters support Feathers common query syntax. However, some adapters like feathers-nedb, feathers-mongodb and feathers-mongoose allow passing other queries as well. The next version of all database adapters will no longer allow any non-Feathers queries that are directly passed to the database. Instead, additional database specific query properties that should be allowed have to be explicitly whitelisted and secured appropriately:

const mongoose = require('feathers-mongoose');

app.use('/messages', mongoose({
  // Allowing $populate in Mongoose can expose protected properties!
  // They now have to be removed manually in this service
  whitelist: [ '$populate' ],
  paginate: {
    default: 2,
    max: 4
  }
}));

Improved `feathers-service-tests`

How to write your own database adapter is a question that comes up quite often. Although the most straightforward answer is to create a custom service, there is also some infrastructure in feathers-service-tests in place that makes sure that the official adapters all support the common syntax properly. The tests will now be split into stages to make it easier to implement other adapters that work the same way step-by-step:

Stage 1: Basic get, create, patch, update and remove functionality and find by properties
Stage 2: Pagination, $limit, $skip, $sort and $select, multi update, patch and remove
Stage 3: Query syntax properties ($lt, $in etc.)

feathers-rethinkdb

Because of the still uncertain future of RethinkDB, low download numbers and some notable challenges working with RethinkDB, I am not planning on including feathers-rethinkdb in these updates. It will continue to work as is an and can be added to your project manually but it will not be included in future versions of the generator.

sjones6 commented 6 years ago

@daffl Fully support the direction of these breaking changes, especially the points under Single items and params.query. This will help make application security a bit more straightforward.

daffl commented 6 years ago

For the Non-Feathers query syntax, @vonagam has already done great work and submitted pretty much everything we need in https://github.com/feathersjs/commons/pull/73

sean-nicholas commented 6 years ago

Love this proposal. It addresses exactly the painpoints I head when I was starting with feathers.

Multi updates & Non Feathers query syntax

Do you plan that these features can be set by hooks too or only at service creation? The former one would be really nice. For example: Admins or import / export software should be able to to use mutli but regular users shouldn't.

Same with queries:

Internal calls to services should be able to use special operators. I have a globalSearch service that calls other services with $regex (mongoose)
Based on the users role, etc. some operators might be allowed.

Channels

Another thing that comes to my mind: What about channels? All events are sent to all clients by default. I get that you want to provide a neat first-use experience and therefore the system should work right out of the box. And there is a warning in the console about this behavior, too. But do you think that this is sufficient or should the developer need to uncomment something in channels.js to get this functionality so that it is more obvious whats happening?

EDIT:

Data

Is there anything planned to remove special operators from the data the user sends to the service? For example mongodb supports $set syntax. Might be a special case for mongo that should be addresses in the specific database adapter. Just curious if you did think about it :)

daffl commented 6 years ago

Multi updates & Non Feathers query syntax

Once enabled this should already possible with a hook right?

context => {
  if(Array.isArray(context.data) && !isAdmin(context.params.user)) {
   throw new Error('Multi update not allowed');
  }
}

Channels

This is no longer the case in Feathers Buzzard. By default a generated application is set up to publish to all authenticated users but that is explicitly defined in the generated channels.js file.

Data

Hm, I haven't really thought about that. Should it remove those parameters? Are there any that could be a potential security issue?

sean-nicholas commented 6 years ago

Multi updates & Non Feathers query syntax

Yeah, that works. Thought about like setting a special param or something to allow multi updates. But your way is way more clean by not using some magic params :smile:

Channels

I did not express myself correctly. I meant the new behavior. Just replace "All events are sent to all clients by default." with "All events are sent to all authenticated clients by default." in my comment. Which is a bit more secure but if your app is open for registration it quickly loses this advantage.

I just want to discuss if it might be better to comment this line out: return app.channel('authenticated'); and leave it to the developer to add it in. One has to balance between "easy setup & realtime just works after generation" & "emphasize what part of the app might be a security risk". I'm not in favor of either solution, just want to discuss it.

I can say from my own experience that I ignored how channels work at first. It just worked out and I was satisfied. Only later did I look at the details and changed the default implementation. My laziness may have come from being accustomed to firebase, not having to worry about event handling.

Data

Well, it depends on how you do your validation. For Example: You are stripping the role attribute from your users input, so that he could not make himself an admin. Here is how a malicious user could gain admin access:

// userSchema
interface User {
  displayName: string // Can be changed by user
  role: 'admin' | 'user' // Stripped in before hook if it is an user
  // ...
}

// Set role
patch({ something: 'foo', $set: { role: 'admin' } })

// Another Way
patch({ displayName: 'admin' })
patch({ $rename: { 'displayName': 'admin' } })

But if you use something like JSON Schema with additionalProperties: false that might not be a problem. I'm currently not aware if update operators can only be at the top level or if they might be nestable.

daffl commented 5 years ago

Released @feathersjs/adapter-commons. To be updated:

[x] feathers-memory as v3.0.0
[x] feathers-localstorage as v3.0.0
[x] feathers-nedb as v4.0.0
[x] feathers-mongodb as v4.0.0
[x] feathers-mongoose as v7.0.0
[x] feathers-knex as v5.0.0
[x] feathers-sequelize as v4.0.0
[ ] feathers-elasticsearch
[ ] feathers-objection
[ ] feathers-cassandra
[ ] feathers-arangodb

Migration instructions can be found at https://crow.docs.feathersjs.com/migrating.html#database-adapters

dekelev commented 5 years ago

@daffl I started to work on integrating these changes into feathers-objection & feathers-cassandra. I'm using the migration guide and your feathers-sequelize PR as a reference.

daffl commented 5 years ago

@dekelev Awesome! Let me know if you need anything. The basic steps are

Extend from the AdapterService class in @feathersjs/adapter-commons. It provides the actual service methods (which check the multi option for you) and a filterQuery(params) which does the filtering with all settings
Implement the hook-less service methods (_get, _find, _create, _patch, _update and _remove)

You can use the new test suite via

const adaptertests = require('@feathersjs/adapter-commons/tests');
const testSuite = adaptertests([
]);

This will not run any adapter tests (yet) and just print a list of test that have been skipped. Then you can selectively work on the tests you want to run (instead of when all tests ran at once). For example

const testSuite = adaptertests([
  '.get',
  '.get + $select'
]);

Will run the test for get and get with $select. Each test also does a single create and remove before and after. Another thing I found helpful was to change your own tests to async/await and make sure that they clean up all the data they create.

dekelev commented 5 years ago

@daffl Thanks! feathers-objection is ready, fully tested and released as v2.0.0. I've followed the Sequelize PR and your suggestions. It runs the same testSuite list as in feathers-sequelize.

I'll start adding support in feathers-cassandra soon, though it will be tricky to run tests there using the testSuite from @feathersjs/adapter-commons.

daffl commented 5 years ago

It wasn't using the previous test suite either right? Part of the idea for the new one was to not having to enable all tests but still having partial adapter functionality covered. What are the limitations for Cassandra?

dekelev commented 5 years ago

It is using a modified version of the previous test suite. The limitations that I remember is that pagination & skip does not exists in Cassandra. There's also no $or, $ne or $nin and some queries requires the allowFiltering param or using the PK fields in some other way to make it work. There's no auto-increment PK and sorting is very limited. Some of the querying limitations were solved by using SASI indexes.

I'll check if I can use the new test suite there and will let you know what I found, but for now I'll use what I got.

Here are the operators I used as default in feathers-cassandra until now. I guess that it should also work with the defaults for security reasons, from the kind that can drastically affects the DB server performance if the client used $allowFiltering on column without SASI index, using $if to add transaction like operation when it should be forbidden or the $noSelect to effect the results the server might expects. Relation load are not an issue since there are no model relations in feathers-cassandra.

const OPERATORS = {
  eq: '$eq',
  ne: '$ne', // applicable for IF conditions only
  isnt: '$isnt', // applicable for materialized view filters only
  gte: '$gte',
  gt: '$gt',
  lte: '$lte',
  lt: '$lt',
  in: '$in',
  notIn: '$nin', // not supported
  like: '$like', // applicable for SASI indexes only
  notLike: '$notLike', // not supported
  ilike: '$ilike', // not supported
  notILike: '$notILike', // not supported
  or: '$or', // not supported
  and: '$and',
  token: '$token', // applicable for token queries only
  keys: '$keys', // applicable for token queries only
  condition: '$condition', // applicable for token queries only
  contains: '$contains', // applicable for indexed collections only
  containsKey: '$containsKey', // applicable for indexed maps only
  if: '$if',
  ifExists: '$ifExists',
  ifNotExists: '$ifNotExists',
  allowFiltering: '$allowFiltering',
  limitPerPartition: '$limitPerPartition',
  minTimeuuid: '$minTimeuuid',
  maxTimeuuid: '$maxTimeuuid',
  filters: '$filters', // filter results using pre-defined model's filters,
  noSelect: '$noSelect', // skips SELECT queries in create, update, patch & remove requests. response data will be based on the input data
  timestamp: '$timestamp', // sets a timestamp for data in a column to expire. use in create, update & patch requests
  ttl: '$ttl' // sets a time in seconds for data in a column to expire. use in create, update & patch requests
}

dekelev commented 5 years ago

feathers-cassandra is ready, currently tested with the modified previous test suite and released as v2.0.0.

@daffl adapter-tests is now integrated in feathers-cassandra.

feathersjs / feathers