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

fix: address codeql warning with hostname matches #415

Merged
merged 3 commits into from
Sep 14, 2021
Merged

fix: address codeql warning with hostname matches #415

merged 3 commits into from
Sep 14, 2021

Conversation

JustinBeckwith
Copy link
Contributor

@JustinBeckwith JustinBeckwith commented Jul 13, 2021

I never write a regex without @alexander-fenster checking me first ...

Verified

This commit was signed with the committer’s verified signature.
himdel Martin Hradil
@JustinBeckwith JustinBeckwith requested a review from a team as a code owner July 13, 2021 20:09
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 13, 2021
@@ -83,7 +83,7 @@ function skipProxy(url: string) {
const parsedURL = new URL(url);
return !!noProxyUrls.find(url => {
if (url.startsWith('*.') || url.startsWith('.')) {
url = url.replace('*', '');
url = url.replace(/^\*\./, '.');
Copy link
Contributor

Choose a reason for hiding this comment

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

This behavior looks a little different, I think we were originally replacing a leading * with ``.

Now we replace a leading *. with ``.

I believe these are the two unit tests that this behavior is meant to facilitate:

    it('should proxy if no_proxy env variable has asterisk, but URL is not matching', async () => {
      const url = 'https://domain.example2.com';
      sandbox.stub(process, 'env').value({
        https_proxy: 'https://fake.proxy',
        no_proxy: '*.example.com',
      });
      const body = {hello: '🌎'};
      const scope = nock(url).get('/').reply(200, body);
      const res = await request({url});
      scope.done();
      assert.deepStrictEqual(res.data, body);
      assert.ok(res.config.agent instanceof HttpsProxyAgent);
    });

    it('should not proxy if no_proxy env variable starts with a dot, and URL partially matches', async () => {
      const url = 'https://domain.example.com';
      sandbox.stub(process, 'env').value({
        https_proxy: 'https://fake.proxy',
        no_proxy: '.example.com',
      });
      const body = {hello: '🌎'};
      const scope = nock(url).get('/').reply(200, body);
      const res = await request({url});
      scope.done();
      assert.deepStrictEqual(res.data, body);
      assert.strictEqual(res.config.agent, undefined);
    });

I'm scratching my head and wondering why the second continues working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The replace passes in a '.' now.

Copy link
Contributor

@bcoe bcoe Jul 14, 2021

Choose a reason for hiding this comment

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

Can't we get rid of the || url.startsWith('.') then? or what am I missing?

seems like it wouldn't have been doing anything.

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 was trying to change as little as possible 😆 I don't fully understand what this function does.

Copy link
Contributor

Choose a reason for hiding this comment

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

The goal is that if you provide a wild card URL like *.google.com in NO_PROXY, foo.google.com, bar.google.com, etc. should all not be proxied. This is also true of providing .google.com.

Verified

This commit was signed with the committer’s verified signature.
himdel Martin Hradil
@tmatsuo tmatsuo added the automerge Merge the pull request once unit tests and other checks pass. label Aug 17, 2021
@gcf-merge-on-green
Copy link
Contributor

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 17, 2021

Verified

This commit was signed with the committer’s verified signature.
himdel Martin Hradil
@bcoe bcoe merged commit 5a4d060 into main Sep 14, 2021
@bcoe bcoe deleted the reggie branch September 14, 2021 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants