Skip to content

Conversation

@ShogunPanda
Copy link
Contributor

This PR fixes SSL connection attempts by not attempting to restore SSL information (like server name) if such informations are not available anymore.

Fixes#48000.

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem. labels May 26, 2023
@ShogunPandaShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 26, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 26, 2023
@nodejs-github-bot
Copy link
Collaborator

const{request } = require('https');

request(
'https://nodejs.org/en',
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this test go in test/internet/ if it's attempting to reach an internet host?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, I didn't know about it. Fixed now.

const{request } = require('https');

request(
'https://nodejs.org/en',
Copy link
Member

Choose a reason for hiding this comment

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

If this is reaching out to the internet this should go in internet and not parallel. It should also be using a constant from common/internet to allow people to run the test suites in places that restrict access to some addresses.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks, I didn't know about this. This is fixed now.

@ShogunPandaShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 28, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 28, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ShogunPandaShogunPandaforce-pushed the fix-family-autoselection-ssl-connect branch from 7425b2d to 5150216CompareMay 29, 2023 11:01
@ShogunPandaShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 29, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 29, 2023
@nodejs-github-bot
Copy link
Collaborator

@ShogunPanda
Copy link
ContributorAuthor

@nodejs/build I have a failure in GH suite but I don't think it's related to my changes. Can I ignore or do you first want to address it?

@ShogunPanda
Copy link
ContributorAuthor

Landed in 26450c5

ShogunPanda added a commit that referenced this pull request May 31, 2023
PR-URL: #48189 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
@targos
Copy link
Member

The test-internet failure was confirmed to be unrelated and we opened #48262 for it.

@ShogunPandaShogunPanda deleted the fix-family-autoselection-ssl-connect branch June 1, 2023 20:50
targos pushed a commit that referenced this pull request Jun 4, 2023
PR-URL: #48189 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
@targostargos mentioned this pull request Jun 4, 2023
franciszek-koltuniuk-red pushed a commit to franciszek-koltuniuk-red/node that referenced this pull request Jun 7, 2023
PR-URL: nodejs#48189 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48189 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48189 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 15, 2023
PR-URL: nodejs#48189 Backport-URL: Reviewed-By: James M Snell <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 15, 2023
PR-URL: nodejs#48189 Backport-PR-URL: Reviewed-By: James M Snell <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 15, 2023
PR-URL: nodejs#48189 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
ruyadorno pushed a commit that referenced this pull request Aug 17, 2023
PR-URL: #48189 Backport-PR-URL: #49183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
@ruyadornoruyadorno mentioned this pull request Aug 17, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.tlsIssues and PRs related to the tls subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assertion (wrap->ssl_) != nullptr failed in TLSWrap::GetServername

10 participants

@ShogunPanda@nodejs-github-bot@targos@mscdex@jasnell@richardlau@marco-ippolito@ruyadorno@danielleadams@mhdawson