From 2b581ab7dc1a1f91d3e8d59b25e15f5d5f2437be Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Sat, 1 Jul 2023 14:31:21 -0400 Subject: [PATCH] (core) Fix issue with lodash's map interpreting objects with length as array-like Summary: Here's a series of badness that easily leads to a crash, in reverse order: - Lodash's map() function interprets an object with a .length property as an array. - Some very old code generated human-friendly descriptions of user actions, applying map() to parts of them. It so happens that this generated description isn't even used. - If a user action is encountered with a sufficiently large length propery, map() would exhaust the server memory. Fixed by removing old unneeded code, and replacing some other occurrences of lodash's map() with native equivalents. Test Plan: Tested manually on a local reproduction of the issue. Reviewers: paulfitz Reviewed By: paulfitz Subscribers: paulfitz Differential Revision: https://phab.getgrist.com/D3938 --- app/client/components/ActionLog.ts | 11 ++-- app/client/components/BaseView.js | 16 +----- app/client/lib/tableUtil.ts | 8 +-- app/common/DocActions.ts | 17 ------- app/common/ThemePrefs-ti.ts | 2 + app/server/lib/ActionHistory.ts | 81 ++---------------------------- 6 files changed, 16 insertions(+), 119 deletions(-) diff --git a/app/client/components/ActionLog.ts b/app/client/components/ActionLog.ts index 46553a53..5dd5fd02 100644 --- a/app/client/components/ActionLog.ts +++ b/app/client/components/ActionLog.ts @@ -6,7 +6,6 @@ import * as dispose from 'app/client/lib/dispose'; import dom from 'app/client/lib/dom'; import {timeFormat} from 'app/common/timeFormat'; import * as ko from 'knockout'; -import map = require('lodash/map'); import koArray from 'app/client/lib/koArray'; import {KoArray} from 'app/client/lib/koArray'; @@ -17,8 +16,8 @@ import {GristDoc} from 'app/client/components/GristDoc'; import {ActionGroup} from 'app/common/ActionGroup'; import {ActionSummary, asTabularDiffs, defunctTableName, getAffectedTables, LabelDelta} from 'app/common/ActionSummary'; -import {CellDelta} from 'app/common/TabularDiff'; -import {IDomComponent} from 'grainjs'; +import {CellDelta, TabularDiff} from 'app/common/TabularDiff'; +import {DomContents, IDomComponent} from 'grainjs'; import {makeT} from 'app/client/lib/localization'; /** @@ -141,12 +140,12 @@ export class ActionLog extends dispose.Disposable implements IDomComponent { * @param {string} txt - a textual description of the action * @param {ActionGroupWithState} ag - the full action information we have */ - public renderTabularDiffs(sum: ActionSummary, txt: string, ag?: ActionGroupWithState) { + public renderTabularDiffs(sum: ActionSummary, txt: string, ag?: ActionGroupWithState): HTMLElement { const act = asTabularDiffs(sum); const editDom = dom('div', this._renderTableSchemaChanges(sum, ag), this._renderColumnSchemaChanges(sum, ag), - map(act, (tdiff, table) => { + Object.entries(act).map(([table, tdiff]: [string, TabularDiff]) => { if (tdiff.cells.length === 0) { return dom('div'); } return dom('table.action_log_table', koDom.show(() => this._showForTable(table, ag)), @@ -238,7 +237,7 @@ export class ActionLog extends dispose.Disposable implements IDomComponent { 'Loading...'), koDom.foreach(this._displayStack, (ag: ActionGroupWithState) => { const timestamp = ag.time ? timeFormat("D T", new Date(ag.time)) : ""; - let desc = ag.desc || ""; + let desc: DomContents = ag.desc || ""; if (ag.actionSummary) { desc = this.renderTabularDiffs(ag.actionSummary, desc, ag); } diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index 391eb3ed..43406dc0 100644 --- a/app/client/components/BaseView.js +++ b/app/client/components/BaseView.js @@ -3,7 +3,6 @@ const _ = require('underscore'); const ko = require('knockout'); const moment = require('moment-timezone'); -const {getSelectionDesc} = require('app/common/DocActions'); const {nativeCompare, roundDownToMultiple, waitObs} = require('app/common/gutil'); const gutil = require('app/common/gutil'); const MANUALSORT = require('app/common/gristTypes').MANUALSORT; @@ -646,20 +645,7 @@ BaseView.prototype.sendPasteActions = function(cutCallback, actions) { // If the cut occurs on an edit restricted cell, there may be no cut action. if (cutAction) { actions.unshift(cutAction); } } - return this.gristDoc.docData.sendActions(actions, - this._getPasteDesc(actions[actions.length - 1], cutAction)); -}; - -/** - * Returns a string which describes a cut/copy action. - */ -BaseView.prototype._getPasteDesc = function(pasteAction, optCutAction) { - if (optCutAction) { - return `Moved ${getSelectionDesc(optCutAction, true)} to ` + - `${getSelectionDesc(pasteAction, true)}.`; - } else { - return `Pasted data to ${getSelectionDesc(pasteAction, true)}.`; - } + return this.gristDoc.docData.sendActions(actions); }; BaseView.prototype.buildDom = function() { diff --git a/app/client/lib/tableUtil.ts b/app/client/lib/tableUtil.ts index 64d15395..5db8153a 100644 --- a/app/client/lib/tableUtil.ts +++ b/app/client/lib/tableUtil.ts @@ -8,7 +8,6 @@ import {safeJsonParse} from 'app/common/gutil'; import type {TableData} from 'app/common/TableData'; import {tsvEncode} from 'app/common/tsvFormat'; import {dom} from 'grainjs'; -import map = require('lodash/map'); import zipObject = require('lodash/zipObject'); const G = getBrowserGlobals('document', 'DOMParser'); @@ -134,8 +133,11 @@ export function parsePasteHtml(data: string): RichPasteObject[][] { } // Helper function to add css style properties to an html tag -function _styleAttr(style: object) { - return map(style, (value, prop) => `${prop}: ${value};`).join(' '); +function _styleAttr(style: object|undefined) { + if (typeof style !== 'object') { + return ''; + } + return Object.entries(style).map(([prop, value]) => `${prop}: ${value};`).join(' '); } /** diff --git a/app/common/DocActions.ts b/app/common/DocActions.ts index 503e01a3..1a64afea 100644 --- a/app/common/DocActions.ts +++ b/app/common/DocActions.ts @@ -14,8 +14,6 @@ export interface AllCellVersions { } export type CellVersions = Partial; -import map = require('lodash/map'); - export type AddRecord = ['AddRecord', string, number, ColValues]; export type BulkAddRecord = ['BulkAddRecord', string, number[], BulkColValues]; export type RemoveRecord = ['RemoveRecord', string, number]; @@ -150,21 +148,6 @@ export type UserAction = Array; // Actions that trigger formula calculations in the data engine export const CALCULATING_USER_ACTIONS = new Set(['Calculate', 'UpdateCurrentTime', 'RespondToRequests']); -/** - * Gives a description for an action which involves setting values to a selection. - * @param {Array} action - The (Bulk)AddRecord/(Bulk)UpdateRecord action to describe. - * @param {Boolean} optExcludeVals - Indicates whether the values should be excluded from - * the description. - */ -export function getSelectionDesc(action: UserAction, optExcludeVals: boolean): string { - const table = action[1]; - const rows = action[2]; - const colValues: number[] = action[3] as any; // TODO: better typing - but code may evaporate - const columns = map(colValues, (values, col) => optExcludeVals ? col : `${col}: ${values}`); - const s = typeof rows === 'object' ? 's' : ''; - return `table ${table}, row${s} ${rows}; ${columns.join(", ")}`; -} - export function getNumRows(action: DocAction): number { return !isDataAction(action) ? 0 : Array.isArray(action[2]) ? action[2].length diff --git a/app/common/ThemePrefs-ti.ts b/app/common/ThemePrefs-ti.ts index 4bb4f66d..78a60b3a 100644 --- a/app/common/ThemePrefs-ti.ts +++ b/app/common/ThemePrefs-ti.ts @@ -185,6 +185,8 @@ export const ThemeColors = t.iface([], { "left-panel-page-options-selected-hover-bg": "string", "left-panel-page-initials-fg": "string", "left-panel-page-initials-bg": "string", + "left-panel-page-emoji-fg": "string", + "left-panel-page-emoji-outline": "string", "right-panel-tab-fg": "string", "right-panel-tab-bg": "string", "right-panel-tab-icon": "string", diff --git a/app/server/lib/ActionHistory.ts b/app/server/lib/ActionHistory.ts index 0bc16f46..35ac9d3e 100644 --- a/app/server/lib/ActionHistory.ts +++ b/app/server/lib/ActionHistory.ts @@ -16,9 +16,7 @@ import {LocalActionBundle} from 'app/common/ActionBundle'; import {ActionGroup, MinimalActionGroup} from 'app/common/ActionGroup'; import {createEmptyActionSummary} from 'app/common/ActionSummary'; -import {getSelectionDesc, UserAction} from 'app/common/DocActions'; import {DocState} from 'app/common/UserAPI'; -import toPairs = require('lodash/toPairs'); import {summarizeAction} from 'app/common/ActionSummarizer'; export interface ActionGroupOptions { @@ -163,81 +161,6 @@ export abstract class ActionHistory { } -/** - * Old helper to display the actionGroup in a human-readable way. Being maintained - * to avoid having to change too much at once. - */ -export function humanDescription(actions: UserAction[]): string { - const action = actions[0]; - if (!action) { return ""; } - let output = ''; - // Common names for various action parameters - const name = action[0]; - const table = action[1]; - const rows = action[2]; - const colId = action[2]; - const columns: any = action[3]; // TODO - better typing - but code may evaporate - switch (name) { - case 'UpdateRecord': - case 'BulkUpdateRecord': - case 'AddRecord': - case 'BulkAddRecord': - output = name + ' ' + getSelectionDesc(action, columns); - break; - case 'ApplyUndoActions': - // Currently cannot display information about what action was undone, as the action comes - // with the description of the "undo" message, which might be very different - // Also, cannot currently properly log redos as they are not distinguished from others in any way - // TODO: make an ApplyRedoActions type for redoing actions - output = 'Undo Previous Action'; - break; - case 'InitNewDoc': - output = 'Initialized new Document'; - break; - case 'AddColumn': - output = 'Added column ' + colId + ' to ' + table; - break; - case 'RemoveColumn': - output = 'Removed column ' + colId + ' from ' + table; - break; - case 'RemoveRecord': - case 'BulkRemoveRecord': - output = 'Removed record(s) ' + rows + ' from ' + table; - break; - case 'EvalCode': - output = 'Evaluated Code ' + action[1]; - break; - case 'AddTable': - output = 'Added table ' + table; - break; - case 'RemoveTable': - output = 'Removed table ' + table; - break; - case 'ModifyColumn': - // TODO: The Action Log currently only logs user actions, - // But ModifyColumn/Rename Column are almost always triggered from the client - // through a meta-table UpdateRecord. - // so, this is a case where making use of explicit sandbox engine 'looged' actions - // may be useful - output = 'Modify column ' + colId + ", "; - for (const [col, val] of toPairs(columns)) { - output += col + ": " + val + ", "; - } - output += ' in table ' + table; - break; - case 'RenameColumn': { - const newColId = action[3]; - output = 'Renamed Column ' + colId + ' to ' + newColId + ' in ' + table; - break; - } - default: - output = name + ' [No Description]'; - } - // A period for good grammar - output += '.'; - return output; -} - /** * Convert an ActionBundle into an ActionGroup. ActionGroups are the representation of * actions on the client. @@ -260,7 +183,9 @@ export function asActionGroup(history: ActionHistory, return { actionNum: act.actionNum, actionHash: act.actionHash || "", - desc: info.desc || humanDescription(act.userActions), + // Desc is a human-readable description of the user action set in a few places by client-side + // code, but is mostly (or maybe completely) unused. + desc: info.desc, actionSummary: summarize ? summarizeAction(act) : createEmptyActionSummary(), fromSelf, linkId: info.linkId,