Skip to content

Internal proxy server doesn't work if http proxy is set #3662

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

Closed
mfukala opened this issue Jun 24, 2021 · 7 comments
Closed

Internal proxy server doesn't work if http proxy is set #3662

mfukala opened this issue Jun 24, 2021 · 7 comments
Labels
bug Something isn't working waiting-for-info Waiting for more information from submitter
Milestone

Comments

@mfukala
Copy link

mfukala commented Jun 24, 2021

Internal code-server's proxy for accessing services doesn't work if http proxy is configured.

Test case:

  1. create vscode extension that runs http server that returns "ok" on /status route, bind the server to port 3000
  2. install the extension into code-server
  3. run code-server --auth=none -vvv (so curl examples below work w/ auth) from an environment where http_proxy, https_proxy and no_proxy is set (ensure address 0.0.0.0 is in no_proxy)
  4. curl http://localhost:3000/status
  5. => you get OK response
  6. now (assuming code-server runs on port 7777) via proxy: curl http://localhost:7777/proxy/3000/status
  7. => you get 500 response with message "Maximum callstack exceeded"

Following code calls itself in an endless loop:

    at ProxyAgent.callback (/usr/local/Cellar/code-server/3.10.2/libexec/node_modules/proxy-agent/index.js:215:11)
    at /usr/local/Cellar/code-server/3.10.2/libexec/node_modules/agent-base/dist/src/promisify.js:6:16
    at new Promise (<anonymous>)
    at ProxyAgent.promisifiedCallback (/usr/local/Cellar/code-server/3.10.2/libexec/node_modules/agent-base/dist/src/promisify.js:5:16)
    at ProxyAgent.addRequest (/usr/local/Cellar/code-server/3.10.2/libexec/node_modules/agent-base/dist/src/index.js:187:38)
    at ProxyAgent.callback (/usr/local/Cellar/code-server/3.10.2/libexec/node_modules/proxy-agent/index.js:215:11)
    at /usr/local/Cellar/code-server/3.10.2/libexec/node_modules/agent-base/dist/src/promisify.js:6:16
    at new Promise (<anonymous>)
    at ProxyAgent.promisifiedCallback (/usr/local/Cellar/code-server/3.10.2/libexec/node_modules/agent-base/dist/src/promisify.js:5:16)

The reason is monkey patching of http(s).globalAgent done in proxy_agent.js.
The code in node_modules/proxy-agent/index.js (#httpOrHttps) is supposed to return bare http(s).globalAgent in case the processed http request is not supposed to be proxied but it returns the monkey patched instance which is the initiator of the call.

A very ugly workaround is to ensure proxy_agent#shouldEnableProxy returns false by adding "example.com" to no_proxy as the code comment suggests :-)

// But that's drastically unlikely.
function shouldEnableProxy() {
...```

@Chilipp
Copy link

Chilipp commented Aug 20, 2021

hey! thanks a lot @mfukala for this workaround! it would have taken me hours to come up with such a solution. I can confirm that setting an environment variable like NO_PROXY=example.com, <some-other-domains> works well!

Are there any ideas how to solve this? Or is this part of the investigation in #3841?

@jsjoeio jsjoeio added the bug Something isn't working label Aug 23, 2021
@jsjoeio jsjoeio added this to the Backlog milestone Aug 23, 2021
@jsjoeio
Copy link
Contributor

jsjoeio commented Aug 23, 2021

Thank you for the detailed notes, explanation of how to test and solution for fixing ♥️

Or is this part of the investigation in #3841?

I guess it is part of that investigation now! The plan is to add more tests for that part of the codebase. So now, we can fix that issue and add a test thanks to @mfukala's work!

@jsjoeio
Copy link
Contributor

jsjoeio commented Aug 23, 2021

I can't promise when we will get to this, but I'll move it to "On Deck" meaning it's ready to be placed in an upcoming version milestone.

@jsjoeio jsjoeio modified the milestones: Backlog, On Deck Aug 23, 2021
@stale
Copy link

stale bot commented Feb 19, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no activity occurs in the next 5 days.

@stale stale bot added the stale label Feb 19, 2022
@Chilipp
Copy link

Chilipp commented Feb 19, 2022

@jsjoeio do you know if this has been solved? I must admit that I did but test it within the past couple of months

@stale stale bot removed the stale label Feb 19, 2022
@jsjoeio
Copy link
Contributor

jsjoeio commented Feb 22, 2022

@Chilipp I haven't tested it recently (or implemented a fix specifically). So you're saying you did test it and it's fixed?

Otherwise, I wonder if you can simplify the repro steps and I can test

@jsjoeio jsjoeio added the waiting-for-info Waiting for more information from submitter label Feb 22, 2022
@code-asher
Copy link
Member

Closing for inactivity. But also this might be OK now? I think we only monkey patch in the code-server process, not in the extension host.

@code-asher code-asher closed this as not planned Won't fix, can't repro, duplicate, stale Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working waiting-for-info Waiting for more information from submitter
Projects
None yet
Development

No branches or pull requests

4 participants