Skip to content

Commit

Permalink
馃敀 Security update (#2678) (#2681)
Browse files Browse the repository at this point in the history 
* Fix https://huntr.dev/bounties/bfd935f4-2d1d-4d3f-8b59-522abe7dd065/

* Fix access control over posting messages to channels / threads

* Fix typo

* Fix some tests

* Fix one of the tests

* Fix test

* Fix another test

* Still fixing the search one

* Fix 2 tests cases

* Fixed some stuff

* Fixed some stuff

* Finished fixing tests
  • Loading branch information
@RomaricMourgues
RomaricMourgues committed Jan 4, 2023
1 parent fd3de79 commit 295f5ca
Show file tree
Hide file tree
Showing 13 changed files with 162 additions and 126 deletions.
32 changes: 31 additions & 1 deletion twake/backend/node/src/services/messages/web/controllers/messages.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
ThreadExecutionContext,
} from "../../types";
import { handleError } from "../../../../utils/handleError";
import { Pagination } from "../../../../core/platform/framework/api/crud-service";
import { CrudException, Pagination } from "../../../../core/platform/framework/api/crud-service";
import { getThreadMessageWebsocketRoom } from "../realtime";
import { ThreadPrimaryKey } from "../../entities/threads";
import { extendExecutionContentWithChannel } from "./index";
Expand Down Expand Up @@ -74,6 +74,36 @@ export class MessagesController
throw "Message must be in a thread";
}

let hasOneMembership = false;
for (const participant of thread.participants) {
if (thread.created_by === context.user.id) {
hasOneMembership = true;
break;
}
if (participant.type === "channel") {
const isMember = await gr.services.channels.members.getChannelMember(
{ id: context.user.id },
{
company_id: participant.company_id,
workspace_id: participant.workspace_id,
id: participant.id,
},
);
if (isMember) {
hasOneMembership = true;
break;
}
} else if (participant.type === "user") {
if (participant.id === context.user.id) {
hasOneMembership = true;
break;
}
}
}
if (!hasOneMembership) {
throw CrudException.notFound("You can't post in this thread");
}

const result = await gr.services.messages.messages.save(
{
id: request.params.message_id || undefined,
Expand Down
22 changes: 22 additions & 0 deletions twake/backend/node/src/services/messages/web/controllers/threads.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { handleError } from "../../../../utils/handleError";
import { CompanyExecutionContext } from "../../types";
import { ParticipantObject, Thread } from "../../entities/threads";
import gr from "../../../global-resolver";
import { CrudException } from "../../../../core/platform/framework/api/crud-service";

export class ThreadsController
implements
Expand Down Expand Up @@ -39,6 +40,27 @@ export class ThreadsController
reply: FastifyReply,
): Promise<ResourceCreateResponse<Thread>> {
const context = getCompanyExecutionContext(request);

const participants =
(request.body.resource.participants?.length
? request.body.resource?.participants
: request.body.options?.participants?.add) || [];
for (const participant of participants) {
if (participant.type === "channel") {
const isMember = await gr.services.channels.members.getChannelMember(
{ id: context.user.id },
{
company_id: participant.company_id,
workspace_id: participant.workspace_id,
id: participant.id,
},
);
if (!isMember) {
throw CrudException.notFound("Channel not found");
}
}
}

try {
const result = await gr.services.messages.threads.save(
{
Expand Down
18 changes: 17 additions & 1 deletion twake/backend/node/src/services/messages/web/controllers/views.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ import { FastifyReply, FastifyRequest } from "fastify";
import { ResourceListResponse } from "../../../../utils/types";
import { Message } from "../../entities/messages";
import { handleError } from "../../../../utils/handleError";
import { ListResult, Pagination } from "../../../../core/platform/framework/api/crud-service";
import {
CrudException,
ListResult,
Pagination,
} from "../../../../core/platform/framework/api/crud-service";
import {
ChannelViewExecutionContext,
FlatFileFromMessage,
Expand Down Expand Up @@ -39,6 +43,18 @@ export class ViewsController {
const query = { ...request.query, include_users: request.query.include_users };
const context = getChannelViewExecutionContext(request);

const isMember = await gr.services.channels.members.getChannelMember(
{ id: context.user.id },
{
company_id: request.params.company_id,
workspace_id: request.params.workspace_id,
id: request.params.channel_id,
},
);
if (!isMember) {
throw CrudException.notFound("Channel not found");
}

let resources: ListResult<MessageWithReplies | FlatFileFromMessage | FlatPinnedFromMessage>;

try {
Expand Down
105 changes: 11 additions & 94 deletions twake/backend/node/test/e2e/messages/messages.messages.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,13 @@ import {
} from "../../../src/utils/types";
import { deserialize } from "class-transformer";
import { ParticipantObject, Thread } from "../../../src/services/messages/entities/threads";
import { createMessage, e2e_createMessage, e2e_createThread } from "./utils";
import {
createMessage,
createParticipant,
e2e_createChannel,
e2e_createMessage,
e2e_createThread,
} from "./utils";
import { Message } from "../../../src/services/messages/entities/messages";
import { v1 as uuidv1 } from "uuid";
import { MessageWithReplies } from "../../../src/services/messages/types";
Expand Down Expand Up @@ -98,98 +104,10 @@ describe("The Messages feature", () => {
expect(listResponse.statusCode).toBe(200);
expect(listResult.resources.length).toBe(3);
});

it("should move the message in threads", async () => {
const userA = uuidv1();
const userB = uuidv1();
const userC = uuidv1();

const thread1Request = await e2e_createThread(
platform,
[],
createMessage({ text: "Message A in thread 1", user_id: userA }),
);
const thread1Result: ResourceUpdateResponse<Thread> = deserialize(
ResourceUpdateResponse,
thread1Request.body,
);
const thread1: Thread = thread1Result.resource;

const thread2Request = await e2e_createThread(
platform,
[],
createMessage({ text: "Message B in thread 2", user_id: userB }),
);
const thread2Result: ResourceUpdateResponse<Thread> = deserialize(
ResourceUpdateResponse,
thread2Request.body,
);
const thread2: Thread = thread2Result.resource;

const messageCRequest = await e2e_createMessage(
platform,
thread1.id,
createMessage({ text: "Message C in thread 1", user_id: userC }),
);
const messageCResult: ResourceUpdateResponse<Message> = deserialize(
ResourceUpdateResponse,
messageCRequest.body,
);
const messageC: Message = messageCResult.resource;

const messageCAfterMoveRequest = await platform.app.inject({
method: "POST",
url: `${url}/companies/${platform.workspace.company_id}/threads/${thread2.id}/messages/${messageC.id}`,
headers: {
authorization: `Bearer ${await platform.auth.getJWTToken({ sub: userA })}`,
},
payload: {
resource: messageC,
options: {
previous_thread: thread1.id,
},
},
});
const messageCAfterMoveResult: ResourceUpdateResponse<Message> = deserialize(
ResourceUpdateResponse,
messageCAfterMoveRequest.body,
);
const messageCAfterMove: Message = messageCAfterMoveResult.resource;

//See if message was moved correctly to the new thread
expect(messageCAfterMove.user_id).toBe(userC);
expect(messageCAfterMove.thread_id).toBe(thread2.id);

const messageCAfterMove2Request = await platform.app.inject({
method: "POST",
url: `${url}/companies/${platform.workspace.company_id}/threads/${messageC.id}/messages/${messageC.id}`,
headers: {
authorization: `Bearer ${await platform.auth.getJWTToken({ sub: userA })}`,
},
payload: {
resource: messageC,
options: {
previous_thread: thread2.id,
},
},
});
const messageCAfterMove2Result: ResourceUpdateResponse<Message> = deserialize(
ResourceUpdateResponse,
messageCAfterMove2Request.body,
);
const messageCAfter2Move: Message = messageCAfterMove2Result.resource;

//See if message was moved correctly to new standalone thread
expect(messageCAfter2Move.user_id).toBe(userC);
expect(messageCAfter2Move.thread_id).toBe(messageCAfter2Move.id);

//TODO check counts
});
});

describe("Inbox", () => {
it("Should get recent user messages", async done => {
const channel = channelUtils.getChannel();
const directChannelIn = channelUtils.getDirectChannel();
const members = [platform.currentUser.id, uuidv1()];
const directWorkspace: Workspace = {
Expand All @@ -198,7 +116,6 @@ describe("The Messages feature", () => {
};

await Promise.all([
gr.services.channels.channels.save(channel, {}, getContext()),
gr.services.channels.channels.save<ChannelSaveOptions>(
directChannelIn,
{
Expand All @@ -209,12 +126,12 @@ describe("The Messages feature", () => {
]);

const recipient: ParticipantObject = {
company_id: platform.workspace.company_id,
created_at: 0,
created_by: "",
id: channel.id,
type: "channel",
company_id: directChannelIn.company_id,
workspace_id: "direct",
id: directChannelIn.id,
};

for (let i = 0; i < 6; i++) {
Expand Down Expand Up @@ -270,8 +187,8 @@ describe("The Messages feature", () => {
application_id: null,
text: expect.any(String),
cache: {
company_id: channel.company_id,
channel_id: channel.id,
company_id: directChannelIn.company_id,
channel_id: directChannelIn.id,
},
});
}
Expand Down
29 changes: 17 additions & 12 deletions twake/backend/node/test/e2e/messages/messages.search.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { afterAll, beforeAll, describe, expect, it } from "@jest/globals";
import { init, TestPlatform } from "../setup";
import { TestDbService } from "../utils.prepare.db";
import { v1 as uuidv1 } from "uuid";
import { createMessage, e2e_createMessage, e2e_createThread } from "./utils";
import { createMessage, e2e_createChannel, e2e_createMessage, e2e_createThread } from "./utils";
import { ResourceUpdateResponse } from "../../../src/utils/types";
import { ParticipantObject, Thread } from "../../../src/services/messages/entities/threads";
import { deserialize } from "class-transformer";
Expand Down Expand Up @@ -121,22 +121,22 @@ describe("The /messages API", () => {
done();
});
it("Filter out messages from channels we are not member of", async done => {
const channel = await createChannel();
const anotherChannel = await createChannel(uuidv1());
const anotherUserId = uuidv1();
const channel = await e2e_createChannel(platform, [platform.currentUser.id, anotherUserId]);
const anotherChannel = await e2e_createChannel(platform, [anotherUserId], anotherUserId); //Test user is not the owner

const participant = {
type: "channel",
id: channel.id,
company_id: platform.workspace.company_id,
workspace_id: platform.workspace.workspace_id,
id: channel.resource.id,
company_id: channel.resource.company_id,
workspace_id: channel.resource.workspace_id,
} as ParticipantObject;

const participant2 = {
type: "channel",
id: anotherChannel.id,
company_id: platform.workspace.company_id,
workspace_id: platform.workspace.workspace_id,
id: anotherChannel.resource.id,
company_id: anotherChannel.resource.company_id,
workspace_id: anotherChannel.resource.workspace_id,
} as ParticipantObject;

const file = new MessageFile();
Expand All @@ -148,7 +148,7 @@ describe("The /messages API", () => {
await createReply(firstThreadId, "Filtered message 1-3");
await createReply(firstThreadId, "Filtered message 1-4", { files: [file] });

const secondThreadId = await createThread("Filtered thread 2", [participant2]);
const secondThreadId = await createThread("Filtered thread 2", [participant2], anotherUserId);
await createReply(secondThreadId, "Filtered message 2-1");
await createReply(secondThreadId, "Filtered message 2-2");
await createReply(secondThreadId, "Filtered message 2-3");
Expand Down Expand Up @@ -204,8 +204,13 @@ describe("The /messages API", () => {
return creationResult.entity;
}

async function createThread(text, participants: ParticipantObject[]) {
const response = await e2e_createThread(platform, participants, createMessage({ text: text }));
async function createThread(text, participants: ParticipantObject[], owner?: string) {
const response = await e2e_createThread(
platform,
participants,
createMessage({ text: text, user_id: owner }),
owner,
);

const result: ResourceUpdateResponse<Thread> = deserialize(
ResourceUpdateResponse,
Expand Down
24 changes: 18 additions & 6 deletions twake/backend/node/test/e2e/messages/messages.views.channel.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,13 @@ import { ResourceListResponse, ResourceUpdateResponse } from "../../../src/utils
import { deserialize } from "class-transformer";
import { v4 as uuidv4 } from "uuid";
import { Thread } from "../../../src/services/messages/entities/threads";
import { createMessage, createParticipant, e2e_createMessage, e2e_createThread } from "./utils";
import {
createMessage,
createParticipant,
e2e_createChannel,
e2e_createMessage,
e2e_createThread,
} from "./utils";
import { MessageWithReplies } from "../../../src/services/messages/types";

describe("The Messages feature", () => {
Expand Down Expand Up @@ -42,15 +48,17 @@ describe("The Messages feature", () => {

describe("On user use messages in channel view", () => {
it("should create a message and retrieve it in channel view", async () => {
const channelId = uuidv4();
const channel = await e2e_createChannel(platform, [platform.currentUser.id]);

const response = await e2e_createThread(
platform,
[
createParticipant(
{
type: "channel",
id: channelId,
id: channel.resource.id,
workspace_id: channel.resource.workspace_id,
company_id: channel.resource.company_id,
},
platform,
),
Expand All @@ -73,7 +81,9 @@ describe("The Messages feature", () => {
createParticipant(
{
type: "channel",
id: channelId,
id: channel.resource.id,
workspace_id: channel.resource.workspace_id,
company_id: channel.resource.company_id,
},
platform,
),
Expand All @@ -89,7 +99,9 @@ describe("The Messages feature", () => {
createParticipant(
{
type: "channel",
id: channelId,
id: channel.resource.id,
workspace_id: channel.resource.workspace_id,
company_id: channel.resource.company_id,
},
platform,
),
Expand All @@ -100,7 +112,7 @@ describe("The Messages feature", () => {
const jwtToken = await platform.auth.getJWTToken();
const listResponse = await platform.app.inject({
method: "GET",
url: `${url}/companies/${platform.workspace.company_id}/workspaces/${platform.workspace.workspace_id}/channels/${channelId}/feed?replies_per_thread=3&include_users=1`,
url: `${url}/companies/${channel.resource.company_id}/workspaces/${channel.resource.workspace_id}/channels/${channel.resource.id}/feed?replies_per_thread=3&include_users=1`,
headers: {
authorization: `Bearer ${jwtToken}`,
},
Expand Down

1 comment on commit 295f5ca

@Christynorl
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @RomaricMourgues.I found this vulnerability still exist.The video link of the specific operation is below, I want to know the specific reason, thank you very much.
video link

Please sign in to comment.