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

Jump to bottom

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

Changes from 1 commit
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
2 changes: 1 addition & 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,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))) {
Copy link
Contributor

@aibaars aibaars Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

+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

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 like your suggestions, and sure I can add a test-case. I'll try to find time over the weekend!

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'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)

return true
}
}
Expand Down