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

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

Merged
merged 5 commits into from
Apr 23, 2022

Conversation

panva
Copy link
Member

@panva panva commented Apr 21, 2022

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
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
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@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

@nodejs-github-bot
Copy link
Collaborator

aduh95
aduh95 previously requested changes Apr 22, 2022
lib/internal/crypto/webcrypto.js Outdated Show resolved Hide resolved
@panva panva requested a review from aduh95 April 22, 2022 14:23
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

@aduh95
Copy link
Contributor

aduh95 commented Apr 22, 2022

@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

@nodejs-github-bot
Copy link
Collaborator

@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
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
PR-URL: nodejs#42815
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit that referenced this pull request Apr 28, 2022
PR-URL: #42815
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@targos targos mentioned this pull request May 2, 2022
juanarbol pushed a commit that referenced this pull request May 31, 2022
PR-URL: #42815
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
PR-URL: #42815
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #42815
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #42815
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#42815
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
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. 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
Development

Successfully merging this pull request may close these issues.

5 participants