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

module: exports check hasOwnProperty #31427

Closed
wants to merge 0 commits into from

Conversation

guybedford
Copy link
Contributor

This is a possible fix for the issues discussed in #31001 (comment), ensuring a strict property check.

I suspect the bug may be prototype pollution although I haven't been able to test this. I'm 80% sure this should resolve the issue though given the information.

I haven't included a test as this is more of a correctness feature... testing object pollution sounds more like something that could be covered by some kind of general JS pollution fuzz testing approach than something to be unit tested case-by-case.

I will be away for the next 8 hours, please feel free to merge if this works, or to revert the conditional exports unflagging PR if it doesn't.

//cc @Trott @richardlau @codebytere

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@BridgeAR

This comment has been minimized.

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

Tried this locally and it didn't fix the issue. I've run CITGM on bluebird with 13.6.0 (which doesn't contain #31001) and NODE_OPTIONS=--experimental-conditional-exports and it produces the same failure: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/727/

@guybedford
Copy link
Contributor Author

guybedford commented Jan 20, 2020 via email

@richardlau
Copy link
Member

richardlau commented Jan 20, 2020

are you 100% sure this change didn’t fix the issue?

Yes, I just double checked.

CITGM (bluebird only on our fastest platform (s390x)): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2231/

@GeoffreyBooth GeoffreyBooth added the esm Issues and PRs related to the ECMAScript Modules implementation. label Jan 20, 2020
@GeoffreyBooth
Copy link
Member

@richardlau Do you mind please providing specific steps to reproduce?

@richardlau
Copy link
Member

Do you mind please providing specific steps to reproduce?

😅

I have git clones of nodejs/node and nodejs/citgm (with npm install already run inside the citgm clone).

  • Make the edits in the clone of nodejs/node (vi lib/internal/modules/cjs/loader.js)
  • build (make -j128 2>&1 | tee ../build.linux-x64.log)
  • install (tools/install.py install '' /home/riclau/sandbox/bin/testnode/) (this is so we can run with CITGM)
  • set path (export PATH=/home/riclau/sandbox/bin/testnode/bin:$PATH)
  • Change to the CITGM clone directory
  • run node bin/citgm.js bluebird

@richardlau
Copy link
Member

It may be possible to reduce what bluebird is doing into a simpler testcase... From just looking at the code it appears that:

  • package.json contains "main": "./js/release/bluebird.js", and lists under devDependencies "bluebird": "^2.9.2",
  • tools/build.js's first line is var Promise = require("bluebird");

So I'm guessing that it used to "work" by falling back to the version of bluebird installed under node_modules (as one of the devDependencies) when Node.js couldn't load the non-existant main?

@guybedford
Copy link
Contributor Author

Thanks @richardlau for the replication instructions there - I managed to finally track this down and have updated the PR with the fix.

Basically, for some reason, all package.json files are being detected as containing an undefined "exports" field in these cases. So shifting the check to be based on "exports" not being undefined instead of being a property fixes the issue.

Tracking down why "exports" is being set as an undefined property on the internally loaded package.json objects is likely a good idea - and entirely unrelated to this PR. I'm not sure if it's due to userland code or internal code.

Either way we are good to land with this fix if we can get this fast tracked now.

@GeoffreyBooth GeoffreyBooth added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 20, 2020
@GeoffreyBooth
Copy link
Member

Assuming @richardlau can verify that this PR fixes the issue, I propose we fast-track.

@richardlau
Copy link
Member

richardlau commented Jan 20, 2020

@nodejs-github-bot

This comment has been minimized.

@@ -435,7 +435,7 @@ function resolveBasePath(basePath, exts, isMain, trailingSlash, request) {

function trySelf(parentPath, isMain, request) {
const { data: pkg, path: basePath } = readPackageScope(parentPath) || {};
if (!pkg || 'exports' in pkg === false) return false;
if (!pkg || pkg.exports === undefined) return false;
Copy link
Member

Choose a reason for hiding this comment

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

this still would pass if an inherited exports property existed.

Suggested change
if (!pkg || pkg.exports === undefined) return false;
if (!pkg || !HasOwnProperty(pkg, 'exports')) return false;

(also bring in HasOwnProperty from primordials at the top)

Copy link
Member

Choose a reason for hiding this comment

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

Isn't that similar to the first commit (d3f5674) which doesn't work (pkg.exports does exist but has an undefined value)?

Copy link
Member

Choose a reason for hiding this comment

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

in that case you'd want both checks.

@richardlau
Copy link
Member

Can confirm locally that this appears to fix the issue (and the results so far in https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2232/ are in agreement).

For posterity I also came to a similar discovery about exports existing but being undefined (@guybedford PRed the second commit fix before I could post):

With the following hacky debug (on top of d3f5674):

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
index 01d96a4..7c256e0 100644
--- a/lib/internal/modules/cjs/loader.js
+++ b/lib/internal/modules/cjs/loader.js
@@ -435,9 +435,9 @@ function resolveBasePath(basePath, exts, isMain, trailingSlash, request) {

 function trySelf(parentPath, isMain, request) {
   const { data: pkg, path: basePath } = readPackageScope(parentPath) || {};
-  if (!pkg || 'exports' in pkg === false) return false;
+  if (!pkg || !ObjectPrototypeHasOwnProperty(pkg, 'exports')) return false;
   if (typeof pkg.name !== 'string') return false;
-
+  if (pkg.name === 'bluebird') console.warn(pkg);
   let expansion;
   if (request === pkg.name) {
     expansion = '';

I got this out of the CITGM output:

warn:                         | {
warn:                         | name: 'bluebird',
warn:                         | main: './js/release/bluebird.js',
warn:                         | exports: undefined,
warn:                         | type: undefined
warn:                         | }

@richardlau richardlau mentioned this pull request Jan 20, 2020
@ljharb
Copy link
Member

ljharb commented Jan 20, 2020

What is supposed to happen if someone does Object.prototype.exports = {}?

@BridgeAR
Copy link
Member

BridgeAR commented Jan 20, 2020

@guybedford the reason is that readPackageScope calls readPackage and that returns an object with four properties, no matter if they are defined in the package.json or not. One of those properties is exports.

@nodejs-github-bot

This comment has been minimized.

@codebytere codebytere self-requested a review January 20, 2020 22:32
@codebytere
Copy link
Member

@ljharb PTAL - do you still think this needs both checks? i'm gonna land this in a bit otherwise :)

@ljharb
Copy link
Member

ljharb commented Jan 21, 2020

@codebytere if the only reason for the bug was #31427 (comment) - and if readPackage only reads own properties - then both checks are not needed. However, if readPackage reads inherited properties, then i think both checks are still needed.

Trott
Trott approved these changes Jan 21, 2020
@Trott
Copy link
Member

Trott commented Jan 21, 2020

Would be good to see a test added for this.

@Trott
Copy link
Member

Trott commented Jan 21, 2020

Would be good to see a test added for this.

I took the liberty of adding a test. Hope that's OK. Feel free to remote it or modify it or whatever.

@nodejs-github-bot

This comment has been minimized.

@codebytere
Copy link
Member

@Trott do you not feel this should be backported to #31382 then? My other option was to take out the offending unflag commit but i was planning to pick this back if possible

@Trott
Copy link
Member

Trott commented Jan 21, 2020

@Trott do you not feel this should be backported to #31382 then? My other option was to take out the offending unflag commit but i was planning to pick this back if possible

I support putting this in the release rather than taking out the unflagging. But I'm fine with pulling out the unflagging and waiting until the next semver minor if you or anyone else is concerned. If it causes widespread breakage, the burden will fall to the releasers to get out a quick fix, so I'll defer to you on what the best thing to do here is. But I'm +1 on getting this into the release.

@GeoffreyBooth
Copy link
Member

Unflagging conditional exports is very important for ESM adoption. Many package maintainers are waiting for conditional exports before they add ESM support to their packages, especially conditional exports in LTS; so I would err on the side of keeping the unflagging if it all possible.

@richardlau
Copy link
Member

@Trott We can (should?) probably cut out a lot of stuff from the two package.json files in the test fixture (e.g. @petkaantonov 's email address). Maybe in a follow up if it would otherwise hold up landing this.

@Trott
Copy link
Member

Trott commented Jan 21, 2020

@Trott We can (should?) probably cut out a lot of stuff from the two package.json files in the test fixture (e.g. @petkaantonov 's email address). Maybe in a follow up if it would otherwise hold up landing this.

Agreed, that should happen (and probably rename it from something other than "bluebird") and also someone should follow up on @ljharb's observation and see if the other check is necessary or not.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Jan 21, 2020

https://ci.nodejs.org/job/node-test-pull-request/28525/ is green. Landing.

@Trott
Copy link
Member

Trott commented Jan 21, 2020

Landed in 343ddff...dcba128

Trott pushed a commit to Trott/io.js that referenced this pull request Jan 21, 2020
Refs: nodejs#31001 (comment)

PR-URL: nodejs#31427
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Trott added a commit that referenced this pull request Jan 21, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott Trott closed this Jan 21, 2020
codebytere pushed a commit that referenced this pull request Jan 21, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
codebytere pushed a commit that referenced this pull request Jan 21, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@guybedford
Copy link
Contributor Author

Thanks all for the help getting this one sorted.

MylesBorins pushed a commit that referenced this pull request Jan 28, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Jan 30, 2020
Refs: nodejs#31001 (comment)

PR-URL: nodejs#31427
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Jan 30, 2020
Refs: nodejs#31001 (comment)

PR-URL: nodejs#31427
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 30, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 30, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants