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.

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

add proxy support for --verify #170

Closed

Conversation

chewiebug
Copy link

@chewiebug chewiebug commented Aug 31, 2018

I would like to use "gulp --verify" behind a proxy server. The current implementation does not seem to be able to support a proxy configuration. Using the hints given in #166, I used https-proxy-agent and configuration via .gulp.* to achieve proxy support.

Note: To maintain backwards compatibility to node 0.10.x and 0.12.x, I have used https-proxy-agent:1.0.0 instead of the latest version (2.2.1). According to https://hackerone.com/reports/319532, the 1.0.0 version contains a vulnerability, which is only fixed with 2.2.0.

Does this look ok to you?

@chewiebug
Copy link
Author

After doing some research, I suppose, adding a polyfill (safer-buffer including an eslint rule to prevent new usage fo the deprecated api) will be the better option, than to introduce the known vulnerability.

And since gulp just added node 10 to the CI matrices, it would probably also be a good idea to make sure, this runs on node 10, as well.

@phated, @sttk: If you have any input / thougts on this, they are very much appreciated!

@chewiebug
Copy link
Author

Trying to update to the latest version of https-proxy-agent fails on node 0.10 / 0.12, because "const" is used in agent-base.

Error message

-D:\Users\joerg2\Daten\gulp-cli\node_modules\https-proxy-agent\node_modules\agent-base\index.js:3
-const inherits = require('util').inherits;
-^^^^^
-SyntaxError: Use of const in strict mode.
      -    at Module._compile (module.js:439:25)
      -    at Object.Module._extensions..js (module.js:474:10)
      -    at Module.load (module.js:356:32)
      -    at Function.Module._load (module.js:312:12)
      -    at Module.require (module.js:364:17)
      -    at require (module.js:380:17)
      -    at Object.<anonymous> (D:\Users\joerg2\Daten\gulp-cli\node_modules\https-proxy-agent\index.js:8:13)
      -    at Module._compile (module.js:456:26)
      -    at Object.Module._extensions..js (module.js:474:10)
      -    at Module.load (module.js:356:32)

So, I suppose, polyfilling is not the way to go here.

@chewiebug chewiebug force-pushed the feature/add-proxy-support-for-verify branch from 9ed644f to 11ba91f Compare September 7, 2018 20:59
@chewiebug chewiebug force-pushed the feature/add-proxy-support-for-verify branch from 11ba91f to 33f62b0 Compare September 30, 2018 16:50
@chewiebug
Copy link
Author

chewiebug commented Oct 2, 2018

After some more trying, I was able to make it run on node 0.10 / 0.12: master...chewiebug:feature/add-proxy-support-for-verify-polyfills

Drawbacks

  • Now every run needs the "--harmony" flag:
node --harmony .\bin\gulp --verify
  • I don't know, how to make the unittests run with the --harmony flag. Mocha passes the --harmony flag to node, but I suspect, that somewhere later another process is spawned, where the flag is not passed on.

@chewiebug
Copy link
Author

So, my current bottom line is

  • I don't see a good way to use [email protected] with node 0.10 / 0.12, because it will
    -- only run with the --harmony flag
    -- I don't see how I can make the unittests run
  • I can make it run with [email protected], but
    -- a known vulnerability is introduced, if the someone uses the "Proxy-Authorization" header for proxy configuration

So, this pull request contains the second option. @phated, @sttk I think, I need your opinion here. Any input is very much appreciated.

@chewiebug chewiebug force-pushed the feature/add-proxy-support-for-verify branch from 33f62b0 to af9a3e5 Compare August 19, 2019 15:56
@phated
Copy link
Member

phated commented Oct 21, 2020

With our new website design, the blacklist no longer exists and we don't know if we are bringing it back, so I'm just going to close this.

@phated phated closed this Oct 21, 2020
@chewiebug
Copy link
Author

chewiebug commented Oct 24, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants