From b1cbc0ed16f82a2238d2cd56616824469259b4b0 Mon Sep 17 00:00:00 2001 From: vviers Date: Mon, 14 Oct 2024 16:46:12 +0200 Subject: [PATCH 1/7] DocDrivenDevelopment: document the new env variables --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index a1b97910c4..9b350fe47a 100644 --- a/README.md +++ b/README.md @@ -254,6 +254,8 @@ Grist can be configured in many ways. Here are the main environment variables it | Variable | Purpose | |------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| ACTION_HISTORY_MAX_ROWS | Maximum number of rows allowed in ActionHistory before pruning (up to a 1.25 grace factor). Defaults to 1000. | +| ACTION_HISTORY_MAX_BYTES | Maximum number of rows allowed in ActionHistory before pruning (up to a 1.25 grace factor). Defaults to 1.25Gb. | | ALLOWED_WEBHOOK_DOMAINS | comma-separated list of permitted domains to use in webhooks (e.g. webhook.site,zapier.com). You can set this to `*` to allow all domains, but if doing so, we recommend using a carefully locked-down proxy (see `GRIST_HTTPS_PROXY`) if you do not entirely trust users. Otherwise services on your internal network may become vulnerable to manipulation. | | APP_DOC_URL | doc worker url, set when starting an individual doc worker (other servers will find doc worker urls via redis) | | APP_DOC_INTERNAL_URL | like `APP_DOC_URL` but used by the home server to reach the server using an internal domain name resolution (like in a docker environment). It only makes sense to define this value in the doc worker. Defaults to `APP_DOC_URL`. | From 4f72f1fbb02cd44ea79164e2ebd5932328ca658a Mon Sep 17 00:00:00 2001 From: vviers Date: Mon, 14 Oct 2024 16:47:15 +0200 Subject: [PATCH 2/7] Introduce ACTION_HISTORY env variables --- app/server/lib/ActionHistoryImpl.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/server/lib/ActionHistoryImpl.ts b/app/server/lib/ActionHistoryImpl.ts index 3930d864ec..4dfef89dbb 100644 --- a/app/server/lib/ActionHistoryImpl.ts +++ b/app/server/lib/ActionHistoryImpl.ts @@ -17,9 +17,9 @@ import {ISQLiteDB, ResultRow} from './SQLiteDB'; // on rows and the maximum total number of bytes in the "body" column. // Pruning is done when the history has grown above these limits, to // the specified factor. -const ACTION_HISTORY_MAX_ROWS = 1000; -const ACTION_HISTORY_MAX_BYTES = 1000 * 1000 * 1000; // 1 GB. -const ACTION_HISTORY_GRACE_FACTOR = 1.25; // allow growth to 1250 rows / 1.25 GB. +const ACTION_HISTORY_MAX_ROWS = process.env.ACTION_HISTORY_MAX_ROWS || 1000; +const ACTION_HISTORY_MAX_BYTES = process.env.ACTION_HISTORY_MAX_BYTES || 1000 * 1000 * 1000; // 1 GB. +const ACTION_HISTORY_GRACE_FACTOR = 1.25; // allow growth to 1.25 times the above limits. const ACTION_HISTORY_CHECK_PERIOD = 10; // number of actions between size checks. /** From f0cae5fe32d860df9d394cfbc28fa4f52875a1db Mon Sep 17 00:00:00 2001 From: vviers Date: Mon, 14 Oct 2024 17:02:09 +0200 Subject: [PATCH 3/7] fix: env vars must be integers --- app/server/lib/ActionHistoryImpl.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/server/lib/ActionHistoryImpl.ts b/app/server/lib/ActionHistoryImpl.ts index 4dfef89dbb..fdd0ee48d7 100644 --- a/app/server/lib/ActionHistoryImpl.ts +++ b/app/server/lib/ActionHistoryImpl.ts @@ -17,8 +17,8 @@ import {ISQLiteDB, ResultRow} from './SQLiteDB'; // on rows and the maximum total number of bytes in the "body" column. // Pruning is done when the history has grown above these limits, to // the specified factor. -const ACTION_HISTORY_MAX_ROWS = process.env.ACTION_HISTORY_MAX_ROWS || 1000; -const ACTION_HISTORY_MAX_BYTES = process.env.ACTION_HISTORY_MAX_BYTES || 1000 * 1000 * 1000; // 1 GB. +const ACTION_HISTORY_MAX_ROWS = parseInt(process.env.ACTION_HISTORY_MAX_ROWS, 10) || 1000; +const ACTION_HISTORY_MAX_BYTES = parseInt(process.env.ACTION_HISTORY_MAX_BYTES, 10) || 1000 * 1000 * 1000; // 1 GB. const ACTION_HISTORY_GRACE_FACTOR = 1.25; // allow growth to 1.25 times the above limits. const ACTION_HISTORY_CHECK_PERIOD = 10; // number of actions between size checks. From 07adb83ddd935b529a4062be2c431be1965b3fb8 Mon Sep 17 00:00:00 2001 From: fflorent Date: Tue, 15 Oct 2024 12:17:54 +0200 Subject: [PATCH 4/7] Use AppSettings and introduce minValue and maxValue AppSettings seems more future-proof and already raises error when passed value is not a number. We also introduce minValue and maxValue so we can constraint the configured value to be within a certain range and avoid erratic behavior when the value has not been expected. --- README.md | 4 +-- app/server/lib/ActionHistoryImpl.ts | 18 ++++++++-- app/server/lib/AppSettings.ts | 52 +++++++++++++++++++++++++---- app/server/lib/OIDCConfig.ts | 1 + 4 files changed, 64 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 9b350fe47a..ab1bc7ea7a 100644 --- a/README.md +++ b/README.md @@ -254,8 +254,6 @@ Grist can be configured in many ways. Here are the main environment variables it | Variable | Purpose | |------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| ACTION_HISTORY_MAX_ROWS | Maximum number of rows allowed in ActionHistory before pruning (up to a 1.25 grace factor). Defaults to 1000. | -| ACTION_HISTORY_MAX_BYTES | Maximum number of rows allowed in ActionHistory before pruning (up to a 1.25 grace factor). Defaults to 1.25Gb. | | ALLOWED_WEBHOOK_DOMAINS | comma-separated list of permitted domains to use in webhooks (e.g. webhook.site,zapier.com). You can set this to `*` to allow all domains, but if doing so, we recommend using a carefully locked-down proxy (see `GRIST_HTTPS_PROXY`) if you do not entirely trust users. Otherwise services on your internal network may become vulnerable to manipulation. | | APP_DOC_URL | doc worker url, set when starting an individual doc worker (other servers will find doc worker urls via redis) | | APP_DOC_INTERNAL_URL | like `APP_DOC_URL` but used by the home server to reach the server using an internal domain name resolution (like in a docker environment). It only makes sense to define this value in the doc worker. Defaults to `APP_DOC_URL`. | @@ -264,6 +262,8 @@ Grist can be configured in many ways. Here are the main environment variables it | APP_STATIC_URL | url prefix for static resources | | APP_STATIC_INCLUDE_CUSTOM_CSS | set to "true" to include custom.css (from APP_STATIC_URL) in static pages | | APP_UNTRUSTED_URL | URL at which to serve/expect plugin content. | +| GRIST_ACTION_HISTORY_MAX_ROWS | Maximum number of rows allowed in ActionHistory before pruning (up to a 1.25 grace factor). Defaults to 1000. | +| GRIST_ACTION_HISTORY_MAX_BYTES | Maximum number of rows allowed in ActionHistory before pruning (up to a 1.25 grace factor). Defaults to 1.25Gb. | | GRIST_ADAPT_DOMAIN | set to "true" to support multiple base domains (careful, host header should be trustworthy) | | GRIST_APP_ROOT | directory containing Grist sandbox and assets (specifically the sandbox and static subdirectories). | | GRIST_BACKUP_DELAY_SECS | wait this long after a doc change before making a backup | diff --git a/app/server/lib/ActionHistoryImpl.ts b/app/server/lib/ActionHistoryImpl.ts index fdd0ee48d7..dcd2ce5494 100644 --- a/app/server/lib/ActionHistoryImpl.ts +++ b/app/server/lib/ActionHistoryImpl.ts @@ -12,13 +12,27 @@ import mapValues = require('lodash/mapValues'); import {ActionGroupOptions, ActionHistory, ActionHistoryUndoInfo, asActionGroup, asMinimalActionGroup} from './ActionHistory'; import {ISQLiteDB, ResultRow} from './SQLiteDB'; +import { appSettings } from './AppSettings'; + +const section = appSettings.section('history').section('action'); // History will from time to time be pruned back to within these limits // on rows and the maximum total number of bytes in the "body" column. // Pruning is done when the history has grown above these limits, to // the specified factor. -const ACTION_HISTORY_MAX_ROWS = parseInt(process.env.ACTION_HISTORY_MAX_ROWS, 10) || 1000; -const ACTION_HISTORY_MAX_BYTES = parseInt(process.env.ACTION_HISTORY_MAX_BYTES, 10) || 1000 * 1000 * 1000; // 1 GB. +const ACTION_HISTORY_MAX_ROWS = section.flag('maxRows').requireInt({ + envVar: 'GRIST_ACTION_HISTORY_MAX_ROWS', + defaultValue: 1000, + + minValue: 1, +}); + +const ACTION_HISTORY_MAX_BYTES = section.flag('maxBytes').requireInt({ + envVar: 'GRIST_ACTION_HISTORY_MAX_BYTES', + defaultValue: 1_000_0000_0000, // 1 GB. + minValue: 1, // 1 B. +}); + const ACTION_HISTORY_GRACE_FACTOR = 1.25; // allow growth to 1.25 times the above limits. const ACTION_HISTORY_CHECK_PERIOD = 10; // number of actions between size checks. diff --git a/app/server/lib/AppSettings.ts b/app/server/lib/AppSettings.ts index 1bf9ccba53..42908f7bf0 100644 --- a/app/server/lib/AppSettings.ts +++ b/app/server/lib/AppSettings.ts @@ -99,7 +99,7 @@ export class AppSettings { /** * As for readInt() but fail if nothing was found. */ - public requireInt(query: AppSettingQuery): number { + public requireInt(query: AppSettingQueryInt): number { const result = this.readInt(query); if (result === undefined) { throw new Error(`missing environment variable: ${query.envVar}`); @@ -122,9 +122,19 @@ export class AppSettings { * As for read() but type (and store, and report) the result as * an integer (well, a number). */ - public readInt(query: AppSettingQuery): number|undefined { + public readInt(query: AppSettingQueryInt): number|undefined { this.readString(query); const result = this.getAsInt(); + + if (result !== undefined) { + if (query.minValue !== undefined && result < query.minValue) { + throw new Error(`value ${result} is less than minimum ${query.minValue}`); + } + if (query.maxValue !== undefined && result > query.maxValue) { + throw new Error(`value ${result} is greater than maximum ${query.maxValue}`); + } + } + this._value = result; return result; } @@ -213,11 +223,39 @@ export const appSettings = new AppSettings('grist'); * environment variables and default values. */ export interface AppSettingQuery { - envVar: string|string[]; // environment variable(s) to check. - preferredEnvVar?: string; // "Canonical" environment variable to suggest. - // Should be in envVar (though this is not checked). - defaultValue?: JSONValue; // value to use if variable(s) unavailable. - censor?: boolean; // should the value of the setting be obscured when printed. + /** + * Environment variable(s) to check. + */ + envVar: string|string[]; + /** + * "Canonical" environment variable to suggest. Should be in envVar (though this is not checked). + */ + preferredEnvVar?: string; + /** + * Value to use if the variable(s) is/are unavailable. + */ + defaultValue?: JSONValue; + /** + * When set to true, the value is obscured when printed. + */ + censor?: boolean; +} + +export interface AppSettingQueryInt extends AppSettingQuery { + /** + * Value to use if variable(s) unavailable. + */ + defaultValue?: number; + /** + * Minimum value allowed. Raises an error if the value is lower than this. + * If the value is undefined, the setting is not checked. + */ + minValue?: number; + /** + * Maximum value allowed. Raises an error if the value is greater than this. + * If the value is undefined, the setting is not checked. + */ + maxValue?: number; } /** diff --git a/app/server/lib/OIDCConfig.ts b/app/server/lib/OIDCConfig.ts index 2343f468a1..9e500d6c4f 100644 --- a/app/server/lib/OIDCConfig.ts +++ b/app/server/lib/OIDCConfig.ts @@ -141,6 +141,7 @@ export class OIDCConfig { }); const httpTimeout = section.flag('httpTimeout').readInt({ envVar: 'GRIST_OIDC_SP_HTTP_TIMEOUT', + minValue: 0, // 0 means no timeout }); this._namePropertyKey = section.flag('namePropertyKey').readString({ envVar: 'GRIST_OIDC_SP_PROFILE_NAME_ATTR', From ffa260943fc5740273c9452b4933ac2443fc27b9 Mon Sep 17 00:00:00 2001 From: Florent Date: Tue, 15 Oct 2024 17:20:01 +0200 Subject: [PATCH 5/7] Update README.md Co-authored-by: Vincent Viers <30295971+vviers@users.noreply.github.com> --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index ab1bc7ea7a..392242a9f6 100644 --- a/README.md +++ b/README.md @@ -262,8 +262,8 @@ Grist can be configured in many ways. Here are the main environment variables it | APP_STATIC_URL | url prefix for static resources | | APP_STATIC_INCLUDE_CUSTOM_CSS | set to "true" to include custom.css (from APP_STATIC_URL) in static pages | | APP_UNTRUSTED_URL | URL at which to serve/expect plugin content. | -| GRIST_ACTION_HISTORY_MAX_ROWS | Maximum number of rows allowed in ActionHistory before pruning (up to a 1.25 grace factor). Defaults to 1000. | -| GRIST_ACTION_HISTORY_MAX_BYTES | Maximum number of rows allowed in ActionHistory before pruning (up to a 1.25 grace factor). Defaults to 1.25Gb. | +| GRIST_ACTION_HISTORY_MAX_ROWS | Maximum number of rows allowed in ActionHistory before pruning (up to a 1.25 grace factor). Defaults to 1000. ⚠️ A too low value may make the "[Work on a copy](https://support.getgrist.com/newsletters/2021-06/#work-on-a-copy)" feature [malfunction](https://github.com/gristlabs/grist-core/issues/1121#issuecomment-2248112023) | +| GRIST_ACTION_HISTORY_MAX_BYTES | Maximum number of rows allowed in ActionHistory before pruning (up to a 1.25 grace factor). Defaults to 1Gb. ⚠️ A too low value may make the "[Work on a copy](https://support.getgrist.com/newsletters/2021-06/#work-on-a-copy)" feature [malfunction](https://github.com/gristlabs/grist-core/issues/1121#issuecomment-2248112023) | | GRIST_ADAPT_DOMAIN | set to "true" to support multiple base domains (careful, host header should be trustworthy) | | GRIST_APP_ROOT | directory containing Grist sandbox and assets (specifically the sandbox and static subdirectories). | | GRIST_BACKUP_DELAY_SECS | wait this long after a doc change before making a backup | From 9c11572ff308ab14e80e94eeae77d885c9f84806 Mon Sep 17 00:00:00 2001 From: fflorent Date: Wed, 16 Oct 2024 10:34:50 +0200 Subject: [PATCH 6/7] Start testing AppSettings --- test/server/lib/AppSettings.ts | 96 ++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 test/server/lib/AppSettings.ts diff --git a/test/server/lib/AppSettings.ts b/test/server/lib/AppSettings.ts new file mode 100644 index 0000000000..77ab06d7f2 --- /dev/null +++ b/test/server/lib/AppSettings.ts @@ -0,0 +1,96 @@ +import { AppSettings } from 'app/server/lib/AppSettings'; +import { EnvironmentSnapshot } from '../testUtils'; + +import { assert } from 'chai'; + +describe('AppSettings', () => { + let appSettings: AppSettings; + let env: EnvironmentSnapshot; + beforeEach(() => { + appSettings = new AppSettings('test'); + env = new EnvironmentSnapshot(); + }); + + afterEach(() => { + env.restore(); + }); + + describe('for integers', () => { + function testIntMethod(method: 'readInt' | 'requireInt') { + it('should throw an error if the value is less than the minimum', () => { + process.env.TEST = '4'; + assert.throws(() => { + appSettings[method]({ envVar: 'TEST', minValue: 5 }); + }, 'value 4 is less than minimum 5'); + }); + + it('should throw an error if the value is greater than the maximum', () => { + process.env.TEST = '6'; + assert.throws(() => { + appSettings[method]({ envVar: 'TEST', maxValue: 5 }); + }, 'value 6 is greater than maximum 5'); + }); + + it('should throw if the value is NaN', () => { + process.env.TEST = 'not a number'; + assert.throws(() => appSettings[method]({ envVar: 'TEST' }), 'not a number does not look like a number'); + }); + + it('should throw if the default value is not finite', () => { + assert.throws( + () => appSettings[method]({ envVar: 'TEST', defaultValue: Infinity }), + 'Infinity does not look like a number' + ); + }); + + it('should throw if the default value is not within the range', () => { + assert.throws( + () => appSettings[method]({ + envVar: 'TEST', + defaultValue: 6, + minValue: 7, + maxValue: 9, + }), + 'value 6 is less than minimum 7' + ); + }); + + it('should return the default value if it is within the range', () => { + const result = appSettings[method]({ + envVar: 'TEST', + defaultValue: 5, + minValue: 5, + maxValue: 12 + }); + assert.strictEqual(result, 5); + }); + + it('should return the value if it is within the range', () => { + process.env.TEST = '5'; + assert.strictEqual(appSettings[method]({ envVar: 'TEST', minValue: 5 }), 5); + }); + + it('should return the integer value of a float', () => { + process.env.TEST = '5.9'; + assert.strictEqual(appSettings[method]({ envVar: 'TEST' }), 5); + }); + } + + describe('readInt()', () => { + testIntMethod('readInt'); + + it('should return undefined when no value nor default value is passed', () => { + const result = appSettings.readInt({ envVar: 'TEST', maxValue: 5 }); + assert.isUndefined(result); + }); + }); + + describe('requireInt()', () => { + testIntMethod('requireInt'); + + it('should throw if env variable is not set and no default value is passed', () => { + assert.throws(() => appSettings.requireInt({ envVar: 'TEST' }), 'missing environment variable: TEST'); + }); + }); + }); +}); From 5f091ef876758aa95065fa24486794b1b82d5c84 Mon Sep 17 00:00:00 2001 From: fflorent Date: Wed, 16 Oct 2024 11:59:23 +0200 Subject: [PATCH 7/7] Fix default value --- app/server/lib/ActionHistoryImpl.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/server/lib/ActionHistoryImpl.ts b/app/server/lib/ActionHistoryImpl.ts index dcd2ce5494..b1e2c83c02 100644 --- a/app/server/lib/ActionHistoryImpl.ts +++ b/app/server/lib/ActionHistoryImpl.ts @@ -29,7 +29,7 @@ const ACTION_HISTORY_MAX_ROWS = section.flag('maxRows').requireInt({ const ACTION_HISTORY_MAX_BYTES = section.flag('maxBytes').requireInt({ envVar: 'GRIST_ACTION_HISTORY_MAX_BYTES', - defaultValue: 1_000_0000_0000, // 1 GB. + defaultValue: 1e9, // 1 GB. minValue: 1, // 1 B. });