From 53bdd6c8e144d0ded293d1ddb9bf77a94955aabf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Mon, 29 Nov 2021 21:12:45 +0100 Subject: [PATCH] (core) Exposing more descriptive errors from exports Summary: Exports used to show generic message on error. Adding error description to the message. Test Plan: Updated tests Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D3157 --- app/gen-server/ApiServer.ts | 18 ++++++++--------- app/gen-server/lib/Housekeeper.ts | 2 +- app/server/lib/DocApi.ts | 14 ++++++------- app/server/lib/DocWorker.ts | 16 +++++++-------- app/server/lib/Export.ts | 33 ++++++++++++++++--------------- app/server/lib/ExportCSV.ts | 33 ++++++++++++------------------- app/server/lib/ExportXLSX.ts | 22 +++++---------------- app/server/lib/FlexServer.ts | 2 +- app/server/lib/GoogleAuth.ts | 16 +++++++-------- app/server/lib/GranularAccess.ts | 2 +- app/server/lib/requestUtils.ts | 10 +++++----- 11 files changed, 75 insertions(+), 93 deletions(-) diff --git a/app/gen-server/ApiServer.ts b/app/gen-server/ApiServer.ts index b871cd0d..6abbb2fd 100644 --- a/app/gen-server/ApiServer.ts +++ b/app/gen-server/ApiServer.ts @@ -53,7 +53,7 @@ function helpScoutSign(email: string): string|undefined { * - If there is no identifier available, a 400 error is thrown. */ export function getOrgKey(req: Request): string|number { - let orgKey: string|null = stringParam(req.params.oid); + let orgKey: string|null = stringParam(req.params.oid, 'oid'); if (orgKey === 'current') { orgKey = getOrgFromRequest(req); } @@ -119,7 +119,7 @@ export class ApiServer { // GET /api/workspace/:wid // Get workspace by id, returning nested documents that user has access to. this._app.get('/api/workspaces/:wid', expressWrap(async (req, res) => { - const wsId = integerParam(req.params.wid); + const wsId = integerParam(req.params.wid, 'wid'); const query = await this._dbManager.getWorkspace(getScope(req), wsId); return sendReply(req, res, query); })); @@ -183,7 +183,7 @@ export class ApiServer { // Body params: name // Update the specified workspace. this._app.patch('/api/workspaces/:wid', expressWrap(async (req, res) => { - const wsId = integerParam(req.params.wid); + const wsId = integerParam(req.params.wid, 'wid'); const query = await this._dbManager.updateWorkspace(getScope(req), wsId, req.body); return sendReply(req, res, query); })); @@ -191,7 +191,7 @@ export class ApiServer { // // DELETE /api/workspaces/:wid // Delete the specified workspace and all included docs. this._app.delete('/api/workspaces/:wid', expressWrap(async (req, res) => { - const wsId = integerParam(req.params.wid); + const wsId = integerParam(req.params.wid, 'wid'); const query = await this._dbManager.deleteWorkspace(getScope(req), wsId); return sendReply(req, res, query); })); @@ -200,7 +200,7 @@ export class ApiServer { // Soft-delete the specified workspace. If query parameter "permanent" is set, // delete permanently. this._app.post('/api/workspaces/:wid/remove', expressWrap(async (req, res) => { - const wsId = integerParam(req.params.wid); + const wsId = integerParam(req.params.wid, 'wid'); if (isParameterOn(req.query.permanent)) { const query = await this._dbManager.deleteWorkspace(getScope(req), wsId); return sendReply(req, res, query); @@ -214,7 +214,7 @@ export class ApiServer { // Recover the specified workspace if it was previously soft-deleted and is // still available. this._app.post('/api/workspaces/:wid/unremove', expressWrap(async (req, res) => { - const wsId = integerParam(req.params.wid); + const wsId = integerParam(req.params.wid, 'wid'); await this._dbManager.undeleteWorkspace(getScope(req), wsId); return sendOkReply(req, res); })); @@ -222,7 +222,7 @@ export class ApiServer { // POST /api/workspaces/:wid/docs // Create a new doc owned by the specific workspace. this._app.post('/api/workspaces/:wid/docs', expressWrap(async (req, res) => { - const wsId = integerParam(req.params.wid); + const wsId = integerParam(req.params.wid, 'wid'); const query = await this._dbManager.addDocument(getScope(req), wsId, req.body); return sendReply(req, res, query); })); @@ -265,7 +265,7 @@ export class ApiServer { // PATCH /api/workspaces/:wid/access // Update the specified workspace acl rules. this._app.patch('/api/workspaces/:wid/access', expressWrap(async (req, res) => { - const workspaceId = integerParam(req.params.wid); + const workspaceId = integerParam(req.params.wid, 'wid'); const delta = req.body.delta; const query = await this._dbManager.updateWorkspacePermissions(getScope(req), workspaceId, delta); return sendReply(req, res, query); @@ -316,7 +316,7 @@ export class ApiServer { // GET /api/workspaces/:wid/access // Get user access information regarding a workspace this._app.get('/api/workspaces/:wid/access', expressWrap(async (req, res) => { - const workspaceId = integerParam(req.params.wid); + const workspaceId = integerParam(req.params.wid, 'wid'); const query = await this._dbManager.getWorkspaceAccess(getScope(req), workspaceId); return sendReply(req, res, query); })); diff --git a/app/gen-server/lib/Housekeeper.ts b/app/gen-server/lib/Housekeeper.ts index fb1aff93..cb32b629 100644 --- a/app/gen-server/lib/Housekeeper.ts +++ b/app/gen-server/lib/Housekeeper.ts @@ -228,7 +228,7 @@ export class Housekeeper { if (userId !== this._dbManager.getSupportUserId()) { throw new ApiError('access denied', 403); } - const docId = stringParam(req.params.docId); + const docId = stringParam(req.params.docId, 'docId'); const permitKey = await this._permitStore.setPermit({docId}); try { const result = await callback(docId, { diff --git a/app/server/lib/DocApi.ts b/app/server/lib/DocApi.ts index cf27ebfb..ee816f28 100644 --- a/app/server/lib/DocApi.ts +++ b/app/server/lib/DocApi.ts @@ -356,8 +356,8 @@ export class DocWorkerApi { // Initiate a fork. Used internally to implement ActiveDoc.fork. Only usable via a Permit. this._app.post('/api/docs/:docId/create-fork', canEdit, throttled(async (req, res) => { - const docId = stringParam(req.params.docId); - const srcDocId = stringParam(req.body.srcDocId); + const docId = stringParam(req.params.docId, 'docId'); + const srcDocId = stringParam(req.body.srcDocId, 'srcDocId'); if (srcDocId !== req.specialPermit?.otherDocId) { throw new Error('access denied'); } const fname = await this._docManager.storageManager.prepareFork(srcDocId, docId); await filterDocumentInPlace(docSessionFromRequest(req), fname); @@ -629,7 +629,7 @@ export class DocWorkerApi { this._app.post('/api/docs/:docId/states/remove', isOwner, withDoc(async (activeDoc, req, res) => { const docSession = docSessionFromRequest(req); - const keep = integerParam(req.body.keep); + const keep = integerParam(req.body.keep, 'keep'); res.json(await activeDoc.deleteActions(docSession, keep)); })); @@ -683,8 +683,8 @@ export class DocWorkerApi { // Give details about what changed between two versions of a document. this._app.get('/api/docs/:docId/compare', canView, withDoc(async (activeDoc, req, res) => { // This could be a relatively slow operation if actions are large. - const left = stringParam(req.query.left || 'HEAD'); - const right = stringParam(req.query.right || 'HEAD'); + const left = stringParam(req.query.left || 'HEAD', 'left'); + const right = stringParam(req.query.right || 'HEAD', 'right'); const docSession = docSessionFromRequest(req); const {states} = await this._getStates(docSession, activeDoc); res.json(await this._getChanges(docSession, activeDoc, states, left, right)); @@ -695,8 +695,8 @@ export class DocWorkerApi { // actual file uploads, so no worries here about large request bodies.) this._app.post('/api/workspaces/:wid/import', expressWrap(async (req, res) => { const userId = getUserId(req); - const wsId = integerParam(req.params.wid); - const uploadId = integerParam(req.body.uploadId); + const wsId = integerParam(req.params.wid, 'wid'); + const uploadId = integerParam(req.body.uploadId, 'uploadId'); const result = await this._docManager.importDocToWorkspace(userId, uploadId, wsId, req.body.browserSettings); res.json(result); })); diff --git a/app/server/lib/DocWorker.ts b/app/server/lib/DocWorker.ts index f4c74f11..868cebd8 100644 --- a/app/server/lib/DocWorker.ts +++ b/app/server/lib/DocWorker.ts @@ -31,10 +31,10 @@ export class DocWorker { public async getAttachment(req: express.Request, res: express.Response): Promise { try { - const docSession = this._getDocSession(stringParam(req.query.clientId), - integerParam(req.query.docFD)); + 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)); + const ext = path.extname(stringParam(req.query.ident, 'ident')); const type = mimeTypes.lookup(ext); let inline = Boolean(req.query.inline); @@ -43,8 +43,8 @@ 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), {type: contentDispType}); - const data = await activeDoc.getAttachmentData(docSession, stringParam(req.query.ident)); + const contentDispHeader = contentDisposition(stringParam(req.query.name, 'name'), {type: contentDispType}); + const data = await activeDoc.getAttachmentData(docSession, stringParam(req.query.ident, 'ident')); res.status(200) .type(ext) .set('Content-Disposition', contentDispHeader) @@ -138,8 +138,8 @@ export class DocWorker { let urlId: string|undefined; try { if (optStringParam(req.query.clientId)) { - const activeDoc = this._getDocSession(stringParam(req.query.clientId), - integerParam(req.query.docFD)).activeDoc; + const activeDoc = this._getDocSession(stringParam(req.query.clientId, 'clientId'), + integerParam(req.query.docFD, 'docFD')).activeDoc; // TODO: The docId should be stored in the ActiveDoc class. Currently docName is // used instead, which will coincide with the docId for hosted grist but not for // standalone grist. @@ -147,7 +147,7 @@ export class DocWorker { } else { // Otherwise, if being used without a client, expect the doc query parameter to // be the docId. - urlId = stringParam(req.query.doc); + urlId = stringParam(req.query.doc, 'doc'); } if (!urlId) { return res.status(403).send({error: 'missing document id'}); } diff --git a/app/server/lib/Export.ts b/app/server/lib/Export.ts index f4313cf4..55c92833 100644 --- a/app/server/lib/Export.ts +++ b/app/server/lib/Export.ts @@ -1,19 +1,20 @@ -import { buildColFilter } from 'app/common/ColumnFilterFunc'; -import { RowRecord } from 'app/common/DocActions'; -import { DocData } from 'app/common/DocData'; -import { DocumentSettings } from 'app/common/DocumentSettings'; +import {ApiError} from 'app/common/ApiError'; +import {buildColFilter} from 'app/common/ColumnFilterFunc'; +import {RowRecord} from 'app/common/DocActions'; +import {DocData} from 'app/common/DocData'; +import {DocumentSettings} from 'app/common/DocumentSettings'; import * as gristTypes from 'app/common/gristTypes'; import * as gutil from 'app/common/gutil'; -import { buildRowFilter } from 'app/common/RowFilterFunc'; -import { SchemaTypes } from 'app/common/schema'; -import { SortFunc } from 'app/common/SortFunc'; -import { Sort } from 'app/common/SortSpec'; -import { TableData } from 'app/common/TableData'; -import { ActiveDoc } from 'app/server/lib/ActiveDoc'; -import { RequestWithLogin } from 'app/server/lib/Authorizer'; -import { docSessionFromRequest } from 'app/server/lib/DocSession'; -import { optIntegerParam, optJsonParam, stringParam } from 'app/server/lib/requestUtils'; -import { ServerColumnGetters } from 'app/server/lib/ServerColumnGetters'; +import {buildRowFilter} from 'app/common/RowFilterFunc'; +import {SchemaTypes} from 'app/common/schema'; +import {SortFunc} from 'app/common/SortFunc'; +import {Sort} from 'app/common/SortSpec'; +import {TableData} from 'app/common/TableData'; +import {ActiveDoc} from 'app/server/lib/ActiveDoc'; +import {RequestWithLogin} from 'app/server/lib/Authorizer'; +import {docSessionFromRequest} from 'app/server/lib/DocSession'; +import {optIntegerParam, optJsonParam, stringParam} from 'app/server/lib/requestUtils'; +import {ServerColumnGetters} from 'app/server/lib/ServerColumnGetters'; import * as express from 'express'; import * as _ from 'underscore'; @@ -82,7 +83,7 @@ export interface ExportParameters { * Gets export parameters from a request. */ export function parseExportParameters(req: express.Request): ExportParameters { - const tableId = stringParam(req.query.tableId); + const tableId = stringParam(req.query.tableId, 'tableId'); const viewSectionId = optIntegerParam(req.query.viewSection); const sortOrder = optJsonParam(req.query.activeSortSpec, []) as number[]; const filters: Filter[] = optJsonParam(req.query.filters, []); @@ -97,7 +98,7 @@ export function parseExportParameters(req: express.Request): ExportParameters { // Makes assertion that value does exists or throws an error function safe(value: T, msg: string) { - if (!value) { throw new Error(msg); } + if (!value) { throw new ApiError(msg, 404); } return value as NonNullable; } diff --git a/app/server/lib/ExportCSV.ts b/app/server/lib/ExportCSV.ts index 789c5162..f13230c4 100644 --- a/app/server/lib/ExportCSV.ts +++ b/app/server/lib/ExportCSV.ts @@ -1,11 +1,12 @@ +import {ApiError} from 'app/common/ApiError'; import {createFormatter} from 'app/common/ValueFormatter'; import {ActiveDoc} from 'app/server/lib/ActiveDoc'; import {ExportData, exportSection, exportTable, Filter} from 'app/server/lib/Export'; +import * as log from 'app/server/lib/log'; import * as bluebird from 'bluebird'; +import * as contentDisposition from 'content-disposition'; import * as csv from 'csv'; import * as express from 'express'; -import * as log from 'app/server/lib/log'; -import * as contentDisposition from 'content-disposition'; export interface DownloadCSVOptions { filename: string; @@ -25,24 +26,12 @@ export async function downloadCSV(activeDoc: ActiveDoc, req: express.Request, res: express.Response, options: DownloadCSVOptions) { log.info('Generating .csv file...'); const {filename, tableId, viewSectionId, filters, sortOrder} = options; - - try { - const data = viewSectionId ? - await makeCSVFromViewSection(activeDoc, viewSectionId, sortOrder, filters, req) : - await makeCSVFromTable(activeDoc, tableId, req); - res.set('Content-Type', 'text/csv'); - res.setHeader('Content-Disposition', contentDisposition(filename + '.csv')); - res.send(data); - } catch (err) { - log.error("Exporting to CSV has failed. Request url: %s", req.url, err); - const errHtml = - ` - - There was an unexpected error while generating a csv file. - -`; - res.status(400).send(errHtml); - } + const data = viewSectionId ? + await makeCSVFromViewSection(activeDoc, viewSectionId, sortOrder, filters, req) : + await makeCSVFromTable(activeDoc, tableId, req); + res.set('Content-Type', 'text/csv'); + res.setHeader('Content-Disposition', contentDisposition(filename + '.csv')); + res.send(data); } /** @@ -88,6 +77,10 @@ export async function makeCSVFromTable( const tables = activeDoc.docData.getTable('_grist_Tables')!; const tableRef = tables.findRow('tableId', tableId); + if (tableRef === 0) { + throw new ApiError(`Table ${tableId} not found.`, 404); + } + const data = await exportTable(activeDoc, tableRef, req); const file = convertToCsv(data); return file; diff --git a/app/server/lib/ExportXLSX.ts b/app/server/lib/ExportXLSX.ts index 8100c858..360afe94 100644 --- a/app/server/lib/ExportXLSX.ts +++ b/app/server/lib/ExportXLSX.ts @@ -16,23 +16,11 @@ export interface DownloadXLSXOptions { export async function downloadXLSX(activeDoc: ActiveDoc, req: express.Request, res: express.Response, {filename}: DownloadXLSXOptions) { log.debug(`Generating .xlsx file`); - try { - const data = await makeXLSX(activeDoc, req); - res.set('Content-Type', 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet'); - res.setHeader('Content-Disposition', contentDisposition(filename + '.xlsx')); - res.send(data); - log.debug('XLSX file generated'); - } catch (err) { - log.error("Exporting to XLSX has failed. Request url: %s", req.url, err); - // send a generic information to client - const errHtml = - ` - - There was an unexpected error while generating a xlsx file. - -`; - res.status(400).send(errHtml); - } + const data = await makeXLSX(activeDoc, req); + res.set('Content-Type', 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet'); + res.setHeader('Content-Disposition', contentDisposition(filename + '.xlsx')); + res.send(data); + log.debug('XLSX file generated'); } /** diff --git a/app/server/lib/FlexServer.ts b/app/server/lib/FlexServer.ts index 310b6b58..2650a3c1 100644 --- a/app/server/lib/FlexServer.ts +++ b/app/server/lib/FlexServer.ts @@ -319,7 +319,7 @@ export class FlexServer implements GristServer { if (this._check('router')) { return; } this.app.get('/test/router', (req, res) => { const act = optStringParam(req.query.act) || 'none'; - const port = stringParam(req.query.port); // port is trusted in mock; in prod it is not. + const port = stringParam(req.query.port, 'port'); // port is trusted in mock; in prod it is not. if (act === 'add' || act === 'remove') { const host = `localhost:${port}`; return res.status(200).json({ diff --git a/app/server/lib/GoogleAuth.ts b/app/server/lib/GoogleAuth.ts index d659504d..3cc0b568 100644 --- a/app/server/lib/GoogleAuth.ts +++ b/app/server/lib/GoogleAuth.ts @@ -97,7 +97,7 @@ export async function googleAuthTokenMiddleware( try { const oAuth2Client = getGoogleAuth(); // Decrypt code that was send back from Google Auth service. Uses GOOGLE_CLIENT_SECRET key. - const tokenResponse = await oAuth2Client.getToken(stringParam(req.query.code)); + const tokenResponse = await oAuth2Client.getToken(stringParam(req.query.code, 'code')); // Get the access token (access token will be present in a default request configuration). const access_token = tokenResponse.tokens.access_token!; req.query.access_token = access_token; @@ -136,20 +136,20 @@ export function addGoogleAuthEndpoint( if (optStringParam(req.query.code)) { log.debug("GoogleAuth - response from Google with valid code"); - messagePage(req, res, { code: stringParam(req.query.code), - origin: stringParam(req.query.state) }); + messagePage(req, res, { code: stringParam(req.query.code, 'code'), + origin: stringParam(req.query.state, 'state') }); } else if (optStringParam(req.query.error)) { - log.debug("GoogleAuth - response from Google with error code", stringParam(req.query.error)); - if (stringParam(req.query.error) === "access_denied") { - messagePage(req, res, { error: stringParam(req.query.error), - origin: stringParam(req.query.state) }); + log.debug("GoogleAuth - response from Google with error code", stringParam(req.query.error, 'error')); + if (stringParam(req.query.error, 'error') === "access_denied") { + messagePage(req, res, { error: stringParam(req.query.error, 'error'), + origin: stringParam(req.query.state, 'state') }); } else { // This should not happen, either code or error is a mandatory query parameter. throw new ApiError("Error authenticating with Google", 500); } } else { const oAuth2Client = getGoogleAuth(); - const scope = stringParam(req.query.scope); + const scope = stringParam(req.query.scope, 'scope'); // Create url for origin parameter for a popup window. const origin = getOriginUrl(req); const authUrl = oAuth2Client.generateAuthUrl({ diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index a19b23c5..4270bbfb 100644 --- a/app/server/lib/GranularAccess.ts +++ b/app/server/lib/GranularAccess.ts @@ -1351,7 +1351,7 @@ export class GranularAccess implements GranularAccessForBundle { // Look up user information in database. if (!this._homeDbManager) { throw new Error('database required'); } const dbUser = linkParameters.aclAsUserId ? - (await this._homeDbManager.getUser(integerParam(linkParameters.aclAsUserId))) : + (await this._homeDbManager.getUser(integerParam(linkParameters.aclAsUserId, 'aclAsUserId'))) : (await this._homeDbManager.getExistingUserByLogin(linkParameters.aclAsUser)); if (!dbUser && linkParameters.aclAsUser) { // Look further for the user, in user attribute tables or examples. diff --git a/app/server/lib/requestUtils.ts b/app/server/lib/requestUtils.ts index 6248f782..f4f132f4 100644 --- a/app/server/lib/requestUtils.ts +++ b/app/server/lib/requestUtils.ts @@ -215,16 +215,16 @@ export function optStringParam(p: any): string|undefined { return undefined; } -export function stringParam(p: any, allowed?: string[]): string { - if (typeof p !== 'string') { throw new Error(`parameter should be a string: ${p}`); } - if (allowed && !allowed.includes(p)) { throw new Error(`parameter ${p} should be one of ${allowed}`); } +export function stringParam(p: any, name: string, allowed?: string[]): string { + if (typeof p !== 'string') { throw new Error(`${name} parameter should be a string: ${p}`); } + if (allowed && !allowed.includes(p)) { throw new Error(`${name} parameter ${p} should be one of ${allowed}`); } return p; } -export function integerParam(p: any): number { +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(`parameter should be an integer: ${p}`); + throw new Error(`${name} parameter should be an integer: ${p}`); } export function optIntegerParam(p: any): number|undefined {