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

https: Use secureProtocol in Agent#getName #9452

Closed
wants to merge 1 commit into from
Closed

https: Use secureProtocol in Agent#getName #9452

wants to merge 1 commit into from

Conversation

papandreou
Copy link
Contributor

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

https

Description of change

Fix oversight pointed out by @bnoordhuis in #9324. The value of the secureProtocol option should be used as a "discriminator" when determining whether an existing HTTPS connection can be reused.

@nodejs-github-bot nodejs-github-bot added the https Issues or PRs related to the https subsystem. label Nov 3, 2016
@papandreou papandreou mentioned this pull request Nov 3, 2016
Closed
bnoordhuis
bnoordhuis requested changes Nov 4, 2016
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Left some comments.

test/parallel/test-https-agent-secure-protocol.js
@@ -0,0 +1,67 @@
'use strict';
var assert = require('assert');
var common = require('../common');
Copy link
Member

Choose a reason for hiding this comment

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

Can you use const here and elsewhere where it makes sense?

test/parallel/test-https-agent-secure-protocol.js
secureProtocol: 'SSLv23_method'
}, function(res) {
res.resume();
globalAgent.once('free', function () {
Copy link
Member

Choose a reason for hiding this comment

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

Style issue: can you drop the blank in function (). Same issue a few lines below.

test/parallel/test-https-agent-secure-protocol.js
var keys = Object.keys(globalAgent.freeSockets);
assert.equal(keys.length, 2);
assert.notEqual(keys[0].indexOf(':SSLv23_method'), -1);
assert.notEqual(keys[1].indexOf(':TLSv1_method'), -1);
Copy link
Member

Choose a reason for hiding this comment

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

Can you use assert.strictEqual and assert.notStrictEqual here? Also, instead of .indexOf() you can use .includes().

Copy link
Member

Choose a reason for hiding this comment

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

This is still unfixed.

test/parallel/test-https-agent-secure-protocol.js
});
}).on('error', function(e) {
console.log(e.message);
process.exit(1);
Copy link
Member

Choose a reason for hiding this comment

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

Don't do this, just let the exception bubble up.

@papandreou
Copy link
Contributor Author

@bnoordhuis, thanks for taking a look! I believe I fixed the problems you pointed out now.

bnoordhuis
bnoordhuis requested changes Nov 4, 2016
View reviewed changes
test/parallel/test-https-agent-secure-protocol.js
var options = {
key: fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'),
cert: fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem'),
ca: fs.readFileSync(common.fixturesDir + '/keys/ca1-cert.pem')
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nit: there is an extra blank here.

test/parallel/test-https-agent-secure-protocol.js
var keys = Object.keys(globalAgent.freeSockets);
assert.equal(keys.length, 2);
assert.notEqual(keys[0].indexOf(':SSLv23_method'), -1);
assert.notEqual(keys[1].indexOf(':TLSv1_method'), -1);
Copy link
Member

Choose a reason for hiding this comment

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

This is still unfixed.

test/parallel/test-https-agent-secure-protocol.js
});
});
});
});
Copy link
Member

Choose a reason for hiding this comment

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

There are still a couple of places where you could s/var/const/.

@papandreou papandreou force-pushed the fix/agentWithSecureProtocol branch from 2106fb8 to 43124ca Compare November 4, 2016 12:33
@papandreou
Copy link
Contributor Author

@bnoordhuis ... There, that should do it.

For some reason I didn't notice that the test-https-agent-getname.js was also affected by this (due to the extra colons showing up in those keys). Fixed that as well and squashed+rebased.

bnoordhuis
bnoordhuis approved these changes Nov 4, 2016
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/4788/

@cjihrig
Copy link
Contributor

cjihrig commented Nov 7, 2016

One nit: you might use common.mustCall() to ensure that the callbacks are actually being invoked and your assertions are running.

@papandreou papandreou force-pushed the fix/agentWithSecureProtocol branch from 43124ca to 67965c9 Compare November 7, 2016 16:56
@papandreou
Copy link
Contributor Author

@cjihrig Good point -- I added common.mustCall and force pushed now.

bnoordhuis
bnoordhuis approved these changes Nov 8, 2016
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

CI: https://ci.nodejs.org/job/node-test-pull-request/4804/

@jasnell
Copy link
Member

jasnell commented Jan 11, 2017

Looks like this fell through the cracks. New CI: https://ci.nodejs.org/job/node-test-pull-request/5804/

jasnell pushed a commit that referenced this pull request Jan 11, 2017
@papandreou @jasnell
https: Use secureProtocol in Agent#getName
a469f85 
Refs: #9324
PR-URL: #9452
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jan 11, 2017

Landed in a469f85

@jasnell jasnell closed this Jan 11, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@papandreou @italoacasas
https: Use secureProtocol in Agent#getName
b6ed00b 
Refs: nodejs#9324
PR-URL: nodejs#9452
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
@papandreou @italoacasas
https: Use secureProtocol in Agent#getName
6add91d 
Refs: nodejs#9324
PR-URL: nodejs#9452
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
@papandreou @italoacasas
https: Use secureProtocol in Agent#getName
86aec97 
Refs: nodejs#9324
PR-URL: nodejs#9452
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
@papandreou @italoacasas
https: Use secureProtocol in Agent#getName
c012dd7 
Refs: nodejs#9324
PR-URL: nodejs#9452
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
Merged
@MylesBorins
Copy link
Contributor

Is this something we want to backport? It seems like it changes some behavior... even if it does so to a more correct behavior

@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

hmmm.. I'd say maybe let it sit for a while longer before putting into v6.x-staging. Perhaps skip v4.x-staging

@MylesBorins MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. and removed lts-watch-v4.x labels May 8, 2017
MylesBorins pushed a commit that referenced this pull request Nov 14, 2017
@papandreou @MylesBorins
https: Use secureProtocol in Agent#getName
92b13e4 
Refs: #9324
PR-URL: #9452
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins added land-on-v6.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Nov 14, 2017
@MylesBorins
Copy link
Contributor

It seems like this PR has baked long enough. Landing on v6.x

@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
https Issues or PRs related to the https subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@papandreou @cjihrig @jasnell @MylesBorins @bnoordhuis @nodejs-github-bot