Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: remove allowed routes for query and fields #33622

Merged
merged 11 commits into from
Oct 19, 2024
Merged

chore: remove allowed routes for query and fields #33622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
be37e18
chore: remove allowed routes for query and fields
ricardogarim Oct 17, 2024
6cee742
Merge branch 'develop' into chore/remove-allowed-routes-for-deprecate…
ricardogarim Oct 17, 2024
3193c55
Merge branch 'develop' into chore/remove-allowed-routes-for-deprecate…
ggazzo Oct 18, 2024
15e96d0
Merge from chore/channels-online-query-param
ricardogarim Oct 18, 2024
c5c305e
Merge branch 'develop' into chore/remove-allowed-routes-for-deprecate…
ricardogarim Oct 19, 2024
fd5922b
fix: remove tests that rely solely on fields to make assertion
MarcosSpessatto Oct 19, 2024
090ca7d
fix: stop relying directly on fields
MarcosSpessatto Oct 19, 2024
51b7eba
fix: remove test relying exclusively on query
MarcosSpessatto Oct 19, 2024
6d877de
Merge branch 'develop' into chore/remove-allowed-routes-for-deprecate…
MarcosSpessatto Oct 19, 2024
b0cb26e
fix: type
MarcosSpessatto Oct 19, 2024
544058c
Merge branch 'develop' into chore/remove-allowed-routes-for-deprecate…
kodiakhq[bot] Oct 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 2 additions & 14 deletions apps/meteor/app/api/server/helpers/parseJsonQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, 0 | 1> | 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<string, 0 | 1>;
Expand Down Expand Up @@ -120,7 +108,7 @@ export async function parseJsonQuery(api: PartialThis): Promise<{
}

let query: Record<string, any> = {};
if (params.query && (isUnsafeQueryParamsAllowed || !hasSupportedRoutes)) {
if (params.query && isUnsafeQueryParamsAllowed) {
apiDeprecationLogger.parameter(route, 'query', '8.0.0', response, messageGenerator);
try {
query = ejson.parse(params.query);
Expand Down
4 changes: 1 addition & 3 deletions apps/meteor/app/api/server/v1/users.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']) ||
Expand All @@ -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,
Expand Down
17 changes: 0 additions & 17 deletions apps/meteor/tests/end-to-end/api/rooms.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
40 changes: 5 additions & 35 deletions apps/meteor/tests/end-to-end/api/users.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -750,14 +750,15 @@ 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
.get(api('users.info'))
.set(credentials)
.query({
userId: credentials['X-User-Id'],
fields: JSON.stringify({ userRooms: 1 }),
includeUserRooms: true,
})
.expect('Content-Type', 'application/json')
.expect(200)
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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'))
Expand Down
10 changes: 10 additions & 0 deletions packages/rest-typings/src/v1/users/UsersInfoParamsGet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const ajv = new Ajv({

export type UsersInfoParamsGet = ({ userId: string } | { username: string } | { importId: string }) & {
fields?: string;
includeUserRooms?: string;
};

const UsersInfoParamsGetSchema = {
Expand All @@ -16,6 +17,9 @@ const UsersInfoParamsGetSchema = {
userId: {
type: 'string',
},
includeUserRooms: {
type: 'string',
},
fields: {
type: 'string',
nullable: true,
Expand All @@ -30,6 +34,9 @@ const UsersInfoParamsGetSchema = {
username: {
type: 'string',
},
includeUserRooms: {
type: 'string',
},
fields: {
type: 'string',
nullable: true,
Expand All @@ -44,6 +51,9 @@ const UsersInfoParamsGetSchema = {
importId: {
type: 'string',
},
includeUserRooms: {
type: 'string',
},
fields: {
type: 'string',
nullable: true,
Expand Down
Loading