Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tristanrobert
Copy link

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

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@CLAassistant
Copy link

CLAassistant commented Nov 15, 2024

CLA assistant check
All committers have signed the CLA.

@Joffcom
Copy link
Member

Joffcom commented Nov 15, 2024

@tristanrobert have you seen #11004 already? This looks similar and may also have the same risks.

Copy link
Member

@netroy netroy left a 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.

@n8n-assistant n8n-assistant bot added community Authored by a community member core Enhancement outside /nodes-base and /editor-ui in linear Issue or PR has been created in Linear for internal review labels Nov 15, 2024
@Joffcom
Copy link
Member

Joffcom commented Nov 15, 2024

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.

@tristanrobert
Copy link
Author

tristanrobert commented Nov 15, 2024

@tristanrobert have you seen #11004 already? This looks similar and may also have the same risks.

I have just wrapped the https agent (same axiosconfig except I deactivate direct proxy) with a proxy-agent.

@tristanrobert
Copy link
Author

tristanrobert commented Nov 15, 2024

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.

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:

export async function parseRequestObject(requestObject: IRequestOptions) {
when Mattermost uses this function:
function convertN8nRequestToAxios(n8nRequest: IHttpRequestOptions): AxiosRequestConfig {
I agree that it should be refactored and I think all the proxy conf should be deleted and let proxy-agent do the job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member core Enhancement outside /nodes-base and /editor-ui in linear Issue or PR has been created in Linear for internal review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SELF-HOSTED] Behind a corporate http proxy https credentials test fail
4 participants