Skip to content

Commit d433a1d

Browse files
ricardogarimricardogarim
committed
fix: detect quote permalinks by query param instead of hostname
1 parent daef4a1 commit d433a1d

4 files changed

Lines changed: 79 additions & 113 deletions

File tree

‎.changeset/shy-pillows-repair.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@rocket.chat/meteor': patch
3+
---
4+
5+
Fixes quote attachments not rendering when client hostname differs from Site_Url

‎apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts‎

Lines changed: 19 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,15 @@
1-
import QueryString from 'querystring';
21
import URL from 'url';
32

4-
import type { MessageAttachment, IMessage, IUser, IOmnichannelRoom, IRoom } from '@rocket.chat/core-typings';
3+
import type { MessageAttachment, MessageUrl, IMessage, IUser, IOmnichannelRoom, IRoom } from '@rocket.chat/core-typings';
54
import { isOmnichannelRoom, isQuoteAttachment } from '@rocket.chat/core-typings';
65

76
import { createQuoteAttachment } from '../../../../lib/createQuoteAttachment';
87

8+
const getMessageIdFromUrl = (url: string): string | undefined => {
9+
const { query } = URL.parse(url, true);
10+
return typeof query?.msg === 'string' ? query.msg : undefined;
11+
};
12+
913
const recursiveRemoveAttachments = (attachments: MessageAttachment, deep = 1, quoteChainLimit: number): MessageAttachment => {
1014
if (attachments && isQuoteAttachment(attachments)) {
1115
if (deep < quoteChainLimit - 1) {
@@ -80,7 +84,6 @@ export class BeforeSaveJumpToMessage {
8084
user: Pick<IUser, '_id' | 'username' | 'name' | 'language'>;
8185
config: {
8286
chainLimit: number;
83-
siteUrl: string;
8487
useRealName: boolean;
8588
};
8689
}): Promise<IMessage> {
@@ -92,27 +95,15 @@ export class BeforeSaveJumpToMessage {
9295
return message;
9396
}
9497

95-
const linkedMessages = message.urls
96-
.filter((item) => item.url.includes(config.siteUrl))
97-
.map((item) => {
98-
const urlObj = URL.parse(item.url);
99-
100-
// if the URL doesn't have query params (doesn't reference message) skip
101-
if (!urlObj.query) {
102-
return;
103-
}
104-
105-
const { msg: msgId } = QueryString.parse(urlObj.query);
106-
107-
if (typeof msgId !== 'string') {
108-
return;
109-
}
110-
111-
return { msgId, url: item.url };
112-
})
113-
.filter(Boolean);
98+
const linkedMessages: { msgId: string; urlItem: MessageUrl }[] = [];
99+
for (const urlItem of message.urls) {
100+
const msgId = getMessageIdFromUrl(urlItem.url);
101+
if (msgId) {
102+
linkedMessages.push({ msgId, urlItem });
103+
}
104+
}
114105

115-
const msgs = await this.getMessages(linkedMessages.map((linkedMsg) => linkedMsg?.msgId) as string[]);
106+
const msgs = await this.getMessages(linkedMessages.map((linkedMsg) => linkedMsg.msgId));
116107

117108
const validMessages = msgs.filter((msg) => validateAttachmentDeepness(msg, config.chainLimit));
118109

@@ -138,17 +129,8 @@ export class BeforeSaveJumpToMessage {
138129

139130
const quotes = [];
140131

141-
for (const item of message.urls) {
142-
if (!item.url.includes(config.siteUrl)) {
143-
continue;
144-
}
145-
146-
const linkedMessage = linkedMessages.find((msg) => msg?.url === item.url);
147-
if (!linkedMessage) {
148-
continue;
149-
}
150-
151-
const messageFromUrl = validMessages.find((msg) => msg._id === linkedMessage.msgId);
132+
for (const { msgId, urlItem } of linkedMessages) {
133+
const messageFromUrl = validMessages.find((msg) => msg._id === msgId);
152134
if (!messageFromUrl) {
153135
continue;
154136
}
@@ -157,9 +139,10 @@ export class BeforeSaveJumpToMessage {
157139
continue;
158140
}
159141

160-
item.ignoreParse = true;
142+
// prevent OEmbed from also fetching this URL for a link preview
143+
urlItem.ignoreParse = true;
161144

162-
quotes.push(createQuoteAttachment(messageFromUrl, item.url, useRealName, this.getUserAvatarURL(messageFromUrl.u.username)));
145+
quotes.push(createQuoteAttachment(messageFromUrl, urlItem.url, useRealName, this.getUserAvatarURL(messageFromUrl.u.username)));
163146
}
164147

165148
if (quotes.length > 0) {

‎apps/meteor/server/services/messages/service.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,6 @@ export class MessageService extends ServiceClassInternal implements IMessageServ
254254
user,
255255
config: {
256256
chainLimit: settings.get<number>('Message_QuoteChainLimit'),
257-
siteUrl: settings.get<string>('Site_Url'),
258257
useRealName: settings.get<boolean>('UI_Use_Real_Name'),
259258
},
260259
});

‎apps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts‎

Lines changed: 55 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -61,17 +61,13 @@ describe('Create attachments for message URLs', () => {
6161
const message = await jumpToMessage.createAttachmentForMessageURLs({
6262
message: createMessage('hey'),
6363
user: createUser(),
64-
config: {
65-
chainLimit: 10,
66-
siteUrl: 'https://open.rocket.chat',
67-
useRealName: true,
68-
},
64+
config: { chainLimit: 10, useRealName: true },
6965
});
7066

7167
return expect(message).to.not.have.property('urls');
7268
});
7369

74-
it('should do nothing if URL is not from SiteUrl', async () => {
70+
it('should do nothing if URL does not have a msg query parameter', async () => {
7571
const jumpToMessage = new BeforeSaveJumpToMessage({
7672
getMessages: async () => [createMessage('linked message', { _id: 'linked' })],
7773
getRooms: async () => [createRoom()],
@@ -89,18 +85,14 @@ describe('Create attachments for message URLs', () => {
8985
],
9086
}),
9187
user: createUser(),
92-
config: {
93-
chainLimit: 10,
94-
siteUrl: 'https://open.rocket.chat',
95-
useRealName: true,
96-
},
88+
config: { chainLimit: 10, useRealName: true },
9789
});
9890

9991
expect(message).to.have.property('urls').of.length(1);
10092
expect(message).to.not.have.property('attachments');
10193
});
10294

103-
it('should do nothing if URL is from SiteUrl but not have a query string', async () => {
95+
it('should do nothing if URL has a non-msg query parameter', async () => {
10496
const jumpToMessage = new BeforeSaveJumpToMessage({
10597
getMessages: async () => [createMessage('linked message', { _id: 'linked' })],
10698
getRooms: async () => [createRoom()],
@@ -112,24 +104,20 @@ describe('Create attachments for message URLs', () => {
112104
message: createMessage('hey', {
113105
urls: [
114106
{
115-
url: 'https://open.rocket.chat',
107+
url: 'https://open.rocket.chat/?token=value',
116108
meta: {},
117109
},
118110
],
119111
}),
120112
user: createUser(),
121-
config: {
122-
chainLimit: 10,
123-
siteUrl: 'https://open.rocket.chat',
124-
useRealName: true,
125-
},
113+
config: { chainLimit: 10, useRealName: true },
126114
});
127115

128116
expect(message).to.have.property('urls').of.length(1);
129117
expect(message).to.not.have.property('attachments');
130118
});
131119

132-
it('should do nothing if URL is from SiteUrl but not have a msgId query string', async () => {
120+
it('should reject query parameter pollution (msg[$gt]=)', async () => {
133121
const jumpToMessage = new BeforeSaveJumpToMessage({
134122
getMessages: async () => [createMessage('linked message', { _id: 'linked' })],
135123
getRooms: async () => [createRoom()],
@@ -141,24 +129,55 @@ describe('Create attachments for message URLs', () => {
141129
message: createMessage('hey', {
142130
urls: [
143131
{
144-
url: 'https://open.rocket.chat/?token=value',
132+
url: 'https://open.rocket.chat/?msg[$gt]=',
145133
meta: {},
146134
},
147135
],
148136
}),
149137
user: createUser(),
150-
config: {
151-
chainLimit: 10,
152-
siteUrl: 'https://open.rocket.chat',
153-
useRealName: true,
154-
},
138+
config: { chainLimit: 10, useRealName: true },
155139
});
156140

157141
expect(message).to.have.property('urls').of.length(1);
158142
expect(message).to.not.have.property('attachments');
159143
});
160144

161-
it('should do nothing if it do not find a msg from the URL', async () => {
145+
it('should create quote attachment even when client hostname differs from server (e.g. localhost vs 127.0.0.1)', async () => {
146+
const jumpToMessage = new BeforeSaveJumpToMessage({
147+
getMessages: async () => [createMessage('linked message', { _id: 'linked' })],
148+
getRooms: async () => [createRoom()],
149+
canAccessRoom: async () => true,
150+
getUserAvatarURL: () => 'url',
151+
});
152+
153+
const message = await jumpToMessage.createAttachmentForMessageURLs({
154+
message: createMessage('hey', {
155+
urls: [
156+
{
157+
url: 'http://localhost:3000/channel/general?msg=linked',
158+
meta: {},
159+
},
160+
],
161+
}),
162+
user: createUser(),
163+
config: { chainLimit: 10, useRealName: true },
164+
});
165+
166+
expect(message).to.have.property('urls').and.to.have.lengthOf(1);
167+
168+
const [url] = message.urls ?? [];
169+
expect(url).to.include({
170+
url: 'http://localhost:3000/channel/general?msg=linked',
171+
ignoreParse: true,
172+
});
173+
174+
expect(message).to.have.property('attachments').and.to.have.lengthOf(1);
175+
176+
const [attachment] = message.attachments ?? [];
177+
expect(attachment).to.have.property('text', 'linked message');
178+
});
179+
180+
it('should do nothing if it does not find a message from the URL', async () => {
162181
const jumpToMessage = new BeforeSaveJumpToMessage({
163182
getMessages: async () => [],
164183
getRooms: async () => [createRoom()],
@@ -176,11 +195,7 @@ describe('Create attachments for message URLs', () => {
176195
],
177196
}),
178197
user: createUser(),
179-
config: {
180-
chainLimit: 10,
181-
siteUrl: 'https://open.rocket.chat',
182-
useRealName: true,
183-
},
198+
config: { chainLimit: 10, useRealName: true },
184199
});
185200

186201
expect(message).to.have.property('urls').of.length(1);
@@ -205,18 +220,14 @@ describe('Create attachments for message URLs', () => {
205220
],
206221
}),
207222
user: createUser(),
208-
config: {
209-
chainLimit: 10,
210-
siteUrl: 'https://open.rocket.chat',
211-
useRealName: true,
212-
},
223+
config: { chainLimit: 10, useRealName: true },
213224
});
214225

215226
expect(message).to.have.property('urls').of.length(1);
216227
expect(message).to.not.have.property('attachments');
217228
});
218229

219-
it('should do nothing if user dont have access to the room of the message from the URL', async () => {
230+
it('should do nothing if user does not have access to the room of the message from the URL', async () => {
220231
const jumpToMessage = new BeforeSaveJumpToMessage({
221232
getMessages: async () => [createMessage('linked message', { _id: 'linked' })],
222233
getRooms: async () => [createRoom()],
@@ -234,11 +245,7 @@ describe('Create attachments for message URLs', () => {
234245
],
235246
}),
236247
user: createUser(),
237-
config: {
238-
chainLimit: 10,
239-
siteUrl: 'https://open.rocket.chat',
240-
useRealName: true,
241-
},
248+
config: { chainLimit: 10, useRealName: true },
242249
});
243250

244251
expect(message).to.have.property('urls').of.length(1);
@@ -272,11 +279,7 @@ describe('Create attachments for message URLs', () => {
272279
],
273280
}),
274281
user: createUser(),
275-
config: {
276-
chainLimit: 10,
277-
siteUrl: 'https://open.rocket.chat',
278-
useRealName: true,
279-
},
282+
config: { chainLimit: 10, useRealName: true },
280283
});
281284

282285
expect(message).to.have.property('urls').and.to.have.lengthOf(1);
@@ -317,11 +320,7 @@ describe('Create attachments for message URLs', () => {
317320
],
318321
}),
319322
user: createUser(),
320-
config: {
321-
chainLimit: 10,
322-
siteUrl: 'https://open.rocket.chat',
323-
useRealName: true,
324-
},
323+
config: { chainLimit: 10, useRealName: true },
325324
});
326325

327326
expect(message).to.have.property('attachments').and.to.have.lengthOf(0);
@@ -349,11 +348,7 @@ describe('Create attachments for message URLs', () => {
349348
],
350349
}),
351350
user: createUser(),
352-
config: {
353-
chainLimit: 10,
354-
siteUrl: 'https://open.rocket.chat',
355-
useRealName: true,
356-
},
351+
config: { chainLimit: 10, useRealName: true },
357352
});
358353

359354
expect(message).to.have.property('attachments').and.to.have.lengthOf(1);
@@ -379,11 +374,7 @@ describe('Create attachments for message URLs', () => {
379374
],
380375
}),
381376
user: createUser(),
382-
config: {
383-
chainLimit: 10,
384-
siteUrl: 'https://open.rocket.chat',
385-
useRealName: true,
386-
},
377+
config: { chainLimit: 10, useRealName: true },
387378
});
388379

389380
expect(message).to.have.property('urls').and.to.have.lengthOf(1);
@@ -453,7 +444,6 @@ describe('Create attachments for message URLs', () => {
453444
user: createUser(),
454445
config: {
455446
chainLimit: 3,
456-
siteUrl: 'https://open.rocket.chat',
457447
useRealName: true,
458448
},
459449
});
@@ -536,7 +526,6 @@ describe('Create attachments for message URLs', () => {
536526
user: createUser(),
537527
config: {
538528
chainLimit: 3,
539-
siteUrl: 'https://open.rocket.chat',
540529
useRealName: true,
541530
},
542531
});
@@ -567,11 +556,7 @@ describe('Create attachments for message URLs', () => {
567556
token: 'livechatToken',
568557
}),
569558
user: createUser(),
570-
config: {
571-
chainLimit: 10,
572-
siteUrl: 'https://open.rocket.chat',
573-
useRealName: true,
574-
},
559+
config: { chainLimit: 10, useRealName: true },
575560
});
576561

577562
expect(message).to.have.property('urls').and.to.have.lengthOf(1);
@@ -609,11 +594,7 @@ describe('Create attachments for message URLs', () => {
609594
token: 'another-token',
610595
}),
611596
user: createUser(),
612-
config: {
613-
chainLimit: 10,
614-
siteUrl: 'https://open.rocket.chat',
615-
useRealName: true,
616-
},
597+
config: { chainLimit: 10, useRealName: true },
617598
});
618599

619600
expect(message).to.have.property('urls').and.to.have.lengthOf(1);
@@ -653,7 +634,6 @@ describe('Create attachments for message URLs', () => {
653634
user: createUser(),
654635
config: {
655636
chainLimit: 1,
656-
siteUrl: 'https://open.rocket.chat',
657637
useRealName: true,
658638
},
659639
});
@@ -708,7 +688,6 @@ describe('Create attachments for message URLs', () => {
708688
user: createUser(),
709689
config: {
710690
chainLimit: 1,
711-
siteUrl: 'https://open.rocket.chat',
712691
useRealName: true,
713692
},
714693
});

0 commit comments

Comments
 (0)