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

crypto: validate this in all webcrypto methods and getters #42815

Merged
merged 5 commits into from
Apr 23, 2022
Merged

crypto: validate this in all webcrypto methods and getters #42815

merged 5 commits into from
Apr 23, 2022

Conversation

panva
Copy link
Member

@panva panva commented Apr 21, 2022 •
edited
Loading

This PR aligns this validation in webcrypto with other implementations.

  • forbids calling the constructor on Crypto and SubtleCrypto (that become accessible with --experimental-global-webcrypto).
  • validates getRandomValues() this to be the actual instance and not an instance of
  • validates this to be the actual instance for all other methods and getters

See with the following in other runtimes.

const notCrypto = Reflect.construct(function() {}, [], Crypto);
const notSubtle = Reflect.construct(function() {}, [], SubtleCrypto);

notCrypto.subtle
notCrypto.randomUUID()
notCrypto.getRandomValues()
notSubtle.digest()
  • Chromium: TypeError: Illegal invocation
  • Firefox: TypeError: '...' called on an object that does not implement interface ....
  • Safari: TypeError: Can only call Crypto.getRandomValues on instances of ...
  • Deno: TypeError: Illegal invocation
  • Node.js: Given correct arguments were provided, no error.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Apr 21, 2022
@panva panva added experimental Issues and PRs related to experimental features. webcrypto crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. and removed crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Apr 21, 2022
aduh95
aduh95 approved these changes Apr 21, 2022
View reviewed changes
lib/internal/crypto/webcrypto.js Outdated Show resolved Hide resolved
lib/internal/crypto/webcrypto.js Outdated Show resolved Hide resolved
lib/internal/crypto/webcrypto.js Outdated Show resolved Hide resolved
@panva panva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 22, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

aduh95
aduh95 previously requested changes Apr 22, 2022
View reviewed changes
lib/internal/crypto/webcrypto.js Outdated Show resolved Hide resolved
@panva panva requested a review from aduh95 April 22, 2022 14:23
aduh95
aduh95 reviewed Apr 22, 2022
View reviewed changes
lib/internal/crypto/webcrypto.js Outdated Show resolved Hide resolved
lib/internal/crypto/webcrypto.js Outdated Show resolved Hide resolved
@panva panva dismissed aduh95’s stale review April 22, 2022 14:41

adressed, thank you!

@panva panva requested a review from aduh95 April 22, 2022 14:41
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95
Copy link
Contributor

aduh95 commented Apr 22, 2022 •
edited
Loading

@aduh95 ... I think what @panva did here was perfectly acceptable. The prior review issue was handled and they immediately requested review again. That seems like a perfectly reasonable workflow.

That seems perfectly reasonable to me, but it's not in accordance with the collaborator guide (or my interpretation of it). Anyway, my advice still stands, dismissing objections can create unnecessary "drama", so let's not do it. No hard feelings :)

@panva
Copy link
Member Author

panva commented Apr 22, 2022

My deepest apologies, did not mean to create needless drama, FWIW, I dismissed an objection I have understood the purpose of and already addressed it. The collaborators guide you quote deals with objections one does not understand, asks for clarification, and doesn't get a response for 7 days.

Let's not delve into this any further. No feelings were hurt <3

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Apr 23, 2022
Open
21 tasks
@panva panva added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 23, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 23, 2022
@nodejs-github-bot nodejs-github-bot merged commit 603803e into nodejs:master Apr 23, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 603803e

@panva panva deleted the webcrypto-illegal-invocations branch April 23, 2022 18:03
This was referenced Apr 24, 2022
Open
Open
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
@panva @xtx1130
crypto: validate this in all webcrypto methods and getters
cda53fa 
PR-URL: nodejs#42815
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
This was referenced Apr 26, 2022
Open
Open
Open
targos pushed a commit that referenced this pull request Apr 28, 2022
@panva @targos
crypto: validate this in all webcrypto methods and getters
7681e60 
PR-URL: #42815
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@targos targos mentioned this pull request May 2, 2022
Merged
juanarbol pushed a commit that referenced this pull request May 31, 2022
@panva @juanarbol
crypto: validate this in all webcrypto methods and getters
1884f52 
PR-URL: #42815
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@juanarbol juanarbol mentioned this pull request May 31, 2022
Closed
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
@panva @danielleadams
crypto: validate this in all webcrypto methods and getters
d25f405 
PR-URL: #42815
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
@panva @targos
crypto: validate this in all webcrypto methods and getters
33ea445 
PR-URL: #42815
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@panva @targos
crypto: validate this in all webcrypto methods and getters
3f43dad 
PR-URL: #42815
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@panva @guangwong
crypto: validate this in all webcrypto methods and getters
d68d90c 
PR-URL: nodejs/node#42815
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@tniessen tniessen tniessen approved these changes

@jasnell jasnell jasnell 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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. crypto Issues and PRs related to the crypto subsystem. experimental Issues and PRs related to experimental features. needs-ci PRs that need a full CI run. webcrypto
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@panva @nodejs-github-bot @aduh95 @jasnell @tniessen