-
Notifications
You must be signed in to change notification settings - Fork 8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixes: #11758 with a workaround with https proxy-agent #11759
base: master
Are you sure you want to change the base?
Conversation
@tristanrobert have you seen #11004 already? This looks similar and may also have the same risks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI: This has been attempted a couple times before, including #11004.
And every time it has broken some of the custom stuff we do with agents (see the comment in the other PR).
What we need to do is create our own custom Agent type that encapsulates all our custom code and proxy support.
The current code is a mess, lacks test coverage, and if we merge this we risk creating regressions that are likely not worth the value proxy support brings.
We definitely want to do this, and will get to it when we can.
Hey @tristanrobert, Thanks for the PR, We have created "GHC-450" as the internal reference to get this reviewed. One of us will be in touch if there are any changes needed, in most cases this is normally within a couple of weeks but it depends on the current workload of the team. |
I have just wrapped the https agent (same axiosconfig except I deactivate direct proxy) with a proxy-agent. |
Axios does not run well with proxy config, the easier workaround would be to deactivate any direct proxy config in axios and pass a wrapped https agent with proxy-agent. I agree code would need refactoring. When I fixed Mattemost credentials, I have seen that Grist credentials does not use the same function to build the axiosconfig, the proxy connection config. It uses this temporary function: n8n/packages/core/src/NodeExecuteFunctions.ts Line 305 in bd924c7
n8n/packages/core/src/NodeExecuteFunctions.ts Line 907 in bd924c7
|
Summary
Fixes 400 Bad request in credentials with https transport (https://mattermost... or https://grist...) when n8n is self hosted or launched in dev (pnpm start) behind a corporate proxy with http (http://proxy...), see #11758
Related Linear tickets, Github issues, and Community forum posts
fixes #11758
Review / Merge checklist
release/backport
(if the PR is an urgent fix that needs to be backported)