Skip to content
This repository was archived by the owner on Jan 3, 2026. It is now read-only.

fix: address codeql warning with hostname matches#415

Merged
bcoe merged 3 commits into
mainfrom
reggie
Sep 14, 2021
Merged

fix: address codeql warning with hostname matches#415
bcoe merged 3 commits into
mainfrom
reggie

Conversation

@JustinBeckwith

@JustinBeckwith JustinBeckwith commented Jul 13, 2021

Copy link
Copy Markdown
Contributor

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

@JustinBeckwith JustinBeckwith requested a review from a team 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
Comment thread src/gaxios.ts
return !!noProxyUrls.find(url => {
if (url.startsWith('*.') || url.startsWith('.')) {
url = url.replace('*', '');
url = url.replace(/^\*\./, '.');

Copy link
Copy Markdown

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
Copy Markdown
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.

@bcoe bcoe Jul 14, 2021

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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.

@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
Copy Markdown
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
@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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants