Incorrectly instantiated proxy URL

[waldekmastykarz]

• Package Name: core-rest-pipeline
• Package Version: latest in source
• Operating system: all
• nodejs
  • version:
• browser
  • name/version:
• typescript
  • version:
• Is the bug related to documentation in
  • README.md
  • source code documentation
  • SDK API docs on https://learn.microsoft.com

Describe the bug
In the proxy policy, we try to instantiate the proxy URL using only the host:

azure-sdk-for-js/sdk/core/core-rest-pipeline/src/policies/proxyPolicy.ts

Line 147 in cf4ef57

parsedProxyUrl = new URL(settings.host);

. This fails with an exception, because URL expects at least a protocol and a host name.

To Reproduce
Steps to reproduce the behavior:
1.

Expected behavior
Change how we instantiate the proxy URL from ProxySettings

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Add any other context about the problem here.

[deyaaeldeen]

Which client library are you using?

[waldekmastykarz]

I came across this issue through applicationinsights for Node.js.

[jeremymeng]

@waldekmastykarz could you please provide more details on how to reproduce this issue? If possible, a simple project, or step-by-step instructions would be greatly appreciated.

[xirzec]

@waldekmastykarz I assume you were passing the proxy information via client options rather than using an environment variable like HTTPS_PROXY and were perhaps surprised that the field is named host but requires the protocol in addition to the host?

I notice the doc comment doesn't specify this or give an example:

azure-sdk-for-js/sdk/core/core-rest-pipeline/src/interfaces.ts

Line 323 in 12bbcbf

* The proxy's host address.

At the very least we could improve the comments to make this more obvious. In addition, I think it's not unreasonable for us to support protocol-less hosts either, but would we assume HTTPS or HTTP in that case?

[waldekmastykarz]

Correct, we're configuring the proxy URL using app insight's options: https://github.com/pnp/cli-microsoft365/blob/b872a3f76422064efbe6d0a86b42d73f0e1d84a7/src/appInsights.ts#L33-L34. This option is described in the docs at https://github.com/microsoft/ApplicationInsights-node.js/blob/03c380558f15fd46c20ba90de343e8d427f1be30/README.md?plain=1#L126-L127.

While we specify the proxy URL as http://127.0.0.1:8000 it seems like the information about the protocol is lost when the core pipeline SDK is parsing the retrieved options object.

[jeremymeng]

@JacksonWeber I remember you worked on a related issue before?

[JacksonWeber]

@jeremymeng this had to do with how we were parsing the env vars for proxy settings on the SDK side, but we were hitting the same logic that was causing a crash I believe microsoft/ApplicationInsights-node.js#1331.

[jeremymeng]

In addition, I think it's not unreasonable for us to support protocol-less hosts either, but would we assume HTTPS or HTTP in that case?

I disagree as the protocol part is important for the proxy server, and I rarely see it is omitted.

While we specify the proxy URL as http://127.0.0.1:8000 it seems like the information about the protocol is lost when the core pipeline SDK is parsing the retrieved options object.

@JacksonWeber could this code be related? I am not familiar how the azure monitor option is passed to core-rest-pipeline

https://github.com/microsoft/ApplicationInsights-node.js/blob/03c380558f15fd46c20ba90de343e8d427f1be30/src/shim/shim-config.ts#L232-L235