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

test: snicallback prioritization issue #54251

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ethan-Arrowood
Copy link
Contributor

This PR contains a new test that demonstrates the issue I outlined in #54235

I really don't think there is anything wrong with Node.js - it feels to me like an OpenSSL thing. Most likely not a bug either, but an intended behavior where certain certificates are prioritized over others.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Aug 7, 2024
Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.11%. Comparing base (926503b) to head (b7afc71).
Report is 35 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54251      +/-   ##
==========================================
- Coverage   87.11%   87.11%   -0.01%     
==========================================
  Files         647      647              
  Lines      181693   181739      +46     
  Branches    34869    34885      +16     
==========================================
+ Hits       158278   158316      +38     
- Misses      16724    16731       +7     
- Partials     6691     6692       +1     

see 24 files with indirect coverage changes

Depending on the content of a cert, it seems that OpenSSL
will prioritize the root cert over the context returned by
SNICallback. This behavior is unexpected. This PR adds a
test that demonstrates the behavior. Ideally there is a fix,
or a explanation for this behavior we can add to the docs.

Fixes: nodejs#54235
PR-URL: nodejs#54251
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants