Skip to content

Conversation

@tniessen
Copy link
Member

@tniessentniessen 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-botnodejs-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
@tniessentniessenforce-pushed the src-make-securecontext-fields-private branch from 8069c9d to 1a85033CompareMay 22, 2022 00:28
@tniessentniessen added the request-ci Add this label to start a Jenkins CI on a PR. label May 22, 2022
@github-actionsgithub-actionsbot 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.
@tniessentniessenforce-pushed the src-make-securecontext-fields-private branch from 1a85033 to 592a360CompareMay 22, 2022 01:22
@tniessen
Copy link
MemberAuthor

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

@tniessentniessen added the request-ci Add this label to start a Jenkins CI on a PR. label May 22, 2022
@github-actionsgithub-actionsbot 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

@tniessentniessen 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
@tniessentniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label May 24, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 24, 2022
@nodejs-github-botnodejs-github-bot merged commit 9a3326f into nodejs:masterMay 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 <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
@benglbengl 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 <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
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 <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
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 <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
@targostargos mentioned this pull request Aug 3, 2022
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 fixesnodejs/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 <[email protected]>
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 <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
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 fixesnodejs/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 <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs 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 / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.tlsIssues and PRs related to the tls subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@tniessen@nodejs-github-bot@bnoordhuis@JungMinu