OIDC issuer behind a proxy cannot be accessed #1363

tristanrobert · 2025-01-06T13:12:20Z

Fixes #942

Context

Self hosted instance behind corporate proxy with OIDC issuer outside corporate LAN.

Proposed solution

Add ./ProxyAgent agent in openid-client custom http_options if issuer is on Internet

Related issues

Has this been tested?

👍 yes, I added tests to the test suite
💭 no, because this PR is a draft and still needs work
🙅 no, because this is not relevant here
🙋 no, because I need help

Screenshots / Screencasts

fflorent

Thank you for your patch @tristanrobert!

Some tests are failling. Some are just because of instabilities like Importer2, we can ignore them.
But some of them are due to the enhancement you introduced (these tests). Are you comfortable with adapting them? Or do you want me to do that for you?

The tests to adapt are here:
https://github.com/gristlabs/grist-core/blob/main/test/server/lib/OIDCConfig.ts

To setup the environment and to run the tests: https://github.com/gristlabs/grist-core/blob/main/documentation/develop.md

tristanrobert · 2025-01-06T15:25:29Z

Thank you for your patch @tristanrobert!

Some tests are failling. Some are just because of instabilities like Importer2, we can ignore them. But some of them are due to the enhancement you introduced (these tests). Are you comfortable with adapting them? Or do you want me to do that for you?

The tests to adapt are here: https://github.com/gristlabs/grist-core/blob/main/test/server/lib/OIDCConfig.ts

To setup the environment and to run the tests: https://github.com/gristlabs/grist-core/blob/main/documentation/develop.md

I have fixed the tests @fflorent

fflorent

Sounds almost good.

I left you some remarks. Also I need to setup an environment to test your work. I am doing that ASAP.

app/server/lib/OIDCConfig.ts

test/server/lib/OIDCConfig.ts

tristanrobert · 2025-01-08T13:27:30Z

As we previously discussed @fflorent, I added the proxy agent tests in OIDCConfig test. On the other hand, a frontend test (code which I have not touched) is failing. I think recent changes are in error and need to be fixed

app/server/lib/OIDCConfig.ts

test/server/lib/OIDCConfig.ts

fflorent

LGTM, thank you @tristanrobert!

test/server/lib/OIDCConfig.ts

dsagal · 2025-01-14T21:45:09Z

The code here looks fine for doing what it intends, but I am not sure we want GRIST_HTTPS_PROXY to be used for OIDC automatically without an explicit change in configuration by the user.

GRIST_HTTPS_PROXY is used for a couple of things, notably webhooks, in particular because those could be made to URLs specified by the user of a document, and out of the control of the administrator. In other words, the proxy is used for untrusted requests. For OIDC, I can imagine wanting a separate proxy since the use-case is different. For example, for OIDC requests security may be more important, and more important to keep the proxy itself up-to-date. I can also imagine one might want to enable the proxy for OIDC because it's needed for logins, without having to use that proxy (or any proxy) for untrusted requests.

@paulfitz , what do you think?

paulfitz · 2025-01-14T23:14:52Z

That's a good point @dsagal. There are two distinct usages here, and they could conflict. A proxy set up specifically for untrusted requests might not even be able to reach some internal OIDC-related server.

Not sure I have a good idea how to deal with this. Perhaps the proxying of untrusted traffic should have distinct configuration? The standard environment variable for this in other tools is https_proxy as opposed to our GRIST_HTTPS_PROXY so perhaps we could restrict the later to the "special" untrusted traffic and respect https_proxy more generally? Would that be too confusing? There was already a PR to use the proxy for fetching custom widget manifest that might need revisiting.

paulfitz · 2025-01-16T15:47:27Z

I think we should plan for a future where support for HTTP_PROXY/HTTPS_PROXY/NO_PROXY becomes more common in node apps through better native node support for it, following the trajectory of other languages. GRIST_HTTPS_PROXY's introduction was for a special purpose: routing of requests that may be relatively untrusted, which complicates things a little but shouldn't block progress on clear cases like OIDC.

@dsagal what do you think of this proposal:

An operator setting the semi-standard HTTPS_PROXY and running Grist should see all app server traffic go via that proxy.
An operator setting GRIST_HTTPS_PROXY should see all traffic to destinations controlled by non-admins go though that proxy, but trusted traffic will flow directly.
An operator setting GRIST_HTTPS_PROXY and HTTPS_PROXY to different values should see untrusted traffic use the first and trusted traffic use the second.
An operator setting GRIST_HTTPS_PROXY=direct and HTTPS_PROXY should see trusted traffic routed to a proxy, but untrusted traffic flow directly.

Suppose we rename the current function proxyAgent to proxyAgentForUntrustedRequests and add a separate proxyAgentForTrustedRequests to use for OIDC.

proxyAgentForUntrustedRequests should be sensitive to environment variables GRIST_HTTPS_PROXY, HTTPS_PROXY, and https_proxy in that order. The value direct is acceptable for GRIST_HTTPS_PROXY and prevents looking further.
proxyAgentForTrustedRequests should be sensitive to HTTPS_PROXY and https_proxy only.
(Obviously the implementation of the two methods should be largely shared)

I include https_proxy because it is a very common name variant.

The use of proxyAgent in WidgetRepository.ts should ideally become proxyAgentForTrustedRequests. This is a breaking change, which I think may be ok at this stage, but open to suggestions here.

Hopefully in the future proxyAgentForTrustedRequests can be phased out in favor of something like undici's EnvHttpProxyAgent or whatever way node goes on this.

The name of GRIST_HTTPS_PROXY, in retrospect, is a bit unfortunate. THE README docs for it will need a careful update, along with a related entry for HTTPS_PROXY. It would be nice if Grist didn't need to be "special" but I don't see an easy way around it.

paulfitz · 2025-01-16T17:41:48Z

Talked to @dsagal about #1363 (comment) offline and it seems acceptable. One modification: we'd make HTTPS_PROXY_UNTRUSTED_URLS an alias for GRIST_HTTPS_PROXY and start deprecating GRIST_HTTPS_PROXY.

@tristanrobert let me know if you're up for making these changes, or want to hand them off.

tristanrobert · 2025-01-17T08:13:55Z

Talked to @dsagal about #1363 (comment) offline and it seems acceptable. One modification: we'd make HTTPS_PROXY_UNTRUSTED_URLS an alias for GRIST_HTTPS_PROXY and start deprecating GRIST_HTTPS_PROXY.

@tristanrobert let me know if you're up for making these changes, or want to hand them off.

I'm trying to make you a proposition.

tristanrobert · 2025-01-17T08:13:55Z

Talked to @dsagal about #1363 (comment) offline and it seems acceptable. One modification: we'd make HTTPS_PROXY_UNTRUSTED_URLS an alias for GRIST_HTTPS_PROXY and start deprecating GRIST_HTTPS_PROXY.

@tristanrobert let me know if you're up for making these changes, or want to hand them off.

I'm trying to make you a proposition.

As discussed in gristlabs#1363 (comment)

tristanrobert · 2025-01-21T12:01:20Z

Talked to @dsagal about #1363 (comment) offline and it seems acceptable. One modification: we'd make HTTPS_PROXY_UNTRUSTED_URLS an alias for GRIST_HTTPS_PROXY and start deprecating GRIST_HTTPS_PROXY.
@tristanrobert let me know if you're up for making these changes, or want to hand them off.

I'm trying to make you a proposition.

I have commited what I have understood from #1363 (comment) and I have tested it in self hosted instance behind corporate proxy.

app/server/lib/ProxyAgent.ts

README.md

test/server/lib/ProxyAgent.ts

fflorent · 2025-01-27T09:52:04Z

app/server/lib/Requests.ts

@@ -91,7 +91,7 @@ export class DocRequests {
      }
      const response = await fetch(urlObj.toString(), {
        headers: headers || {},
-        agent: proxyAgent(urlObj),
+        agent: proxyAgentForTrustedRequests(urlObj),


Should be untrusted, as this module is used for the REQUEST() function.
Quoting the README file:

GRIST_ENABLE_REQUEST_FUNCTION enables the REQUEST function. This function performs HTTP requests in a similar way to requests.request. This function presents a significant security risk, since it can let users call internal endpoints when Grist is available publicly. This function can also cause performance issues. Unset by default.

Suggested change

agent: proxyAgentForTrustedRequests(urlObj),

agent: proxyAgentForUntrustedRequests(urlObj),

I am lost. 😞 OIDCCOnfig tests fail because it is contradictory. Indeed, if GRIST_HTTPS_PROXY enables proxyAgentForUntrustedRequests and HTTPS_PROXY enables proxyAgentForTrustedRequests as ProxyAgent.tx does, then if requests uses proxyAgentForUntrustedRequests we must enable GRIST_HTTPS_PROXY with the same corporate proxy value, but then why two env vars ?

Why two environment variables: as suggested by Paul above, it is useful to be able to distinguish requests that are made to URLs determined by an administrator, e.g. requests to an OIDC provider — we call those "trusted"; from requests made to URLs that are given by Grist end users (i.e. Webhooks and requests made through the REQUEST function if you enable it), over which the administrator has no control — "untrusted" requests.

Having both HTTPS_PROXY for trusted requests and HTTPS_PROXY_UNTRUSTED_URLS (new name for GRIST_HTTPS_PROXY suggested by Paul) for untrusted requests can let an administrator apply more stringent controls on untrusted requests.

For example, in your case you may want to use a lenient outgoing proxy for HTTPS_PROXY, that allows any request to the internet (including those to your OIDC provider); and set up a stricter HTTP proxy pointed to by HTTPS_PROXY_UNTRUSTED_URLS for untrusted requests, for example one that only allows requests to a limited list of domains, and/or limits request verbs, sizes, etc.

@tristanrobert Will you still make the changes suggested above? I find myself in agreement that HTTPS_PROXY_UNTRUSTED_URLS is a better name, as well as noting that REQUEST should use the proxy for untrusted URLs.

I am working on it, and I agree too that this would be a better name!

app/server/lib/ProxyAgent.ts

tristanrobert · 2025-02-14T09:15:32Z

Proxy Agent with corporate settings is needed too in Assistance.ts and Telemetry.ts that use node-fetch.

jordigh

I would like to see an implementation of the changes others have suggested. Changing the variable name, some tweaks to documentation, and ensuring that REQUEST goes through the proxy for untrusted URLs are all reasonable fixes. @tristanrobert, are you still working on this?

jordigh · 2025-02-24T15:57:37Z

README.md

+| GRIST_HTTPS_PROXY                  | if set with url, use this proxy for webhook payload delivery; if not set or set to direct it call `"direct"` webhook payload.                                                                                                                                                                                                                                                                                                          |
+| HTTPS_PROXY or https_proxy         | if set, use this reverse web proxy (corporate proxy) for fetching custom widgets repository from url or OIDC config from issuer url outside the grist stack.                                                                                                                                                                                                                                                                                                          |


Suggested change

| GRIST_HTTPS_PROXY | if set with url, use this proxy for webhook payload delivery; if not set or set to direct it call `"direct"` webhook payload. |

| HTTPS_PROXY or https_proxy | if set, use this reverse web proxy (corporate proxy) for fetching custom widgets repository from url or OIDC config from issuer url outside the grist stack. |

| GRIST_HTTPS_PROXY | Full URL of proxy for delivering webhook payloads. Default value is `direct` for delivering payloads without proxying. |

| HTTPS_PROXY or https_proxy | Full URL of reverse web proxy (corporate proxy) for fetching the custom widgets repository or the OIDC config from the issuer. |

jordigh · 2025-02-24T16:04:21Z

app/server/lib/Requests.ts

@@ -91,7 +91,7 @@ export class DocRequests {
      }
      const response = await fetch(urlObj.toString(), {
        headers: headers || {},
-        agent: proxyAgent(urlObj),
+        agent: proxyAgentForTrustedRequests(urlObj),


@tristanrobert Will you still make the changes suggested above? I find myself in agreement that HTTPS_PROXY_UNTRUSTED_URLS is a better name, as well as noting that REQUEST should use the proxy for untrusted URLs.

tristanrobert · 2025-02-25T08:43:02Z

We are still working on it with @fflorent.

As discussed in gristlabs#1363 (comment)

Fixes gristlabs#942

Co-authored-by: Florent <[email protected]>

Fixes gristlabs#942

As discussed in gristlabs#1363 (comment)

Co-authored-by: Florent <[email protected]>

fflorent self-requested a review January 6, 2025 13:41

fflorent suggested changes Jan 6, 2025

View reviewed changes

fflorent self-requested a review January 6, 2025 16:25

fflorent reviewed Jan 7, 2025

View reviewed changes

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

test/server/lib/OIDCConfig.ts Outdated Show resolved Hide resolved

paulfitz mentioned this pull request Jan 7, 2025

Add Proxy Support for Assistant API Calls in Assistance.ts #1343

Open

fflorent suggested changes Jan 9, 2025

View reviewed changes

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

test/server/lib/OIDCConfig.ts Outdated Show resolved Hide resolved

test/server/lib/OIDCConfig.ts Outdated Show resolved Hide resolved

fflorent approved these changes Jan 10, 2025

View reviewed changes

fflorent requested a review from dsagal January 10, 2025 09:14

fflorent reviewed Jan 10, 2025

View reviewed changes

test/server/lib/OIDCConfig.ts Show resolved Hide resolved

fflorent approved these changes Jan 10, 2025

View reviewed changes

tristanrobert added a commit to tristanrobert/grist-core that referenced this pull request Jan 20, 2025

refactor: ♻️ refactors proxyAgent in two trusted and unstrusted agents

cf4a060

As discussed in gristlabs#1363 (comment)

fflorent reviewed Jan 21, 2025

View reviewed changes

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

fflorent reviewed Jan 21, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

test/server/lib/ProxyAgent.ts Outdated Show resolved Hide resolved

fflorent reviewed Jan 27, 2025

View reviewed changes

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

jordigh reviewed Feb 24, 2025

View reviewed changes

vviers mentioned this pull request Mar 6, 2025

Custom HTTP queries for webhooks #827

Closed

fflorent pushed a commit to betagouv/grist-core that referenced this pull request Mar 18, 2025

refactor: ♻️ refactors proxyAgent in two trusted and unstrusted agents

35c5011

As discussed in gristlabs#1363 (comment)

OIDC issuer behind a proxy cannot be accessed

ab08412

Fixes gristlabs#942

tristanrobert and others added 22 commits March 27, 2025 14:24

fixes oidc config test

7ada3fb

OIDC issuer behind a proxy cannot be accessed

db023c6

Fixes gristlabs#942

Update app/server/lib/OIDCConfig.ts

8e33a77

Co-authored-by: Florent <[email protected]>

Update test/server/lib/OIDCConfig.ts

4074976

Co-authored-by: Florent <[email protected]>

Update test/server/lib/OIDCConfig.ts

771a947

Co-authored-by: Florent <[email protected]>

OIDC issuer behind a proxy cannot be accessed

80cc5c6

Fixes gristlabs#942

refactor: ♻️ refactors proxyAgent in two trusted and unstrusted agents

4d586e9

As discussed in gristlabs#1363 (comment)

refactor: ♻️ refactor proxyagent functions

5462aab

Update README.md

947cd4f

Co-authored-by: Florent <[email protected]>

test: ♻️ rafactoring proxyagent tests

aa6ca34

test: ✅ fixes proxyagent tests

177ba4e

Update app/server/lib/ProxyAgent.ts

e38e524

Co-authored-by: Florent <[email protected]>

fix(telemetry): fixes Telemetry behind a proxy cannot be accessed

ccfaf9f

fix: 🐛 fixes fetch assistant ai behind corporate proxy

00208bf

Fix wrong call for Requests

0c8fbc9

Normalize imports

689f69f

Cleanup tests

0339f8a

Rework of the tests and of the ProxyAgent module

fc86f2b

Fix tests for OIDC

990bcc5

Rename env var and rework module

2ef9ddc

Fix OIDCConfig tests

68af530

Reset imports

0fe0a12

fflorent force-pushed the tristanrobert/issue942 branch from 883369b to 0fe0a12 Compare March 27, 2025 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OIDC issuer behind a proxy cannot be accessed #1363

OIDC issuer behind a proxy cannot be accessed #1363

tristanrobert commented Jan 6, 2025 •

edited

Loading

fflorent left a comment •

edited

Loading

tristanrobert commented Jan 6, 2025 •

edited

Loading

fflorent left a comment

tristanrobert commented Jan 8, 2025 •

edited

Loading

fflorent left a comment

dsagal commented Jan 14, 2025

paulfitz commented Jan 14, 2025

paulfitz commented Jan 16, 2025

paulfitz commented Jan 16, 2025

tristanrobert commented Jan 17, 2025 •

edited

Loading

tristanrobert commented Jan 17, 2025

tristanrobert commented Jan 21, 2025

fflorent Jan 27, 2025

tristanrobert Jan 27, 2025 •

edited

Loading

jonathanperret Jan 27, 2025

jordigh Feb 24, 2025

fflorent Mar 17, 2025 •

edited

Loading

tristanrobert commented Feb 14, 2025

jordigh left a comment •

edited

Loading

jordigh Feb 24, 2025

jordigh Feb 24, 2025

tristanrobert commented Feb 25, 2025

	agent: proxyAgentForTrustedRequests(urlObj),
	agent: proxyAgentForUntrustedRequests(urlObj),

		\| GRIST_HTTPS_PROXY \| if set with url, use this proxy for webhook payload delivery; if not set or set to direct it call `"direct"` webhook payload. \|
		\| HTTPS_PROXY or https_proxy \| if set, use this reverse web proxy (corporate proxy) for fetching custom widgets repository from url or OIDC config from issuer url outside the grist stack. \|

OIDC issuer behind a proxy cannot be accessed #1363

Are you sure you want to change the base?

OIDC issuer behind a proxy cannot be accessed #1363

Conversation

tristanrobert commented Jan 6, 2025 • edited Loading

Context

Proposed solution

Related issues

Has this been tested?

Screenshots / Screencasts

fflorent left a comment • edited Loading

Choose a reason for hiding this comment

tristanrobert commented Jan 6, 2025 • edited Loading

fflorent left a comment

Choose a reason for hiding this comment

tristanrobert commented Jan 8, 2025 • edited Loading

fflorent left a comment

Choose a reason for hiding this comment

dsagal commented Jan 14, 2025

paulfitz commented Jan 14, 2025

paulfitz commented Jan 16, 2025

paulfitz commented Jan 16, 2025

tristanrobert commented Jan 17, 2025 • edited Loading

tristanrobert commented Jan 17, 2025

tristanrobert commented Jan 21, 2025

fflorent Jan 27, 2025

Choose a reason for hiding this comment

tristanrobert Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

jonathanperret Jan 27, 2025

Choose a reason for hiding this comment

jordigh Feb 24, 2025

Choose a reason for hiding this comment

fflorent Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

tristanrobert commented Feb 14, 2025

jordigh left a comment • edited Loading

Choose a reason for hiding this comment

jordigh Feb 24, 2025

Choose a reason for hiding this comment

jordigh Feb 24, 2025

Choose a reason for hiding this comment

tristanrobert commented Feb 25, 2025

tristanrobert commented Jan 6, 2025 •

edited

Loading

fflorent left a comment •

edited

Loading

tristanrobert commented Jan 6, 2025 •

edited

Loading

tristanrobert commented Jan 8, 2025 •

edited

Loading

tristanrobert commented Jan 17, 2025 •

edited

Loading

tristanrobert Jan 27, 2025 •

edited

Loading

fflorent Mar 17, 2025 •

edited

Loading

jordigh left a comment •

edited

Loading