(core) support a wildcard option for ALLOWED_WEBHOOK_DOMAINS

Summary:
Now that webhook payload delivery can be done using a proxy,
it may be desirable to no longer require a set of `ALLOWED_WEBHOOK_DOMAINS`.
This diff allows this variable to be set to `*`. With this setting,
any domain, and both `http` and `https` protocols will now be accepted.

Another possibility would be to default to unchecked
behavior if `ALLOWED_WEBHOOK_DOMAINS` is not set. But this would
introduce a new kind of vulnerability to unconfigured Grist
installations.

Test Plan: switched a test from naming a domain to using `*`

Reviewers: jarek

Reviewed By: jarek

Differential Revision: https://phab.getgrist.com/D3903
This commit is contained in:
Paul Fitzpatrick 2023-05-23 12:51:58 -04:00
parent 4cc19ff748
commit 3f3a0d3aa1
5 changed files with 31 additions and 2 deletions

View File

@ -244,7 +244,7 @@ Grist can be configured in many ways. Here are the main environment variables it
Variable | Purpose Variable | Purpose
-------- | ------- -------- | -------
ALLOWED_WEBHOOK_DOMAINS | comma-separated list of permitted domains to use in webhooks (e.g. webhook.site,zapier.com) 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_URL | doc worker url, set when starting an individual doc worker (other servers will find doc worker urls via redis)
APP_HOME_URL | url prefix for home api (home and doc servers need this) APP_HOME_URL | url prefix for home api (home and doc servers need this)
APP_STATIC_URL | url prefix for static resources APP_STATIC_URL | url prefix for static resources
@ -263,6 +263,7 @@ GRIST_EXPERIMENTAL_PLUGINS | enables experimental plugins
GRIST_HIDE_UI_ELEMENTS | comma-separated list of UI features to disable. Allowed names of parts: `helpCenter,billing,templates,multiSite,multiAccounts,sendToDrive,tutorials`. If a part also exists in GRIST_UI_FEATURES, it will still be disabled. GRIST_HIDE_UI_ELEMENTS | comma-separated list of UI features to disable. Allowed names of parts: `helpCenter,billing,templates,multiSite,multiAccounts,sendToDrive,tutorials`. If a part also exists in GRIST_UI_FEATURES, it will still be disabled.
GRIST_HOME_INCLUDE_STATIC | if set, home server also serves static resources GRIST_HOME_INCLUDE_STATIC | if set, home server also serves static resources
GRIST_HOST | hostname to use when listening on a port. GRIST_HOST | hostname to use when listening on a port.
GRIST_HTTPS_PROXY | if set, use this proxy for webhook payload delivery.
GRIST_ID_PREFIX | for subdomains of form o-*, expect or produce o-${GRIST_ID_PREFIX}*. GRIST_ID_PREFIX | for subdomains of form o-*, expect or produce o-${GRIST_ID_PREFIX}*.
GRIST_IGNORE_SESSION | if set, Grist will not use a session for authentication. GRIST_IGNORE_SESSION | if set, Grist will not use a session for authentication.
GRIST_INST_DIR | path to Grist instance configuration files, for Grist server. GRIST_INST_DIR | path to Grist instance configuration files, for Grist server.

View File

@ -1325,6 +1325,22 @@ export class FlexServer implements GristServer {
} }
} }
public checkOptionCombinations() {
// Check for some bad combinations we should warn about.
const allowedWebhookDomains = appSettings.section('integrations').flag('allowedWebhookDomains').readString({
envVar: 'ALLOWED_WEBHOOK_DOMAINS',
});
const proxy = appSettings.section('integrations').flag('proxy').readString({
envVar: 'GRIST_HTTPS_PROXY',
});
// If all webhook targets are accepted, and no proxy is defined, issue
// a warning. This warning can be removed by explicitly setting the proxy
// to the empty string.
if (allowedWebhookDomains === '*' && proxy === undefined) {
log.warn("Setting an ALLOWED_WEBHOOK_DOMAINS wildcard without a GRIST_HTTPS_PROXY exposes your internal network");
}
}
public async start() { public async start() {
if (this._check('start')) { return; } if (this._check('start')) { return; }

View File

@ -772,6 +772,17 @@ export function isUrlAllowed(urlString: string) {
return false; return false;
} }
// Support at most https and http.
if (url.protocol !== "https:" && url.protocol !== "http:") {
return false;
}
// Support a wildcard that allows all domains.
// Allow either https or http if it is set.
if (process.env.ALLOWED_WEBHOOK_DOMAINS === '*') {
return true;
}
// http (no s) is only allowed for localhost for testing. // http (no s) is only allowed for localhost for testing.
// localhost still needs to be explicitly permitted, and it shouldn't be outside dev // localhost still needs to be explicitly permitted, and it shouldn't be outside dev
if (url.protocol !== "https:" && url.hostname !== "localhost") { if (url.protocol !== "https:" && url.hostname !== "localhost") {

View File

@ -141,6 +141,7 @@ export async function main(port: number, serverTypes: ServerType[],
server.finalize(); server.finalize();
server.checkOptionCombinations();
server.summary(); server.summary();
return server; return server;
} }

View File

@ -20,7 +20,7 @@ describe('WebhookPage', function () {
before(async function () { before(async function () {
oldEnv = new EnvironmentSnapshot(); oldEnv = new EnvironmentSnapshot();
host = new URL(server.getHost()).host; host = new URL(server.getHost()).host;
process.env.ALLOWED_WEBHOOK_DOMAINS = host; process.env.ALLOWED_WEBHOOK_DOMAINS = '*';
await server.restart(); await server.restart();
session = await gu.session().teamSite.login(); session = await gu.session().teamSite.login();
const api = session.createHomeApi(); const api = session.createHomeApi();