Potential security holes with merge params

[theGC]

I've been working with this database adapter and the functionality is amazing. That said I became a little concerned that the old adage "with great power comes great responsibility" is perhaps being overlooked by the merge params, expressly: mergeAllowInsert and mergeAllowUpsert.

When testing an app which has users connected to other users via a RelationMapping, say friends I found as expected I was able to use eager loading, graph inserts and upserts. Where I became concerned was with the potential opacity of what's happening under the hood. As we are utilising objections model to insert or update these users I worry that developers may not realise that the hooks they place within Feathers to create, patch or update users will be bypassed.

Whilst I appreciate we have controls to limit the actions an external request can take, namely, via upsertGraphOptions, the requirement to effectively shut down options didn't immediately spring to mind when working with services where I hadn't specifically enabled GraphUpserts or GraphInserts, i.e.

app.use('/users', service({
    model: User,
    allowedEager: 'friends',
})

The above service looks on the face of it perfectly safe, I haven't enabled GraphUpserts or GraphInserts so these are both null. I therefore wouldn't be inclined to set any properties for insertGraphOptions or upsertGraphOptions as they aren't needed. However, the ability to utilise merge params on the external requests throws all my assumptions out the window. If the end-user realises that friends is a RelationMapping they can override my service options and do something like the following:

app.service('users').patch(my.id, {
    friends: [{
        name: 'bad user',
        role: 'admin',
        email: '[email protected]',
        password: 'secret',
    }],
}, {
    provider: 'rest',
    authentication: {
        strategy: 'jwt',
        accessToken: my.accessToken,
    },
    mergeAllowUpsert: 'friends',
})

This means requests like the above can be used to generate new users or modify one's self, potentially elevating privileges or just updating things that they shouldn't. The issue here is that I can put Feathers hooks in place that prevent malicious requests on create / patch / update and reject them. However, because upsertGraph and insertGraph are dealing directly with an Objection model we skip those hooks and I'd assume most developers wouldn't think through all the scenario's like above that they need to protect against.

I feel that the benefits the merge params provide are somewhat dwarfed by the security holes they might unwittingly open up. Personally, I'd either remove them or guard against the above by turning them off by default. Then by asking developers to enable them, they hopefully also think through what they need to guard against.

I put a CodeSandbox together to show what I'm concerned about:

https://codesandbox.io/s/feather-objection-security-testing-lhw46?file=/package.json

Once it's loaded you can hit the little + icon to the right side of the terminal tab (below the down arrow):

Then enter npm run mocha to run the tests:

and you should get the output below:

I've tried to put together as simple a project as I could. The service has before hooks for create, patch and update that strips any role passed to it. Furhermore, the service doesn't allow graph insert or upserts by default.

These settings and hooks are then shown to be null and void by the last 3 tests which manipulate external requests via the merge params. Allowing us to elevate a users role to admin.

Whilst I appreciate I could do a lot more to prevent this within the Feathers hooks my concern is the opacity of how it's done means developers might never think to.

[theGC]

here's the GitHub repo for the demo I made: https://github.com/theGC/feathers-objection-merge-request-testing

[dekelev]

@theGC, Thanks for the detailed report and demo.

params in Feathers service calls are considered safe, because the client-side cannot pass them as is in service calls. Each param would need to be approved explicitly in the server-side using something like the paramsFromClient hook.

I personally do not use the insert/upsert features in my projects, but there's a similar behavior with allowedEager & mergeAllowEager that I use and it is safe enough in my opinion.

Maybe a new rule can be added to enforce that allowedEager is not set to null or undefined when mergeAllowEager is used, so that a developer would require to at least set service.options.allowedEager to an empty string/object if he wants the service to support the mergeAllowEager params operator.

An equivalent enforcement can be set on the mergeAllowInsert & mergeAllowUpsert params. What do you think?