Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

tools,doc: add guards against prototype pollution when creating proxies #43391

Merged
merged 1 commit into from
Jun 15, 2022

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jun 12, 2022

When defining a Proxy, the handler object could be at risk of prototype
pollution when using a plain object literal:

// User-land
Object.prototype.get = () => 'Unrelated user-provided data';
// Core
const objectToProxy = { someProperty: 'genuine value' };
const proxyWithPlainObjectLiteral = new Proxy(objectToProxy, {
  has() { return false; },
});
console.log(proxyWithPlainObjectLiteral.someProperty); // Unrelated user-provided data
const proxyWithNullPrototypeObject = new Proxy(objectToProxy, {
  __proto__: null,
  has() { return false; },
});
console.log(proxyWithNullPrototypeObject.someProperty); // genuine value

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Jun 12, 2022
@aduh95 aduh95 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. and removed 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 Jun 15, 2022
Copy link
Contributor

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

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

LGTM with a nit

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 15, 2022
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 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 Jun 15, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 15, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/43391
✔  Done loading data for nodejs/node/pull/43391
----------------------------------- PR info ------------------------------------
Title      tools,doc: add guards against prototype pollution when creating proxies (#43391)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     aduh95:proxy-prototype-pollution -> nodejs:main
Labels     tools, author ready, needs-ci, commit-queue-squash
Commits    4
 - tools,doc: add guards against prototype pollution when creating proxies
 - fixup! tools,doc: add guards against prototype pollution when creatin…
 - fixup! tools,doc: add guards against prototype pollution when creatin…
 - Update tools/eslint-rules/avoid-prototype-pollution.js
Committers 2
 - Antoine du Hamel 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/43391
Reviewed-By: James M Snell 
Reviewed-By: LiviaMedeiros 
Reviewed-By: Сковорода Никита Андреевич 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43391
Reviewed-By: James M Snell 
Reviewed-By: LiviaMedeiros 
Reviewed-By: Сковорода Никита Андреевич 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 12 Jun 2022 11:53:36 GMT
   ✔  Approvals: 3
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/43391#pullrequestreview-1004583796
   ✔  - LiviaMedeiros (@LiviaMedeiros): https://github.com/nodejs/node/pull/43391#pullrequestreview-1007161498
   ✔  - Сковорода Никита Андреевич (@ChALkeR) (TSC): https://github.com/nodejs/node/pull/43391#pullrequestreview-1007417535
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-06-15T20:46:03Z: https://ci.nodejs.org/job/node-test-pull-request/44599/
- Querying data for job/node-test-pull-request/44599/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
   70b516e4db..9119382555  main       -> origin/main
✔  origin/main is now up-to-date
main is out of sync with origin/main. Mismatched commits:
 - 9119382555 tools: report unsafe string and regex primordials as lint errors
--------------------------------------------------------------------------------
HEAD is now at 9119382555 tools: report unsafe string and regex primordials as lint errors
   ✔  Reset to origin/main
- Downloading patch for 43391
From https://github.com/nodejs/node
 * branch                  refs/pull/43391/merge -> FETCH_HEAD
✔  Fetched commits as 70b516e4dbdf..a92ce2efa40a
--------------------------------------------------------------------------------
Auto-merging test/parallel/test-eslint-avoid-prototype-pollution.js
CONFLICT (content): Merge conflict in test/parallel/test-eslint-avoid-prototype-pollution.js
Auto-merging tools/eslint-rules/avoid-prototype-pollution.js
CONFLICT (content): Merge conflict in tools/eslint-rules/avoid-prototype-pollution.js
error: could not apply 86dc079e91... tools,doc: add guards against prototype pollution when creating proxies
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
   ✖  Failed to apply patches
https://github.com/nodejs/node/actions/runs/2505466684

PR-URL: nodejs#43391
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@aduh95 aduh95 force-pushed the proxy-prototype-pollution branch from a92ce2e to 358008f Compare June 15, 2022 22:27
@aduh95 aduh95 merged commit 358008f into nodejs:main Jun 15, 2022
@aduh95
Copy link
Contributor Author

aduh95 commented Jun 15, 2022

Landed in 358008f

@aduh95 aduh95 deleted the proxy-prototype-pollution branch June 15, 2022 22:31
danielleadams pushed a commit that referenced this pull request Jun 16, 2022
PR-URL: #43391
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@danielleadams danielleadams mentioned this pull request Jun 16, 2022
aduh95 added a commit to aduh95/node that referenced this pull request Aug 1, 2022
PR-URL: nodejs#43391
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
aduh95 added a commit to aduh95/node that referenced this pull request Aug 1, 2022
PR-URL: nodejs#43391
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
targos pushed a commit that referenced this pull request Aug 2, 2022
PR-URL: #43391
Backport-PR-URL: #44081
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43391
Backport-PR-URL: nodejs/node#44081
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this 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. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants