hildjj / dohdec

Lookup and decode DNS records using DNS-over-HTTPS (DoH)
MIT License
21 stars 5 forks source link

Support got agent option #39

Open showi opened 2 years ago

showi commented 2 years ago

Hello, We want to be able to use a proxy so it will be nice to be able to pass the Agent option of GOT.

hildjj commented 2 years ago

I'm interested in taking this.

Can you please write a test for this? Please add yourself to the contributors section in package.json. This will land after #38, so there will probably need to be a rebase before we're done.

showi commented 2 years ago

Hello, I have test it locally by hacking the client to use a proxy (BURP). I have a hard time figuring out how to test the proxy within nock. We are basically just passing the option to got so... but i understand the requirement of the tests. Maybe you have some guidance :)

hildjj commented 2 years ago

I don't think we need an end-to-end proxy test. It would be enough to do a simple pass-through Agent like:

class TestAgent extends http.Agent {
  constructor(options) {
    super(options)
    this.called = 0
  }
  createConnection(options, listener) {
    this.called++
    super.createConnection(options, listener)
  }
}

then assert that called has the correct value.

showi commented 2 years ago

It seems that i can't just pass the agent, my guess is that nock has already override http.request so the agent instance is never called.

Mikescops commented 1 year ago

Hey, @hildjj can you help with the tests?

Indeed it seems that nock is intercepting the got call before, so using a custom TestAgent doesn't work.

Probably using mitm package would be better to intercept the network call and not the function call (but I might be mistaken as I'm not used to nock).

Thanks!

hildjj commented 2 months ago

I'm not going to take this for the release I'm working on right now. After I get the release out, we can rebase this and see if we can figure out a way to test it.

Dropping got in favor of the built-in fetch implementation is probably correct, but that moves us to requiring node 20+, which I'm not quite ready to do.

hildjj commented 2 months ago

ok, if you'd be willing to rebase this without adding anything else, I'm ready to work on getting it tested.