From 5f3ae1c249883c85deb0fca293dc93b737c9037f Mon Sep 17 00:00:00 2001 From: Guilherme Gazzo Date: Sat, 19 Oct 2024 11:20:17 -0300 Subject: [PATCH 1/2] regression: server randomly failing on startup (#33675) --- apps/meteor/ee/app/license/server/index.ts | 1 - apps/meteor/ee/app/license/server/startup.ts | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/meteor/ee/app/license/server/index.ts b/apps/meteor/ee/app/license/server/index.ts index efef260a6cb0d..9177532a9e213 100644 --- a/apps/meteor/ee/app/license/server/index.ts +++ b/apps/meteor/ee/app/license/server/index.ts @@ -1,3 +1,2 @@ import './settings'; import './methods'; -import './airGappedRestrictions'; diff --git a/apps/meteor/ee/app/license/server/startup.ts b/apps/meteor/ee/app/license/server/startup.ts index 14f88bc27d613..eb80fcaf771d1 100644 --- a/apps/meteor/ee/app/license/server/startup.ts +++ b/apps/meteor/ee/app/license/server/startup.ts @@ -115,6 +115,7 @@ export const startLicense = async () => { return new Promise((resolve) => { // When settings are loaded, apply the current license if there is one. settings.onReady(async () => { + import('./airGappedRestrictions'); if (!(await applyLicense(settings.get('Enterprise_License') ?? '', false))) { // License from the envvar is always treated as new, because it would have been saved on the setting if it was already in use. if (process.env.ROCKETCHAT_LICENSE && !License.hasValidLicense()) { From 6acbadc96c22b6d87fb5f6e89429f85ccc3a9345 Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Sat, 19 Oct 2024 12:25:32 -0300 Subject: [PATCH 2/2] chore: remove allowed routes for query and fields (#33622) Co-authored-by: Marcos Spessatto Defendi <15324204+MarcosSpessatto@users.noreply.github.com> --- .../app/api/server/helpers/parseJsonQuery.ts | 16 +------- apps/meteor/app/api/server/v1/users.ts | 4 +- apps/meteor/tests/end-to-end/api/rooms.ts | 17 -------- apps/meteor/tests/end-to-end/api/users.ts | 40 +++---------------- .../src/v1/users/UsersInfoParamsGet.ts | 10 +++++ 5 files changed, 18 insertions(+), 69 deletions(-) diff --git a/apps/meteor/app/api/server/helpers/parseJsonQuery.ts b/apps/meteor/app/api/server/helpers/parseJsonQuery.ts index 35fe6155a3963..807f72080e4bb 100644 --- a/apps/meteor/app/api/server/helpers/parseJsonQuery.ts +++ b/apps/meteor/app/api/server/helpers/parseJsonQuery.ts @@ -53,24 +53,12 @@ export async function parseJsonQuery(api: PartialThis): Promise<{ } } - // TODO: Remove this once we have all routes migrated to the new API params - const hasSupportedRoutes = [ - '/api/v1/directory', - '/api/v1/channels.files', - '/api/v1/integrations.list', - '/api/v1/custom-user-status.list', - '/api/v1/custom-sounds.list', - '/api/v1/channels.list', - '/api/v1/channels.online', - '/api/v1/emoji-custom.list', - ].includes(route); - const isUnsafeQueryParamsAllowed = process.env.ALLOW_UNSAFE_QUERY_AND_FIELDS_API_PARAMS?.toUpperCase() === 'TRUE'; const messageGenerator = ({ endpoint, version, parameter }: { endpoint: string; version: string; parameter: string }): string => `The usage of the "${parameter}" parameter in endpoint "${endpoint}" breaks the security of the API and can lead to data exposure. It has been deprecated and will be removed in the version ${version}.`; let fields: Record | undefined; - if (params.fields && (isUnsafeQueryParamsAllowed || !hasSupportedRoutes)) { + if (params.fields && isUnsafeQueryParamsAllowed) { try { apiDeprecationLogger.parameter(route, 'fields', '8.0.0', response, messageGenerator); fields = JSON.parse(params.fields) as Record; @@ -120,7 +108,7 @@ export async function parseJsonQuery(api: PartialThis): Promise<{ } let query: Record = {}; - if (params.query && (isUnsafeQueryParamsAllowed || !hasSupportedRoutes)) { + if (params.query && isUnsafeQueryParamsAllowed) { apiDeprecationLogger.parameter(route, 'query', '8.0.0', response, messageGenerator); try { query = ejson.parse(params.query); diff --git a/apps/meteor/app/api/server/v1/users.ts b/apps/meteor/app/api/server/v1/users.ts index 172a55024ccea..b7187ec8cd81d 100644 --- a/apps/meteor/app/api/server/v1/users.ts +++ b/apps/meteor/app/api/server/v1/users.ts @@ -409,8 +409,6 @@ API.v1.addRoute( { authRequired: true, validateParams: isUsersInfoParamsGetProps }, { async get() { - const { fields } = await this.parseJsonQuery(); - const searchTerms: [string, 'id' | 'username' | 'importId'] | false = ('userId' in this.queryParams && !!this.queryParams.userId && [this.queryParams.userId, 'id']) || ('username' in this.queryParams && !!this.queryParams.username && [this.queryParams.username, 'username']) || @@ -426,7 +424,7 @@ API.v1.addRoute( return API.v1.failure('User not found.'); } const myself = user._id === this.userId; - if (fields.userRooms === 1 && (myself || (await hasPermissionAsync(this.userId, 'view-other-user-channels')))) { + if (this.queryParams.includeUserRooms === 'true' && (myself || (await hasPermissionAsync(this.userId, 'view-other-user-channels')))) { return API.v1.success({ user: { ...user, diff --git a/apps/meteor/tests/end-to-end/api/rooms.ts b/apps/meteor/tests/end-to-end/api/rooms.ts index 0a1fb1a082868..712e1917441dc 100644 --- a/apps/meteor/tests/end-to-end/api/rooms.ts +++ b/apps/meteor/tests/end-to-end/api/rooms.ts @@ -1290,23 +1290,6 @@ describe('[Rooms]', () => { }) .end(done); }); - it('should return name and _id of public channel when it has the "fields" query parameter limiting by name', (done) => { - void request - .get(api('rooms.info')) - .set(credentials) - .query({ - roomId: testChannel._id, - fields: JSON.stringify({ name: 1 }), - }) - .expect(200) - .expect((res) => { - expect(res.body).to.have.property('success', true); - expect(res.body).to.have.property('room').and.to.be.an('object'); - expect(res.body.room).to.have.property('name').and.to.be.equal(testChannelName); - expect(res.body.room).to.have.all.keys(['_id', 'name']); - }) - .end(done); - }); it('should not return parent & team for room thats not on a team nor is a discussion', async () => { await request diff --git a/apps/meteor/tests/end-to-end/api/users.ts b/apps/meteor/tests/end-to-end/api/users.ts index 2752e1168073d..87a27ecedfb4c 100644 --- a/apps/meteor/tests/end-to-end/api/users.ts +++ b/apps/meteor/tests/end-to-end/api/users.ts @@ -722,7 +722,7 @@ describe('[Users]', () => { .set(credentials) .query({ userId: targetUser._id, - fields: JSON.stringify({ userRooms: 1 }), + includeUserRooms: true, }) .expect('Content-Type', 'application/json') .expect(200) @@ -750,6 +750,7 @@ describe('[Users]', () => { }) .end(done); }); + it('should return the rooms when the user request your own rooms but he does NOT have the necessary permission', (done) => { void updatePermission('view-other-user-channels', []).then(() => { void request @@ -757,7 +758,7 @@ describe('[Users]', () => { .set(credentials) .query({ userId: credentials['X-User-Id'], - fields: JSON.stringify({ userRooms: 1 }), + includeUserRooms: true, }) .expect('Content-Type', 'application/json') .expect(200) @@ -1100,37 +1101,6 @@ describe('[Users]', () => { .end(done); }); - it('should query all users in the system by custom fields', (done) => { - const query = { - fields: JSON.stringify({ - username: 1, - _id: 1, - customFields: 1, - }), - query: JSON.stringify({ - 'customFields.customFieldText': 'success', - }), - }; - - void request - .get(api('users.list')) - .query(query) - .set(credentials) - .expect('Content-Type', 'application/json') - .expect(200) - .expect((res) => { - expect(res.body).to.have.property('success', true); - expect(res.body).to.have.property('count'); - expect(res.body).to.have.property('total'); - expect(res.body).to.have.property('users'); - const queriedUser = (res.body.users as IUser[]).find((u) => u._id === user._id); - assert.isDefined(queriedUser); - expect(queriedUser).to.have.property('customFields'); - expect(queriedUser.customFields).to.have.property('customFieldText', 'success'); - }) - .end(done); - }); - it('should sort for user statuses and check if deactivated user is correctly sorted', (done) => { const query = { fields: JSON.stringify({ @@ -1204,7 +1174,7 @@ describe('[Users]', () => { await request.get(api('users.list')).set(user2Credentials).expect('Content-Type', 'application/json').expect(403); }); - it('should exclude inviteToken in the user item for privileged users even when fields={inviteToken:1} is specified', async () => { + it('should exclude inviteToken in the user item for privileged users', async () => { await request .post(api('useInviteToken')) .set(user2Credentials) @@ -1236,7 +1206,7 @@ describe('[Users]', () => { }); }); - it('should exclude inviteToken in the user item for normal users even when fields={inviteToken:1} is specified', async () => { + it('should exclude inviteToken in the user item for normal users', async () => { await updateSetting('API_Apply_permission_view-outside-room_on_users-list', false); await request .post(api('useInviteToken')) diff --git a/packages/rest-typings/src/v1/users/UsersInfoParamsGet.ts b/packages/rest-typings/src/v1/users/UsersInfoParamsGet.ts index 1f07bed18475f..ad1199881517a 100644 --- a/packages/rest-typings/src/v1/users/UsersInfoParamsGet.ts +++ b/packages/rest-typings/src/v1/users/UsersInfoParamsGet.ts @@ -6,6 +6,7 @@ const ajv = new Ajv({ export type UsersInfoParamsGet = ({ userId: string } | { username: string } | { importId: string }) & { fields?: string; + includeUserRooms?: string; }; const UsersInfoParamsGetSchema = { @@ -16,6 +17,9 @@ const UsersInfoParamsGetSchema = { userId: { type: 'string', }, + includeUserRooms: { + type: 'string', + }, fields: { type: 'string', nullable: true, @@ -30,6 +34,9 @@ const UsersInfoParamsGetSchema = { username: { type: 'string', }, + includeUserRooms: { + type: 'string', + }, fields: { type: 'string', nullable: true, @@ -44,6 +51,9 @@ const UsersInfoParamsGetSchema = { importId: { type: 'string', }, + includeUserRooms: { + type: 'string', + }, fields: { type: 'string', nullable: true,