(core) deal with write access for attachments

Summary:
Attachments are a special case for granular access control. A user is now allowed to read a given attachment if they have read access to a cell containing its id. So when a user writes to a cell in an attachment column, it is important that they can only write the ids of cells to which they have access. This diff allows a user to add an attachment id in a cell if:

  * The user already has access to that a attachment via some existing cell, or
  * The user recently updated the attachment, or
  * The attachment change is from an undo/redo of a previous action attributed to that user

Test Plan: Updated tests

Reviewers: georgegevoian, dsagal

Reviewed By: georgegevoian, dsagal

Differential Revision: https://phab.getgrist.com/D3681

app/client/widgets/AttachmentsEditor.ts

@@ -203,6 +203,7 @@ export class AttachmentsEditor extends NewBaseEditor {
       ...this._docComm.getUrlParams(),
       name: filename,
       ...cell,
+      maybeNew: 1,  // The attachment may be uploaded by the user but not stored in the cell yet.
       attId,
       ...(inline ? {inline: 1} : {})
     });

app/common/ActionBundle.ts

@@ -3,6 +3,7 @@
  * See also EncActionBundle for how these are packaged for encryption.
  */
 
+import {ApplyUAOptions} from 'app/common/ActiveDocAPI';
 import {DocAction, UserAction} from 'app/common/DocActions';
 import {RowCounts} from 'app/common/DocUsage';
 
@@ -50,6 +51,7 @@ export function getEnvContent<Content>(items: Array<EnvContent<Content>>): Conte
 export interface UserActionBundle {
   info: ActionInfo;
   userActions: UserAction[];
+  options?: ApplyUAOptions;
 }
 
 // ActionBundle as received from the sandbox. It does not have some action metadata, but does have

app/common/ActiveDocAPI.ts

@@ -12,10 +12,18 @@ export interface ApplyUAOptions {
   desc?: string;      // Overrides the description of the action.
   otherId?: number;   // For undo/redo; the actionNum of the original action to which it applies.
   linkId?: number;    // For bundled actions, actionNum of the previous action in the bundle.
-  bestEffort?: boolean; // If set, action may be applied in part if it cannot be applied completely.
   parseStrings?: boolean;  // If true, parses string values in some actions based on the column
 }
 
+export interface ApplyUAExtendedOptions extends ApplyUAOptions {
+  bestEffort?: boolean; // If set, action may be applied in part if it cannot be applied completely.
+  fromOwnHistory?: boolean; // If set, action is confirmed to be a redo/undo taken from history, from
+                            // an action marked as being by the current user.
+  oldestSource?: number;  // If set, gives the timestamp of the oldest source the undo/redo
+                          // action was built from, expressed as number of milliseconds
+                          // elapsed since January 1, 1970 00:00:00 UTC
+}
+
 export interface ApplyUAResult {
   actionNum: number;      // number of the action that got recorded.
   retValues: any[];       // array of return values, one for each of the passed-in user actions.

app/common/GranularAccessClause.ts

@@ -46,6 +46,7 @@ export interface UserInfo {
   LinkKey: Record<string, string | undefined>;
   UserID: number | null;
   UserRef: string | null;
+  SessionID: string | null;
   [attributes: string]: unknown;
   toJSON(): {[key: string]: any};
 }

app/server/lib/ActiveDoc.ts

@@ -15,6 +15,7 @@ import {ActionGroup, MinimalActionGroup} from 'app/common/ActionGroup';
 import {ActionSummary} from "app/common/ActionSummary";
 import {
   AclTableDescription,
+  ApplyUAExtendedOptions,
   ApplyUAOptions,
   ApplyUAResult,
   DataSourceTransformed,
@@ -55,7 +56,6 @@ import {
   RowCounts,
 } from 'app/common/DocUsage';
 import {normalizeEmail} from 'app/common/emails';
-import {ErrorWithCode} from 'app/common/ErrorWithCode';
 import {Product} from 'app/common/Features';
 import {FormulaProperties, getFormulaProperties} from 'app/common/GranularAccessClause';
 import {parseUrlId} from 'app/common/gristUrls';
@@ -113,7 +113,7 @@ import {
   makeExceptionalDocSession,
   OptDocSession
 } from './DocSession';
-import {createAttachmentsIndex, DocStorage} from './DocStorage';
+import {createAttachmentsIndex, DocStorage, REMOVE_UNUSED_ATTACHMENTS_DELAY} from './DocStorage';
 import {expandQuery} from './ExpandedQuery';
 import {GranularAccess, GranularAccessForBundle} from './GranularAccess';
 import {OnDemandActions} from './OnDemandActions';
@@ -121,6 +121,7 @@ import {getLogMetaFromDocSession, timeoutReached} from './serverUtils';
 import {findOrAddAllEnvelope, Sharing} from './Sharing';
 import cloneDeep = require('lodash/cloneDeep');
 import flatten = require('lodash/flatten');
+import pick = require('lodash/pick');
 import remove = require('lodash/remove');
 import sum = require('lodash/sum');
 import without = require('lodash/without');
@@ -141,9 +142,6 @@ const ACTIVEDOC_TIMEOUT = (process.env.NODE_ENV === 'production') ? 30 : 5;
 // We'll wait this long between re-measuring sandbox memory.
 const MEMORY_MEASUREMENT_INTERVAL_MS = 60 * 1000;
 
-// Cleanup expired attachments every hour (also happens when shutting down)
-const REMOVE_UNUSED_ATTACHMENTS_DELAY = {delayMs: 60 * 60 * 1000, varianceMs: 30 * 1000};
-
 // Apply the UpdateCurrentTime user action every hour
 const UPDATE_CURRENT_TIME_DELAY = {delayMs: 60 * 60 * 1000, varianceMs: 30 * 1000};
 
@@ -571,6 +569,12 @@ export class ActiveDoc extends EventEmitter {
     } finally {
       this._docManager.removeActiveDoc(this);
     }
+    try {
+      await this._granularAccess.close();
+    } catch (err) {
+      // This should not happen.
+      this._log.error(docSession, "failed to shutdown granular access", err);
+    }
     this._log.debug(docSession, "shutdown complete");
   }
 
@@ -844,6 +848,7 @@ export class ActiveDoc extends EventEmitter {
       this._updateAttachmentsSize().catch(e => {
         this._log.warn(docSession, 'failed to update attachments size', e);
       });
+      await this._granularAccess.noteUploads(docSession, result.retValues);
       return result.retValues;
     } finally {
       await globalUploadSet.cleanup(uploadId);
@@ -868,47 +873,40 @@ export class ActiveDoc extends EventEmitter {
 
   /**
    * Given a _gristAttachments record, returns a promise for the attachment data.
+   * Can optionally take a cell in which the attachment is expected to be
+   * referenced, and/or a `maybeNew` flag which, when set, specifies that the
+   * attachment may be a recent upload that is not yet referenced in the document.
    * @returns {Promise<Buffer>} Promise for the data of this attachment; rejected on error.
    */
   public async getAttachmentData(docSession: OptDocSession, attRecord: MetaRowRecord<"_grist_Attachments">,
-                                 cell?: SingleCell): Promise<Buffer> {
+                                 options?: {
+                                   cell?: SingleCell,
+                                   maybeNew?: boolean,
+                                 }): Promise<Buffer> {
     const attId = attRecord.id;
     const fileIdent = attRecord.fileIdent;
+    const cell = options?.cell;
+    const maybeNew = options?.maybeNew;
     if (
       await this._granularAccess.canReadEverything(docSession) ||
       await this.canDownload(docSession)
     ) {
       // Do not need to sweat over access to attachments if user can
       // read everything or download everything.
-    } else if (cell) {
-      // Only provide the download if the user has access to the cell
-      // they specified, and that cell is in an attachment column,
-      // and the cell contains the specified attachment.
-      await this._granularAccess.assertAttachmentAccess(docSession, cell, attId);
     } else {
-      // Find cells that refer to the given attachment.
+      if (maybeNew && await this._granularAccess.isAttachmentUploadedByUser(docSession, attId)) {
-      const cells = await this.docStorage.findAttachmentReferences(attId);
+        // Fine, this is an attachment the user uploaded (recently).
-      // Run through them to see if the user has access to any of them.
+      } else if (cell) {
-      // If so, we'll allow the download. We'd expect in a typical document
+        // Only provide the download if the user has access to the cell
-      // this this will be a small list of cells, typically 1 or less, but
+        // they specified, and that cell is in an attachment column,
-      // of course extreme cases are possible.
+        // and the cell contains the specified attachment.
-      let goodCell: SingleCell|undefined;
+        await this._granularAccess.assertAttachmentAccess(docSession, cell, attId);
-      for (const possibleCell of cells) {
+      } else {
-        try {
+        if (!await this._granularAccess.findAttachmentCellForUser(docSession, attId)) {
-          await this._granularAccess.assertAttachmentAccess(docSession, possibleCell, attId);
+          // We found no reason to allow this user to access the attachment.
-          goodCell = possibleCell;
+          throw new ApiError('Cannot access attachment', 403);
-          break;
-        } catch (e) {
-          if (e instanceof ErrorWithCode && e.code === 'ACL_DENY') {
-            continue;
-          }
-          throw e;
         }
       }
-      if (!goodCell) {
-        // We found no reason to allow this user to access the attachment.
-        throw new ApiError('Cannot access attachment', 403);
-      }
     }
     const data = await this.docStorage.getFileData(fileIdent);
     if (!data) { throw new ApiError("Invalid attachment identifier", 404); }
@@ -1165,24 +1163,9 @@ export class ActiveDoc extends EventEmitter {
    *                                          actionGroup.
    */
   public async applyUserActions(docSession: OptDocSession, actions: UserAction[],
-                                options?: ApplyUAOptions): Promise<ApplyUAResult> {
+                                unsanitizedOptions?: ApplyUAOptions): Promise<ApplyUAResult> {
-    assert(Array.isArray(actions), "`actions` parameter should be an array.");
+    const options = sanitizeApplyUAOptions(unsanitizedOptions);
-    // Be careful not to sneak into user action queue before Calculate action, otherwise
+    return this._applyUserActionsWithExtendedOptions(docSession, actions, options);
-    // there'll be a deadlock.
-    await this.waitForInitialization();
-
-    if (
-      this.dataLimitStatus === "deleteOnly" &&
-      !actions.every(action => [
-          'RemoveTable', 'RemoveColumn', 'RemoveRecord', 'BulkRemoveRecord',
-          'RemoveViewSection', 'RemoveView', 'ApplyUndoActions', 'RespondToRequests',
-        ].includes(action[0] as string))
-    ) {
-      throw new Error("Document is in delete-only mode");
-    }
-
-    // Granular access control implemented in _applyUserActions.
-    return await this._applyUserActions(docSession, actions, options);
   }
 
   /**
@@ -1200,12 +1183,25 @@ export class ActiveDoc extends EventEmitter {
                                     actionNums: number[],
                                     actionHashes: string[],
                                     undo: boolean,
-                                    options?: ApplyUAOptions): Promise<ApplyUAResult> {
+                                    unsanitizedOptions?: ApplyUAOptions): Promise<ApplyUAResult> {
+    const options = sanitizeApplyUAOptions(unsanitizedOptions);
     const actionBundles = await this._actionHistory.getActions(actionNums);
+    let fromOwnHistory: boolean = true;
+    const user = getDocSessionUser(docSession);
+    let oldestSource: number = Date.now();
     for (const [index, bundle] of actionBundles.entries()) {
       const actionNum = actionNums[index];
       const actionHash = actionHashes[index];
       if (!bundle) { throw new Error(`Could not find actionNum ${actionNum}`); }
+      const info = bundle.info[1];
+      const bundleEmail = info.user || '';
+      const sessionEmail = user?.email || '';
+      if (normalizeEmail(sessionEmail) !== normalizeEmail(bundleEmail)) {
+        fromOwnHistory = false;
+      }
+      if (info.time && info.time < oldestSource) {
+        oldestSource = info.time;
+      }
       if (actionHash !== bundle.actionHash) {
         throw new Error(`Hash mismatch for actionNum ${actionNum}: ` +
                         `expected ${actionHash} but got ${bundle.actionHash}`);
@@ -1221,7 +1217,10 @@ export class ActiveDoc extends EventEmitter {
     // It could be that error cases and timing etc leak some info prior to this
     // point.
     // Undos are best effort now by default.
-    return this.applyUserActions(docSession, actions, {bestEffort: undo, ...(options||{})});
+    return this._applyUserActionsWithExtendedOptions(
+      docSession, actions, {bestEffort: undo,
+                            oldestSource,
+                            fromOwnHistory, ...(options||{})});
   }
 
   /**
@@ -1685,8 +1684,9 @@ export class ActiveDoc extends EventEmitter {
    * granular access rules.
    */
   public getGranularAccessForBundle(docSession: OptDocSession, docActions: DocAction[], undo: DocAction[],
-                                    userActions: UserAction[], isDirect: boolean[]): GranularAccessForBundle {
+                                    userActions: UserAction[], isDirect: boolean[],
-    this._granularAccess.getGranularAccessForBundle(docSession, docActions, undo, userActions, isDirect);
+                                    options: ApplyUAOptions|null): GranularAccessForBundle {
+    this._granularAccess.getGranularAccessForBundle(docSession, docActions, undo, userActions, isDirect, options);
     return this._granularAccess;
   }
 
@@ -1784,7 +1784,7 @@ export class ActiveDoc extends EventEmitter {
    */
   @ActiveDoc.keepDocOpen
   protected async _applyUserActions(docSession: OptDocSession, actions: UserAction[],
-                                    options: ApplyUAOptions = {}): Promise<ApplyUAResult> {
+                                    options: ApplyUAExtendedOptions = {}): Promise<ApplyUAResult> {
 
     const client = docSession.client;
     this._log.debug(docSession, "_applyUserActions(%s, %s)%s", client, shortDesc(actions),
@@ -1796,13 +1796,14 @@ export class ActiveDoc extends EventEmitter {
     }
 
     if (options?.bestEffort) {
-      actions = await this._granularAccess.prefilterUserActions(docSession, actions);
+      actions = await this._granularAccess.prefilterUserActions(docSession, actions, options);
     }
     await this._granularAccess.checkUserActions(docSession, actions);
 
     // Create the UserActionBundle.
     const action: UserActionBundle = {
       info: this._makeInfo(docSession, options),
+      options,
       userActions: actions,
     };
 
@@ -1818,6 +1819,27 @@ export class ActiveDoc extends EventEmitter {
     return result;
   }
 
+  private async _applyUserActionsWithExtendedOptions(docSession: OptDocSession, actions: UserAction[],
+                                                     options?: ApplyUAExtendedOptions): Promise<ApplyUAResult> {
+    assert(Array.isArray(actions), "`actions` parameter should be an array.");
+    // Be careful not to sneak into user action queue before Calculate action, otherwise
+    // there'll be a deadlock.
+    await this.waitForInitialization();
+
+    if (
+      this.dataLimitStatus === "deleteOnly" &&
+      !actions.every(action => [
+          'RemoveTable', 'RemoveColumn', 'RemoveRecord', 'BulkRemoveRecord',
+          'RemoveViewSection', 'RemoveView', 'ApplyUndoActions', 'RespondToRequests',
+        ].includes(action[0] as string))
+    ) {
+      throw new Error("Document is in delete-only mode");
+    }
+
+    // Granular access control implemented in _applyUserActions.
+    return await this._applyUserActions(docSession, actions, options);
+  }
+
   /**
    * Create a new document file without using or initializing the data engine.
    */
@@ -2306,3 +2328,7 @@ export function tableIdToRef(metaTables: { [p: string]: TableDataAction }, table
   }
   return tableRefs[tableRowIndex];
 }
+
+export function sanitizeApplyUAOptions(options?: ApplyUAOptions): ApplyUAOptions {
+  return pick(options||{}, ['desc', 'otherId', 'linkId', 'parseStrings']);
+}

app/server/lib/DocApi.ts

@@ -290,7 +290,7 @@ export class DocWorkerApi {
       const ext = path.extname(fileIdent);
       const origName = attRecord.fileName as string;
       const fileName = ext ? path.basename(origName, path.extname(origName)) + ext : origName;
-      const fileData = await activeDoc.getAttachmentData(docSessionFromRequest(req), attRecord, cell);
+      const fileData = await activeDoc.getAttachmentData(docSessionFromRequest(req), attRecord, {cell});
       res.status(200)
         .type(ext)
         // Construct a content-disposition header of the form 'attachment; filename="NAME"'

app/server/lib/DocStorage.ts

@@ -47,6 +47,10 @@ const PENDING_VALUE = [GristObjCode.Pending];
 // that someone would delete a reference to an attachment and then undo that action this many days later.
 export const ATTACHMENTS_EXPIRY_DAYS = 7;
 
+// Cleanup expired attachments every hour (also happens when shutting down).
+export const REMOVE_UNUSED_ATTACHMENTS_DELAY = {delayMs: 60 * 60 * 1000, varianceMs: 30 * 1000};
+
+
 export class DocStorage implements ISQLiteDB, OnDemandStorage {
 
   // ======================================================================

app/server/lib/DocWorker.ts

@@ -2,6 +2,7 @@
  * DocWorker collects the methods and endpoints that relate to a single Grist document.
  * In hosted environment, this comprises the functionality of the DocWorker instance type.
  */
+import {isAffirmative} from 'app/common/gutil';
 import {HomeDBManager} from 'app/gen-server/lib/HomeDBManager';
 import {ActionHistoryImpl} from 'app/server/lib/ActionHistoryImpl';
 import {assertAccess, getOrSetDocAuth, RequestWithLogin} from 'app/server/lib/Authorizer';
@@ -42,6 +43,7 @@ export class DocWorker {
       const tableId = stringParam(req.query.tableId, 'tableId');
       const rowId = integerParam(req.query.rowId, 'rowId');
       const cell = {colId, tableId, rowId};
+      const maybeNew = isAffirmative(req.query.maybeNew);
       const attId = integerParam(req.query.attId, 'attId');
       const attRecord = activeDoc.getAttachmentMetadata(attId);
       const ext = path.extname(attRecord.fileIdent);
@@ -54,7 +56,7 @@ export class DocWorker {
       // Construct a content-disposition header of the form 'inline|attachment; filename="NAME"'
       const contentDispType = inline ? "inline" : "attachment";
       const contentDispHeader = contentDisposition(stringParam(req.query.name, 'name'), {type: contentDispType});
-      const data = await activeDoc.getAttachmentData(docSession, attRecord, cell);
+      const data = await activeDoc.getAttachmentData(docSession, attRecord, {cell, maybeNew});
       res.status(200)
         .type(ext)
         .set('Content-Disposition', contentDispHeader)

app/server/lib/GranularAccess.ts

@@ -2,8 +2,9 @@ import { ALL_PERMISSION_PROPS } from 'app/common/ACLPermissions';
 import { ACLRuleCollection, SPECIAL_RULES_TABLE_ID } from 'app/common/ACLRuleCollection';
 import { ActionGroup } from 'app/common/ActionGroup';
 import { createEmptyActionSummary } from 'app/common/ActionSummary';
-import { ServerQuery } from 'app/common/ActiveDocAPI';
+import { ApplyUAExtendedOptions, ServerQuery } from 'app/common/ActiveDocAPI';
 import { ApiError } from 'app/common/ApiError';
+import { MapWithTTL } from 'app/common/AsyncCreate';
 import {
   AddRecord,
   BulkAddRecord,
@@ -27,7 +28,7 @@ import { ErrorWithCode } from 'app/common/ErrorWithCode';
 import { AclMatchInput, InfoEditor, InfoView } from 'app/common/GranularAccessClause';
 import { UserInfo } from 'app/common/GranularAccessClause';
 import * as gristTypes from 'app/common/gristTypes';
-import { getSetMapValue, isNonNullish, pruneArray } from 'app/common/gutil';
+import { getSetMapValue, isNonNullish, isNumber, pruneArray } from 'app/common/gutil';
 import { MetaRowRecord, SingleCell } from 'app/common/TableData';
 import { canEdit, canView, isValidRole, Role } from 'app/common/roles';
 import { FullUser, UserAccessData } from 'app/common/UserAPI';
@@ -37,12 +38,13 @@ import { compileAclFormula } from 'app/server/lib/ACLFormula';
 import { DocClients } from 'app/server/lib/DocClients';
 import { getDocSessionAccess, getDocSessionAltSessionId, getDocSessionUser,
          OptDocSession } from 'app/server/lib/DocSession';
-import { DocStorage } from 'app/server/lib/DocStorage';
+import { DocStorage, REMOVE_UNUSED_ATTACHMENTS_DELAY } from 'app/server/lib/DocStorage';
 import log from 'app/server/lib/log';
 import { IPermissionInfo, PermissionInfo, PermissionSetWithContext } from 'app/server/lib/PermissionInfo';
 import { TablePermissionSetWithContext } from 'app/server/lib/PermissionInfo';
 import { integerParam } from 'app/server/lib/requestUtils';
-import { getRelatedRows, getRowIdsFromDocAction } from 'app/server/lib/RowAccess';
+import { getColIdsFromDocAction, getColValuesFromDocAction, getRelatedRows,
+         getRowIdsFromDocAction } from 'app/server/lib/RowAccess';
 import cloneDeep = require('lodash/cloneDeep');
 import fromPairs = require('lodash/fromPairs');
 import memoize = require('lodash/memoize');
@@ -176,6 +178,19 @@ const OTHER_RECOGNIZED_ACTIONS = new Set([
   'RemoveViewSection',
 ]);
 
+// When an attachment is uploaded, it isn't immediately added to a cell in
+// the document. We grant the uploader a special period where they can freely
+// add or re-add the attachment to the document without access control fuss.
+// We keep that period within the time range where an unused attachment
+// would get deleted.
+const UPLOADED_ATTACHMENT_OWNERSHIP_PERIOD =
+  (REMOVE_UNUSED_ATTACHMENTS_DELAY.delayMs - REMOVE_UNUSED_ATTACHMENTS_DELAY.varianceMs) / 2;
+
+// When a user undoes their own action or actions, checks of attachment ownership
+// are handled specially. This special handling will not apply for undoes of actions
+// older than this limit.
+const HISTORICAL_ATTACHMENT_OWNERSHIP_PERIOD = 24 * 60 * 60 * 1000;
+
 interface DocUpdateMessage {
   actionGroup: ActionGroup;
   docActions: DocAction[];
@@ -229,6 +244,7 @@ export class GranularAccess implements GranularAccessForBundle {
   // garbage-collection once docSession is no longer in use.
   private _userAttributesMap = new WeakMap<OptDocSession, UserAttributes>();
   private _prevUserAttributesMap: WeakMap<OptDocSession, UserAttributes>|undefined;
+  private _attachmentUploads = new MapWithTTL<number, string>(UPLOADED_ATTACHMENT_OWNERSHIP_PERIOD);
 
   // When broadcasting a sequence of DocAction[]s, this contains the state of
   // affected rows for the relevant table before and after each DocAction.  It
@@ -251,6 +267,7 @@ export class GranularAccess implements GranularAccessForBundle {
     // Flag for whether doc actions mention a rule change, even if passive due to
     // schema changes.
     hasAnyRuleChange: boolean,
+    options: ApplyUAExtendedOptions|null,
   }|null;
 
   public constructor(
@@ -263,8 +280,13 @@ export class GranularAccess implements GranularAccessForBundle {
     private _docId: string) {
   }
 
+  public async close() {
+    this._attachmentUploads.clear();
+  }
+
   public getGranularAccessForBundle(docSession: OptDocSession, docActions: DocAction[], undo: DocAction[],
-                                    userActions: UserAction[], isDirect: boolean[]): void {
+                                    userActions: UserAction[], isDirect: boolean[],
+                                    options: ApplyUAExtendedOptions|null): void {
     if (this._activeBundle) { throw new Error('Cannot start a bundle while one is already in progress'); }
     // This should never happen - attempts to write to a pre-fork session should be
     // caught by an Authorizer.  But let's be paranoid, since we may be pretending to
@@ -273,7 +295,8 @@ export class GranularAccess implements GranularAccessForBundle {
     if (docSession.forkingAsOwner) { throw new Error('Should never modify a prefork'); }
     this._activeBundle = {
       docSession, docActions, undo, userActions, isDirect,
-      applied: false, hasDeliberateRuleChange: false, hasAnyRuleChange: false
+      applied: false, hasDeliberateRuleChange: false, hasAnyRuleChange: false,
+      options,
     };
     this._activeBundle.hasDeliberateRuleChange =
       scanActionsRecursively(userActions, (a) => isAclTable(String(a[1])));
@@ -300,6 +323,16 @@ export class GranularAccess implements GranularAccessForBundle {
     return access.getUser();
   }
 
+  /**
+   * Represent fields from the session in an input object for ACL rules.
+   * Just one field currently, "user".
+   */
+  public async inputs(docSession: OptDocSession): Promise<AclMatchInput> {
+    return {
+      user: await this._getUser(docSession),
+    };
+  }
+
   /**
    * Check whether user has any access to table.
    */
@@ -348,7 +381,7 @@ export class GranularAccess implements GranularAccessForBundle {
       return fail();
     }
     const rec = new RecordView(rows, 0);
-    const input: AclMatchInput = {user: await this._getUser(docSession), rec, newRec: rec};
+    const input: AclMatchInput = {...await this.inputs(docSession), rec, newRec: rec};
     const rowPermInfo = new PermissionInfo(this._ruler.ruleCollection, input);
     const rowAccess = rowPermInfo.getTableAccess(cell.tableId).perms.read;
     if (rowAccess === 'deny') { fail(); }
@@ -382,6 +415,42 @@ export class GranularAccess implements GranularAccessForBundle {
     }
   }
 
+  /**
+   * Check whether the specified attachment is known to have been uploaded
+   * by the user (identified by SessionID) recently.
+   */
+  public async isAttachmentUploadedByUser(docSession: OptDocSession, attId: number): Promise<boolean> {
+    const user = await this.getUser(docSession);
+    const id = user.SessionID || '';
+    return (this._attachmentUploads.get(attId) === id);
+  }
+
+  /**
+   * Find a cell in an attachment column that contains the specified attachment,
+   * and which is accessible by the user associated with the session.
+   */
+  public async findAttachmentCellForUser(docSession: OptDocSession, attId: number): Promise<SingleCell|undefined> {
+    // Find cells that refer to the given attachment.
+    const cells = await this._docStorage.findAttachmentReferences(attId);
+    // Run through them to see if the user has access to any of them.
+    // We'd expect in a typical document that this will be a small
+    // list of cells, typically 1 or less, but of course extreme cases
+    // are possible.
+    for (const possibleCell of cells) {
+      try {
+        await this.assertAttachmentAccess(docSession, possibleCell, attId);
+        return possibleCell;
+      } catch (e) {
+        if (e instanceof ErrorWithCode && e.code === 'ACL_DENY') {
+          continue;
+        }
+        throw e;
+      }
+    }
+    // Nothing found.
+    return undefined;
+  }
+
   /**
    * Called after UserAction[]s have been applied in the sandbox, and DocAction[]s have been
    * computed, but before we have committed those DocAction[]s to the database.  If this
@@ -612,7 +681,8 @@ export class GranularAccess implements GranularAccessForBundle {
    * Any filtering done here is NOT a security measure, and the output should
    * not be granted any level of automatic trust.
    */
-  public async prefilterUserActions(docSession: OptDocSession, actions: UserAction[]): Promise<UserAction[]> {
+  public async prefilterUserActions(docSession: OptDocSession, actions: UserAction[],
+                                    options: ApplyUAExtendedOptions|null): Promise<UserAction[]> {
     // Currently we only attempt prefiltering for an ApplyUndoActions.
     if (actions.length !== 1) { return actions; }
     const userAction = actions[0];
@@ -646,7 +716,7 @@ export class GranularAccess implements GranularAccessForBundle {
       // any case (though we could rearrange to limit how undo actions are
       // requested).
       this.getGranularAccessForBundle(docSession, docActions, [], docActions,
-                                      docActions.map(() => true));
+                                      docActions.map(() => true), options);
       for (const [actionIdx, action] of docActions.entries()) {
         // A single action might contain forbidden material at cell, row, column,
         // or table level.  Retaining permitted material may require refactoring the
@@ -880,6 +950,36 @@ export class GranularAccess implements GranularAccessForBundle {
                                                (_docSession) => this._filterDocUpdate(_docSession, message));
   }
 
+  /**
+   * Called when uploads occur. We record the fact that the specified attachment
+   * ids originated in uploads by the current user, for a certain length of time.
+   * During that time, attempts by the user to use these attachment ids in an
+   * attachment column will be accepted. The user is identified by SessionID,
+   * which is a user id for logged in users, and a session-unique id for
+   * anonymous users accessing Grist from a browser.
+   *
+   * A remaining weakness of this protection could be if attachment ids were
+   * reused, and reused quickly. Attachments can be deleted after
+   * REMOVE_UNUSED_ATTACHMENTS_DELAY and on document shutdown. We keep
+   * UPLOADED_ATTACHMENT_OWNERSHIP_PERIOD less than REMOVE_UNUSED_ATTACHMENTS_DELAY,
+   * and wipe our records on document shutdown.
+   */
+  public async noteUploads(docSession: OptDocSession, attIds: number[]) {
+    const user = await this.getUser(docSession);
+    const id = user.SessionID;
+    if (!id) {
+      log.rawError('noteUploads needs a SessionID', {
+        docId: this._docId,
+        attIds,
+        userId: user.UserID,
+      });
+      return;
+    }
+    for (const attId of attIds) {
+      this._attachmentUploads.set(attId, id);
+    }
+  }
+
   // Remove cached access information for a given session.
   public flushAccess(docSession: OptDocSession) {
     this._ruler.flushAccess(docSession);
@@ -1465,7 +1565,7 @@ export class GranularAccess implements GranularAccessForBundle {
 
     const rec = new RecordView(rowsRec, undefined);
     const newRec = new RecordView(rowsNewRec, undefined);
-    const input: AclMatchInput = {user: await this._getUser(docSession), rec, newRec};
+    const input: AclMatchInput = {...await this.inputs(docSession), rec, newRec};
 
     const [, tableId, , colValues] = action;
     let filteredColValues: ColValues | BulkColValues | undefined | null = null;
@@ -1547,7 +1647,7 @@ export class GranularAccess implements GranularAccessForBundle {
                                   colId?: string): Promise<number[]> {
     const ruler = await this._getRuler(cursor);
     const rec = new RecordView(data, undefined);
-    const input: AclMatchInput = {user: await this._getUser(cursor.docSession), rec};
+    const input: AclMatchInput = {...await this.inputs(cursor.docSession), rec};
 
     const [, tableId, rowIds] = data;
     const toRemove: number[] = [];
@@ -1893,7 +1993,9 @@ export class GranularAccess implements GranularAccessForBundle {
     const needMeta = docActions.some(a => isSchemaAction(a) || getTableId(a).startsWith('_grist_'));
     if (!needMeta) {
       // Sometimes, the intermediate states are trivial.
-      return docActions.map(action => ({action}));
+      // TODO: look into whether it would be worth caching attachment columns.
+      const attachmentColumns = this._getAttachmentColumns(this._docData);
+      return docActions.map(action => ({action, attachmentColumns}));
     }
     const metaDocData = new DocData(
       async (tableId) => {
@@ -1944,11 +2046,33 @@ export class GranularAccess implements GranularAccessForBundle {
         replaceRuler = false;
       }
       step.ruler = ruler;
+      step.attachmentColumns = this._getAttachmentColumns(metaDocData);
       steps.push(step);
     }
     return steps;
   }
 
+  /**
+   * Enumerate attachment columns, represented as a map from tableId to
+   * a set of colIds.
+   */
+  private _getAttachmentColumns(metaDocData: DocData): Map<string, Set<string>> {
+    const tablesTable = metaDocData.getMetaTable('_grist_Tables');
+    const columnsTable = metaDocData.getMetaTable('_grist_Tables_column');
+    const attachmentColumns: Map<string, Set<string>> = new Map();
+    for (const col of columnsTable.filterRecords({type: 'Attachments'})) {
+      const table = tablesTable.getRecord(col.parentId);
+      const tableId = table?.tableId;
+      if (!tableId) { throw new Error('table not found'); /* should not happen */ }
+      const colId = col.colId;
+      if (!attachmentColumns.has(tableId)) {
+        attachmentColumns.set(tableId, new Set());
+      }
+      attachmentColumns.get(tableId)!.add(colId);
+    }
+    return attachmentColumns;
+  }
+
   /**
    * Return any permitted parts of an action.  A completely forbidden
    * action results in an empty list.  Forbidden columns and rows will
@@ -2095,6 +2219,7 @@ export class GranularAccess implements GranularAccessForBundle {
   }
 
   private async _checkIncomingDocAction(cursor: ActionCursor): Promise<void> {
+    await this._checkIncomingAttachmentChanges(cursor);
     const {action, docSession} = cursor;
     const accessCheck = await this._getAccessForActionType(docSession, action, 'fatal');
     const tableId = getTableId(action);
@@ -2111,6 +2236,69 @@ export class GranularAccess implements GranularAccessForBundle {
     this._pruneColumns(action, permInfo, tableId, accessCheck);
   }
 
+  /**
+   * Take a look at the DocAction and see if it might allow the user to
+   * introduce attachment ids into a cell. If so, make sure the user
+   * has the right to access any attachments mentioned.
+   */
+  private async _checkIncomingAttachmentChanges(cursor: ActionCursor): Promise<void> {
+    const options = this._activeBundle?.options;
+    if (options?.fromOwnHistory && options.oldestSource &&
+      Date.now() - options.oldestSource < HISTORICAL_ATTACHMENT_OWNERSHIP_PERIOD) {
+      return;
+    }
+    const {action, docSession} = cursor;
+    if (!isDataAction(action)) { return; }
+    if (isRemoveRecordAction(action)) { return; }
+    const tableId = getTableId(action);
+    const step = await this._getMetaStep(cursor);
+    const attachmentColumns = step.attachmentColumns;
+    if (!attachmentColumns) { return; }
+    const ac = attachmentColumns.get(tableId);
+    if (!ac) { return; }
+    const colIds = getColIdsFromDocAction(action);
+    if (!colIds.some(colId => ac.has(colId))) { return; }
+    if (await this.isOwner(docSession) || await this.canReadEverything(docSession)) { return; }
+    const attIds = new Set<number>();
+    for (const colId of colIds) {
+      if (!ac.has(colId)) { continue; }
+      const values = getColValuesFromDocAction(action, colId);
+      if (!values) { continue; }
+      for (const v of values) {
+        // We expect an array. What should we do with other types?
+        // If we were confident no part of Grist would interpret non-array
+        // values as attachment ids, then we should let them be added, as
+        // part of Grist's spreadsheet-style willingness to allow invalid
+        // data. I decided to go ahead and require that numbers or number-like
+        // strings should be checked as if they were attachment ids, just in
+        // case. But if this proves awkward for someone, it could be reasonable
+        // to only check ids in an array after confirming Grist is strict in
+        // how it interprets material in attachment cells.
+        if (typeof v === 'number') {
+          attIds.add(v);
+        } else if (Array.isArray(v)) {
+          for (const p of v) {
+            if (typeof p === 'number') {
+              attIds.add(p);
+            }
+          }
+        } else if (typeof v === 'boolean' || v === null) {
+          // Nothing obvious to do here.
+        } else if (isNumber(v)) {
+          attIds.add(Math.round(parseFloat(v)));
+        }
+      }
+    }
+    for (const attId of attIds) {
+      if (!await this.isAttachmentUploadedByUser(docSession, attId) &&
+        !await this.findAttachmentCellForUser(docSession, attId)) {
+        throw new ErrorWithCode('ACL_DENY', 'Cannot access attachment', {
+          status: 403,
+        });
+      }
+    }
+  }
+
   private async _getRuler(cursor: ActionCursor) {
     if (cursor.actionIdx === null) { return this._ruler; }
     const step = await this._getMetaStep(cursor);
@@ -2192,7 +2380,7 @@ export class GranularAccess implements GranularAccessForBundle {
     };
     const ruler = await this._getRuler(cursor);
     const permInfo = await ruler.getAccess(docSession);
-    const user = await this._getUser(docSession);
+    const inputs = await this.inputs(docSession);
     // Cache some data, as they are checked.
     const readRows = memoize(this._fetchQueryFromDB.bind(this));
     const hasAccess = async (cell: SingleCell) => {
@@ -2223,7 +2411,7 @@ export class GranularAccess implements GranularAccessForBundle {
         }
       }
       const rec = rows ? new RecordView(rows, 0) : undefined;
-      const input: AclMatchInput = {user, rec, newRec: rec};
+      const input: AclMatchInput = {...inputs, rec, newRec: rec};
       const rowPermInfo = new PermissionInfo(ruler.ruleCollection, input);
       const rowAccess = rowPermInfo.getTableAccess(cell.tableId).perms.read;
       if (rowAccess === 'deny') { return false; }
@@ -2286,7 +2474,7 @@ export class Ruler {
     // TODO The intent of caching is to avoid duplicating rule evaluations while processing a
     // single request. Caching based on docSession is riskier since those persist across requests.
     return getSetMapValue(this._permissionInfoMap as Map<OptDocSession, Promise<PermissionInfo>>, docSession,
-      async () => new PermissionInfo(this.ruleCollection, {user: await this._owner.getUser(docSession)}));
+      async () => new PermissionInfo(this.ruleCollection, await this._owner.inputs(docSession)));
   }
 
   public flushAccess(docSession: OptDocSession) {
@@ -2314,6 +2502,7 @@ export class Ruler {
 
 export interface RulerOwner {
   getUser(docSession: OptDocSession): Promise<UserInfo>;
+  inputs(docSession: OptDocSession): Promise<AclMatchInput>;
 }
 
 /**
@@ -2331,6 +2520,7 @@ export interface MetaStep {
   metaBefore?: {[key: string]: TableDataAction};  // cached structural metadata before action
   metaAfter?: {[key: string]: TableDataAction};   // cached structural metadata after action
   ruler?: Ruler;                          // rules at this step
+  attachmentColumns?: Map<string, Set<string>>;        // attachment columns after this step
 }
 
 /**
@@ -2497,7 +2687,7 @@ class CellAccessHelper {
   private _tableAccess: Map<string, boolean> = new Map();
   private _rowPermInfo: Map<string, Map<number, PermissionInfo>> = new Map();
   private _rows: Map<string, TableDataAction> = new Map();
-  private _user!: UserInfo;
+  private _inputs!: AclMatchInput;
 
   constructor(
     private _granular: GranularAccess,
@@ -2511,7 +2701,7 @@ class CellAccessHelper {
    * Resolves access for all cells, and save the results in the cache.
    */
   public async calculate(cells: SingleCell[]) {
-    this._user = await this._granular.getUser(this._docSession);
+    this._inputs = await this._granular.inputs(this._docSession);
     const tableIds = new Set(cells.map(cell => cell.tableId));
     for (const tableId of tableIds) {
       this._tableAccess.set(tableId, await this._granular.hasTableAccess(this._docSession, tableId));
@@ -2521,7 +2711,7 @@ class CellAccessHelper {
         for(const [idx, rowId] of rows[2].entries()) {
           if (rowIds.has(rowId) === false) { continue; }
           const rec = new RecordView(rows, idx);
-          const input: AclMatchInput = {user: this._user, rec, newRec: rec};
+          const input: AclMatchInput = {...this._inputs, rec, newRec: rec};
           const rowPermInfo = new PermissionInfo(this._ruler.ruleCollection, input);
           if (!this._rowPermInfo.has(tableId)) {
             this._rowPermInfo.set(tableId, new Map());
@@ -2892,6 +3082,7 @@ export class User implements UserInfo {
   public Origin: string | null = null;
   public LinkKey: Record<string, string | undefined> = {};
   public Email: string | null = null;
+  public SessionID: string | null = null;
   public UserRef: string | null = null;
   [attribute: string]: any;
 

app/server/lib/RowAccess.ts

@@ -1,5 +1,6 @@
-import { AddRecord, BulkAddRecord, BulkRemoveRecord, BulkUpdateRecord, DocAction, getTableId,
+import { AddRecord, BulkAddRecord, BulkRemoveRecord, BulkUpdateRecord,
-         RemoveRecord, ReplaceTableData, TableDataAction, UpdateRecord } from "app/common/DocActions";
+         CellValue, DocAction, getTableId, RemoveRecord, ReplaceTableData,
+         TableDataAction, UpdateRecord } from "app/common/DocActions";
 import { getSetMapValue } from "app/common/gutil";
 
 /**
@@ -78,7 +79,7 @@ export function getRowIdsFromDocAction(docActions: RemoveRecord | BulkRemoveReco
 }
 
 /**
- * Tiny helper to get the row ids mentioned in a record-related DocAction as a list
+ * Tiny helper to get the col ids mentioned in a record-related DocAction as a list
  * (even if the action is not a bulk action). When the action touches the whole row,
  * it returns ["*"].
  */
@@ -88,3 +89,21 @@ export function getColIdsFromDocAction(docActions: RemoveRecord | BulkRemoveReco
   if (docActions[3]) { return Object.keys(docActions[3]); }
   return ['*'];
 }
+
+/**
+ * Extract column values for a particular column as CellValue[] from a
+ * record-related DocAction. Undefined if absent.
+ */
+export function getColValuesFromDocAction(docAction: RemoveRecord | BulkRemoveRecord | AddRecord |
+  BulkAddRecord | UpdateRecord | BulkUpdateRecord | ReplaceTableData |
+  TableDataAction, colId: string): CellValue[]|undefined {
+  const colValues = docAction[3];
+  if (!colValues) { return undefined; }
+  const cellValues = colValues[colId];
+  if (!cellValues) { return undefined; }
+  if (Array.isArray(docAction[2])) {
+    return cellValues as CellValue[];
+  } else {
+    return [cellValues as CellValue];
+  }
+}

app/server/lib/Sharing.ts

@@ -6,6 +6,7 @@ import {
   LocalActionBundle,
   UserActionBundle
 } from 'app/common/ActionBundle';
+import {ApplyUAExtendedOptions} from 'app/common/ActiveDocAPI';
 import {CALCULATING_USER_ACTIONS, DocAction, getNumRows, UserAction} from 'app/common/DocActions';
 import {allToken} from 'app/common/sharing';
 import log from 'app/server/lib/log';
@@ -195,15 +196,16 @@ export class Sharing {
     const userActions: UserAction[] = [
       ['ApplyDocActions', action.stored.map(envContent => envContent[1])]
     ];
-    return this._doApplyUserActions(action.info[1], userActions, Branch.Shared, null);
+    return this._doApplyUserActions(action.info[1], userActions, Branch.Shared, null, null);
   }
 
   private _doApplyUserActionBundle(action: UserActionBundle, docSession: OptDocSession|null): Promise<UserResult> {
-    return this._doApplyUserActions(action.info, action.userActions, Branch.Local, docSession);
+    return this._doApplyUserActions(action.info, action.userActions, Branch.Local, docSession, action.options || null);
   }
 
   private async _doApplyUserActions(info: ActionInfo, userActions: UserAction[],
-                                   branch: Branch, docSession: OptDocSession|null): Promise<UserResult> {
+                                    branch: Branch, docSession: OptDocSession|null,
+                                    options: ApplyUAExtendedOptions|null): Promise<UserResult> {
     const client = docSession && docSession.client;
 
     if (docSession?.linkId) {
@@ -211,7 +213,7 @@ export class Sharing {
     }
 
     const {sandboxActionBundle, undo, accessControl} =
-      await this._modificationLock.runExclusive(() => this._applyActionsToDataEngine(docSession, userActions));
+      await this._modificationLock.runExclusive(() => this._applyActionsToDataEngine(docSession, userActions, options));
 
     try {
 
@@ -389,7 +391,8 @@ export class Sharing {
         shortDesc(envAction[1])));
   }
 
-  private async _applyActionsToDataEngine(docSession: OptDocSession|null, userActions: UserAction[]) {
+  private async _applyActionsToDataEngine(docSession: OptDocSession|null, userActions: UserAction[],
+                                          options: ApplyUAExtendedOptions|null) {
     const sandboxActionBundle = await this._activeDoc.applyActionsToDataEngine(docSession, userActions);
     const undo = getEnvContent(sandboxActionBundle.undo);
     const docActions = getEnvContent(sandboxActionBundle.stored).concat(
@@ -397,7 +400,8 @@ export class Sharing {
     const isDirect = getEnvContent(sandboxActionBundle.direct);
 
     const accessControl = this._activeDoc.getGranularAccessForBundle(
-      docSession || makeExceptionalDocSession('share'), docActions, undo, userActions, isDirect
+      docSession || makeExceptionalDocSession('share'), docActions, undo, userActions, isDirect,
+      options
     );
     try {
       // TODO: see if any of the code paths that have no docSession are relevant outside