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

tls.SecureContext documentation contradictions #47408

Closed
shollander opened this issue Apr 4, 2023 · 3 comments · Fixed by #47570
Closed

tls.SecureContext documentation contradictions #47408

shollander opened this issue Apr 4, 2023 · 3 comments · Fixed by #47570
Labels
feature request Issues that request new features to be added to Node.js. tls Issues and PRs related to the tls subsystem.

Comments

@shollander
Copy link

shollander commented Apr 4, 2023

Affected URL(s)

https://nodejs.org/api/tls.html#tlscreatesecurecontextoptions
https://nodejs.org/api/tls.html#serveraddcontexthostname-context

Description of the problem

The documentation for tls.createSecureContext() states:

The tls.createSecureContext() method creates a SecureContext object. It is usable as an argument to several tls APIs, such as server.addContext(), but has no public methods. The tls.Server constructor and the tls.createServer() method do not support the secureContext option.

So according to the documentation, server.addContext() should take a SecureContext object as an argument. However, according to the documention of server.addContext(), the only valid argument for options is SecureContextOptions, but not the SecureContext object itself.

I have tried passing a SecureContext object to server.addContext(). While it did not throw any errors, it did not configure the secure context properly.

It would really be nice if server.addContext() would take a SecureContext object. Ideally, you should fix the code and update the documentation for server.addContext().

Thank you.

@shollander shollander added the doc Issues and PRs related to the documentations. label Apr 4, 2023
@bnoordhuis
Copy link
Member

Pull request welcome. You can check inside addContext() if the context argument is already instanceof SecureContext and then use context.context instead of passing context to tls.createSecureContext():

node/lib/_tls_wrap.js

Lines 1479 to 1480 in 6fd147c

ArrayPrototypePush(this._contexts,
[re, tls.createSecureContext(context).context]);

@bnoordhuis bnoordhuis added tls Issues and PRs related to the tls subsystem. feature request Issues that request new features to be added to Node.js. and removed doc Issues and PRs related to the documentations. labels Apr 4, 2023
@github-project-automation github-project-automation bot moved this to Pending Triage in Node.js feature requests Apr 4, 2023
@shollander
Copy link
Author

Seems like a relatively simple change. Unfortunately, I don't have an environment where I can readily build and test node.js.

@HinataKah0
Copy link
Contributor

If you want to try, you can refer to this for building Node JS locally.

HinataKah0 added a commit to HinataKah0/node that referenced this issue Apr 15, 2023
Do not call tls.createSecureContext() if the context provided
is already an instance of tls.SecureContext.

Fixes: nodejs#47408
HinataKah0 added a commit to HinataKah0/node that referenced this issue Apr 16, 2023
Do not call tls.createSecureContext() if the context provided
is already an instance of tls.SecureContext.

Fixes: nodejs#47408
nodejs-github-bot pushed a commit that referenced this issue Apr 26, 2023
Do not call tls.createSecureContext() if the context provided
is already an instance of tls.SecureContext.

Fixes: #47408
PR-URL: #47570
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
yjl9903 pushed a commit to yjl9903/node that referenced this issue Apr 28, 2023
Do not call tls.createSecureContext() if the context provided
is already an instance of tls.SecureContext.

Fixes: nodejs#47408
PR-URL: nodejs#47570
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
yjl9903 pushed a commit to yjl9903/node that referenced this issue Apr 28, 2023
Do not call tls.createSecureContext() if the context provided
is already an instance of tls.SecureContext.

Fixes: nodejs#47408
PR-URL: nodejs#47570
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
yjl9903 pushed a commit to yjl9903/node that referenced this issue Apr 29, 2023
Do not call tls.createSecureContext() if the context provided
is already an instance of tls.SecureContext.

Fixes: nodejs#47408
PR-URL: nodejs#47570
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue May 2, 2023
Do not call tls.createSecureContext() if the context provided
is already an instance of tls.SecureContext.

Fixes: #47408
PR-URL: #47570
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this issue Jul 6, 2023
Do not call tls.createSecureContext() if the context provided
is already an instance of tls.SecureContext.

Fixes: #47408
PR-URL: #47570
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this issue Jul 6, 2023
Do not call tls.createSecureContext() if the context provided
is already an instance of tls.SecureContext.

Fixes: nodejs#47408
PR-URL: nodejs#47570
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RedYetiDev RedYetiDev moved this from Awaiting Triage to Done in Node.js feature requests Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants