[REF] mail: discuss model html field and trusted insert

This commit allows to define html fields in discuss models,
so that when insert is trusted they are automatically markup.

By default, all model insert are untrusted. To make a trusted
insertion, we should pass 2nd parameter `{ html: true }` to
an `insert()` method. The param is intentionally named `html`
instead of `trusted`, so that it triggers ci/security for
reviewers awareness.

It's still possible to immediately assign a markup on the html
fields, like before.

This changes simplifies the code, so that we don't have to dissect
model data everytime and make sure all bits are properly markup.
The dissect operation added many LOCs and we frequently missed to
markup the data in some flows. Being able to flag html fields at
model definition and flagging an insert safe keeps the security
concern while helping properly markup-ing all the data.

Part-of: odoo/odoo#139501
This commit is contained in:
Alexandre Kühn
2023-10-26 13:41:16 +00:00
parent 46e3594118
commit c0da919697
28 changed files with 170 additions and 160 deletions
@@ -19,7 +19,7 @@ export class Attachment extends FileModelMixin(Record) {
if (!("id" in data)) {
throw new Error("Cannot insert attachment: id is missing in data");
}
return super.insert(data);
return super.insert(...arguments);
}
static new(data) {
/** @type {import("models").Attachment} */
@@ -12,7 +12,7 @@ export class CannedResponse extends Record {
}
/** @returns {import("models").CannedResponse} */
static insert(data) {
return super.insert(data);
return super.insert(...arguments);
}
/** @type {number} */
@@ -19,7 +19,7 @@ export class ChannelMember extends Record {
}
/** @returns {import("models").ChannelMember} */
static insert(data) {
return super.insert(data);
return super.insert(...arguments);
}
/** @type {number} */
@@ -15,11 +15,15 @@ export class ChatWindow extends Record {
static get(data) {
return super.get(data);
}
/** @returns {import("models").ChatWindow} */
static insert() {
return super.insert(...arguments);
}
/**
* @param {ChatWindowData} [data]
* @returns {import("models").ChatWindow}
*/
static insert(data = {}) {
static _insert(data = {}) {
const chatWindow = this.store.discuss.chatWindows.find((c) => c.thread?.eq(data.thread));
if (!chatWindow) {
/** @type {import("models").ChatWindow} */
@@ -14,7 +14,7 @@ export class Composer extends Record {
if (Boolean(message) === Boolean(thread)) {
throw new Error("Composer shall have a thread xor a message.");
}
return super.insert(data);
return super.insert(...arguments);
}
attachments = Record.many("Attachment");
@@ -10,7 +10,7 @@ export class DiscussAppCategory extends Record {
}
/** @returns {import("models").DiscussAppCategory} */
static insert(data) {
return super.insert(data);
return super.insert(...arguments);
}
/** @type {string} */
@@ -39,7 +39,7 @@ export class DiscussApp extends Record {
}
/** @returns {import("models").DiscussApp} */
static insert(data) {
return super.insert(data);
return super.insert(...arguments);
}
/** @type {'mailbox'|'all'|'channel'|'chat'|'livechat'} */
@@ -27,8 +27,11 @@ export class Failure extends Record {
}
/** @returns {import("models").Failure} */
static insert(data) {
return super.insert(...arguments);
}
static _insert() {
/** @type {import("models").Failure} */
const failure = super.insert(data);
const failure = super._insert(...arguments);
if (failure.notifications.length === 0) {
failure.delete();
} else {
@@ -12,7 +12,7 @@ export class Follower extends Record {
}
/** @returns {import("models").Follower} */
static insert(data) {
return super.insert(data);
return super.insert(...arguments);
}
followedThread = Record.one("Thread");
@@ -10,7 +10,7 @@ export class LinkPreview extends Record {
}
/** @returns {import("models").LinkPreview} */
static insert(data) {
return super.insert(data);
return super.insert(...arguments);
}
/** @type {number} */
@@ -2,7 +2,7 @@
import { removeFromArrayWithPredicate } from "@mail/utils/common/arrays";
import { markup, reactive } from "@odoo/owl";
import { reactive } from "@odoo/owl";
import { registry } from "@web/core/registry";
@@ -55,14 +55,16 @@ export class MailCoreCommon {
});
this.busService.subscribe("mail.message/notification_update", (payload) => {
payload.elements.map((message) => {
this.store.Message.insert({
...message,
body: markup(message.body),
// implicit: failures are sent by the server at
// initialization only if the current partner is
// author of the message
author: this.store.self,
});
this.store.Message.insert(
{
...message,
// implicit: failures are sent by the server at
// initialization only if the current partner is
// author of the message
author: this.store.self,
},
{ html: true }
);
});
});
this.busService.subscribe("mail.message/toggle_star", (payload) => {
@@ -105,10 +107,7 @@ export class MailCoreCommon {
}
const { Message: messageData } = payload;
if (messageData) {
this.store.Message.insert({
...messageData,
body: messageData.body ? markup(messageData.body) : messageData.body,
});
this.store.Message.insert(messageData, { html: true });
}
});
});
@@ -35,13 +35,16 @@ export class Message extends Record {
}
/** @returns {import("models").Message} */
static insert(data) {
return super.insert(...arguments);
}
static _insert(data) {
if (data.res_id) {
this.store.Thread.insert({
model: data.model,
id: data.res_id,
});
}
return super.insert(data);
return super._insert(...arguments);
}
/** @param {Object} data */
@@ -89,7 +92,7 @@ export class Message extends Record {
attachments = Record.many("Attachment", { inverse: "message" });
author = Record.one("Persona");
body = Record.attr("");
body = Record.attr("", { html: true });
composer = Record.one("Composer", { inverse: "message", onDelete: (r) => r.delete() });
/** @type {string} */
default_subject;
@@ -10,7 +10,7 @@ export class MessageReactions extends Record {
}
/** @returns {import("models").MessageReactions} */
static insert(data) {
return super.insert(data);
return super.insert(...arguments);
}
/** @type {string} */
@@ -3,8 +3,6 @@
import { removeFromArrayWithPredicate } from "@mail/utils/common/arrays";
import { convertBrToLineBreak, prettifyMessageContent } from "@mail/utils/common/format";
import { markup } from "@odoo/owl";
import { _t } from "@web/core/l10n/translation";
import { registry } from "@web/core/registry";
@@ -47,11 +45,7 @@ export class MessageService {
message_id: message.id,
partner_ids: validMentions?.partners?.map((partner) => partner.id),
});
this.store.Message.insert(
Object.assign(messageData, {
body: messageData.body ? markup(messageData.body) : messageData.body,
})
);
this.store.Message.insert(messageData, { html: true });
if (!message.isEmpty && this.store.hasLinkPreviewFeature) {
this.rpc(
"/mail/link_preview",
@@ -119,27 +113,6 @@ export class MessageService {
return validMentions;
}
/**
* Create a transient message, i.e. a message which does not come
* from a member of the channel. Usually a log message, such as one
* generated from a command with ('/').
*
* @param {Object} data
*/
createTransient(data) {
const { body, res_id, model } = data;
const lastMessageId = this.getLastMessageId();
return this.store.Message.insert({
author: this.store.odoobot,
body,
id: lastMessageId + 0.01,
is_note: true,
is_transient: true,
res_id,
model,
});
}
async toggleStar(message) {
await this.orm.silent.call("mail.message", "toggle_message_starred", [[message.id]]);
}
@@ -14,7 +14,7 @@ export class Notification extends Record {
}
/** @returns {import("models").Notification} */
static insert(data) {
return super.insert(data);
return super.insert(...arguments);
}
/** @type {number} */
@@ -22,7 +22,7 @@ export class Persona extends Record {
}
/** @returns {import("models").Persona} */
static insert(data) {
return super.insert(data);
return super.insert(...arguments);
}
update(data) {
+29 -6
View File
@@ -443,6 +443,8 @@ export class RecordList extends Array {
* @property {boolean} [MANY_SYM] true when this is a many relation.
* @property {boolean} [ONE_SYM] true when this is a one relation.
* @property {any} [default] the default value of this attribute.
* @property {boolean} [html] whether the attribute is an html field. Useful to automatically markup
* when the insert is trusted.
* @property {string} [targetModel] model name of records contained in this relational field.
* @property {Function} [compute] if set the field is computed based on provided function.
* The `this` of function is the record, and the function is recalled whenever any field
@@ -455,6 +457,15 @@ export class RecordList extends Array {
*/
export class Record {
/** @param {FieldDefinition} */
static isAttr(definition) {
return Boolean(definition[ATTR_SYM]);
}
/**
* Determines whether the inserts are considered trusted or not.
* Useful to auto-markup html fields when this is set
*/
static trusted = false;
static id;
/** @type {Object<string, Record>} */
static records = {};
@@ -465,7 +476,7 @@ export class Record {
* - key : field name
* - value: Value contains definition of field
*
* @type {Object.<string, {FieldDefinition}>}
* @type {Object.<string, FieldDefinition>}
*/
static _fields = {};
static isRecord(record) {
@@ -611,10 +622,10 @@ export class Record {
*/
static new(data) {
const obj = new this.Class();
obj.Model = this;
const ids = this._retrieveIdFromData(data);
let record = Object.assign(obj, {
[IS_RECORD_SYM]: true,
Model: this,
localId: this.localId(data),
...ids,
});
@@ -662,13 +673,23 @@ export class Record {
/**
* @template T
* @param {T} def;
* @param {boolean} [html] if set, the field value contains html value.
* Useful to automatically markup when the insert is trusted.
* @returns {T}
*/
static attr(def) {
return [ATTR_SYM, { default: def }];
static attr(def, { html } = {}) {
return [ATTR_SYM, { default: def, html }];
}
/** @returns {Record} */
static insert(data) {
static insert(data, { html } = {}) {
const oldTrusted = Record.trusted;
Record.trusted = html ?? Record.trusted;
const res = this._insert(...arguments);
Record.trusted = oldTrusted;
return res;
}
/** @returns {Record} */
static _insert(data) {
const res = this.preinsert(data);
res.update(data);
return res;
@@ -808,8 +829,10 @@ export class Record {
for (const [name, val] of Object.entries(this._fields)) {
if (RecordList.isMany(val)) {
data[name] = val.map((r) => r.toIdData());
} else {
} else if (RecordList.isOne(val)) {
data[name] = this[name]?.toIdData();
} else {
data[name] = this[name]; // Record.attr()
}
}
delete data._store;
@@ -2,12 +2,18 @@
import { onChange } from "@mail/utils/common/misc";
import { reactive } from "@odoo/owl";
import { markup, reactive } from "@odoo/owl";
import { registry } from "@web/core/registry";
import { debounce } from "@web/core/utils/timing";
import { modelRegistry, Record, RecordUses, RecordList } from "./record";
/**
* Class of markup, useful to detect content that is markup and to
* automatically markup field during trusted insert
*/
const Markup = markup("").constructor;
export class BaseStore extends Record {
/**
* @param {string} localId
@@ -25,7 +31,7 @@ export class BaseStore extends Record {
export class Store extends BaseStore {
/** @returns {import("models").Store} */
static insert() {
return super.insert();
return super.insert(...arguments);
}
/** @type {typeof import("@mail/core/web/activity_model").Activity} */
@@ -172,10 +178,23 @@ export function makeStore(env) {
},
/** @param {Record} receiver */
set(target, name, val, receiver) {
if (!(name in receiver._fields)) {
if (name === "Model" || !(name in receiver.Model._fields)) {
Reflect.set(target, name, val, receiver);
return true;
}
if (Record.isAttr(receiver.Model._fields[name])) {
if (
receiver.Model._fields[name].html &&
Record.trusted &&
typeof val === "string" &&
!(val instanceof Markup)
) {
Reflect.set(target, name, markup(val), receiver);
} else {
Reflect.set(target, name, val, receiver);
}
return true;
}
/** @type {RecordList<Record>} */
const l1 = receiver._fields[name];
if (RecordList.isMany(l1)) {
@@ -61,7 +61,7 @@ export class Thread extends Record {
if (!("model" in data)) {
throw new Error("Cannot insert thread: model is missing in data");
}
return super.insert(data);
return super.insert(...arguments);
}
/** @param {Object} data */
@@ -4,8 +4,6 @@ import { loadEmoji } from "@web/core/emoji_picker/emoji_picker";
import { DEFAULT_AVATAR } from "@mail/core/common/persona_service";
import { prettifyMessageContent } from "@mail/utils/common/format";
import { markup } from "@odoo/owl";
import { browser } from "@web/core/browser/browser";
import { _t } from "@web/core/l10n/translation";
import { registry } from "@web/core/registry";
@@ -193,16 +191,9 @@ export class ThreadService {
after,
before,
});
const messages = rawMessages.reverse().map((data) => {
if (data.parentMessage) {
data.parentMessage.body = data.parentMessage.body
? markup(data.parentMessage.body)
: data.parentMessage.body;
}
return this.store.Message.insert(
Object.assign(data, { body: data.body ? markup(data.body) : data.body })
);
});
const messages = rawMessages
.reverse()
.map((data) => this.store.Message.insert(data, { html: true }));
thread.isLoaded = true;
return messages;
} catch (e) {
@@ -301,15 +292,9 @@ export class ThreadService {
...this.getFetchParams(thread),
around: messageId,
});
thread.messages = messages.reverse().map((message) => {
if (message.parentMessage?.body) {
message.parentMessage.body = markup(message.parentMessage.body);
}
return this.store.Message.insert({
...message,
body: message.body ? markup(message.body) : message.body,
});
});
thread.messages = messages
.reverse()
.map((message) => this.store.Message.insert(message, { html: true }));
thread.loadNewer = messageId ? true : false;
thread.loadOlder = true;
if (messages.length < FETCH_LIMIT) {
@@ -338,14 +323,7 @@ export class ThreadService {
const previews = await this.orm.call("discuss.channel", "channel_fetch_preview", [ids]);
for (const preview of previews) {
const thread = this.store.Thread.get({ model: "discuss.channel", id: preview.id });
const data = Object.assign(preview.last_message, {
body: markup(preview.last_message.body),
});
const message = this.store.Message.insert({
...data,
res_id: thread.id,
model: thread.model,
});
const message = this.store.Message.insert(preview.last_message, { html: true });
if (!thread.isLoaded) {
thread.messages.push(message);
if (message.isNeedaction) {
@@ -718,13 +696,16 @@ export class ThreadService {
}
}
browser.localStorage.setItem("web.emoji.frequent", JSON.stringify(recentEmojis));
tmpMsg = this.store.Message.insert({
...tmpData,
body: markup(prettyContent),
res_id: thread.id,
model: thread.model,
temporary_id: tmpId,
});
tmpMsg = this.store.Message.insert(
{
...tmpData,
body: prettyContent,
res_id: thread.id,
model: thread.model,
temporary_id: tmpId,
},
{ html: true }
);
thread.messages.push(tmpMsg);
thread.seen_message_id = tmpMsg.id;
}
@@ -733,15 +714,10 @@ export class ThreadService {
if (!data) {
return;
}
if (data.parentMessage) {
data.parentMessage.body = data.parentMessage.body
? markup(data.parentMessage.body)
: data.parentMessage.body;
}
if (data.id in this.store.Message.records) {
data.temporary_id = null;
}
const message = this.store.Message.insert(Object.assign(data, { body: markup(data.body) }));
const message = this.store.Message.insert(data, { html: true });
thread.messages.add(message);
if (!message.isEmpty && this.store.hasLinkPreviewFeature) {
this.rpc("/mail/link_preview", { message_id: data.id }, { silent: true });
@@ -992,15 +968,7 @@ export class ThreadService {
return {
count,
loadMore: messages.length === FETCH_LIMIT,
messages: messages.map((message) => {
message.body = markup(message.body);
if (message.parentMessage) {
message.parentMessage.body = message.parentMessage.body
? markup(message.parentMessage.body)
: message.parentMessage.body;
}
return this.store.Message.insert(message);
}),
messages: messages.map((message) => this.store.Message.insert(message, { html: true })),
};
}
}
@@ -49,6 +49,15 @@ export class Activity extends Record {
* @returns {import("models").Activity}
*/
static insert(data, { broadcast = true } = {}) {
return super.insert(...arguments);
}
/**
* @param {Data} data
* @param {Object} [param1]
* @param {boolean} param1.broadcast
* @returns {import("models").Activity}
*/
static _insert(data, { broadcast = true } = {}) {
/** @type {import("models").Activity} */
const activity = this.preinsert(data);
if (data.request_partner_id) {
@@ -101,8 +110,7 @@ export class Activity extends Record {
id;
/** @type {Object[]} */
mail_template_ids;
/** @type {string} */
note;
note = Record.attr("", { html: true });
/** @type {number|false} */
previous_activity_type_id;
/** @type {number|false} */
@@ -2,7 +2,7 @@
import { removeFromArray, removeFromArrayWithPredicate } from "@mail/utils/common/arrays";
import { markup, reactive } from "@odoo/owl";
import { reactive } from "@odoo/owl";
import { registry } from "@web/core/registry";
@@ -24,14 +24,16 @@ export class MailCoreWeb {
this.messagingService.isReady.then(() => {
this.rpc("/mail/load_message_failures", {}, { silent: true }).then((messages) => {
messages.map((messageData) =>
this.store.Message.insert({
...messageData,
body: messageData.body ? markup(messageData.body) : messageData.body,
// implicit: failures are sent by the server at
// initialization only if the current partner is
// author of the message
author: this.store.user,
})
this.store.Message.insert(
{
...messageData,
// implicit: failures are sent by the server at
// initialization only if the current partner is
// author of the message
author: this.store.user,
},
{ html: true }
)
);
this.store.failures.sort((f1, f2) => f2.lastMessage?.id - f1.lastMessage?.id);
});
@@ -52,8 +54,7 @@ export class MailCoreWeb {
}
});
this.busService.subscribe("mail.message/inbox", (payload) => {
const data = Object.assign(payload, { body: markup(payload.body) });
const message = this.store.Message.insert(data);
const message = this.store.Message.insert(payload, { html: true });
const inbox = this.store.discuss.inbox;
if (message.notIn(inbox.messages)) {
inbox.counter++;
@@ -3,8 +3,6 @@
import { ThreadService, threadService } from "@mail/core/common/thread_service";
import { parseEmail } from "@mail/js/utils";
import { markup } from "@odoo/owl";
import { _t } from "@web/core/l10n/translation";
import { patch } from "@web/core/utils/patch";
@@ -51,10 +49,7 @@ patch(ThreadService.prototype, {
if ("activities" in result) {
const existingIds = new Set();
for (const activity of result.activities) {
if (activity.note) {
activity.note = markup(activity.note);
}
existingIds.add(this.store.Activity.insert(activity).id);
existingIds.add(this.store.Activity.insert(activity, { html: true }).id);
}
for (const activity of thread.activities) {
if (!existingIds.has(activity.id)) {
@@ -12,8 +12,11 @@ export class RtcSession extends Record {
}
/** @returns {number, import("models").RtcSession} */
static insert(data) {
return super.insert(...arguments);
}
static _insert() {
/** @type {import("models").RtcSession} */
const session = super.insert(data);
const session = super._insert(...arguments);
session.channel?.rtcSessions.add(session);
return session;
}
@@ -1,6 +1,6 @@
/* @odoo-module */
import { markup, reactive } from "@odoo/owl";
import { reactive } from "@odoo/owl";
import { _t } from "@web/core/l10n/translation";
import { registry } from "@web/core/registry";
@@ -84,8 +84,19 @@ export class DiscussCoreCommon {
model: "discuss.channel",
id: payload.res_id,
});
const message = this.messageService.createTransient(
Object.assign(payload, { body: markup(payload.body) })
const { body, res_id, model } = payload;
const lastMessageId = this.messageService.getLastMessageId();
const message = this.store.Message.insert(
{
author: this.store.odoobot,
body,
id: lastMessageId + 0.01,
is_note: true,
is_transient: true,
res_id,
model,
},
{ html: true }
);
channel.messages.push(message);
channel.transientMessages.push(message);
@@ -204,17 +215,7 @@ export class DiscussCoreCommon {
}
this.store.Message.get(messageData.temporary_id)?.delete();
messageData.temporary_id = null;
if ("parentMessage" in messageData && messageData.parentMessage.body) {
messageData.parentMessage.body = markup(messageData.parentMessage.body);
}
const data = Object.assign(messageData, {
body: markup(messageData.body),
});
const message = this.store.Message.insert({
...data,
res_id: channel.id,
model: channel.model,
});
const message = this.store.Message.insert(messageData, { html: true });
if (message.notIn(channel.messages)) {
if (!channel.loadNewer) {
channel.messages.push(message);
@@ -6,7 +6,7 @@ import { Message as MessageModel } from "@mail/core/common/message_model";
import { Record } from "@mail/core/common/record";
import { Thread } from "@mail/core/common/thread_model";
import { markup, reactive } from "@odoo/owl";
import { reactive } from "@odoo/owl";
import { _t } from "@web/core/l10n/translation";
import { registry } from "@web/core/registry";
@@ -80,13 +80,9 @@ export class MessagePin {
const messagesData = await this.rpcService("/discuss/channel/pinned_messages", {
channel_id: channel.id,
});
messagesData.forEach((messageData) => {
if (messageData.parentMessage) {
messageData.parentMessage.body = markup(messageData.parentMessage.body);
}
messageData.body = markup(messageData.body);
this.store.Message.insert(messageData);
});
messagesData.forEach((messageData) =>
this.store.Message.insert(messageData, { html: true })
);
channel.pinnedMessagesState = "loaded";
} catch (e) {
channel.pinnedMessagesState = "error";
@@ -5,6 +5,7 @@ import { BaseStore, makeStore } from "@mail/core/common/store_service";
import { registry } from "@web/core/registry";
import { clearRegistryWithCleanup, makeTestEnv } from "@web/../tests/helpers/mock_env";
import { markup } from "@odoo/owl";
const serviceRegistry = registry.category("services");
@@ -138,3 +139,16 @@ QUnit.test("Computed relational field", async (assert) => {
thread.members.delete(john);
assert.ok(thread.admin.eq(marc));
});
QUnit.test("Trusted insert on html field with { html: true }", async (assert) => {
(class Message extends Record {
static id = "body";
body = Record.attr("", { html: true });
}).register();
const store = await start();
const hello = store.Message.insert("<p>hello</p>", { html: true });
const world = store.Message.insert("<p>world</p>");
assert.ok(hello.body instanceof markup("").constructor);
assert.strictEqual(hello.body.toString(), "<p>hello</p>");
assert.strictEqual(world.body, "<p>world</p>");
});
@@ -12,7 +12,7 @@ QUnit.test("rating value displayed on the preview", async (assert) => {
const partnerId = pyEnv["res.partner"].create({});
const channelId = pyEnv["discuss.channel"].create({});
const messageId = pyEnv["mail.message"].create([
{ author_id: partnerId, model: "discuss.channel", res_id: channelId },
{ author_id: partnerId, body: "non-empty", model: "discuss.channel", res_id: channelId },
]);
pyEnv["rating.rating"].create({
consumed: true,