Skip to content

Commit

Permalink
PATCH: Allow filtering by customFields in user queries
Browse files Browse the repository at this point in the history
SAPI’s `find_rc_user_by_sapi_user_id` calls RC’s `users.list` API endpoint with

    `/api/v1/users.list?query={"customFields.sa_id": #{sapi_user_id}}`

This call fails under vanilla RC 4.8.4 with

    Error: Invalid attribute: customFields.sa_id [error-invalid-query]
    at Object.get (app/api/server/v1/users.js:330:11)

This problem is related to the Upstream issue RocketChat#25722

    "API Invalid query parameter provided"

and is caused by Upstream PR RocketChat#25648

    "Chore: Rest API query parameters handling"
    commit 31ae30f

which limited which MongoDB query filters may be passed in via the RC API.
That PR was correct to restrict the query, but did not whitelist enough fields.
SAPI’s current integration with RocketChat depends on `customFields` being exposed.

Security Considerations:
Although none of SAPI’s customFields contain sensitive data, other RC installations
might indeed store sensitive data there.  It’s not clear, therefore, whether this
patch should be PR’d upstream.

=====

UPDATE:
Apparently Upstream wasn't didn't share my security concern.  They accepted
a PR that was identical to one of the two changes that were here, namely
    apps/meteor/app/api/server/v1/users.ts
    ```
           inclusiveFieldsKeys.includes('name') && 'name.*',
           inclusiveFieldsKeys.includes('type') && 'type.*',
    +      inclusiveFieldsKeys.includes('customFields') && 'customFields.*',
         ].filter(Boolean) as string[],
    ```
See https://github.com/RocketChat/Rocket.Chat/pull/27423/files

My patch also has a second change, adding customFields to
    apps/meteor/app/api/server/lib/users.ts#getNonEmptyFields()
Since it wasn't included in the upstream PR, maybe it isn't actually needed?
I'm keeping it for now, but we should experiement and see if we can safely drop it.
  • Loading branch information
nmagedman committed Dec 4, 2024
1 parent 62bca16 commit 13fefdf
Showing 1 changed file with 1 addition and 0 deletions.
1 change: 1 addition & 0 deletions apps/meteor/app/api/server/lib/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ export function getNonEmptyFields(fields: Record<string, 1 | 0>): Record<string,
avatarETag: 1,
lastLogin: 1,
type: 1,
customFields: 1,
} as const;

if (!fields || Object.keys(fields).length === 0) {
Expand Down

0 comments on commit 13fefdf

Please sign in to comment.