GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — src: make SecureContext fields private by tniessen · Pull Request #43173 · nodejs/node

Conversation

tniessen commented May 22, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji

All reactions

👍 1 reaction

nodejs-github-bot commented May 22, 2022

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/crypto

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot added c++

Issues and PRs that require attention from people who are familiar with C++.

lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

needs-ci PRs that need a full CI run. labels

May 22, 2022

tniessen force-pushed the src-make-securecontext-fields-private branch from 8069c9d to 1a85033 Compare

May 22, 2022 00:28

tniessen added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

May 22, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

May 22, 2022

This comment was marked as outdated.

bnoordhuis commented May 22, 2022

Copy link

Copy Markdown

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 reacted with laugh emoji

All reactions

😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          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 force-pushed the src-make-securecontext-fields-private branch from 1a85033 to 592a360 Compare

May 22, 2022 01:22

tniessen commented May 22, 2022

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tniessen added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

May 22, 2022

bnoordhuis approved these changes

May 22, 2022

View reviewed changes

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

May 22, 2022

This comment was marked as outdated.

nodejs-github-bot commented May 22, 2022

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tniessen added author ready

PRs with CI started, the required approvals, and no outstanding review comments.

tls Issues and PRs related to the tls subsystem. labels

May 22, 2022

JungMinu approved these changes

May 23, 2022

View reviewed changes

This was referenced May 23, 2022

CI Reliability 2022-05-23 nodejs/reliability#286

Open

CI Reliability 2022-05-24 nodejs/reliability#287

Open

tniessen added the commit-queue PRs queued for automated landing through the Commit Queue. label

May 24, 2022

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

May 24, 2022

nodejs-github-bot merged commit 9a3326f into nodejs:master

May 24, 2022

nodejs-github-bot commented May 24, 2022

Copy link

Copy Markdown

Collaborator

Landed in 9a3326f

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced May 25, 2022

CI Reliability 2022-05-25 nodejs/reliability#288

Open

CI Reliability 2022-05-26 nodejs/reliability#289

Open

This was referenced May 28, 2022

CI Reliability 2022-05-28 nodejs/reliability#291

Open

CI Reliability 2022-05-29 nodejs/reliability#292

Open

bengl pushed a commit that referenced this pull request

May 30, 2022


          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 <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>

bengl mentioned this pull request

May 31, 2022

v18.3.0 proposal #43266

Merged

danielleadams pushed a commit that referenced this pull request

Jun 27, 2022


          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 <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>

targos pushed a commit that referenced this pull request

Jul 12, 2022


          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 <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>

targos pushed a commit that referenced this pull request

Jul 31, 2022


          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 <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>

targos mentioned this pull request

Aug 3, 2022

v16.17.0 release proposal #44098

Merged

codebytere added a commit to electron/electron that referenced this pull request

Aug 22, 2022


          src: make SecureContext fields private

f39f7d5

nodejs/node#43173

codebytere added a commit to electron/electron that referenced this pull request

Aug 24, 2022


          src: make SecureContext fields private

34d0395

nodejs/node#43173

codebytere added a commit to electron/electron that referenced this pull request

Aug 25, 2022


          src: make SecureContext fields private

9aa457e

nodejs/node#43173

jkleinsc pushed a commit to electron/electron that referenced this pull request

Aug 29, 2022


          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 <[email protected]>

guangwong pushed a commit to noslate-project/node that referenced this pull request

Oct 10, 2022


          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 <[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 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 <[email protected]>

Reviewers

bnoordhuis

bnoordhuis approved these changes

JungMinu

JungMinu approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

c++

Issues and PRs that require attention from people who are familiar with C++.

lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

tniessen commented May 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented May 22, 2022

Uh oh!

This comment was marked as outdated.

bnoordhuis commented May 22, 2022

Uh oh!

tniessen commented May 22, 2022

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented May 22, 2022

Uh oh!

nodejs-github-bot commented May 24, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tniessen commented May 22, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.