Skip to content

Commit

Permalink
Introduce APP_HOME_INTERNAL_URL and fix duplicate docs (#915)
Browse files Browse the repository at this point in the history 
Context:

On self-hosted instances, some places in the code rely on the fact that we resolves public domains while being behind reverse proxies. This leads to cases where features are not available, such as the "Duplicate document" one.

Bugs that are solved - n self-hosted instances:

Impossible to open templates and tutorials right after having converted them;
Impossible to submit forms since version 1.1.13;
Impossible to restore a previous version of a document (snapshot);
Impossible to copy a document;

Solution:

Introduce the APP_HOME_INTERNAL_URL env variable, which is quite the same as APP_DOC_INTERNAL_URL except that it may point to any home worker;
Make /api/worker/:assignmentId([^/]+)/?* return not only the doc worker public url but also the internal one, and adapt the call points like fetchDocs;
Ensure that the home and doc worker internal urls are trusted by trustOrigin;

---------

Co-authored-by: jordigh <[email protected]>
  • Loading branch information
@fflorent @jordigh
fflorent and jordigh authored May 14, 2024
1 parent 85f1040 commit 5e3cd94
Show file tree
Hide file tree
Showing 19 changed files with 516 additions and 159 deletions.
5 changes: 3 additions & 2 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,10 +235,11 @@ Grist can be configured in many ways. Here are the main environment variables it

| Variable | Purpose |
|------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| 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. |
| 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). Defaults to `APP_DOC_URL` |
| 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`. |
| APP_HOME_URL | url prefix for home api (home and doc servers need this) |
| APP_HOME_INTERNAL_URL | like `APP_HOME_URL` but used by the home and the doc servers to reach any home workers using an internal domain name resolution (like in a docker environment). Defaults to `APP_HOME_URL` |
| 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. |
Expand Down
49 changes: 32 additions & 17 deletions app/common/UserAPI.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -773,11 +773,11 @@ export class UserAPIImpl extends BaseAPI implements UserAPI {
}

public async getWorker(key: string): Promise<string> {
const json = await this.requestJson(`${this._url}/api/worker/${key}`, {
const json = (await this.requestJson(`${this._url}/api/worker/${key}`, {
method: 'GET',
credentials: 'include'
});
return getDocWorkerUrl(this._homeUrl, json);
})) as PublicDocWorkerUrlInfo;
return getPublicDocWorkerUrl(this._homeUrl, json);
}

public async getWorkerAPI(key: string): Promise<DocWorkerAPI> {
Expand Down Expand Up @@ -1156,6 +1156,27 @@ export class DocAPIImpl extends BaseAPI implements DocAPI {
}
}

/**
* Represents information to build public doc worker url.
*
* Structure that may contain either **exclusively**:
* - a selfPrefix when no pool of doc worker exist.
* - a public doc worker url otherwise.
*/
export type PublicDocWorkerUrlInfo = {
selfPrefix: string;
docWorkerUrl: null;
} | {
selfPrefix: null;
docWorkerUrl: string;
}

export function getUrlFromPrefix(homeUrl: string, prefix: string) {
const url = new URL(homeUrl);
url.pathname = prefix + url.pathname;
return url.href;
}

/**
* Get a docWorkerUrl from information returned from backend. When the backend
* is fully configured, and there is a pool of workers, this is straightforward,
Expand All @@ -1164,19 +1185,13 @@ export class DocAPIImpl extends BaseAPI implements DocAPI {
* use the homeUrl of the backend, with extra path prefix information
* given by selfPrefix. At the time of writing, the selfPrefix contains a
* doc-worker id, and a tag for the codebase (used in consistency checks).
*
* @param {string} homeUrl
* @param {string} docWorkerInfo The information to build the public doc worker url
* (result of the call to /api/worker/:docId)
*/
export function getDocWorkerUrl(homeUrl: string, docWorkerInfo: {
docWorkerUrl: string|null,
selfPrefix?: string,
}): string {
if (!docWorkerInfo.docWorkerUrl) {
if (!docWorkerInfo.selfPrefix) {
// This should never happen.
throw new Error('missing selfPrefix for docWorkerUrl');
}
const url = new URL(homeUrl);
url.pathname = docWorkerInfo.selfPrefix + url.pathname;
return url.href;
}
return docWorkerInfo.docWorkerUrl;
export function getPublicDocWorkerUrl(homeUrl: string, docWorkerInfo: PublicDocWorkerUrlInfo) {
return docWorkerInfo.selfPrefix !== null ?
getUrlFromPrefix(homeUrl, docWorkerInfo.selfPrefix) :
docWorkerInfo.docWorkerUrl;
}
27 changes: 22 additions & 5 deletions app/common/gristUrls.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,23 @@ export interface OrgUrlInfo {
orgInPath?: string; // If /o/{orgInPath} should be used to access the requested org.
}

function isDocInternalUrl(host: string) {
if (!process.env.APP_DOC_INTERNAL_URL) { return false; }
const internalUrl = new URL('/', process.env.APP_DOC_INTERNAL_URL);
return internalUrl.host === host;
function hostMatchesUrl(host?: string, url?: string) {
return host !== undefined && url !== undefined && new URL(url).host === host;
}

/**
* Returns true if:
* - the server is a home worker and the host matches APP_HOME_INTERNAL_URL;
* - or the server is a doc worker and the host matches APP_DOC_INTERNAL_URL;
*
* @param {string?} host The host to check
*/
function isOwnInternalUrlHost(host?: string) {
// Note: APP_HOME_INTERNAL_URL may also be defined in doc worker as well as in home worker
if (process.env.APP_HOME_INTERNAL_URL && hostMatchesUrl(host, process.env.APP_HOME_INTERNAL_URL)) {
return true;
}
return Boolean(process.env.APP_DOC_INTERNAL_URL) && hostMatchesUrl(host, process.env.APP_DOC_INTERNAL_URL);
}

/**
Expand All @@ -209,7 +222,11 @@ export function getHostType(host: string, options: {

const hostname = host.split(":")[0];
if (!options.baseDomain) { return 'native'; }
if (hostname === 'localhost' || isDocInternalUrl(host) || hostname.endsWith(options.baseDomain)) {
if (
hostname === 'localhost' ||
isOwnInternalUrlHost(host) ||
hostname.endsWith(options.baseDomain)
) {
return 'native';
}
return 'custom';
Expand Down
6 changes: 5 additions & 1 deletion app/gen-server/lib/DocApiForwarder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,11 @@ export class DocApiForwarder {
url.pathname = removeTrailingSlash(docWorkerUrl.pathname) + url.pathname;

const headers: {[key: string]: string} = {
...getTransitiveHeaders(req),
// At this point, we have already checked and trusted the origin of the request.
// See FlexServer#addApiMiddleware(). So don't include the "Origin" header.
// Including this header also would break features like form submissions,
// as the "Host" header is not retrieved when calling getTransitiveHeaders().
...getTransitiveHeaders(req, { includeOrigin: false }),
'Content-Type': req.get('Content-Type') || 'application/json',
};
for (const key of ['X-Sort', 'X-Limit']) {
Expand Down
49 changes: 21 additions & 28 deletions app/server/lib/AppEndpoint.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,18 @@ import {ApiError} from 'app/common/ApiError';
import {getSlugIfNeeded, parseUrlId, SHARE_KEY_PREFIX} from 'app/common/gristUrls';
import {LocalPlugin} from "app/common/plugin";
import {TELEMETRY_TEMPLATE_SIGNUP_COOKIE_NAME} from 'app/common/Telemetry';
import {Document as APIDocument} from 'app/common/UserAPI';
import {Document as APIDocument, PublicDocWorkerUrlInfo} from 'app/common/UserAPI';
import {Document} from "app/gen-server/entity/Document";
import {HomeDBManager} from 'app/gen-server/lib/HomeDBManager';
import {assertAccess, getTransitiveHeaders, getUserId, isAnonymousUser,
RequestWithLogin} from 'app/server/lib/Authorizer';
import {DocStatus, IDocWorkerMap} from 'app/server/lib/DocWorkerMap';
import {customizeDocWorkerUrl, getWorker, useWorkerPool} from 'app/server/lib/DocWorkerUtils';
import {
customizeDocWorkerUrl, getDocWorkerInfoOrSelfPrefix, getWorker, useWorkerPool
} from 'app/server/lib/DocWorkerUtils';
import {expressWrap} from 'app/server/lib/expressWrap';
import {DocTemplate, GristServer} from 'app/server/lib/GristServer';
import {getCookieDomain} from 'app/server/lib/gristSessions';
import {getAssignmentId} from 'app/server/lib/idUtils';
import log from 'app/server/lib/log';
import {addOrgToPathIfNeeded, pruneAPIResult, trustOrigin} from 'app/server/lib/requestUtils';
import {ISendAppPageOptions} from 'app/server/lib/sendAppPage';
Expand Down Expand Up @@ -48,32 +49,18 @@ export function attachAppEndpoint(options: AttachOptions): void {
app.get('/apiconsole', expressWrap(async (req, res) =>
sendAppPage(req, res, {path: 'apiconsole.html', status: 200, config: {}})));

app.get('/api/worker/:assignmentId([^/]+)/?*', expressWrap(async (req, res) => {
if (!useWorkerPool()) {
// Let the client know there is not a separate pool of workers,
// so they should continue to use the same base URL for accessing
// documents. For consistency, return a prefix to add into that
// URL, as there would be for a pool of workers. It would be nice
// to go ahead and provide the full URL, but that requires making
// more assumptions about how Grist is configured.
// Alternatives could be: have the client to send their base URL
// in the request; or use headers commonly added by reverse proxies.
const selfPrefix = "/dw/self/v/" + gristServer.getTag();
res.json({docWorkerUrl: null, selfPrefix});
return;
}
app.get('/api/worker/:docId([^/]+)/?*', expressWrap(async (req, res) => {
if (!trustOrigin(req, res)) { throw new Error('Unrecognized origin'); }
res.header("Access-Control-Allow-Credentials", "true");

if (!docWorkerMap) {
return res.status(500).json({error: 'no worker map'});
}
const assignmentId = getAssignmentId(docWorkerMap, req.params.assignmentId);
const {docStatus} = await getWorker(docWorkerMap, assignmentId, '/status');
if (!docStatus) {
return res.status(500).json({error: 'no worker'});
}
res.json({docWorkerUrl: customizeDocWorkerUrl(docStatus.docWorker.publicUrl, req)});
const {selfPrefix, docWorker} = await getDocWorkerInfoOrSelfPrefix(
req.params.docId, docWorkerMap, gristServer.getTag()
);
const info: PublicDocWorkerUrlInfo = selfPrefix ?
{ docWorkerUrl: null, selfPrefix } :
{ docWorkerUrl: customizeDocWorkerUrl(docWorker!.publicUrl, req), selfPrefix: null };

return res.json(info);
}));

// Handler for serving the document landing pages. Expects the following parameters:
Expand Down Expand Up @@ -160,7 +147,7 @@ export function attachAppEndpoint(options: AttachOptions): void {
// TODO docWorkerMain needs to serve app.html, perhaps with correct base-href already set.
const headers = {
Accept: 'application/json',
...getTransitiveHeaders(req),
...getTransitiveHeaders(req, { includeOrigin: true }),
};
const workerInfo = await getWorker(docWorkerMap, docId, `/${docId}/app.html`, {headers});
docStatus = workerInfo.docStatus;
Expand Down Expand Up @@ -206,10 +193,16 @@ export function attachAppEndpoint(options: AttachOptions): void {
});
}

// Without a public URL, we're in single server mode.
// Use a null workerPublicURL, to signify that the URL prefix serving the
// current endpoint is the only one available.
const publicUrl = docStatus?.docWorker?.publicUrl;
const workerPublicUrl = publicUrl !== undefined ? customizeDocWorkerUrl(publicUrl, req) : null;

await sendAppPage(req, res, {path: "", content: body.page, tag: body.tag, status: 200,
googleTagManager: 'anon', config: {
assignmentId: docId,
getWorker: {[docId]: customizeDocWorkerUrl(docStatus?.docWorker?.publicUrl, req)},
getWorker: {[docId]: workerPublicUrl },
getDoc: {[docId]: pruneAPIResult(doc as unknown as APIDocument)},
plugins
}});
Expand Down
8 changes: 6 additions & 2 deletions app/server/lib/Authorizer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -677,21 +677,25 @@ export function assertAccess(
* Pull out headers to pass along to a proxied service. Focused primarily on
* authentication.
*/
export function getTransitiveHeaders(req: Request): {[key: string]: string} {
export function getTransitiveHeaders(
req: Request,
{ includeOrigin }: { includeOrigin: boolean }
): {[key: string]: string} {
const Authorization = req.get('Authorization');
const Cookie = req.get('Cookie');
const PermitHeader = req.get('Permit');
const Organization = (req as RequestWithOrg).org;
const XRequestedWith = req.get('X-Requested-With');
const Origin = req.get('Origin'); // Pass along the original Origin since it may
// play a role in granular access control.

const result: Record<string, string> = {
...(Authorization ? { Authorization } : undefined),
...(Cookie ? { Cookie } : undefined),
...(Organization ? { Organization } : undefined),
...(PermitHeader ? { Permit: PermitHeader } : undefined),
...(XRequestedWith ? { 'X-Requested-With': XRequestedWith } : undefined),
...(Origin ? { Origin } : undefined),
...((includeOrigin && Origin) ? { Origin } : undefined),
};
const extraHeader = process.env.GRIST_FORWARD_AUTH_HEADER;
const extraHeaderValue = extraHeader && req.get(extraHeader);
Expand Down
4 changes: 2 additions & 2 deletions app/server/lib/BootProbes.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ const _homeUrlReachableProbe: Probe = {
id: 'reachable',
name: 'Grist is reachable',
apply: async (server, req) => {
const url = server.getHomeUrl(req);
const url = server.getHomeInternalUrl();
try {
const resp = await fetch(url);
if (resp.status !== 200) {
Expand All @@ -102,7 +102,7 @@ const _statusCheckProbe: Probe = {
id: 'health-check',
name: 'Built-in Health check',
apply: async (server, req) => {
const baseUrl = server.getHomeUrl(req);
const baseUrl = server.getHomeInternalUrl();
const url = new URL(baseUrl);
url.pathname = removeTrailingSlash(url.pathname) + '/status';
try {
Expand Down
23 changes: 14 additions & 9 deletions app/server/lib/DocApi.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1098,10 +1098,11 @@ export class DocWorkerApi {
if (req.body.sourceDocId) {
options.sourceDocId = await this._confirmDocIdForRead(req, String(req.body.sourceDocId));
// Make sure that if we wanted to download the full source, we would be allowed.
const result = await fetch(this._grist.getHomeUrl(req, `/api/docs/${options.sourceDocId}/download?dryrun=1`), {
const homeUrl = this._grist.getHomeInternalUrl(`/api/docs/${options.sourceDocId}/download?dryrun=1`);
const result = await fetch(homeUrl, {
method: 'GET',
headers: {
...getTransitiveHeaders(req),
...getTransitiveHeaders(req, { includeOrigin: false }),
'Content-Type': 'application/json',
}
});
Expand All @@ -1111,10 +1112,10 @@ export class DocWorkerApi {
}
// We should make sure the source document has flushed recently.
// It may not be served by the same worker, so work through the api.
await fetch(this._grist.getHomeUrl(req, `/api/docs/${options.sourceDocId}/flush`), {
await fetch(this._grist.getHomeInternalUrl(`/api/docs/${options.sourceDocId}/flush`), {
method: 'POST',
headers: {
...getTransitiveHeaders(req),
...getTransitiveHeaders(req, { includeOrigin: false }),
'Content-Type': 'application/json',
}
});
Expand Down Expand Up @@ -1170,12 +1171,16 @@ export class DocWorkerApi {
const showDetails = isAffirmative(req.query.detail);
const docSession = docSessionFromRequest(req);
const {states} = await this._getStates(docSession, activeDoc);
const ref = await fetch(this._grist.getHomeUrl(req, `/api/docs/${req.params.docId2}/states`), {
const ref = await fetch(this._grist.getHomeInternalUrl(`/api/docs/${req.params.docId2}/states`), {
headers: {
...getTransitiveHeaders(req),
...getTransitiveHeaders(req, { includeOrigin: false }),
'Content-Type': 'application/json',
}
});
if (!ref.ok) {
res.status(ref.status).send(await ref.text());
return;
}
const states2: DocState[] = (await ref.json()).states;
const left = states[0];
const right = states2[0];
Expand All @@ -1199,9 +1204,9 @@ export class DocWorkerApi {

// Calculate changes from the (common) parent to the current version of the other document.
const url = `/api/docs/${req.params.docId2}/compare?left=${parent.h}`;
const rightChangesReq = await fetch(this._grist.getHomeUrl(req, url), {
const rightChangesReq = await fetch(this._grist.getHomeInternalUrl(url), {
headers: {
...getTransitiveHeaders(req),
...getTransitiveHeaders(req, { includeOrigin: false }),
'Content-Type': 'application/json',
}
});
Expand Down Expand Up @@ -1644,7 +1649,7 @@ export class DocWorkerApi {
let uploadResult;
try {
const accessId = makeAccessId(req, getAuthorizedUserId(req));
uploadResult = await fetchDoc(this._grist, sourceDocumentId, req, accessId, asTemplate);
uploadResult = await fetchDoc(this._grist, this._docWorkerMap, sourceDocumentId, req, accessId, asTemplate);
globalUploadSet.changeUploadName(uploadResult.uploadId, accessId, `${documentName}.grist`);
} catch (err) {
if ((err as ApiError).status === 403) {
Expand Down
Loading

0 comments on commit 5e3cd94

Please sign in to comment.