From bfd0fa8c7f07747b975a3943a185aad587743c91 Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Mon, 4 Sep 2023 09:21:18 -0400 Subject: [PATCH] add an endpoint for doing SQL selects (#641) * add an endpoint for doing SQL selects This adds an endpoint for doing SQL selects directly on a Grist document. Other kinds of statements are not supported. There is a default timeout of a second on queries. This follows loosely an API design by Alex Hall. Co-authored-by: jarek --- app/gen-server/lib/DocApiForwarder.ts | 1 + app/plugin/DocApiTypes-ti.ts | 7 ++ app/plugin/DocApiTypes.ts | 13 ++ app/server/lib/ActiveDoc.ts | 5 + app/server/lib/AppSettings.ts | 22 ++++ app/server/lib/DocApi.ts | 100 ++++++++++++++- app/server/lib/DocStorage.ts | 9 ++ app/server/lib/SQLiteDB.ts | 10 +- app/server/lib/SqliteCommon.ts | 32 ++++- app/server/lib/SqliteNode.ts | 13 +- test/server/lib/DocApi.ts | 174 ++++++++++++++++++++++++++ 11 files changed, 382 insertions(+), 4 deletions(-) diff --git a/app/gen-server/lib/DocApiForwarder.ts b/app/gen-server/lib/DocApiForwarder.ts index e825a70e..b29a5f9e 100644 --- a/app/gen-server/lib/DocApiForwarder.ts +++ b/app/gen-server/lib/DocApiForwarder.ts @@ -61,6 +61,7 @@ export class DocApiForwarder { app.use('/api/docs/:docId/webhooks/queue', withDoc); app.use('/api/docs/:docId/webhooks', withDoc); app.use('/api/docs/:docId/assistant', withDoc); + app.use('/api/docs/:docId/sql', withDoc); app.use('^/api/docs$', withoutDoc); } diff --git a/app/plugin/DocApiTypes-ti.ts b/app/plugin/DocApiTypes-ti.ts index 366eb22f..ed2d8a0b 100644 --- a/app/plugin/DocApiTypes-ti.ts +++ b/app/plugin/DocApiTypes-ti.ts @@ -84,6 +84,12 @@ export const TablesPatch = t.iface([], { "tables": t.tuple("RecordWithStringId", t.rest(t.array("RecordWithStringId"))), }); +export const SqlPost = t.iface([], { + "sql": "string", + "args": t.opt(t.array("any")), + "timeout": t.opt("number"), +}); + const exportedTypeSuite: t.ITypeSuite = { NewRecord, NewRecordWithStringId, @@ -101,5 +107,6 @@ const exportedTypeSuite: t.ITypeSuite = { TablePost, TablesPost, TablesPatch, + SqlPost, }; export default exportedTypeSuite; diff --git a/app/plugin/DocApiTypes.ts b/app/plugin/DocApiTypes.ts index 998b635b..ed99c3b3 100644 --- a/app/plugin/DocApiTypes.ts +++ b/app/plugin/DocApiTypes.ts @@ -107,3 +107,16 @@ export interface TablesPost { export interface TablesPatch { tables: [RecordWithStringId, ...RecordWithStringId[]]; // at least one table is required } + +/** + * JSON schema for the body of api /sql POST endpoint + */ +export interface SqlPost { + sql: string; + args?: any[]; // (It would be nice to support named parameters, but + // that feels tricky currently with node-sqlite3) + timeout?: number; // In msecs. Can only be reduced from server default, + // not increased. Note timeout of a query could affect + // other queued queries on same document, because of + // limitations of API node-sqlite3 exposes. +} diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index 34e7f53c..023991c0 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -914,6 +914,11 @@ export class ActiveDoc extends EventEmitter { return this._granularAccess.canCopyEverything(docSession); } + // Check if user has rights to read everything in this doc. + public async canCopyEverything(docSession: OptDocSession) { + return this._granularAccess.canCopyEverything(docSession); + } + // Check if it is appropriate for the user to be treated as an owner of // the document for granular access purposes when in "prefork" mode // (meaning a document has been opened with the intent to fork it, but diff --git a/app/server/lib/AppSettings.ts b/app/server/lib/AppSettings.ts index dbb7ef17..1bf9ccba 100644 --- a/app/server/lib/AppSettings.ts +++ b/app/server/lib/AppSettings.ts @@ -96,6 +96,17 @@ export class AppSettings { return result; } + /** + * As for readInt() but fail if nothing was found. + */ + public requireInt(query: AppSettingQuery): number { + const result = this.readInt(query); + if (result === undefined) { + throw new Error(`missing environment variable: ${query.envVar}`); + } + return result; + } + /** * As for read() but type (and store, and report) the result as * a boolean. @@ -107,6 +118,17 @@ export class AppSettings { return result; } + /** + * As for read() but type (and store, and report) the result as + * an integer (well, a number). + */ + public readInt(query: AppSettingQuery): number|undefined { + this.readString(query); + const result = this.getAsInt(); + this._value = result; + return result; + } + /* set this setting 'manually' */ public set(value: JSONValue): void { this._value = value; diff --git a/app/server/lib/DocApi.ts b/app/server/lib/DocApi.ts index 378243bf..47fda170 100644 --- a/app/server/lib/DocApi.ts +++ b/app/server/lib/DocApi.ts @@ -32,6 +32,7 @@ import { TableOperationsPlatform } from 'app/plugin/TableOperationsImpl'; import {ActiveDoc, colIdToRef as colIdToReference, tableIdToRef} from "app/server/lib/ActiveDoc"; +import {appSettings} from "app/server/lib/AppSettings"; import {sendForCompletion} from 'app/server/lib/Assistance'; import { assertAccess, @@ -96,16 +97,25 @@ const MAX_PARALLEL_REQUESTS_PER_DOC = 10; // then the _dailyUsage cache may become unreliable and users may be able to exceed their allocated requests. const MAX_ACTIVE_DOCS_USAGE_CACHE = 1000; +// Maximum duration of a call to /sql. Does not apply to internal calls to SQLite. +const MAX_CUSTOM_SQL_MSEC = appSettings.section('integrations') + .section('sql').flag('timeout').requireInt({ + envVar: 'GRIST_SQL_TIMEOUT_MSEC', + defaultValue: 1000, + }); + type WithDocHandler = (activeDoc: ActiveDoc, req: RequestWithLogin, resp: Response) => Promise; // Schema validators for api endpoints that creates or updates records. const { RecordsPatch, RecordsPost, RecordsPut, ColumnsPost, ColumnsPatch, ColumnsPut, + SqlPost, TablesPost, TablesPatch, } = t.createCheckers(DocApiTypesTI, GristDataTI); -for (const checker of [RecordsPatch, RecordsPost, RecordsPut, ColumnsPost, ColumnsPatch, TablesPost, TablesPatch]) { +for (const checker of [RecordsPatch, RecordsPost, RecordsPut, ColumnsPost, ColumnsPatch, + SqlPost, TablesPost, TablesPatch]) { checker.setReportedPath("body"); } @@ -518,6 +528,27 @@ export class DocWorkerApi { }) ); + // A GET /sql endpoint that takes a query like ?q=select+*+from+Table1 + // Not very useful, apart from testing - see the POST endpoint for + // serious use. + // If SQL statements that modify the DB are ever supported, they should + // not be permitted by this endpoint. + this._app.get( + '/api/docs/:docId/sql', canView, + withDoc(async (activeDoc, req, res) => { + const sql = stringParam(req.query.q, 'q'); + await this._runSql(activeDoc, req, res, { sql }); + })); + + // A POST /sql endpoint, accepting a body like: + // { "sql": "select * from Table1 where name = ?", "args": ["Paul"] } + // Only SELECT statements are currently supported. + this._app.post( + '/api/docs/:docId/sql', canView, validate(SqlPost), + withDoc(async (activeDoc, req, res) => { + await this._runSql(activeDoc, req, res, req.body); + })); + // Create columns in a table, given as records of the _grist_Tables_column metatable. this._app.post('/api/docs/:docId/tables/:tableId/columns', canEdit, validate(ColumnsPost), withDoc(async (activeDoc, req, res) => { @@ -1502,6 +1533,73 @@ export class DocWorkerApi { await this._dbManager.flushSingleDocAuthCache(scope, docId); await this._docManager.interruptDocClients(docId); } + + private async _runSql(activeDoc: ActiveDoc, req: RequestWithLogin, res: Response, + options: Types.SqlPost) { + if (!await activeDoc.canCopyEverything(docSessionFromRequest(req))) { + throw new ApiError('insufficient document access', 403); + } + const statement = options.sql; + // A very loose test, just for early error message + if (!(statement.toLowerCase().includes('select'))) { + throw new ApiError('only select statements are supported', 400); + } + const sqlOptions = activeDoc.docStorage.getOptions(); + if (!sqlOptions?.canInterrupt || !sqlOptions?.bindableMethodsProcessOneStatement) { + throw new ApiError('The available SQLite wrapper is not adequate', 500); + } + const timeout = + Math.max(0, Math.min(MAX_CUSTOM_SQL_MSEC, + optIntegerParam(options.timeout) || MAX_CUSTOM_SQL_MSEC)); + // Wrap in a select to commit to the SELECT branch of SQLite + // grammar. Note ; isn't a problem. + // + // The underlying SQLite functions used will only process the + // first statement in the supplied text. For node-sqlite3, the + // remainder is placed in a "tail string" ignored by that library. + // So a Robert'); DROP TABLE Students;-- style attack isn't applicable. + // + // Since Grist is used with multiple SQLite wrappers, not just + // node-sqlite3, we have added a bindableMethodsProcessOneStatement + // flag that will need adding for each wrapper, and this endpoint + // will not operate unless that flag is set to true. + // + // The text is wrapped in select * from (USER SUPPLIED TEXT) which + // puts SQLite unconditionally onto the SELECT branch of its + // grammar. It is straightforward to break out of such a wrapper + // with multiple statements, but again, only the first statement + // is processed. + const wrappedStatement = `select * from (${statement})`; + const interrupt = setTimeout(async () => { + await activeDoc.docStorage.interrupt(); + }, timeout); + try { + const records = await activeDoc.docStorage.all(wrappedStatement, + ...(options.args || [])); + res.status(200).json({ + statement, + records: records.map( + rec => ({ + fields: rec, + }) + ), + }); + } catch (e) { + if (e?.code === 'SQLITE_INTERRUPT') { + res.status(400).json({ + error: "a slow statement resulted in a database interrupt", + }); + } else if (e?.code === 'SQLITE_ERROR') { + res.status(400).json({ + error: e?.message, + }); + } else { + throw e; + } + } finally { + clearTimeout(interrupt); + } + } } export function addDocApiRoutes( diff --git a/app/server/lib/DocStorage.ts b/app/server/lib/DocStorage.ts index f5658c2a..584f6cbe 100644 --- a/app/server/lib/DocStorage.ts +++ b/app/server/lib/DocStorage.ts @@ -31,6 +31,7 @@ import {ISQLiteDB, MigrationHooks, OpenMode, PreparedStatement, quoteIdent, import chunk = require('lodash/chunk'); import cloneDeep = require('lodash/cloneDeep'); import groupBy = require('lodash/groupBy'); +import { MinDBOptions } from './SqliteCommon'; // Run with environment variable NODE_DEBUG=db (may include additional comma-separated sections) @@ -1419,6 +1420,14 @@ export class DocStorage implements ISQLiteDB, OnDemandStorage { } } + public interrupt(): Promise { + return this._getDB().interrupt(); + } + + public getOptions(): MinDBOptions|undefined { + return this._getDB().getOptions(); + } + public all(sql: string, ...args: any[]): Promise { return this._getDB().all(sql, ...args); } diff --git a/app/server/lib/SQLiteDB.ts b/app/server/lib/SQLiteDB.ts index 9551309f..deecd2ba 100644 --- a/app/server/lib/SQLiteDB.ts +++ b/app/server/lib/SQLiteDB.ts @@ -72,7 +72,7 @@ import {timeFormat} from 'app/common/timeFormat'; import {create} from 'app/server/lib/create'; import * as docUtils from 'app/server/lib/docUtils'; import log from 'app/server/lib/log'; -import {MinDB, MinRunResult, PreparedStatement, ResultRow, +import {MinDB, MinDBOptions, MinRunResult, PreparedStatement, ResultRow, SqliteVariant, Statement} from 'app/server/lib/SqliteCommon'; import {NodeSqliteVariant} from 'app/server/lib/SqliteNode'; import assert from 'assert'; @@ -258,6 +258,14 @@ export class SQLiteDB implements ISQLiteDB { private constructor(protected _db: MinDB, private _dbPath: string) { } + public async interrupt(): Promise { + return this._db.interrupt?.(); + } + + public getOptions(): MinDBOptions|undefined { + return this._db.getOptions?.(); + } + public async all(sql: string, ...args: any[]): Promise { const result = await this._db.all(sql, ...args); return result; diff --git a/app/server/lib/SqliteCommon.ts b/app/server/lib/SqliteCommon.ts index 4224a55b..b4e3f48c 100644 --- a/app/server/lib/SqliteCommon.ts +++ b/app/server/lib/SqliteCommon.ts @@ -12,20 +12,50 @@ import { OpenMode, quoteIdent } from 'app/server/lib/SQLiteDB'; // eslint-disable-next-line @typescript-eslint/no-empty-interface export interface Statement {} +// Some facts about the wrapper implementation. +export interface MinDBOptions { + // is interruption implemented? + canInterrupt: boolean; + + // Do all methods apart from exec() process at most one + // statement? + bindableMethodsProcessOneStatement: boolean; +} + export interface MinDB { + // This method is expected to be able to handle multiple + // semicolon-separated statements, as for sqlite3_exec: + // https://www.sqlite.org/c3ref/exec.html exec(sql: string): Promise; + + // For all these methods, sql should ultimately be passed + // to sqlite3_prepare_v2 or later, and any tail text ignored after + // the first complete statement, so only the first statement is + // used if there are multiple. + // https://www.sqlite.org/c3ref/prepare.html run(sql: string, ...params: any[]): Promise; get(sql: string, ...params: any[]): Promise; all(sql: string, ...params: any[]): Promise; prepare(sql: string, ...params: any[]): Promise; runAndGetId(sql: string, ...params: any[]): Promise; - close(): Promise; allMarshal(sql: string, ...params: any[]): Promise; + close(): Promise; + /** * Limit the number of ATTACHed databases permitted. */ limitAttach(maxAttach: number): Promise; + + /** + * Stop all current queries. + */ + interrupt?(): Promise; + + /** + * Get some facts about the wrapper. + */ + getOptions?(): MinDBOptions; } export interface MinRunResult { diff --git a/app/server/lib/SqliteNode.ts b/app/server/lib/SqliteNode.ts index dd9858d8..4f7d071e 100644 --- a/app/server/lib/SqliteNode.ts +++ b/app/server/lib/SqliteNode.ts @@ -1,6 +1,6 @@ import * as sqlite3 from '@gristlabs/sqlite3'; import { fromCallback } from 'app/server/lib/serverUtils'; -import { MinDB, PreparedStatement, ResultRow, SqliteVariant } from 'app/server/lib/SqliteCommon'; +import { MinDB, MinDBOptions, PreparedStatement, ResultRow, SqliteVariant } from 'app/server/lib/SqliteCommon'; import { OpenMode, RunResult } from 'app/server/lib/SQLiteDB'; export class NodeSqliteVariant implements SqliteVariant { @@ -84,6 +84,17 @@ export class NodeSqlite3DatabaseAdapter implements MinDB { this._db.close(); } + public async interrupt(): Promise { + this._db.interrupt(); + } + + public getOptions(): MinDBOptions { + return { + canInterrupt: true, + bindableMethodsProcessOneStatement: true, + }; + } + public async allMarshal(sql: string, ...params: any[]): Promise { // allMarshal isn't in the typings, because it is our addition to our fork of sqlite3 JS lib. return fromCallback(cb => (this._db as any).allMarshal(sql, ...params, cb)); diff --git a/test/server/lib/DocApi.ts b/test/server/lib/DocApi.ts index 4116a631..036bed68 100644 --- a/test/server/lib/DocApi.ts +++ b/test/server/lib/DocApi.ts @@ -874,6 +874,15 @@ function testDocApi() { resp = await axios.post(`${serverUrl}/api/docs/${docIds.Timesheets}/tables/_grist_Tables/data/delete`, [2, 3, 4, 5, 6], chimpy); assert.equal(resp.status, 200); + + // Despite deleting tables (even in a more official way than above), + // there are rules lingering relating to them. TODO: look into this. + resp = await axios.post(`${serverUrl}/api/docs/${docIds.Timesheets}/tables/_grist_ACLRules/data/delete`, + [2, 3], chimpy); + assert.equal(resp.status, 200); + resp = await axios.post(`${serverUrl}/api/docs/${docIds.Timesheets}/tables/_grist_ACLResources/data/delete`, + [2, 3], chimpy); + assert.equal(resp.status, 200); }); describe("/docs/{did}/tables/{tid}/columns", function () { @@ -3174,6 +3183,7 @@ function testDocApi() { users: {"kiwi@getgrist.com": 'editors' as string | null} }; let accessResp = await axios.patch(`${homeUrl}/api/docs/${docIds.Timesheets}/access`, {delta}, chimpy); + await flushAuth(); assert.equal(accessResp.status, 200); const check = userCheck.bind(null, kiwi); @@ -3198,6 +3208,7 @@ function testDocApi() { delta.users['kiwi@getgrist.com'] = null; accessResp = await axios.patch(`${homeUrl}/api/docs/${docIds.Timesheets}/access`, {delta}, chimpy); assert.equal(accessResp.status, 200); + await flushAuth(); }); it("DELETE /docs/{did}/tables/webhooks should not be allowed for not-owner", async function () { @@ -3210,6 +3221,7 @@ function testDocApi() { }; let accessResp = await axios.patch(`${homeUrl}/api/docs/${docIds.Timesheets}/access`, {delta}, chimpy); assert.equal(accessResp.status, 200); + await flushAuth(); // Actually unsubscribe with the same unsubscribeKey that was returned by registration - it shouldn't be accepted await check(subscribeResponse.webhookId, 403, /No owner access/); @@ -3219,6 +3231,7 @@ function testDocApi() { delta.users['kiwi@getgrist.com'] = null; accessResp = await axios.patch(`${homeUrl}/api/docs/${docIds.Timesheets}/access`, {delta}, chimpy); assert.equal(accessResp.status, 200); + await flushAuth(); }); }); @@ -4502,6 +4515,160 @@ function testDocApi() { }); + + it ("GET /docs/{did}/sql is functional", async function () { + const query = 'select+*+from+Table1+order+by+id'; + const resp = await axios.get(`${homeUrl}/api/docs/${docIds.Timesheets}/sql?q=${query}`, chimpy); + assert.equal(resp.status, 200); + assert.deepEqual(resp.data, { + statement: 'select * from Table1 order by id', + records: [ + { + fields: { + id: 1, + manualSort: 1, + A: 'hello', + B: '', + C: '', + D: null, + E: 'HELLO' + }, + }, + { + fields: { id: 2, manualSort: 2, A: '', B: 'world', C: '', D: null, E: '' } + }, + { + fields: { id: 3, manualSort: 3, A: '', B: '', C: '', D: null, E: '' } + }, + { + fields: { id: 4, manualSort: 4, A: '', B: '', C: '', D: null, E: '' } + }, + ] + }); + }); + + it ("POST /docs/{did}/sql is functional", async function () { + let resp = await axios.post( + `${homeUrl}/api/docs/${docIds.Timesheets}/sql`, + { sql: "select A from Table1 where id = ?", args: [ 1 ] }, + chimpy); + assert.equal(resp.status, 200); + assert.deepEqual(resp.data.records, [{ + fields: { + A: 'hello', + } + }]); + + resp = await axios.post( + `${homeUrl}/api/docs/${docIds.Timesheets}/sql`, + { nosql: "select A from Table1 where id = ?", args: [ 1 ] }, + chimpy); + assert.equal(resp.status, 400); + assert.deepEqual(resp.data, { + error: 'Invalid payload', + details: { userError: 'Error: body.sql is missing' } + }); + }); + + it ("POST /docs/{did}/sql has access control", async function () { + // Check non-viewer doesn't have access. + const url = `${homeUrl}/api/docs/${docIds.Timesheets}/sql`; + const query = { sql: "select A from Table1 where id = ?", args: [ 1 ] }; + let resp = await axios.post(url, query, kiwi); + assert.equal(resp.status, 403); + assert.deepEqual(resp.data, { + error: 'No view access', + }); + + try { + // Check a viewer would have access. + const delta = { + users: { 'kiwi@getgrist.com': 'viewers' }, + }; + await axios.patch(`${homeUrl}/api/docs/${docIds.Timesheets}/access`, {delta}, chimpy); + await flushAuth(); + resp = await axios.post(url, query, kiwi); + assert.equal(resp.status, 200); + + // Check a viewer would not have access if there is some private material. + await axios.post( + `${homeUrl}/api/docs/${docIds.Timesheets}/apply`, [ + ['AddTable', 'TablePrivate', [{id: 'A', type: 'Int'}]], + ['AddRecord', '_grist_ACLResources', -1, {tableId: 'TablePrivate', colIds: '*'}], + ['AddRecord', '_grist_ACLRules', null, { + resource: -1, aclFormula: '', permissionsText: 'none', + }], + ], chimpy); + resp = await axios.post(url, query, kiwi); + assert.equal(resp.status, 403); + } finally { + // Remove extra viewer; remove extra table. + const delta = { + users: { 'kiwi@getgrist.com': null }, + }; + await axios.patch(`${homeUrl}/api/docs/${docIds.Timesheets}/access`, {delta}, chimpy); + await flushAuth(); + await axios.post( + `${homeUrl}/api/docs/${docIds.Timesheets}/apply`, [ + ['RemoveTable', 'TablePrivate'], + ], chimpy); + } + }); + + it ("POST /docs/{did}/sql accepts only selects", async function () { + async function check(accept: boolean, sql: string, ...args: any[]) { + const resp = await axios.post( + `${homeUrl}/api/docs/${docIds.Timesheets}/sql`, + { sql, args }, + chimpy); + if (accept) { + assert.equal(resp.status, 200); + } else { + assert.equal(resp.status, 400); + } + return resp.data; + } + await check(true, 'select * from Table1'); + await check(true, ' SeLeCT * from Table1'); + await check(true, 'with results as (select 1) select * from results'); + + // rejected quickly since no select + await check(false, 'delete from Table1'); + await check(false, ''); + + // rejected because deletes/updates/... can't be nested within a select + await check(false, "delete from Table1 where id in (select id from Table1) and 'selecty' = 'selecty'"); + await check(false, "update Table1 set A = ? where 'selecty' = 'selecty'", 'test'); + await check(false, "pragma data_store_directory = 'selecty'"); + await check(false, "create table selecty(x, y)"); + await check(false, "attach database 'selecty' AS test"); + + // rejected because ";" can't be nested + await check(false, 'select * from Table1; delete from Table1'); + + // Of course, we can get out of the wrapping select, but we can't + // add on more statements. For example, the following runs with no + // trouble - but only the SELECT part. The DELETE is discarded. + // (node-sqlite3 doesn't expose enough to give an error message for + // this, though we could extend it). + await check(true, 'select * from Table1); delete from Table1 where id in (select id from Table1'); + const {records} = await check(true, 'select * from Table1'); + // Double-check the deletion didn't happen. + assert.lengthOf(records, 4); + }); + + it ("POST /docs/{did}/sql timeout is effective", async function () { + const slowQuery = 'WITH RECURSIVE r(i) AS (VALUES(0) ' + + 'UNION ALL SELECT i FROM r LIMIT 1000000) ' + + 'SELECT i FROM r WHERE i = 1'; + const resp = await axios.post( + `${homeUrl}/api/docs/${docIds.Timesheets}/sql`, + { sql: slowQuery, timeout: 10 }, + chimpy); + assert.equal(resp.status, 400); + assert.match(resp.data.error, /database interrupt/); + }); + // PLEASE ADD MORE TESTS HERE } @@ -4558,3 +4725,10 @@ async function setupDataDir(dir: string) { 'ApiDataRecordsTest.grist', path.resolve(dir, docIds.ApiDataRecordsTest + '.grist')); } + +// The access control level of a user on a document may be cached for a +// few seconds. This method flushes that cache. +async function flushAuth() { + await home.testingHooks.flushAuthorizerCache(); + await docs.testingHooks.flushAuthorizerCache(); +}