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.

Sign up for GitHub

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

Jump to bottom

src: make SecureContext fields private #43173

Merged
merged 1 commit into from
May 24, 2022
Merged

src: make SecureContext fields private #43173

merged 1 commit into from
May 24, 2022

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented May 22, 2022 •
edited
Loading

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.

Sign in to view
@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)

@tniessen
src: make SecureContext fields private
592a360 
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.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44128/

@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
This was referenced May 23, 2022
Open
Open
@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

This was referenced May 25, 2022
Open
Open
This was referenced May 28, 2022
Open
Open
bengl pushed a commit that referenced this pull request May 30, 2022
@tniessen @bengl
src: make SecureContext fields private
6ee9fb1 
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
Merged
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
@tniessen @danielleadams
src: make SecureContext fields private
dca9242 
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
@tniessen @targos
src: make SecureContext fields private
6c4ce46 
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
@tniessen @targos
src: make SecureContext fields private
07b7f83 
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 targos mentioned this pull request Aug 3, 2022
Merged
codebytere added a commit to electron/electron that referenced this pull request Aug 22, 2022
@codebytere
src: make SecureContext fields private
f39f7d5 
nodejs/node#43173
codebytere added a commit to electron/electron that referenced this pull request Aug 24, 2022
@codebytere
src: make SecureContext fields private
34d0395 
nodejs/node#43173
codebytere added a commit to electron/electron that referenced this pull request Aug 25, 2022
@codebytere
src: make SecureContext fields private
9aa457e 
nodejs/node#43173
jkleinsc pushed a commit to electron/electron that referenced this pull request Aug 29, 2022
@electron-roller @codebytere
chore: bump node to v16.17.0 (main) (#35350)
d0e220c 
* 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
@tniessen @guangwong
src: make SecureContext fields private
057d663 
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
@electron-roller @codebytere @khalwa
chore: bump node to v16.17.0 (main) (electron#35350)
a2ca4b7 
* 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
Reviewers

@JungMinu JungMinu JungMinu approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tniessen @nodejs-github-bot @bnoordhuis @JungMinu