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.

Sign up for GitHub

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: remove custom proxy handling #143

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
+14,580 −11,681
Draft

fix: remove custom proxy handling #143

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ae98c18
fix: remove custom proxy handling
parkerbxyz Jun 13, 2024
98eac26
fix: use unidici fetch
parkerbxyz Jun 13, 2024
fa33f86
Update code comment
parkerbxyz Jun 13, 2024
5cb4b2f
fix: implement `EnvHttpProxyAgent`
parkerbxyz Sep 3, 2024
28dfb1e
Merge branch 'main' into pr/143
parkerbxyz Sep 3, 2024
19e441f
Merge branch 'main' into 134-remove-custom-proxy-functionality
parkerbxyz Sep 3, 2024
1f00388
Update node version for `undici` proxy support
parkerbxyz Sep 4, 2024
6aa1ac7
Don't warn if using a node version greater than 20
parkerbxyz Sep 4, 2024
073d254
Remove `EnvHttpProxyAgent` implementation
parkerbxyz Sep 4, 2024
adf089d
Update package-lock.json
parkerbxyz Sep 4, 2024
4e61ab3
Update dist files
parkerbxyz Sep 4, 2024
ed89981
Merge branch 'main' into pr/143
parkerbxyz Sep 6, 2024
02b34af
Re-implement `EnvHttpProxyAgent` as local agent
parkerbxyz Sep 6, 2024
ff0675e
Use native fetch
parkerbxyz Sep 7, 2024
7a505dc
Update dist files
parkerbxyz Sep 7, 2024
a0d0cbe
Merge branch 'main' into 134-remove-custom-proxy-functionality
parkerbxyz Oct 31, 2024
489a969
Re-implement global dispatcher
parkerbxyz Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ jobs:

- uses: actions/setup-node@v4
with:
node-version-file: .node-version
node-version-file: package.json
cache: 'npm'

- run: npm ci
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ jobs:

- uses: actions/setup-node@v4
with:
node-version-file: .node-version
node-version-file: package.json
cache: 'npm'

- run: npm ci
Expand Down
1 change: 0 additions & 1 deletion .node-version
View file
Open in desktop

This file was deleted.

256 changes: 175 additions & 81 deletions dist/main.cjs
View file
Open in desktop

Large diffs are not rendered by default.

105 changes: 53 additions & 52 deletions dist/post.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,10 +560,10 @@ var require_proxy = __commonJS({
})();
if (proxyVar) {
try {
return new URL(proxyVar);
return new DecodedURL(proxyVar);
} catch (_a) {
if (!proxyVar.startsWith("http://") && !proxyVar.startsWith("https://"))
return new URL(`http://${proxyVar}`);
return new DecodedURL(`http://${proxyVar}`);
}
} else {
return void 0;
Expand Down Expand Up @@ -606,6 +606,19 @@ var require_proxy = __commonJS({
const hostLower = host.toLowerCase();
return hostLower === "localhost" || hostLower.startsWith("127.") || hostLower.startsWith("[::1]") || hostLower.startsWith("[0:0:0:0:0:0:0:1]");
}
var DecodedURL = class extends URL {
constructor(url, base) {
super(url, base);
this._decodedUsername = decodeURIComponent(super.username);
this._decodedPassword = decodeURIComponent(super.password);
}
get username() {
return this._decodedUsername;
}
get password() {
return this._decodedPassword;
}
};
}
});

Expand Down Expand Up @@ -11396,7 +11409,7 @@ var require_proxy_agent = __commonJS({
function defaultFactory(origin, opts) {
return new Pool(origin, opts);
}
var ProxyAgent2 = class extends DispatcherBase {
var ProxyAgent = class extends DispatcherBase {
constructor(opts) {
super(opts);
this[kProxy] = buildProxyOptions(opts);
Expand Down Expand Up @@ -11509,7 +11522,7 @@ var require_proxy_agent = __commonJS({
throw new InvalidArgumentError("Proxy-Authorization should be sent in ProxyAgent constructor");
}
}
module2.exports = ProxyAgent2;
module2.exports = ProxyAgent;
}
});

Expand Down Expand Up @@ -17477,7 +17490,7 @@ var require_undici = __commonJS({
var MockAgent = require_mock_agent();
var MockPool = require_mock_pool();
var mockErrors = require_mock_errors();
var ProxyAgent2 = require_proxy_agent();
var ProxyAgent = require_proxy_agent();
var RetryHandler = require_RetryHandler();
var { getGlobalDispatcher, setGlobalDispatcher } = require_global2();
var DecoratorHandler = require_DecoratorHandler();
Expand All @@ -17496,7 +17509,7 @@ var require_undici = __commonJS({
module2.exports.Pool = Pool;
module2.exports.BalancedPool = BalancedPool;
module2.exports.Agent = Agent;
module2.exports.ProxyAgent = ProxyAgent2;
module2.exports.ProxyAgent = ProxyAgent;
module2.exports.RetryHandler = RetryHandler;
module2.exports.DecoratorHandler = DecoratorHandler;
module2.exports.RedirectHandler = RedirectHandler;
Expand Down Expand Up @@ -17703,8 +17716,8 @@ var require_lib = __commonJS({
MediaTypes2["ApplicationJson"] = "application/json";
})(MediaTypes || (exports2.MediaTypes = MediaTypes = {}));
function getProxyUrl(serverUrl) {
const proxyUrl2 = pm.getProxyUrl(new URL(serverUrl));
return proxyUrl2 ? proxyUrl2.href : "";
const proxyUrl = pm.getProxyUrl(new URL(serverUrl));
return proxyUrl ? proxyUrl.href : "";
}
exports2.getProxyUrl = getProxyUrl;
var HttpRedirectCodes = [
Expand Down Expand Up @@ -18039,12 +18052,12 @@ var require_lib = __commonJS({
}
getAgentDispatcher(serverUrl) {
const parsedUrl = new URL(serverUrl);
const proxyUrl2 = pm.getProxyUrl(parsedUrl);
const useProxy = proxyUrl2 && proxyUrl2.hostname;
const proxyUrl = pm.getProxyUrl(parsedUrl);
const useProxy = proxyUrl && proxyUrl.hostname;
if (!useProxy) {
return;
}
return this._getProxyAgentDispatcher(parsedUrl, proxyUrl2);
return this._getProxyAgentDispatcher(parsedUrl, proxyUrl);
}
_prepareRequest(method, requestUrl, headers) {
const info = {};
Expand Down Expand Up @@ -18084,8 +18097,8 @@ var require_lib = __commonJS({
}
_getAgent(parsedUrl) {
let agent;
const proxyUrl2 = pm.getProxyUrl(parsedUrl);
const useProxy = proxyUrl2 && proxyUrl2.hostname;
const proxyUrl = pm.getProxyUrl(parsedUrl);
const useProxy = proxyUrl && proxyUrl.hostname;
if (this._keepAlive && useProxy) {
agent = this._proxyAgent;
}
Expand All @@ -18100,16 +18113,16 @@ var require_lib = __commonJS({
if (this.requestOptions) {
maxSockets = this.requestOptions.maxSockets || http.globalAgent.maxSockets;
}
if (proxyUrl2 && proxyUrl2.hostname) {
if (proxyUrl && proxyUrl.hostname) {
const agentOptions = {
maxSockets,
keepAlive: this._keepAlive,
proxy: Object.assign(Object.assign({}, (proxyUrl2.username || proxyUrl2.password) && {
proxyAuth: `${proxyUrl2.username}:${proxyUrl2.password}`
}), { host: proxyUrl2.hostname, port: proxyUrl2.port })
proxy: Object.assign(Object.assign({}, (proxyUrl.username || proxyUrl.password) && {
proxyAuth: `${proxyUrl.username}:${proxyUrl.password}`
}), { host: proxyUrl.hostname, port: proxyUrl.port })
};
let tunnelAgent;
const overHttps = proxyUrl2.protocol === "https:";
const overHttps = proxyUrl.protocol === "https:";
if (usingSsl) {
tunnelAgent = overHttps ? tunnel.httpsOverHttps : tunnel.httpsOverHttp;
} else {
Expand All @@ -18130,7 +18143,7 @@ var require_lib = __commonJS({
}
return agent;
}
_getProxyAgentDispatcher(parsedUrl, proxyUrl2) {
_getProxyAgentDispatcher(parsedUrl, proxyUrl) {
let proxyAgent;
if (this._keepAlive) {
proxyAgent = this._proxyAgentDispatcher;
Expand All @@ -18139,8 +18152,8 @@ var require_lib = __commonJS({
return proxyAgent;
}
const usingSsl = parsedUrl.protocol === "https:";
proxyAgent = new undici_1.ProxyAgent(Object.assign({ uri: proxyUrl2.href, pipelining: !this._keepAlive ? 0 : 1 }, (proxyUrl2.username || proxyUrl2.password) && {
token: `${proxyUrl2.username}:${proxyUrl2.password}`
proxyAgent = new undici_1.ProxyAgent(Object.assign({ uri: proxyUrl.href, pipelining: !this._keepAlive ? 0 : 1 }, (proxyUrl.username || proxyUrl.password) && {
token: `Basic ${Buffer.from(`${proxyUrl.username}:${proxyUrl.password}`).toString("base64")}`
}));
this._proxyAgentDispatcher = proxyAgent;
if (usingSsl && this._ignoreSslError) {
Expand Down Expand Up @@ -26762,7 +26775,7 @@ var require_proxy_agent2 = __commonJS({
function defaultFactory(origin, opts) {
return new Pool(origin, opts);
}
var ProxyAgent2 = class extends DispatcherBase {
var ProxyAgent = class extends DispatcherBase {
constructor(opts) {
super();
if (!opts || typeof opts === "object" && !(opts instanceof URL3) && !opts.uri) {
Expand Down Expand Up @@ -26889,7 +26902,7 @@ var require_proxy_agent2 = __commonJS({
throw new InvalidArgumentError("Proxy-Authorization should be sent in ProxyAgent constructor");
}
}
module2.exports = ProxyAgent2;
module2.exports = ProxyAgent;
}
});

Expand All @@ -26899,14 +26912,14 @@ var require_env_http_proxy_agent = __commonJS({
"use strict";
var DispatcherBase = require_dispatcher_base2();
var { kClose, kDestroy, kClosed, kDestroyed, kDispatch, kNoProxyAgent, kHttpProxyAgent, kHttpsProxyAgent } = require_symbols6();
var ProxyAgent2 = require_proxy_agent2();
var ProxyAgent = require_proxy_agent2();
var Agent = require_agent2();
var DEFAULT_PORTS = {
"http:": 80,
"https:": 443
};
var experimentalWarned = false;
var EnvHttpProxyAgent = class extends DispatcherBase {
var EnvHttpProxyAgent2 = class extends DispatcherBase {
#noProxyValue = null;
#noProxyEntries = null;
#opts = null;
Expand All @@ -26923,13 +26936,13 @@ var require_env_http_proxy_agent = __commonJS({
this[kNoProxyAgent] = new Agent(agentOpts);
const HTTP_PROXY = httpProxy ?? process.env.http_proxy ?? process.env.HTTP_PROXY;
if (HTTP_PROXY) {
this[kHttpProxyAgent] = new ProxyAgent2({ ...agentOpts, uri: HTTP_PROXY });
this[kHttpProxyAgent] = new ProxyAgent({ ...agentOpts, uri: HTTP_PROXY });
} else {
this[kHttpProxyAgent] = this[kNoProxyAgent];
}
const HTTPS_PROXY = httpsProxy ?? process.env.https_proxy ?? process.env.HTTPS_PROXY;
if (HTTPS_PROXY) {
this[kHttpsProxyAgent] = new ProxyAgent2({ ...agentOpts, uri: HTTPS_PROXY });
this[kHttpsProxyAgent] = new ProxyAgent({ ...agentOpts, uri: HTTPS_PROXY });
} else {
this[kHttpsProxyAgent] = this[kHttpProxyAgent];
}
Expand Down Expand Up @@ -27025,7 +27038,7 @@ var require_env_http_proxy_agent = __commonJS({
return process.env.no_proxy ?? process.env.NO_PROXY ?? "";
}
};
module2.exports = EnvHttpProxyAgent;
module2.exports = EnvHttpProxyAgent2;
}
});

Expand Down Expand Up @@ -36269,8 +36282,8 @@ var require_undici2 = __commonJS({
var Pool = require_pool2();
var BalancedPool = require_balanced_pool2();
var Agent = require_agent2();
var ProxyAgent2 = require_proxy_agent2();
var EnvHttpProxyAgent = require_env_http_proxy_agent();
var ProxyAgent = require_proxy_agent2();
var EnvHttpProxyAgent2 = require_env_http_proxy_agent();
var RetryAgent = require_retry_agent();
var errors = require_errors2();
var util = require_util8();
Expand All @@ -36292,8 +36305,8 @@ var require_undici2 = __commonJS({
module2.exports.Pool = Pool;
module2.exports.BalancedPool = BalancedPool;
module2.exports.Agent = Agent;
module2.exports.ProxyAgent = ProxyAgent2;
module2.exports.EnvHttpProxyAgent = EnvHttpProxyAgent;
module2.exports.ProxyAgent = ProxyAgent;
module2.exports.EnvHttpProxyAgent = EnvHttpProxyAgent2;
module2.exports.RetryAgent = RetryAgent;
module2.exports.RetryHandler = RetryHandler;
module2.exports.DecoratorHandler = DecoratorHandler;
Expand Down Expand Up @@ -36791,11 +36804,11 @@ var RequestError = class extends Error {
response;
constructor(message, statusCode, options) {
super(message);
if (Error.captureStackTrace) {
Error.captureStackTrace(this, this.constructor);
}
this.name = "HttpError";
this.status = statusCode;
this.status = Number.parseInt(statusCode);
if (Number.isNaN(this.status)) {
this.status = 0;
}
if ("response" in options) {
this.response = options.response;
}
Expand Down Expand Up @@ -36977,27 +36990,15 @@ var request = withDefaults2(endpoint, defaults_default);
// lib/request.js
var import_undici = __toESM(require_undici2(), 1);
var baseUrl = import_core.default.getInput("github-api-url").replace(/\/$/, "");
var proxyUrl = process.env.https_proxy || process.env.HTTPS_PROXY || process.env.http_proxy || process.env.HTTP_PROXY;
var proxyFetch = (url, options) => {
const urlHost = new URL(url).hostname;
const noProxy = (process.env.no_proxy || process.env.NO_PROXY || "").split(
","
);
if (!noProxy.includes(urlHost)) {
options = {
...options,
dispatcher: new import_undici.ProxyAgent(String(proxyUrl))
};
}
return (0, import_undici.fetch)(url, options);
};
var envHttpProxyAgent = new import_undici.EnvHttpProxyAgent();
var request_default = request.defaults({
headers: {
"user-agent": "actions/create-github-app-token"
},
baseUrl,
/* c8 ignore next */
request: proxyUrl ? { fetch: proxyFetch } : {}
request: {
dispatcher: envHttpProxyAgent
}
});

// post.js
Expand Down
34 changes: 5 additions & 29 deletions lib/request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,41 +1,17 @@
import core from "@actions/core";
import { request } from "@octokit/request";
import { ProxyAgent, fetch as undiciFetch } from "undici";
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need to explicitly use EnvHttpProxyAgent?
https://github.com/nodejs/undici/blob/7f635e51f6170f4b59abedc7cb45e6bcda7f056d/docs/docs/api/EnvHttpProxyAgent.md#L4

See this comment: nodejs/undici#1650 (comment)

I just wanted to leave that not for when we have another look at it, I don't have time to dig into it now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that in 5cb4b2f, but several tests started failing as a result. After reading through nodejs/undici#2994, I got the impression it was unnecessary to do this.

Added EnvHttpProxyAgent and made it the default global dispatcher.

Automatically detect/support HTTP_PROXY, HTTPS_PROXY, & NO_PROXY

Copy link

Choose a reason for hiding this comment

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

Super random drive-by comment. I got here from that issue. 😅

According to this commit, they ended up not using this as the default agent:
nodejs/undici@f31d3f9

Reasoning:
nodejs/undici#2994 (comment)

So it appears the PR description is wrong. Looks like they'll consider making it the default in a major release.

For now, I was able to get HTTPS_PROXY env variable to work via:

import { EnvHttpProxyAgent, setGlobalDispatcher } from "undici";

setGlobalDispatcher(new EnvHttpProxyAgent());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @bruce-c-liu! I found some additional context here:

https://github.com/nodejs/TSC/blob/41602b95211f30107696a007e5da45b9e933536c/meetings/2024-09-04.md?plain=1#L88-L115

import { EnvHttpProxyAgent } from "undici";

const baseUrl = core.getInput("github-api-url").replace(/\/$/, "");

// https://docs.github.com/actions/hosting-your-own-runners/managing-self-hosted-runners/using-a-proxy-server-with-self-hosted-runners
const proxyUrl =
process.env.https_proxy ||
process.env.HTTPS_PROXY ||
process.env.http_proxy ||
process.env.HTTP_PROXY;

/* c8 ignore start */
// Native support for proxies in Undici is under consideration: https://github.com/nodejs/undici/issues/1650
// Until then, we need to use a custom fetch function to add proxy support.
const proxyFetch = (url, options) => {
const urlHost = new URL(url).hostname;
const noProxy = (process.env.no_proxy || process.env.NO_PROXY || "").split(
","
);

if (!noProxy.includes(urlHost)) {
options = {
...options,
dispatcher: new ProxyAgent(String(proxyUrl)),
};
}

return undiciFetch(url, options);
};
/* c8 ignore stop */
const envHttpProxyAgent = new EnvHttpProxyAgent();

export default request.defaults({
headers: {
"user-agent": "actions/create-github-app-token",
},
baseUrl,
/* c8 ignore next */
request: proxyUrl ? { fetch: proxyFetch } : {},
request: {
dispatcher: envHttpProxyAgent,
Copy link
Contributor

@gr2m gr2m Sep 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this will do anything. There is. no dispatch option in options.request, see all implemented options in https://github.com/octokit/request.js#request

What we have to do is to implement a fetch wrapper

function fetchWithProxyAgent(url, options) {
  return fetch(url, { ... options, dispatcher: envHttpProxyAgent })
}

And then use pass that custom fetch implementation as options.request.fetch

Suggested change
dispatcher: envHttpProxyAgent,
fetch: fetchWithProxyAgent,

},
});
Loading
Loading