(core) support granular read access for attachments

Summary:
When a user requests to read the contents of an attachment, only allow the request if there exists a cell in an attachment column that contains the attachment and which they have read access to.

This does not cover:
 * Granular write access for attachments. In particular, a user who can write to any attachment column should be considered to have full read access to all attachment columns, currently.
 * Access control of attachment metadata such as name and format.

The implementation uses a sql query that requires a scan, and some notes on how this could be optimized in future. The web client was updated to specify the cell to check for access, and performance seemed fine in casual testing on a doc with 1000s of attachments. I'm not sure how performance would hold up as the set of access rules grows as well.

Test Plan: added tests

Reviewers: alexmojaki

Reviewed By: alexmojaki

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

app/client/widgets/AttachmentsEditor.ts

@@ -16,6 +16,7 @@ import {IModalControl, modal} from 'app/client/ui2018/modals';
 import {renderFileType} from 'app/client/widgets/AttachmentsWidget';
 import {NewBaseEditor, Options} from 'app/client/widgets/NewBaseEditor';
 import {CellValue} from 'app/common/DocActions';
+import {SingleCell} from 'app/common/TableData';
 import {clamp, encodeQueryParams} from 'app/common/gutil';
 import {UploadResult} from 'app/common/uploads';
 import * as mimeTypes from 'mime-types';
@@ -60,6 +61,11 @@ export class AttachmentsEditor extends NewBaseEditor {
 
     const docData: DocData = options.gristDoc.docData;
     const cellValue: CellValue = options.cellValue;
+    const cell: SingleCell = {
+      rowId: options.rowId,
+      colId: options.field.colId(),
+      tableId: options.field.column().table().tableId(),
+    };
 
     // editValue is abused slightly to indicate a 1-based index of the attachment.
     const initRowIndex: number|undefined = (options.editValue && parseInt(options.editValue, 0) - 1) || 0;
@@ -79,8 +85,8 @@ export class AttachmentsEditor extends NewBaseEditor {
         fileType,
         filename,
         hasPreview: Boolean(this._attachmentsTable.getValue(val, 'imageHeight')),
-        url: computed((use) => this._getUrl(fileIdent, use(filename))),
-        inlineUrl: computed((use) => this._getUrl(fileIdent, use(filename), true))
+        url: computed((use) => this._getUrl(cell, val, use(filename))),
+        inlineUrl: computed((use) => this._getUrl(cell, val, use(filename), true))
       };
     });
     this._index = makeLiveIndex(this, this._attachments, initRowIndex);
@@ -190,11 +196,12 @@ export class AttachmentsEditor extends NewBaseEditor {
     att.filename.set(this._attachmentsTable.getValue(att.rowId, 'fileName')!);
   }
 
-  private _getUrl(fileIdent: string, filename: string, inline?: boolean): string {
+  private _getUrl(cell: SingleCell, attId: number, filename: string, inline?: boolean): string {
     return this._docComm.docUrl('attachment') + '?' + encodeQueryParams({
       ...this._docComm.getUrlParams(),
-      ident: fileIdent,
       name: filename,
+      ...cell,
+      attId,
       ...(inline ? {inline: 1} : {})
     });
   }

app/client/widgets/AttachmentsWidget.ts

@@ -10,6 +10,7 @@ import {encodeQueryParams} from 'app/common/gutil';
 import {MetaTableData} from 'app/client/models/TableData';
 import {UploadResult} from 'app/common/uploads';
 import {extname} from 'path';
+import { SingleCell } from 'app/common/TableData';
 
 const testId: TestId = makeTestId('test-pw-');
 
@@ -90,6 +91,8 @@ export class AttachmentsWidget extends NewAbstractWidget {
     const values = Computed.create(null, fromKo(cellValue), (use, _cellValue) =>
       Array.isArray(_cellValue) ? _cellValue.slice(1) : []);
 
+    const colId = this.field.colId();
+    const tableId = this.field.column().table().tableId();
     return attachmentWidget(
       dom.autoDispose(values),
 
@@ -98,9 +101,12 @@ export class AttachmentsWidget extends NewAbstractWidget {
         dom.cls('attachment_hover_icon', (use) => use(values).length > 0),
         dom.on('click', () => this._selectAndSave(cellValue))
       ),
-      dom.forEach(values, (value: number) =>
-        isNaN(value) ? null : this._buildAttachment(value, values)
-      ),
+      dom.maybe(_row.id, rowId => {
+        return dom.forEach(values, (value: number) =>
+          isNaN(value) ? null : this._buildAttachment(value, values, {
+            rowId, colId, tableId,
+          }));
+      }),
       dom.on('drop', ev => this._uploadAndSave(cellValue, ev.dataTransfer!.files))
     );
   }
@@ -121,7 +127,7 @@ export class AttachmentsWidget extends NewAbstractWidget {
     );
   }
 
-  protected _buildAttachment(value: number, allValues: Computed<number[]>): Element {
+  protected _buildAttachment(value: number, allValues: Computed<number[]>, cell: SingleCell): Element {
     const filename = this._attachmentsTable.getValue(value, 'fileName')!;
     const fileIdent = this._attachmentsTable.getValue(value, 'fileIdent')!;
     const height = this._attachmentsTable.getValue(value, 'imageHeight')!;
@@ -134,7 +140,7 @@ export class AttachmentsWidget extends NewAbstractWidget {
       dom.style('width', (use) => `${parseInt(use(this._height), 10) * ratio}px`),
       // TODO: Update to legitimately determine whether a file preview exists.
       hasPreview ? dom('img', {style: 'height: 100%; min-width: 100%; vertical-align: top;'},
-        dom.attr('src', this._getUrl(value))
+        dom.attr('src', this._getUrl(value, cell))
       ) : renderFileType(filename, fileIdent, this._height),
       // Open editor as if with input, using it to tell it which of the attachments to show. We
       // pass in a 1-based index. Hitting a key opens the cell, and this approach allows an
@@ -145,18 +151,14 @@ export class AttachmentsWidget extends NewAbstractWidget {
   }
 
   // Returns the attachment download url.
-  private _getUrl(rowId: number): string {
-    const ident = this._attachmentsTable.getValue(rowId, 'fileIdent');
-    if (!ident) {
-      return '';
-    } else {
-      const docComm = this._getDocComm();
-      return docComm.docUrl('attachment') + '?' + encodeQueryParams({
-        ...docComm.getUrlParams(),
-        ident,
-        name: this._attachmentsTable.getValue(rowId, 'fileName')
-      });
-    }
+  private _getUrl(attId: number, cell: SingleCell): string {
+    const docComm = this._getDocComm();
+    return docComm.docUrl('attachment') + '?' + encodeQueryParams({
+      ...docComm.getUrlParams(),
+      ...cell,
+      attId,
+      name: this._attachmentsTable.getValue(attId, 'fileName')
+    });
   }
 
   private async _selectAndSave(value: SavingObservable<number[]>): Promise<void> {

app/client/widgets/FieldEditor.ts

@@ -204,6 +204,7 @@ export class FieldEditor extends Disposable {
       gristDoc: this._gristDoc,
       field: this._field,
       cellValue,
+      rowId: this._editRow.id(),
       formulaError: error,
       editValue,
       cursorPos,

app/client/widgets/FormulaEditor.ts

@@ -292,6 +292,7 @@ export function openFormulaEditor(options: {
   const editor = FormulaEditor.create(holder, {
     gristDoc,
     field,
+    rowId: editRow ? editRow.id() : 0,
     cellValue: column.formula(),
     formulaError: editRow ? getFormulaError(gristDoc, editRow, column) : undefined,
     editValue: options.editValue,

app/client/widgets/NewBaseEditor.ts

@@ -17,6 +17,7 @@ export interface Options {
   gristDoc: GristDoc;
   field: ViewFieldRec;
   cellValue: CellValue;
+  rowId: number;
   formulaError?: Observable<CellValue>;
   editValue?: string;
   cursorPos: number;

app/common/TableData.ts

@@ -22,6 +22,12 @@ interface ColData {
   values: CellValue[];
 }
 
+export interface SingleCell {
+  tableId: string;
+  colId: string;
+  rowId: number;
+}
+
 /**
  * An interface for a table with rows that may be skipped.
  */

app/server/lib/ActiveDoc.ts

@@ -51,13 +51,14 @@ import {
   getUsageRatio,
 } 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';
 import {byteString, countIf, retryOnce, safeJsonParse} from 'app/common/gutil';
 import {InactivityTimer} from 'app/common/InactivityTimer';
 import {schema, SCHEMA_VERSION} from 'app/common/schema';
-import {MetaRowRecord} from 'app/common/TableData';
+import {MetaRowRecord, SingleCell} from 'app/common/TableData';
 import {FetchUrlOptions, UploadResult} from 'app/common/uploads';
 import {DocReplacementOptions, DocState, DocStateComparison} from 'app/common/UserAPI';
 import {convertFromColumn} from 'app/common/ValueConverter';
@@ -659,7 +660,7 @@ export class ActiveDoc extends EventEmitter {
                                                 this._recoveryMode);
 
     await this._actionHistory.initialize();
-    this._granularAccess = new GranularAccess(this.docData, this.docClients, (query) => {
+    this._granularAccess = new GranularAccess(this.docData, this.docStorage, this.docClients, (query) => {
       return this._fetchQueryFromDB(query, false);
     }, this.recoveryMode, this.getHomeDbManager(), this.docName);
     await this._granularAccess.update();
@@ -811,14 +812,12 @@ export class ActiveDoc extends EventEmitter {
    * Returns the record from _grist_Attachments table for the given attachment ID,
    * or throws an error if not found.
    */
-  public getAttachmentMetadata(attId: number|string): MetaRowRecord<'_grist_Attachments'> {
+  public getAttachmentMetadata(attId: number): MetaRowRecord<'_grist_Attachments'> {
     // docData should always be available after loadDoc() or createDoc().
     if (!this.docData) {
       throw new Error("No doc data");
     }
-    // Parse strings into numbers to make more convenient to call from route handlers.
-    const attachmentId: number = (typeof attId === 'string') ? parseInt(attId, 10) : attId;
-    const attRecord = this.docData.getMetaTable('_grist_Attachments').getRecord(attachmentId);
+    const attRecord = this.docData.getMetaTable('_grist_Attachments').getRecord(attId);
     if (!attRecord) {
       throw new ApiError(`Attachment not found: ${attId}`, 404);
     }
@@ -826,16 +825,49 @@ export class ActiveDoc extends EventEmitter {
   }
 
   /**
-   * Given the fileIdent of an attachment, returns a promise for the attachment data.
-   * @param {String} fileIdent: The unique identifier of the attachment (as stored in fileIdent
-   *    field of the _grist_Attachments table).
+   * Given a _gristAttachments record, returns a promise for the attachment data.
    * @returns {Promise<Buffer>} Promise for the data of this attachment; rejected on error.
    */
-  public async getAttachmentData(docSession: OptDocSession, fileIdent: string): Promise<Buffer> {
-    // We don't know for sure whether the attachment is available via a table the user
-    // has access to, but at least they are presenting a SHA1 checksum of the file content,
-    // and they have at least view access to the document to get to this point.  So we go ahead
-    // and serve the attachment.
+  public async getAttachmentData(docSession: OptDocSession, attRecord: MetaRowRecord<"_grist_Attachments">,
+                                 cell?: SingleCell): Promise<Buffer> {
+    const attId = attRecord.id;
+    const fileIdent = attRecord.fileIdent;
+    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.
+      const cells = await this.docStorage.findAttachmentReferences(attId);
+      // Run through them to see if the user has access to any of them.
+      // If so, we'll allow the download. We'd expect in a typical document
+      // this this will be a small list of cells, typically 1 or less, but
+      // of course extreme cases are possible.
+      let goodCell: SingleCell|undefined;
+      for (const possibleCell of cells) {
+        try {
+          await this._granularAccess.assertAttachmentAccess(docSession, possibleCell, attId);
+          goodCell = possibleCell;
+          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); }
     this._log.info(docSession, "getAttachment: %s -> %s bytes", fileIdent, data.length);

app/server/lib/DocApi.ts

@@ -48,6 +48,7 @@ import {
   getScope,
   integerParam,
   isParameterOn,
+  optIntegerParam,
   optStringParam,
   sendOkReply,
   sendReply,
@@ -249,18 +250,27 @@ export class DocWorkerApi {
 
     // Returns cleaned metadata for a given attachment ID (i.e. a rowId in _grist_Attachments table).
     this._app.get('/api/docs/:docId/attachments/:attId', canView, withDoc(async (activeDoc, req, res) => {
-      const attRecord = activeDoc.getAttachmentMetadata(req.params.attId as string);
+      const attId = integerParam(req.params.attId, 'attId');
+      const attRecord = activeDoc.getAttachmentMetadata(attId);
       res.json(cleanAttachmentRecord(attRecord));
     }));
 
     // Responds with attachment contents, with suitable Content-Type and Content-Disposition.
     this._app.get('/api/docs/:docId/attachments/:attId/download', canView, withDoc(async (activeDoc, req, res) => {
-      const attRecord = activeDoc.getAttachmentMetadata(req.params.attId as string);
+      const attId = integerParam(req.params.attId, 'attId');
+      const tableId = optStringParam(req.params.tableId);
+      const colId = optStringParam(req.params.colId);
+      const rowId = optIntegerParam(req.params.rowId);
+      if ((tableId || colId || rowId) && !(tableId && colId && rowId)) {
+        throw new ApiError('define all of tableId, colId and rowId, or none.', 400);
+      }
+      const attRecord = activeDoc.getAttachmentMetadata(attId);
+      const cell = (tableId && colId && rowId) ? {tableId, colId, rowId} : undefined;
       const fileIdent = attRecord.fileIdent as string;
       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), fileIdent);
+      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

@@ -14,6 +14,7 @@ import * as gristTypes from 'app/common/gristTypes';
 import {isList, isListType, isRefListType} from 'app/common/gristTypes';
 import * as marshal from 'app/common/marshal';
 import * as schema from 'app/common/schema';
+import {SingleCell} from 'app/common/TableData';
 import {GristObjCode} from "app/plugin/GristData";
 import {ActionHistoryImpl} from 'app/server/lib/ActionHistoryImpl';
 import {ExpandedQuery} from 'app/server/lib/ExpandedQuery';
@@ -799,6 +800,13 @@ export class DocStorage implements ISQLiteDB, OnDemandStorage {
     return colData.maxId ? colData.maxId + 1 : 1;
   }
 
+  /**
+   * Look up Grist type of column.
+   */
+  public getColumnType(tableId: string, colId: string): string|undefined {
+    return this._docSchema[tableId]?.[colId];
+  }
+
   /**
    * Fetches all rows of the table with the given rowIds.
    */
@@ -1305,6 +1313,32 @@ export class DocStorage implements ISQLiteDB, OnDemandStorage {
     return (await this.all(sql)) as any[];
   }
 
+  /**
+   * Collect all cells that refer to a particular attachment. Ideally this is
+   * something we could use an index for. Regular indexes in SQLite don't help.
+   * FTS5 works, but is somewhat overkill.
+   */
+  public async findAttachmentReferences(attId: number): Promise<Array<SingleCell>> {
+    const queries: string[] = [];
+    // Switch quotes so to insert a table or column name as a string literal
+    // rather than as an identifier.
+    function asLiteral(name: string) {
+      return quoteIdent(name).replace(/"/g, '\'');
+    }
+    for (const [tableId, cols] of Object.entries(this._docSchema)) {
+      for (const [colId, type] of Object.entries(cols)) {
+        if (type !== "Attachments") { continue; }
+        queries.push(`SELECT
+          t.id as rowId,
+          ${asLiteral(tableId)} as tableId,
+          ${asLiteral(colId)} as colId
+        FROM ${quoteIdent(tableId)} AS t, json_each(t.${quoteIdent(colId)}) as a
+        WHERE a.value = ${attId}`);
+      }
+    }
+    return (await this.all(queries.join(' UNION ALL '))) as any[];
+  }
+
   /**
    * Return row IDs of unused attachments in _grist_Attachments.
    * Uses the timeDeleted column which is updated in ActiveDoc.updateUsedAttachmentsIfNeeded.

app/server/lib/DocWorker.ts

@@ -34,7 +34,13 @@ export class DocWorker {
       const docSession = this._getDocSession(stringParam(req.query.clientId, 'clientId'),
                                              integerParam(req.query.docFD, 'docFD'));
       const activeDoc = docSession.activeDoc;
-      const ext = path.extname(stringParam(req.query.ident, 'ident'));
+      const colId = stringParam(req.query.colId, 'colId');
+      const tableId = stringParam(req.query.tableId, 'tableId');
+      const rowId = integerParam(req.query.rowId, 'rowId');
+      const cell = {colId, tableId, rowId};
+      const attId = integerParam(req.query.attId, 'attId');
+      const attRecord = activeDoc.getAttachmentMetadata(attId);
+      const ext = path.extname(attRecord.fileIdent);
       const type = mimeTypes.lookup(ext);
 
       let inline = Boolean(req.query.inline);
@@ -44,7 +50,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, stringParam(req.query.ident, 'ident'));
+      const data = await activeDoc.getAttachmentData(docSession, attRecord, cell);
       res.status(200)
         .type(ext)
         .set('Content-Disposition', contentDispHeader)

app/server/lib/GranularAccess.ts

@@ -21,8 +21,9 @@ import { normalizeEmail } from 'app/common/emails';
 import { ErrorWithCode } from 'app/common/ErrorWithCode';
 import { AclMatchInput, InfoEditor, InfoView } from 'app/common/GranularAccessClause';
 import { UserInfo } from 'app/common/GranularAccessClause';
-import { isCensored } from 'app/common/gristTypes';
+import * as gristTypes from 'app/common/gristTypes';
 import { getSetMapValue, isNonNullish, pruneArray } from 'app/common/gutil';
+import { SingleCell } from 'app/common/TableData';
 import { canEdit, canView, isValidRole, Role } from 'app/common/roles';
 import { FullUser, UserAccessData } from 'app/common/UserAPI';
 import { HomeDBManager } from 'app/gen-server/lib/HomeDBManager';
@@ -31,6 +32,7 @@ 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 log from 'app/server/lib/log';
 import { IPermissionInfo, PermissionInfo, PermissionSetWithContext } from 'app/server/lib/PermissionInfo';
 import { TablePermissionSetWithContext } from 'app/server/lib/PermissionInfo';
@@ -187,6 +189,7 @@ export class GranularAccess implements GranularAccessForBundle {
 
   public constructor(
     private _docData: DocData,
+    private _docStorage: DocStorage,
     private _docClients: DocClients,
     private _fetchQueryFromDB: (query: ServerQuery) => Promise<TableDataAction>,
     private _recoveryMode: boolean,
@@ -239,6 +242,57 @@ export class GranularAccess implements GranularAccessForBundle {
     return this.getReadPermission(pset) !== 'deny';
   }
 
+  /**
+   * Get content of a given cell, if user has read access.
+   * Throws if not.
+   */
+  public async getCellValue(docSession: OptDocSession, cell: SingleCell): Promise<CellValue> {
+    function fail(): never {
+      throw new ErrorWithCode('ACL_DENY', 'Cannot access cell');
+    }
+    const pset = await this.getTableAccess(docSession, cell.tableId);
+    const tableAccess = this.getReadPermission(pset);
+    if (tableAccess === 'deny') { fail(); }
+    const rows = await this._fetchQueryFromDB({
+      tableId: cell.tableId,
+      filters: { id: [cell.rowId] }
+    });
+    if (!rows || rows[2].length === 0) { fail(); }
+    const rec = new RecordView(rows, 0);
+    const input: AclMatchInput = {user: await this._getUser(docSession), rec, newRec: rec};
+    const rowPermInfo = new PermissionInfo(this._ruler.ruleCollection, input);
+    const rowAccess = rowPermInfo.getTableAccess(cell.tableId).perms.read;
+    if (rowAccess === 'deny') { fail(); }
+    if (rowAccess !== 'allow') {
+      const colAccess = rowPermInfo.getColumnAccess(cell.tableId, cell.colId).perms.read;
+      if (colAccess === 'deny') { fail(); }
+    }
+    const colValues = rows[3];
+    if (!(cell.colId in colValues)) { fail(); }
+    return rec.get(cell.colId);
+  }
+
+  /**
+   * Checks whether the specified cell is accessible by the user, and contains
+   * the specified attachment. Throws with ACL_DENY code if not.
+   */
+  public async assertAttachmentAccess(docSession: OptDocSession, cell: SingleCell, attId: number): Promise<void> {
+    const value = await this.getCellValue(docSession, cell);
+
+    // Need to check column is actually an attachment column.
+    if (this._docStorage.getColumnType(cell.tableId, cell.colId) !== 'Attachments') {
+      throw new ErrorWithCode('ACL_DENY', 'not an attachment column');
+    }
+
+    // Check that material in cell includes the attachment.
+    if (!gristTypes.isList(value)) {
+      throw new ErrorWithCode('ACL_DENY', 'not a list');
+    }
+    if (value.indexOf(attId) <= 0) {
+      throw new ErrorWithCode('ACL_DENY', 'attachment not present in cell');
+    }
+  }
+
   /**
    * 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
@@ -1728,7 +1782,7 @@ export class GranularAccess implements GranularAccessForBundle {
       return [filteredAction];
     }
 
-    return filterColValues(filteredAction, (idx) => censoredRows.has(idx), isCensored);
+    return filterColValues(filteredAction, (idx) => censoredRows.has(idx), gristTypes.isCensored);
   }
 
   /**

app/server/lib/requestUtils.ts

@@ -258,8 +258,14 @@ export function stringParam(p: any, name: string, allowed?: string[]): string {
 
 export function integerParam(p: any, name: string): number {
   if (typeof p === 'number') { return Math.floor(p); }
-  if (typeof p === 'string') { return parseInt(p, 10); }
-  throw new Error(`${name} parameter should be an integer: ${p}`);
+  if (typeof p === 'string') {
+    const result = parseInt(p, 10);
+    if (isNaN(result)) {
+      throw new ApiError(`${name} parameter cannot be understood as an integer: ${p}`, 400);
+    }
+    return result;
+  }
+  throw new ApiError(`${name} parameter should be an integer: ${p}`, 400);
 }
 
 export function optIntegerParam(p: any): number|undefined {

test/server/lib/DocApi.ts

@@ -1511,11 +1511,11 @@ function testDocApi() {
       let resp = await axios.get(`${serverUrl}/api/docs/${docIds.TestDoc}/attachments/22`, chimpy);
       checkError(404, /Attachment not found: 22/, resp);
       resp = await axios.get(`${serverUrl}/api/docs/${docIds.TestDoc}/attachments/moo`, chimpy);
-      checkError(404, /Attachment not found: moo/, resp);
+      checkError(400, /parameter cannot be understood as an integer: moo/, resp);
       resp = await axios.get(`${serverUrl}/api/docs/${docIds.TestDoc}/attachments/22/download`, chimpy);
       checkError(404, /Attachment not found: 22/, resp);
       resp = await axios.get(`${serverUrl}/api/docs/${docIds.TestDoc}/attachments/moo/download`, chimpy);
-      checkError(404, /Attachment not found: moo/, resp);
+      checkError(400, /parameter cannot be understood as an integer: moo/, resp);
     });
 
     it("POST /docs/{did}/attachments produces reasonable errors", async function() {