Introduce APP_HOME_INTERNAL_URL and fix duplicate docs #915

fflorent · 2024-03-27T09:33:31Z

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

On 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;

The checkboxes are ticked as I successfully qualified the bug resolution with my patch.

Proposed 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;

Fixes #185

app/common/UserAPI.ts

app/server/lib/FlexServer.ts

CamilleLegeron · 2024-03-28T17:55:18Z

LGTM :)

jonathanperret

I have mostly stylistic suggestions, feel free to disregard them.

More globally, I have doubts about building on the selfPrefix workaround; as described here it only exists because it seems we have uncertainty about the app's URL. Maybe effort would be best spent resolving that uncertainty at the source instead of carrying that URL/prefix duality all the way, with the complexity it entails?

In a similar vein, the APP_HOME_INTERNAL_URL variable we are introducing here would not be necessary if fetchDoc directly consulted the DocWorkerMap to find a document's worker URL (basically copying this logic) instead of bothering a home worker for that information. After all, the doc worker does have access to DocWorkerMap since it registers itself in it. I did prototype this approach locally and it seemed to work, but more testing might be needed as there are so many possible configurations.

Again, feel free to ignore these ramblings 😉

app/server/lib/AppEndpoint.ts

app/common/UserAPI.ts

jonathanperret · 2024-03-28T10:21:17Z

app/server/lib/AppEndpoint.ts

+    res.json({
+      docWorkerUrl: customizeDocWorkerUrl(docStatus.docWorker.publicUrl, req),
+      internalDocWorkerUrl: docStatus.docWorker.internalUrl
+    });


I understand that this endpoint (/api/worker/:id) is called both from the client (in UserAPI) and from a worker (in uploads) but it does feel a bit weird to be returning an internal URL from a public-facing API.
Perhaps the endpoint could be split?

Sounds a good idea. Also would give a hint about who is the caller in the logs.

Maybe then:

/api/worker/:assignmentId/public

and /api/worker/:assignmentId/internal

But there is this /?* in the existing path that puzzles me a bit…

It's an interesting API design problem.

On the one hand, this sound like an API you'd want exposed only to internal callers, which would call for introducing a prefix (so that e.g. these routes could be blocked on the reverse proxy), as in /internalapi/worker/:assignmentId. On the other hand, I can imagine scenarios where an internal call would be made to obtain an external URL, e.g. for redirecting a client to another worker. So, just because it is an internal call doesn't mean you want (only) the internal URL. So you'd end up with /internalapi/worker/:assignmentId/internal I guess. Or maybe just have /internalapi/worker/:assignmentId return both internal and public URLs as the current endpoint does.

Alternately, keep a single (public) endpoint but include the internal URL only if the caller was detected as internal (perhaps through a header that the reverse proxy blocks).

Or, as I suggested above, maybe drop the need for internal calls to this endpoint altogether?

Should be resolved with the latest changes.

app/common/UserAPI.ts

jonathanperret · 2024-03-28T16:38:58Z

app/common/gristUrls.ts

+function isInternalUrl(host: string, envValue?: string) {
+  if (!envValue) { return false; }
+  const internalUrl = new URL('/', envValue);
  return internalUrl.host === host;
 }


I don't understand the need for new URL('/', …) instead of just new URL(…)?

Also, since it is now quite generic, perhaps this function would be better called something like hostMatchesUrl(host: string, url: string?)?

As an aside, I'm slightly puzzled this works at all since APP_DOC_INTERNAL_URL is only defined inside a doc worker, and can contain only the URL for this worker, whose host isn't guaranteed to match any other worker's host.

As an aside, I'm slightly puzzled this works at all since APP_DOC_INTERNAL_URL is only defined inside a doc worker, and can contain only the URL for this worker, whose host isn't guaranteed to match any other worker's host.

Thanks, this sentence led me to change the functions, especially the naming to make it clearer: cba4e6c

The idea is that is should only be tested to check whether it matches its own internal URL, which I hope it is clearer now.

jonathanperret · 2024-03-29T11:09:52Z

app/server/lib/requestUtils.ts

+
+  if (isOwnInternalUrlHost(req.get('Host'))) { return true; }
+


Besides the conflict with the changes I made in #859 (removing the GRIST_HOST exception above, and changing the type of req), I'm not sure what the intent of this exception is?
Something to keep in mind: every exception here is a potential vulnerability. Allowing a request based on the Host header, without even checking Origin seems strange.

The intent is to allow requests made internally, assuming that the url of APP_DOC_INTERNAL_URL and APP_HOME_INTERNAL_URL cannot be resolved outside the internal network.

That being said, and as I understand that the origin header is passed along the fetch call made internally, I should check why it cannot pass the test of the below code.

Do you have a test that fails without this exception?

A manual test at least (duplicating the document), but with what I understand of the code, I have doubts, so I should try again without and see what happens.

OK, I confirm I have this error if I remove this line:

18:12:42 Credentials not supported for cross-origin requests
Duplicate Document

This is because below, the allowHost(req, new URL(origin)) checks whether the Origin header contains the same domain as for the Host header, because internally the Origin header is passed along from the request the client made (the one to copy) to the internal one (the one to fetch the document).

BTW, the justification (that I think is given below) puzzles me a lot 🤔:

grist-core/app/server/lib/Authorizer.ts

Lines 686 to 687 in 3405ce9

const Origin = req.get('Origin'); // Pass along the original Origin since it may

// play a role in granular access control.

@fflorent what you found matches my own research. I had likewise concluded that preserving the Host header in some form when forwarding requests would be required, as access decisions in the Grist codebase seem to often be made by comparing Origin to Host (as an aside, here's an unfortunate detail : the getOrigin function in requestUtils actually returns a value based on the Host header, not the Origin header).

Also, these decisions can be quite involved, and may even require a database access as in the case of custom domains — which is unfortunately a code path that as far as I know no one is exercising apart from Grist SaaS, which makes me wary of suggesting any changes to that logic.

Your proposal that replicates the Host header in getTransitiveHeaders does go in the same direction I had in mind. But I'm afraid it may break some deployments, as the Host header can play a critical role in dispatching an HTTP request. Here's a contrived example that shows how it could break:

Given a Grist instance with a public URL of https://grist.example.org;

Handling an incoming user request at https://grist.example.org/… (therefore, Host is grist.example.org) that triggers a forwarded request to e.g. http://grist-worker.internal;

Copying the Host header from the public request to the forwarded request would result in making a request to http://grist-worker.internal with a Host header value of grist.example.org (as opposed to a Host value of grist-worker.internal as is currently automatically generated by the HTTP client, i.e. node-fetch as far as I can tell).
Depending on how the HTTP server on grist-worker.internal is setup, this could cause the request to fail. If grist-worker.internal resolves directly to a Grist worker, it's likely to work as the Node.JS HTTP server basically ignores the incoming Host value. But if for some reason a reverse proxy/load balancer is in front of that server, it may reject the request or route it to the wrong server.

This very common problem with reverse proxying is generally addressed by copying the Host header value to another header such as X-Forwarded-Host instead, so that the code handling the request can make decisions based on the Host value sent by the initial client, without compromising request routing. Of course, this implies careful handling of whether to trust such a header, since it can easily be spoofed by a client if it is not filtered at the reverse proxy (see e.g. @dsagal 's remark here about the related X-Forwarded-Proto header).

@paulfitz the use of Origin in access rules is another wrinkle I hadn't discovered yet, thanks for bringing it to our attention.

If I understand what you're suggesting, removing the Origin header from forwarded requests would likely let them more easily through, as requests without an Origin header are considered same-origin and exempted from any further checks in trustOrigin.

It seems to me though there are other parts of the code that currently match the Host header with e.g. parts of the session cookie, such as this code in Authorizer — again, this appears to be custom host-specific so difficult to exercise outside of Grist SaaS. Correctly forwarding Host would probably help such code work, even though it doesn't seem to pose a problem currently because it probably isn't hit by any forwarded requests, but that seems rather an accident.

In general, it seems to me that if any header (such as Origin or Cookie) is forwarded, any value that it may be checked against (here, Host) needs to be made available as well to preserve the full security context.

@fflorent In any case, I strongly suggest you rebase your branch onto main as the code in question was affected by the merging of #859 .

This very common problem with reverse proxying is generally addressed by copying the Host header value to another header such as X-Forwarded-Host instead, so that the code handling the request can make decisions based on the Host value sent by the initial client, without compromising request routing. Of course, this implies careful handling of whether to trust such a header, since it can easily be spoofed by a client if it is not filtered at the reverse proxy (see e.g. @dsagal 's remark #859 (comment) about the related X-Forwarded-Proto header).

Could we take advantage of the express 'trust proxy' feature? This way, the call to req.hostname would return the value of the X-Forwarded-Host header. We could introduce another env variable to set IP addresses / subnets written in CSV format, and document it as dangerous unless the X-Forwarded-For, X-Forwarded-Host, and X-Forwarded-Proto are not cleared/overwritten by the RP.

PS:

@fflorent what you found matches my own research.

Actually, I remember you told us such a thing in the tchat, but was not able to retrieve it. This good idea was your own :).

PS2:

@fflorent In any case, I strongly suggest you rebase your branch onto main as the code in question was affected by the merging of #859 .

Done!

Yes, the trust proxy feature of Express is indeed meant to support reading these headers. A few things to keep in mind though:

as explained in the Express docs, that option should not be enabled indiscriminately, so we would probably need to add an environment variable to opt into it;

trust proxy does impact the hostname on the Express Request object, but in several places in grist-core the parent IncomingMessage interface is used instead, because WebSocket connections do not go through Express. This is why for example I had to re-implement Express's req.proto here: Support HTTP long polling as an alternative to WebSockets #859 (comment)

jordigh · 2024-04-11T17:30:57Z

@fflorent I recently merged in #933 which probably fixes at least the problem with duplicate document. There may be other "internal" http requests between Grist home servers and doc workers that might also be doing unnecessary CORS checks.

Does #933 reduce some of the problem's complexity for you?

fflorent · 2024-04-12T13:11:33Z

@jordigh

Does #933 reduce some of the problem's complexity for you?

I am not very certain, I tend to think that we should anyway adapt the getTransitiveHeaders() function to either remove the Origin, or add the X-Forwarded-Host header (like suggested Jonathan and which seems the most promising solution). This way, we won't probably have to delete the Origin header in other call sites.

Also we would still need the APP_HOME_INTERNAL_URL env variable anyway (behind the reverse-proxy, the public urls cannot be resolved in our case).

But maybe my intuition is wrong, I am not working on that subject today (will do the next week)

At least I am quite sure it does not harm, and quite a good news if that solves an issue you had!

dsagal · 2024-04-12T14:53:23Z

To add my two cents, at first glance, the idea of setting Host to match the original request seemed simplest and most appealing. But that would essentially require that the internal requests between the home-servers and doc-workers bypass any reverse proxies (or load-balancers), since those rely on the Host to route the request. Or else those reverse proxies need some very special handling. That would create ops difficulties for our SaaS.

Setting X-Forwarded-Host would also create difficulties when a reverse proxy is used between home-servers and doc-workers (including in our SaaS), since then Grist receives all requests from the proxy, and would need some very careful trust settings -- both in the proxy and in Grist -- to know when X-Forwarded-Host is to be trusted.

One valuable insight from @jordigh is that getTransitiveHeaders() is used for different kinds of requests, including actual forwarding (e.g. API requests that home-server receives and forwards to a doc-worker) and for new requests. The Origin header matters only sometimes -- for the browser (but that function may be handled by the original server receiving the request) and also in certain cases for access rules (Paul's explanation: #915 (comment)). But in most cases of new internal requests (like for duplicating documents) it serves no useful purpose but does create problems.

Maybe what would be clearer is have getTransitiveHeaders(req, options: {includeOrigin: boolean}), and include the Origin only when it matters.

app/server/lib/AppEndpoint.ts

app/common/UserAPI.ts

test/server/lib/helpers/TestServer.ts

Quoting Wikipedia: > The name localhost is a **commonly** defined hostname for the loopback > interface in most TCP/IP systems, resolving to the IP addresses > 127.0.0.1 in IPv4 and ::1 for IPv6.

The cause was that setup() first argument takes the name of the socket used by home and doc servers to listen.

Also use GRIST_ORG_IN_PATH to make it work as expected, and GRIST_SINGLE_PORT=0 to ensure to query the pool of workers.

Co-authored-by: jordigh <jordigh@octave.org>

paulfitz

some small comments

README.md

paulfitz · 2024-05-07T15:31:11Z

README.md

 | 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 server to reach any home workers using an internal domain name resolution (like in a docker environment). Defaults to `APP_HOME_URL`                                                                                                                                                                                 |


Inconsistency in home workers - for better or worse, we call the two types of server home servers and doc workers.

Btw do you know for a fact that the variable is used only by home servers and never by doc workers?

Btw do you know for a fact that the variable is used only by home servers and never by doc workers?

Sorry, I may not understand your question, I may reformulate what I understand: you ask me whether this variable is used by both workers or only by home workers?

It's actually used by both servers. The /compare endpoint is run by the doc worker, and the POST /api/docs is IIRC run by the home server and may be used to copy a document (when passing sourceDocumentId).

I change the description as requested.

paulfitz · 2024-05-07T15:33:19Z

app/common/UserAPI.ts

@@ -1156,6 +1156,27 @@ export class DocAPIImpl extends BaseAPI implements DocAPI {
  }
 }

+/**
+ * Reprensents information to build public doc worker url.


*represents

paulfitz · 2024-05-07T15:34:02Z

app/common/UserAPI.ts

+ *  - a selfPrefix when no pool of doc worker exist.
+ *  - a public doc worker url otherwise.
+ */
+export type PublicDocWorkerUrlInfo = {


Nicely typed. It is a weird structure, sorry.

Ahah, credits to @jonathanperret for the idea!

To be sure, do you ask for changes here?

No, just saying I appreciated it.

paulfitz · 2024-05-07T15:36:14Z

app/common/UserAPI.ts

-  }
-  return docWorkerInfo.docWorkerUrl;
+export function getPublicDocWorkerUrl(homeUrl: string, docWorkerInfo: PublicDocWorkerUrlInfo) {
+  return docWorkerInfo.selfPrefix ?


if you compared with null you might not need the ! at the end of docWorkerInfo.docWorkerUrl!.

paulfitz · 2024-05-07T15:38:20Z

app/common/gristUrls.ts

+ * @param {string?} host The host to check
+ */
+function isOwnInternalUrlHost(host?: string) {
+  // Note: APP_HOME_INTERNAL_URL may also defined in doc worker as well as in Home worker


*be defined
inconsistent capitalization

paulfitz · 2024-05-07T15:44:42Z

app/gen-server/lib/DocApiForwarder.ts

+      // 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().


So do we need a follow-up task to strike out user.Origin in https://support.getgrist.com/access-rules/#access-rule-conditions ?

As previously discussed, I don't know of anyone using this feature.

An alternative could be to copy the origin header to somewhere ineffective like X-Original-Origin and make that available in the user object. Trivially spoof-able outside the browser environment but so is Origin itself.

One way is to see if anyone complain about this not being available anymore, and after a while we remove it 😸 .

:)

I made a quick PR at gristlabs/grist-help#340

paulfitz · 2024-05-07T15:48:46Z

app/server/lib/BootProbes.ts

@@ -77,7 +77,7 @@ const _homeUrlReachableProbe: Probe = {
  id: 'reachable',
  name: 'Grist is reachable',
  apply: async (server, req) => {
-    const url = server.getHomeUrl(req);


Hmm, makes sense, but also points out that this particular test would be best done from the user's browser. Oh well, that's a job for another day.

paulfitz · 2024-05-07T15:50:11Z

app/server/lib/DocWorkerUtils.ts

 import log from 'app/server/lib/log';
 import {adaptServerUrl} from 'app/server/lib/requestUtils';
 import * as express from 'express';
 import fetch, {Response as FetchResponse, RequestInit} from 'node-fetch';
+import { getAssignmentId } from './idUtils';


Can you use same style of import as the rest? It helps with grist-electron/grist-static for messy reasons.

fflorent · 2024-05-07T17:07:14Z

@paulfitz Thanks for your review, I'll take a look either next Friday or next week!

This was a feature that was never fleshed out, which has caused trouble, and which will shortly be removed: gristlabs/grist-core#915 I propose removing the documentation for the feature entirely. Another option would be to strike it out and link to the discussion above. But given that we've no evidence anyone is using it, I've just deleted it. If someone complains we can revisit.

CamilleLegeron reviewed Mar 27, 2024

View reviewed changes

app/common/UserAPI.ts Outdated Show resolved Hide resolved

fflorent commented Mar 28, 2024

View reviewed changes

app/server/lib/FlexServer.ts Outdated Show resolved Hide resolved

fflorent requested a review from CamilleLegeron March 28, 2024 15:48

fflorent force-pushed the introduce-app-home-internal-url branch 4 times, most recently from b76c4c4 to 1bf5fd5 Compare March 28, 2024 16:49

CamilleLegeron approved these changes Mar 28, 2024

View reviewed changes

jonathanperret reviewed Mar 28, 2024

View reviewed changes

fflorent force-pushed the introduce-app-home-internal-url branch 2 times, most recently from 52f5599 to cba4e6c Compare March 28, 2024 20:43

jonathanperret reviewed Mar 29, 2024

View reviewed changes

paulfitz mentioned this pull request Apr 5, 2024

Support HTTP long polling as an alternative to WebSockets #859

Merged

fflorent force-pushed the introduce-app-home-internal-url branch from 93be827 to e36f549 Compare April 9, 2024 09:30

fflorent added the anct label Apr 11, 2024

fflorent force-pushed the introduce-app-home-internal-url branch 2 times, most recently from 283a31d to faf9fa2 Compare April 18, 2024 15:17

fflorent commented Apr 18, 2024

View reviewed changes

app/server/lib/AppEndpoint.ts Outdated Show resolved Hide resolved

app/server/lib/AppEndpoint.ts Outdated Show resolved Hide resolved

app/common/UserAPI.ts Outdated Show resolved Hide resolved

app/common/UserAPI.ts Outdated Show resolved Hide resolved

jordigh added the preview Launch preview deployment of this PR label Apr 24, 2024

fflorent force-pushed the introduce-app-home-internal-url branch from 9944492 to d3d7e02 Compare April 30, 2024 13:15

fflorent mentioned this pull request Apr 30, 2024

GRIST_SUPPORT_ANON and forms #950

Open

fflorent force-pushed the introduce-app-home-internal-url branch from 3c08203 to 79210fc Compare May 2, 2024 15:43

fflorent commented May 2, 2024

View reviewed changes

test/server/lib/helpers/TestServer.ts Outdated Show resolved Hide resolved

fflorent requested review from jonathanperret and jordigh and removed request for jonathanperret May 2, 2024 16:52

fflorent added 18 commits May 7, 2024 08:41

WIP

7e0e216

attempt to fix issue with duplicate doc

ef6957a

Fix WS connection getting closed

e4e0ffe

Cleanup

c061e49

Introduce isOwnInternalUrlHost for more clarity

afa7aa2

Proposal for not requiring changing trustOrigin

1530953

Improve /api/workers/:docId worker to only send public urls

9546a1e

Remove check of whether prefix is set

968d14d

Fix compiling errors

3722fcd

Include Origin header depending on the call site

85fcfae

Fix form submission

c19a136

Add tests

892d902

Use nip.io instead of non-standard .localhost tld

a7bae9a

Quoting Wikipedia: > The name localhost is a **commonly** defined hostname for the loopback > interface in most TCP/IP systems, resolving to the IP addresses > 127.0.0.1 in IPv4 and ::1 for IPv6.

Fix behing-proxy tests aren't running

ac2857c

The cause was that setup() first argument takes the name of the socket used by home and doc servers to listen.

Use nip.io to have a specific hostname for proxy

47c8804

Also use GRIST_ORG_IN_PATH to make it work as expected, and GRIST_SINGLE_PORT=0 to ensure to query the pool of workers.

Adapt tests

99e6257

Minor fixes

55a26c0

Rename TestServerProxy to TestServerReverseProxy

14b2bfa

fflorent force-pushed the introduce-app-home-internal-url branch from a7ab1b3 to 14b2bfa Compare May 7, 2024 06:41

fflorent and others added 4 commits May 7, 2024 08:43

PR remarks

539daf9

Update comment in AppEndpoint.ts as suggested by Jordi

f4defc0

Co-authored-by: jordigh <jordigh@octave.org>

Explain why we don't pass the Origin header in DocApiForwarder

f90d3e5

tests: simplify and disallow workers to call public URLs

1309169

fflorent mentioned this pull request May 7, 2024

Revert "Remove the GRIST_ALLOWED_HOSTS environment variable (#899)" #968

Open

paulfitz reviewed May 7, 2024

View reviewed changes

Paul's remarks

d6e470c

paulfitz mentioned this pull request May 7, 2024

remove user.Origin gristlabs/grist-help#340

Merged

	const Origin = req.get('Origin'); // Pass along the original Origin since it may
	// play a role in granular access control.

		\| 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 server to reach any home workers using an internal domain name resolution (like in a docker environment). Defaults to `APP_HOME_URL` \|

Introduce APP_HOME_INTERNAL_URL and fix duplicate docs #915

Are you sure you want to change the base?

Introduce APP_HOME_INTERNAL_URL and fix duplicate docs #915

Conversation

fflorent commented Mar 27, 2024 • edited

Context

Bugs that are solved

Proposed solution

CamilleLegeron commented Mar 28, 2024

jonathanperret left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fflorent Mar 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fflorent Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordigh commented Apr 11, 2024 • edited

fflorent commented Apr 12, 2024

dsagal commented Apr 12, 2024

paulfitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fflorent May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fflorent commented May 7, 2024

fflorent commented Mar 27, 2024 •

edited

fflorent Mar 28, 2024 •

edited

fflorent Apr 9, 2024 •

edited

jordigh commented Apr 11, 2024 •

edited

fflorent May 7, 2024 •

edited