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

src: make SecureContext fields private #43173

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented May 22, 2022

These fields should not be public. Only ctx_ is used outside of the class itself, and should be accessed through the ctx() function instead.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 22, 2022
@tniessen tniessen force-pushed the src-make-securecontext-fields-private branch from 8069c9d to 1a85033 Compare May 22, 2022 00:28
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label May 22, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 22, 2022
@nodejs-github-bot

This comment was marked as outdated.

@bnoordhuis
Copy link
Member

That ssl_ctx() function was only used once until now though. :-)

IMO, ssl_ctx() is kind of bad in that it hides the fact that the SSL_CTX is stored in a smart pointer; it obscures its lifetime. It'd be clearer to have:

const SSLCtxPointer* ctx() const { return &ctx_; } // call as: sc->ctx()->get()

(Insert not-a-problem-in-Rust comment)

These fields should not be public. Only ctx_ is used outside of the
class itself, and should be accessed through the ctx() function
instead.
@tniessen tniessen force-pushed the src-make-securecontext-fields-private branch from 1a85033 to 592a360 Compare May 22, 2022 01:22
@tniessen
Copy link
Member Author

@bnoordhuis I changed it to return a const reference to the smart pointer instead :)

@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label May 22, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 22, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@tniessen tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tls Issues and PRs related to the tls subsystem. labels May 22, 2022
@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label May 24, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 24, 2022
@nodejs-github-bot nodejs-github-bot merged commit 9a3326f into nodejs:master May 24, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 9a3326f

bengl pushed a commit that referenced this pull request May 30, 2022
These fields should not be public. Only ctx_ is used outside of the
class itself, and should be accessed through the ctx() function
instead.

PR-URL: #43173
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@bengl bengl mentioned this pull request May 31, 2022
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
These fields should not be public. Only ctx_ is used outside of the
class itself, and should be accessed through the ctx() function
instead.

PR-URL: #43173
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
These fields should not be public. Only ctx_ is used outside of the
class itself, and should be accessed through the ctx() function
instead.

PR-URL: #43173
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
These fields should not be public. Only ctx_ is used outside of the
class itself, and should be accessed through the ctx() function
instead.

PR-URL: #43173
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Aug 22, 2022
codebytere added a commit to electron/electron that referenced this pull request Aug 24, 2022
codebytere added a commit to electron/electron that referenced this pull request Aug 25, 2022
jkleinsc pushed a commit to electron/electron that referenced this pull request Aug 29, 2022
* chore: bump node in DEPS to v16.17.0

* chore: fixup asar patch

* lib: use null-prototype objects for property descriptors

nodejs/node#43270

* src: make SecureContext fields private

nodejs/node#43173

* crypto: remove Node.js-specific webcrypto extensions

nodejs/node#43310

* test: refactor to top-level await

nodejs/node#43500

* deps: cherry-pick two libuv fixes

nodejs/node#43950

* src: slim down env-inl.h

nodejs/node#43745

* util: add AggregateError.prototype.errors to inspect output

nodejs/node#43646

* esm: improve performance & tidy tests

nodejs/node#43784

* src: NodeArrayBufferAllocator delegates to v8's allocator

nodejs/node#43594

* chore: update patch indices

* chore: update filenames

* src: refactor IsSupportedAuthenticatedMode

nodejs/node#42368

* src: add --openssl-legacy-provider option

nodejs/node#40478

* lib,src: add source map support for global eval

nodejs/node#43428

* trace_events: trace net connect event

nodejs/node#43903

* deps: update ICU to 71.1

nodejs/node#42655

This fails the test because it's missing https://chromium-review.googlesource.com/c/chromium/deps/icu/+/3841093

* lib: give names to promisified exists() and question()

nodejs/node#43218

* crypto: add CFRG curves to Web Crypto API

nodejs/node#42507

* src: fix memory leak for v8.serialize

nodejs/node#42695

This test does not work for Electron as they do not use V8's
ArrayBufferAllocator.

* buffer: fix atob input validation

nodejs/node#42539

* src: fix ssize_t error from nghttp2.h

nodejs/node#44393

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
These fields should not be public. Only ctx_ is used outside of the
class itself, and should be accessed through the ctx() function
instead.

PR-URL: nodejs/node#43173
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
* chore: bump node in DEPS to v16.17.0

* chore: fixup asar patch

* lib: use null-prototype objects for property descriptors

nodejs/node#43270

* src: make SecureContext fields private

nodejs/node#43173

* crypto: remove Node.js-specific webcrypto extensions

nodejs/node#43310

* test: refactor to top-level await

nodejs/node#43500

* deps: cherry-pick two libuv fixes

nodejs/node#43950

* src: slim down env-inl.h

nodejs/node#43745

* util: add AggregateError.prototype.errors to inspect output

nodejs/node#43646

* esm: improve performance & tidy tests

nodejs/node#43784

* src: NodeArrayBufferAllocator delegates to v8's allocator

nodejs/node#43594

* chore: update patch indices

* chore: update filenames

* src: refactor IsSupportedAuthenticatedMode

nodejs/node#42368

* src: add --openssl-legacy-provider option

nodejs/node#40478

* lib,src: add source map support for global eval

nodejs/node#43428

* trace_events: trace net connect event

nodejs/node#43903

* deps: update ICU to 71.1

nodejs/node#42655

This fails the test because it's missing https://chromium-review.googlesource.com/c/chromium/deps/icu/+/3841093

* lib: give names to promisified exists() and question()

nodejs/node#43218

* crypto: add CFRG curves to Web Crypto API

nodejs/node#42507

* src: fix memory leak for v8.serialize

nodejs/node#42695

This test does not work for Electron as they do not use V8's
ArrayBufferAllocator.

* buffer: fix atob input validation

nodejs/node#42539

* src: fix ssize_t error from nghttp2.h

nodejs/node#44393

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants