Skip to content

test: cover webcrypto prototype pollution systematically - #63520

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
panva:refactor-proto-pollution-test
May 25, 2026
Merged

test: cover webcrypto prototype pollution systematically#63520
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
panva:refactor-proto-pollution-test

Conversation

@panva

@panvapanva commented May 23, 2026

Copy link
Copy Markdown
Member

Followup to #63363 - drive the regression test from the WebCrypto algorithm registry so all supported algorithms and operations are covered regardless of whether they are native-job backed or js-based.

I extracted this from a WIP that adds js-based Hybrid KEM algorithms to Web Cryptography (based on a discussion with the Chromium team, pending spec merge).

Drive the regression test from the WebCrypto algorithm registry so all
supported algorithms and operations must add explicit coverage
regardless of whether they are native-job backed or js-based.
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
@panvapanva added test Issues and PRs related to the tests. webcrypto labels May 23, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-botnodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels May 23, 2026
@panvapanva added dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. dont-land-on-v25.x labels May 23, 2026
@panva
panva requested review from Renegade334 and aduh95May 23, 2026 17:05
Comment on lines 37 to 52
const constructorDescriptor =
Object.getOwnPropertyDescriptor(Promise.prototype, 'constructor');
const speciesDescriptor =
Object.getOwnPropertyDescriptor(Promise, Symbol.species);
let promise;
Object.defineProperty(Promise.prototype, 'constructor', {
__proto__: null,
configurable: true,
get: common.mustNotCall(
`${name} Promise.prototype.constructor getter`),
});
Object.defineProperty(Promise, Symbol.species, {
__proto__: null,
configurable: true,
get: common.mustNotCall(`${name} Promise[Symbol.species] getter`),
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this operation is used a lot, might be an idea to cache the descriptors (originals and dummies) for re-use.

Comment on lines 74 to 79
const descriptor = Object.getOwnPropertyDescriptor(prototype, 'then');
Object.defineProperty(prototype, 'then', {
__proto__: null,
configurable: true,
get: common.mustNotCall(`${name} ${prototypeName}.prototype.then`),
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

@panvapanva 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 May 23, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 23, 2026
@nodejs-github-bot

This comment was marked as outdated.

@aduh95aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@codecov

codecovBot commented May 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.12%. Comparing base (dfe2d47) to head (73f0d8d).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #63520 +/- ##
==========================================
- Coverage 90.13% 90.12% -0.01% 
==========================================
Files 718 718 Lines 228399 228400 +1 Branches 42931 42941 +10 ==========================================
- Hits 205860 205841 -19 - Misses 14283 14314 +31 + Partials 8256 8245 -11 
Files with missing linesCoverage Δ
lib/internal/crypto/util.js96.45% <100.00%> (+<0.01%)⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@panvapanva added the commit-queue Add this label to land a pull request using GitHub Actions. label May 24, 2026
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 25, 2026
@nodejs-github-bot
nodejs-github-bot merged commit 742849d into nodejs:mainMay 25, 2026
114 of 117 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 742849d

@panva
panva deleted the refactor-proto-pollution-test branch May 25, 2026 16:41
@panvapanva removed the dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. label May 25, 2026
aduh95 pushed a commit that referenced this pull request May 27, 2026
Drive the regression test from the WebCrypto algorithm registry so all
supported algorithms and operations must add explicit coverage
regardless of whether they are native-job backed or js-based.
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
PR-URL: #63520
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Jun 25, 2026
Drive the regression test from the WebCrypto algorithm registry so all
supported algorithms and operations must add explicit coverage
regardless of whether they are native-job backed or js-based.
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
PR-URL: #63520
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Jul 30, 2026
Drive the regression test from the WebCrypto algorithm registry so all
supported algorithms and operations must add explicit coverage
regardless of whether they are native-job backed or js-based.
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
PR-URL: #63520
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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.cryptoIssues and PRs related to the crypto subsystem.dont-land-on-v22.xPRs that should not land on the v22.x-staging branch and should not be released in v22.x.needs-ciPRs that need a full CI run.testIssues and PRs related to the tests.webcrypto

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@panva@nodejs-github-bot@jasnell@Renegade334@aduh95