Skip to content

Commit 0526ffd

Browse files
marshallswainmarshallswain
authored
fix(mongodb): Block $rename operator in _patch data by default (CWE-943) (#3665)
1 parent fe1f2bc commit 0526ffd

3 files changed

Lines changed: 102 additions & 1 deletion

File tree

‎docs/api/databases/mongodb.md‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ MongoDB adapter specific options are:
6363
- `Model {Promise<MongoDBCollection>}` (**required**) - A Promise that resolves with the MongoDB collection instance. This can also be the return value of an `async` function without `await`
6464
- `disableObjectify {boolean}` (_optional_, default `false`) - This will disable conversion of the id field to a MongoDB ObjectID if you want to e.g. use normal strings
6565
- `useEstimatedDocumentCount {boolean}` (_optional_, default `false`) - If `true` document counting will rely on `estimatedDocumentCount` instead of `countDocuments`
66+
- `disabledOperators {string[]}` (_optional_, default `['$rename']`) - A list of [MongoDB update operators](https://www.mongodb.com/docs/manual/reference/operator/update/) to block in `patch` data. See [Securing update operators](#securing-update-operators) for details.
6667

6768
The [common API options](./common.md#options) are:
6869

@@ -164,6 +165,52 @@ Note that creating indexes for an existing collection with many entries should b
164165

165166
Additionally to the [common querying mechanism](./querying.md) this adapter also supports [MongoDB's query syntax](https://www.mongodb.com/docs/manual/tutorial/query-documents/) and the `update` method also supports MongoDB [update operators](https://www.mongodb.com/docs/manual/reference/operator/update/).
166167

168+
## Securing update operators
169+
170+
The `patch` method supports MongoDB [update operators](https://www.mongodb.com/docs/manual/reference/operator/update/) like `$push`, `$inc`, and `$unset` in the data payload. While this is powerful, it can be a security risk if patch data from the client is not properly validated. For example, an authenticated user who can patch their own profile could send:
171+
172+
```ts
173+
// Escalate privileges by pushing to a roles array
174+
await app.service('users').patch(userId, { $push: { roles: 'admin' } })
175+
176+
// Expose internal fields by renaming them
177+
await app.service('users').patch(userId, { $rename: { secretField: 'public' } })
178+
```
179+
180+
### Schema validation
181+
182+
The primary defense is to use [schema validation](../schema/validators.md) on your patch data. When your schema only allows known fields with known types, unexpected operators will be rejected before they reach the database.
183+
184+
### The `disabledOperators` option
185+
186+
As an additional layer of defense, the `disabledOperators` option blocks specific update operators from being passed through to MongoDB. By default, `$rename` is blocked.
187+
188+
To block additional operators on a service:
189+
190+
```ts
191+
new MongoDBService({
192+
Model: app.get('mongodbClient').then((db) => db.collection('users')),
193+
disabledOperators: ['$rename', '$unset', '$inc']
194+
})
195+
```
196+
197+
To override per-call via `params.adapter`:
198+
199+
```ts
200+
service.patch(id, data, {
201+
adapter: { disabledOperators: ['$rename', '$unset'] }
202+
})
203+
```
204+
205+
To allow all operators (not recommended without schema validation):
206+
207+
```ts
208+
new MongoDBService({
209+
Model: app.get('mongodbClient').then((db) => db.collection('messages')),
210+
disabledOperators: []
211+
})
212+
```
213+
167214
## Search
168215

169216
<BlockQuote type="warning" label="Important">

‎packages/mongodb/src/adapter.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,12 @@ export interface MongoDBAdapterOptions extends AdapterServiceOptions {
2929
Model: Collection | Promise<Collection>
3030
disableObjectify?: boolean
3131
useEstimatedDocumentCount?: boolean
32+
/**
33+
* A list of MongoDB update operators to block in `patch` data.
34+
* Defaults to `['$rename']`. Any `$`-prefixed key in this list will be
35+
* silently dropped from the update.
36+
*/
37+
disabledOperators?: string[]
3238
}
3339

3440
export interface MongoDBAdapterParams<Q = AdapterQuery> extends AdapterParams<
@@ -404,6 +410,7 @@ export class MongoDbAdapter<
404410
query,
405411
filters: { $sort, $select }
406412
} = this.filterQuery(id, params)
413+
const disabledOperators = this.getOptions(params).disabledOperators || ['$rename']
407414

408415
const replacement = Object.keys(data).reduce(
409416
(current, key) => {
@@ -416,7 +423,7 @@ export class MongoDbAdapter<
416423
...current.$set,
417424
...value
418425
}
419-
} else {
426+
} else if (!disabledOperators.includes(key)) {
420427
current[key] = value
421428
}
422429

‎packages/mongodb/test/index.test.ts‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -839,6 +839,53 @@ describe('Feathers MongoDB Service', () => {
839839
})
840840
})
841841

842+
describe('disabledOperators in _patch', () => {
843+
it('drops $rename by default', async () => {
844+
const person = await app.service('people').create({ name: 'Secure', age: 30 })
845+
846+
const result = await app.service('people').patch(person._id, {
847+
name: 'Updated',
848+
$rename: { age: 'exposed' }
849+
} as any)
850+
851+
assert.strictEqual(result.name, 'Updated')
852+
assert.strictEqual(result.age, 30)
853+
854+
await app.service('people').remove(person._id)
855+
})
856+
857+
it('allows $push and other operators not in the denylist', async () => {
858+
const person = await app.service('people').create({ name: 'PushTest', age: 20 })
859+
860+
const result = await app.service('people').patch(person._id, {
861+
$push: { friends: 'Alice' }
862+
} as any)
863+
864+
assert.strictEqual(result.friends?.length, 1)
865+
assert.strictEqual(result.friends[0], 'Alice')
866+
867+
await app.service('people').remove(person._id)
868+
})
869+
870+
it('drops operators added to disabledOperators', async () => {
871+
const person = await app.service('people').create({ name: 'IncTest', age: 25 })
872+
873+
const result = await app.service('people').patch(
874+
person._id,
875+
{
876+
$inc: { age: 100 }
877+
} as any,
878+
{
879+
adapter: { disabledOperators: ['$rename', '$inc'] }
880+
}
881+
)
882+
883+
assert.strictEqual(result.age, 25)
884+
885+
await app.service('people').remove(person._id)
886+
})
887+
})
888+
842889
describe('NoSQL injection via object id', () => {
843890
let target: Person
844891

0 commit comments

Comments
 (0)