-
Notifications
You must be signed in to change notification settings - Fork 62
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
Conversation
@@ -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(/^\*\./, '.'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
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. |
I never write a regex without @alexander-fenster checking me first ...