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: replace superagent-proxy with direct use of proxy-agent #389

Merged
merged 3 commits into from
Aug 16, 2023

Conversation

robbkidd
Copy link
Member

@robbkidd robbkidd commented Aug 4, 2023

Which problem is this PR solving?

Short description of the changes

This is the equivalent behavior of the previous agent configuration. If a proxy URL is provided in libhoney config, a single ProxyAgent will be created with that proxy URL set for all connections. No nuanced lookup of proxy config from the environment based on target URL protocol.

proxy-agent is a hefty import, but we were importing an early edition of it (v5.0.0) already via superagent-proxy. Update: whew, bundlephobia really doesn't handle direct links well.

TODO?

  • updates to rollup configs
    • replace superagent-proxy with proxy-agent
    • omit proxy-agent from browser bundle?
  • maybe some more thorough proxy testing?

@robbkidd robbkidd requested a review from a team August 4, 2023 14:35
@robbkidd robbkidd added the type: security Security issues/fixes. label Aug 4, 2023
This is the equivalent behavior of the previous agent configuration. If
a proxy URL is provided in libhoney config, a single ProxyAgent will be
created with that proxy URL set for all connections. No nuanced lookup
of proxy config from the environment based on target URL protocol.

proxy-agent is a hefty import, but we were importing an early edition of
it already via superagent-proxy.
@robbkidd robbkidd force-pushed the robb.use-proxy-agent-directly branch from 2820419 to 894354b Compare August 4, 2023 14:38
@robbkidd robbkidd changed the title replace superagent-proxy with direct use of proxy-agent fix: replace superagent-proxy with direct use of proxy-agent Aug 4, 2023
@kyle-watershed
Copy link

@robbkidd thanks for putting up this fix! any movement on this? I'd like to use libhoney-js with the fix as soon as this is merged and released

@JamieDanielson JamieDanielson self-assigned this Aug 16, 2023
Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In manual testing this seems to work as it did before. We may want to merge this in and add additional testing as a follow-up.

@robbkidd robbkidd merged commit 798855a into main Aug 16, 2023
@robbkidd robbkidd deleted the robb.use-proxy-agent-directly branch August 16, 2023 21:42
vreynolds pushed a commit that referenced this pull request Aug 18, 2023
## Which problem is this PR solving?

- adding a bit more confidence in unit testing for proxy usage as a
follow-up from #389

## Short description of the changes

- extra test that confirms http headers and method are passed along to
the proxy server
- optional: added docker-compose for easy spinning up of proxies, along
with some extra attributes for event if the proxy is in use.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: security Security issues/fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sec: critical vulnerability in transitive dep vm2 (via superagent-proxy)
3 participants