Standardize behaviour of no_proxy environmental variable #1223

felixlut · 2022-10-25T20:07:04Z

The behaviour of the no_proxy environmental variable is famously inconsistent, but this change should follow how it usually works (i.e., that no_proxy=domain.com matches its subdomains, such as some.domain.com).

Resolves #1172

aibaars · 2022-10-27T11:18:13Z

packages/http-client/src/proxy.ts

@@ -51,7 +51,7 @@ export function checkBypass(reqUrl: URL): boolean {
    .split(',')
    .map(x => x.trim().toUpperCase())
    .filter(x => x)) {
-    if (upperReqHosts.some(x => x === upperNoProxyItem)) {
+    if (upperReqHosts.some(x => x.includes(upperNoProxyItem))) {


I think it would be safer to use endsWith instead of includes to avoid bad actors from bypassing the proxy. For example if we set no_proxy=mycompany.com then a domain like mycompany.com.evil.org should not bypass the proxy.

Suggested change

if (upperReqHosts.some(x => x.includes(upperNoProxyItem))) {

if (upperReqHosts.some(x => x.endsWith(upperNoProxyItem))) {

The above would still let domains like evilmycompany.com bypass the proxy. Perhaps a better formulation would be

Suggested change

if (upperReqHosts.some(x => x.includes(upperNoProxyItem))) {

if (upperReqHosts.some(x => x === upperNoProxyItem || x.endsWith('.' + upperNoProxyItem))) {

In addition we might want to strip off a leading . to ensure that no_proxy=example.com and no_proxy=.example.com are treated the same.

I think the following should work:

// Compare request host against noproxy for (const upperNoProxyItem of noProxy .split(',') .map(x => x.trim().toUpperCase()) .map(x => x.charAt(0) === '.' ? x.substr(1) : x) .filter(x => x)) { if (upperReqHosts.some(x => x === upperNoProxyItem || x.endsWith('.' + upperNoProxyItem))) { return true } }

+1 for @aibaars suggestions too, ensWith + '.' sounds good
NIT: I'd consider a small refactoring of the block from line 54, it's becoming a bit dense with filters

@felixlut Could you add a test-case that covers the change in https://github.com/actions/toolkit/blob/main/packages/http-client/__tests__/proxy.test.ts ?

PS the PR validation currently fails due to some older packages, I'll take a look in a different PR

I like your suggestions, and sure I can add a test-case. I'll try to find time over the weekend!

I've implemented the suggested changes (in addition to some other suggestions from the article I posted above), as well as test-cases for each of them. Namely the following was added:

* matching all hosts

Strip leading dots (.domain.com --> domain.com)

aibaars

This looks good to me. @fhammerl , could you have a look at this?

TingluoHuang · 2022-11-01T14:04:48Z

@fhammerl We also need to make the actions/runner follow the same rule if we take this change.

fhammerl · 2022-11-01T14:05:59Z

@TingluoHuang actions/runner for sure, can you think of any other services where we use noproxy?

TingluoHuang · 2022-11-01T14:35:47Z

@fhammerl

@TingluoHuang actions/runner for sure, can you think of any other services where we use noproxy?

No

aibaars · 2022-11-01T17:33:42Z

@fhammerl Could you update and merge #1226 ? I think that PR is meant to unblock the audit failure.

felixlut · 2022-11-06T11:51:05Z

Haven't seen any activity in here for a couple of days. Just checking, there is nothing I should do now? This PR is just blocked until #1230 is merged?

felixlut · 2022-11-06T11:51:15Z

Haven't seen any activity in here for a couple of days. Just checking, there is nothing I should do now? This PR is just blocked until #1230 is merged?

fhammerl

Great tests, thanks for the refactoring too.

LGTM

We're holding off with merging it for now as the 'audit' check is failing.
We'll fix that first on main, then merge this branch.

felixlut · 2022-11-25T16:48:50Z

@fhammerl any update?

fhammerl · 2022-12-14T23:27:32Z

We're migrating the project over to Node16, will merge this PR right after.

gendergap · 2022-12-28T08:21:36Z

We're migrating the project over to Node16, will merge this PR right after.

Could you please reference the PR for that migration so that people waiting for this can track the progress?

vmjoseph · 2023-01-24T16:04:21Z

@felixlut Would you mind committing one more time so that our checks can re-run? Once those clear we can get this ready to merge.

felixlut · 2023-02-04T20:23:56Z

@vmjoseph Any update on this?

felixlut · 2023-02-07T14:25:54Z

Any update on this? @fhammerl @aibaars

vmjoseph

Looks like there's a linting error.

packages/http-client/__tests__/proxy.test.ts

This reverts commit 0e925a6.

fhammerl · 2023-02-09T17:57:18Z

Hey @felixlut
This is a long lived PR now, thanks for sticking by!

We'd like to follow the no_proxy system we've established in actions/runner

The runner rules today:

✅ Support matching for subdomains (the meat of this PR)
❌ Do NOT strip leading period in no_proxy entry (aka no_proxy=.github.com should bypass proxy for sub.github.com, but not for vanilla github.com urls)
❌ Do NOT support * wildcard

This would bring the behaviour to that of wget's if I understand the table correctly.

With those changes, I can get this PR merged ASAP.

Let me know if that works for you

felixlut · 2023-02-09T18:03:47Z

Sounds good to me, as you say, the subdomains was the reason i opened this! I can make the required rollbacks later tonight, or more likely tomorrow!

fhammerl · 2023-02-10T13:45:30Z

packages/http-client/__tests__/proxy.test.ts

+    const bypass = pm.checkBypass(new URL('https://anything.whatsoever.com'))
+    expect(bypass).toBeFalsy()
+  })
+


Can We add one more test to chech a subdomain is supported when no_proxy has a leading dot?

it('checkBypass returns true if host with subdomain in no_proxy defined with leading "."', () => { process.env['no_proxy'] = '.myserver.com' const bypass = pm.checkBypass(new URL('https://sub.myserver.com')) expect(bypass).toBeTruthy() })

Ye sure!

Another question also, since you want the no_proxy to be the same as in the actions/runner, do you also want me to re-structure the code to be more similar to how they handle it (see ref)?
Obviously it's not the same language, but might be easier to keep consistency between them if they are implemented using the same logic, just with different languages.

Might be overkill, but I'm willing to do it if that makes it easier maintainable for you guys!

I'd prefer to keep PRs for the functional changes and refactoring changes separate, but really appreciate you asking 👍

Also, here's a good set of unit tests from the runner code if needed

I''ve added the test you wrote, as well a new one for checking partial domains (such that no_proxy=myserver.com won't let evilmyserver.com through).

This required some extra logic

fhammerl · 2023-02-13T14:00:37Z

@felixlut Thanks again for this, I'll ping you as we publish it 👍

match no_proxy to subdomains

da7e8df

felixlut requested a review from a team as a code owner October 25, 2022 20:07

aibaars reviewed Oct 27, 2022

View reviewed changes

strip leading dot + '*' match all + testcases

586ad49

felixlut requested review from fhammerl and aibaars and removed request for fhammerl and aibaars October 30, 2022 16:03

felixlut changed the title ~~Match no_proxy environmental variable to partial domains~~ Standardize behaviour of no_proxy environmental variable Oct 30, 2022

aibaars approved these changes Nov 1, 2022

View reviewed changes

fhammerl approved these changes Nov 7, 2022

View reviewed changes

Update proxy.test.ts

0e925a6

aibaars mentioned this pull request Feb 3, 2023

Supply authorization parameter to toolcache.downloadTool() github/codeql-action#1516

Merged

3 tasks

vmjoseph approved these changes Feb 7, 2023

View reviewed changes

vmjoseph suggested changes Feb 7, 2023

View reviewed changes

packages/http-client/__tests__/proxy.test.ts Outdated Show resolved Hide resolved

Revert "Update proxy.test.ts"

2ceb28b

This reverts commit 0e925a6.

remove support for leading dots and wildcard no_proxy

b91602c

felixlut requested a review from fhammerl February 10, 2023 10:18

change order of tests for logic consistency

2a10602

fhammerl reviewed Feb 10, 2023

View reviewed changes

felixlut added 2 commits February 12, 2023 00:48

add test for working leading dot

f9f138e

add check for partial domain, as opposed to subdomain

bad8f88

felixlut requested a review from vmjoseph February 13, 2023 10:13

fhammerl merged commit d2b7d85 into actions:main Feb 13, 2023

felixlut deleted the partial-no-proxy branch February 14, 2023 16:37

fhammerl mentioned this pull request Mar 6, 2023

Bump http-client to version 2.1.0 #1364

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize behaviour of no_proxy environmental variable #1223

Standardize behaviour of no_proxy environmental variable #1223

felixlut commented Oct 25, 2022

aibaars Oct 27, 2022 •

edited

Loading

fhammerl Oct 28, 2022

felixlut Oct 28, 2022

felixlut Oct 30, 2022

aibaars left a comment

TingluoHuang commented Nov 1, 2022

fhammerl commented Nov 1, 2022

TingluoHuang commented Nov 1, 2022

aibaars commented Nov 1, 2022

felixlut commented Nov 6, 2022

felixlut commented Nov 6, 2022

fhammerl left a comment

felixlut commented Nov 25, 2022

fhammerl commented Dec 14, 2022

gendergap commented Dec 28, 2022

vmjoseph commented Jan 24, 2023

felixlut commented Feb 4, 2023

felixlut commented Feb 7, 2023

vmjoseph left a comment

fhammerl commented Feb 9, 2023

felixlut commented Feb 9, 2023

fhammerl Feb 10, 2023

felixlut Feb 10, 2023 •

edited

Loading

fhammerl Feb 10, 2023

felixlut Feb 12, 2023 •

edited

Loading

fhammerl commented Feb 13, 2023

	if (upperReqHosts.some(x => x.includes(upperNoProxyItem))) {
	if (upperReqHosts.some(x => x.endsWith(upperNoProxyItem))) {

	if (upperReqHosts.some(x => x.includes(upperNoProxyItem))) {
	if (upperReqHosts.some(x => x === upperNoProxyItem \|\| x.endsWith('.' + upperNoProxyItem))) {

Standardize behaviour of no_proxy environmental variable #1223

Standardize behaviour of no_proxy environmental variable #1223

Conversation

felixlut commented Oct 25, 2022

aibaars Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

fhammerl Oct 28, 2022

Choose a reason for hiding this comment

felixlut Oct 28, 2022

Choose a reason for hiding this comment

felixlut Oct 30, 2022

Choose a reason for hiding this comment

aibaars left a comment

Choose a reason for hiding this comment

TingluoHuang commented Nov 1, 2022

fhammerl commented Nov 1, 2022

TingluoHuang commented Nov 1, 2022

aibaars commented Nov 1, 2022

felixlut commented Nov 6, 2022

felixlut commented Nov 6, 2022

fhammerl left a comment

Choose a reason for hiding this comment

felixlut commented Nov 25, 2022

fhammerl commented Dec 14, 2022

gendergap commented Dec 28, 2022

vmjoseph commented Jan 24, 2023

felixlut commented Feb 4, 2023

felixlut commented Feb 7, 2023

vmjoseph left a comment

Choose a reason for hiding this comment

fhammerl commented Feb 9, 2023

felixlut commented Feb 9, 2023

fhammerl Feb 10, 2023

Choose a reason for hiding this comment

felixlut Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

fhammerl Feb 10, 2023

Choose a reason for hiding this comment

felixlut Feb 12, 2023 • edited Loading

Choose a reason for hiding this comment

fhammerl commented Feb 13, 2023

aibaars Oct 27, 2022 •

edited

Loading

felixlut Feb 10, 2023 •

edited

Loading

felixlut Feb 12, 2023 •

edited

Loading