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

fix: respect rejectUnauthorized and ca opts when proxying https #725

Merged
merged 2 commits into from Oct 22, 2023

Conversation

cwilper
Copy link
Contributor

@cwilper cwilper commented Oct 22, 2023

This PR fixes #362

Problem

When Bruno would make SSL/TLS connections through a proxy, it would not respect connection options like rejectUnauthorized or ca. This was making it difficult to use when going through a self-signing MiTM proxy, or using a custom certificate authority.

Cause

The issue was caused by an upstream bug in https-proxy-agent, where it doesn't permit options to be passed down when upgrading sockets to TLS:

The same underlying issue has been reported against multiple versions of that library over the years. As you can see in the comments, people have either switched to other libraries, or patched it to pass down such options:

Solution

The solution employed here is to patch the class, similar to what others have done.

When/if the issue is fixed in proxy-agents, that should be used instead.

Note to Reviewers:

  • I put this in the existing proxy-util.js file, which is currently duplicated for CLI and Electron. I didn't see a good way to just have one copy of the code. Maybe the JS package would make sense eventually -- there seems to be lots of opportunity for consolidating common code there.
  • I was able to test successfully using the UI, but not the CLI -- I think there may be a separate bug in CLI currently where the Collection-configured proxy is not actually attempted to be used -- the code seems to look for proxy.enabled: true but it's saved as proxy.use: true in my bruno.json file in the collection dir. I didn't try to address that as part of the PR, as it seems a separate topic.

Contribution Checklist:

  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have read the contribution guidelines.
  • Linked issue to the pull request.

@helloanoop helloanoop merged commit f22e975 into usebruno:main Oct 22, 2023
2 checks passed
@helloanoop
Copy link
Contributor

@cwilper You're awesome !!!

there seems to be lots of opportunity for consolidating common code there.

Yup, post out v1 launch, we will consolidate this.

I was able to test successfully using the UI, but not the CLI -- I think there may be a separate bug in CLI currently where the Collection-configured proxy is not actually attempted to be used -- the code seems to look for proxy.enabled: true

Yup, this discrepancy needs to be fixed. We will be using enabled in both places.

Thanks again @cwilper !

Merged!

@cwilper cwilper deleted the fix/https-proxy-agent-opts branch October 22, 2023 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Disabling SSL doesn't seem to work
2 participants