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

createConnection option not work for https module #24543

Closed
yibopi opened this issue Nov 21, 2018 · 7 comments
Closed

createConnection option not work for https module #24543

yibopi opened this issue Nov 21, 2018 · 7 comments

Comments

@yibopi
Copy link

yibopi commented Nov 21, 2018

Version: v10.13.0
Platform: Linux 4.15.0-39-generic x86_64

I am trying to use the createConnection option in the https module to set the client local port (localPort=34567 in the example below), but this only works when port = 80, not when port = 443. Following is the code I used to test. Does it mean that https module cannot set a custom socket for https in this way? Thanks.

const https = require('https');
const url = require('url');

var req = https.get({
    host: 'google.com',
    pathname: '/',
    port: 443,   // works when equal to 80, but not 443
    family: 4,
    localPort: 34567,
    createConnection: require('net').createConnection
}, function(res) {
    console.log('localPort:', req.socket.localPort, "remotePort:", req.socket.remotePort);
});

req.on('error', console.error);

@Trott
Copy link
Member

Trott commented Nov 21, 2018

@nodejs/http Seems like a legit bug in the https module?

@yibopi
Copy link
Author

yibopi commented Nov 21, 2018

@leeight , thanks.
Adding localPort does enforce the local port, but it cannot be used together with the createConnection option. Why createConnection only works for http, not https?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 22, 2018

but this only works when port = 80, not when port = 443
Why createConnection only works for http, not https?

I thinks it's because net.createConnection() and port 80 use HTTP, not HTTPS. The HTTPS agent's createConnection() is built around tls.connect().

leeight added a commit to leeight/node that referenced this issue Nov 24, 2018
In `_tls_wrap.js` while calling `socket.connect` the `localPort` was
missing, restore it.

Fix: nodejs#24543
@eecs441staff
Copy link

eecs441staff commented Nov 27, 2018

@cjihrig creatConnection is useful for opening a custom socket. Can this feature be added for HTTPS? Or is there a way to customize socket for HTTPS? Thanks!

@cjihrig
Copy link
Contributor

cjihrig commented Nov 27, 2018

@eecs441staff does createConnection: require('tls').connect not work for you?

@eecs441staff
Copy link

@cjihrig It worked! Thanks.

@Trott
Copy link
Member

Trott commented Dec 1, 2018

Sounds to me like this can be closed, but comment and/or re-open if I've misunderstood.

@Trott Trott closed this as completed Dec 1, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Mar 5, 2019
In `_tls_wrap.js` while calling `socket.connect` the `localPort` was
missing, restore it.

PR-URL: nodejs#24554
Fixes: nodejs#24543
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Mar 12, 2019
In `_tls_wrap.js` while calling `socket.connect` the `localPort` was
missing, restore it.

PR-URL: nodejs#24554
Fixes: nodejs#24543
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this issue Apr 16, 2019
In `_tls_wrap.js` while calling `socket.connect` the `localPort` was
missing, restore it.

PR-URL: #24554
Fixes: #24543
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 a pull request may close this issue.

4 participants