GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — tls: use the most recently added matching SecureContext in default SN… by mkrawczuk · Pull Request #36072 · nodejs/node

Conversation

@mkrawczuk

@mkrawczuk mkrawczuk commented Nov 10, 2020 •
edited by Trott
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Sorry, ruined the previous PR (#34638) with a merge.

Fixes: #34110

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Nov 10, 2020
@mkrawczuk mkrawczuk mentioned this pull request Nov 10, 2020
tls: use the most recently added matching SecureContext in default SNICallback. #34638
Closed
4 tasks done
mscdex
mscdex reviewed Nov 10, 2020
View reviewed changes
Comment thread test/parallel/test-tls-secure-context-usage-order.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
mscdex
mscdex reviewed Nov 10, 2020
View reviewed changes
Comment thread test/parallel/test-tls-secure-context-usage-order.js Outdated

@mscdex mscdex Nov 10, 2020

Copy link
Copy Markdown
Contributor

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.

minor nit: we could be more strict here and assert that these are equal depending on the current step, same for the other conditional below

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Trott
Trott reviewed Nov 12, 2020
View reviewed changes
Comment thread doc/api/tls.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott approved these changes Nov 12, 2020
View reviewed changes
@Trott

Trott commented Nov 12, 2020

Copy link
Copy Markdown
Member

@nodejs/crypto @addaleax
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott requested a review from addaleax November 16, 2020 14:41
@mkrawczuk

mkrawczuk commented Dec 16, 2020

Copy link
Copy Markdown
Contributor Author

Rebased. cc @addaleax
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 23, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 23, 2020
@nodejs-github-bot

nodejs-github-bot commented Dec 23, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35053/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mkrawczuk
mkrawczuk force-pushed the 34110_2 branch from c377e10 to e4838ca Compare December 23, 2020 13:19
@mkrawczuk

mkrawczuk commented Dec 23, 2020

Copy link
Copy Markdown
Contributor Author

Did a clear rebase, because the previous one failed. @Trott you might want to request another CI.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Dec 28, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35104/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes Jan 7, 2021
View reviewed changes
@mkrawczuk @Trott
tls: use recently added matching SecureContext in default SNICallback
8cf5281 
PR-URL: nodejs#36072
Fixes: nodejs#34110
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@Trott
Trott force-pushed the 34110_2 branch from e4838ca to 8cf5281 Compare January 15, 2021 12:50
@Trott

Trott commented Jan 15, 2021

Copy link
Copy Markdown
Member

Landed in 8cf5281
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott merged commit 8cf5281 into nodejs:master Jan 15, 2021
ruyadorno pushed a commit that referenced this pull request Jan 22, 2021
@mkrawczuk @ruyadorno
tls: use recently added matching SecureContext in default SNICallback
864b97b 
PR-URL: #36072
Fixes: #34110
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Jan 22, 2021
v15.7.0 proposal #37020
Merged
@targos targos added the dont-land-on-v14.x label May 25, 2021
@Ethan-Arrowood Ethan-Arrowood mentioned this pull request Aug 6, 2024
Root certificate is prioritized over SNICallback context in HTTPS Server #54235
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes
@addaleax addaleax addaleax approved these changes
+1 more reviewer
@mscdex mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tls: server.addContext does not update the context if there was one registered for a given servername wildcard.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@mkrawczuk @Trott @nodejs-github-bot @mscdex @addaleax @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.