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

Standardize behaviour of no_proxy environmental variable #1223

Merged
merged 8 commits into from
Feb 13, 2023
Merged

Standardize behaviour of no_proxy environmental variable #1223

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
da7e8df
match no_proxy to subdomains
felixlut Oct 25, 2022
586ad49
strip leading dot + '*' match all + testcases
felixlut Oct 30, 2022
0e925a6
Update proxy.test.ts
felixlut Jan 27, 2023
2ceb28b
Revert "Update proxy.test.ts"
felixlut Feb 8, 2023
b91602c
remove support for leading dots and wildcard no_proxy
felixlut Feb 10, 2023
2a10602
change order of tests for logic consistency
felixlut Feb 10, 2023
f9f138e
add test for working leading dot
felixlut Feb 11, 2023
bad8f88
add check for partial domain, as opposed to subdomain
felixlut Feb 12, 2023
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
26 changes: 26 additions & 0 deletions packages/http-client/__tests__/proxy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,32 @@ describe('proxy', () => {
expect(bypass).toBeFalsy()
})

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

it('checkBypass returns false if no_proxy is subdomain', () => {
process.env['no_proxy'] = 'myserver.com'
const bypass = pm.checkBypass(new URL('https://myserver.com.evil.org'))
expect(bypass).toBeFalsy()
})

// Do not strip leading dots as per https://github.com/actions/runner/blob/97195bad5870e2ad0915ebfef1616083aacf5818/docs/adrs/0263-proxy-support.md
it('checkBypass returns false if host with leading dot in no_proxy', () => {
process.env['no_proxy'] = '.myserver.com'
const bypass = pm.checkBypass(new URL('https://myserver.com'))
expect(bypass).toBeFalsy()
})

// Do not match wildcard ("*") as per https://github.com/actions/runner/blob/97195bad5870e2ad0915ebfef1616083aacf5818/docs/adrs/0263-proxy-support.md
it('checkBypass returns true if no_proxy is "*"', () => {
process.env['no_proxy'] = '*'
const bypass = pm.checkBypass(new URL('https://anything.whatsoever.com'))
expect(bypass).toBeFalsy()
})

Copy link
Contributor

Choose a reason for hiding this comment

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

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()
  })

Copy link
Contributor Author

@felixlut felixlut Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@felixlut felixlut Feb 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

it('HttpClient does basic http get request through proxy', async () => {
process.env['http_proxy'] = _proxyUrl
const httpClient = new httpm.HttpClient()
Expand Down
6 changes: 5 additions & 1 deletion packages/http-client/src/proxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,11 @@ 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 === upperNoProxyItem || x.endsWith(`.${upperNoProxyItem}`)
)
) {
return true
}
}
Expand Down