-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
tls: change TLSWrap::GetProtocol to fix #56935 #57036
base: main
Are you sure you want to change the base?
tls: change TLSWrap::GetProtocol to fix #56935 #57036
Conversation
Review requested:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57036 +/- ##
==========================================
+ Coverage 89.11% 89.12% +0.01%
==========================================
Files 665 665
Lines 193193 193206 +13
Branches 37212 37220 +8
==========================================
+ Hits 172158 172194 +36
+ Misses 13775 13757 -18
+ Partials 7260 7255 -5
|
872fcf5
to
12ef396
Compare
args.GetReturnValue().SetNull(); | ||
return; | ||
} | ||
|
||
args.GetReturnValue().Set( | ||
OneByteString(env->isolate(), SSL_get_version(w->ssl_.get()))); |
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.
Is the return value of SSL_get_version()
incorrect or misleading? If not, wouldn't we want to adjust the documentation instead of the code?
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.
On further inspection, I think you're right. It should probably just be a documentation change. That being said, if you look at the first code snippet shared in #56935, getProtocol()
is returning TLSv1.3
for an unconnected server socket. Shouldn't it be returning null as long as a tls handshake has not been completed, or am I missing something?
tls: change tlsSocket.getProtocol() to return null for server sockets
I updated TLSWrap::GetProtocol to return null for server sockets.
I updated a test to ensure that the returned protocol is indeed null for
server sockets.
Fixes: #56935
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.
-->