assert: do not read Node.js modules #18322

BridgeAR · 2018-01-23T15:47:33Z

Prevent reading a Node.js module. That could theoretically lead to
false errors being thrown otherwise.

Update: I also added another commit that addresses a TODO about the generatedMessage when using simple assert.

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

Affected core subsystem(s)

assert

BridgeAR · 2018-01-23T15:48:54Z

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

jasnell · 2018-01-24T13:14:14Z

lib/assert.js

+  // Skip Node.js modules!
+  if (!filename.includes('/') && !filename.includes('\\')) {
+    return;
+  }


would it be better to check explicitly against process.binding('natives') here?

I switched to use that instead. In general: it is possible to show the native code as well, I just think it is not the right thing to do because it might confuse someone getting a error message of e.g. Reflect.apply(fn, this, foobar).

jasnell · 2018-01-24T13:17:07Z

Generally lgtm but left a comment that I'd like to see addressed before signing off.

BridgeAR · 2018-01-27T02:13:31Z

PTAL

joyeecheung · 2018-01-31T09:32:31Z

lib/assert.js

@@ -31,7 +31,9 @@ const { parseExpressionAt } = require('internal/deps/acorn/dist/acorn');
 const { inspect } = require('util');
 const { EOL } = require('os');

-const codeCache = new Map();
+const nativeModules =
+  Object.keys(process.bindung('native')).map((e) => `${e}.js`);


Umm...there is a typo process.bindung here..

joyeecheung · 2018-01-31T09:33:37Z

lib/assert.js

+  const filename = call.getFileName();
+
+  // Skip Node.js modules!
+  if (nativeModules[filename]) {


You can check the existence of an native module via something like require('native_module').exists('assert'), also require('native_module').getSource(id) actually returns the source if you want to use it..

I explicitly want to exclude native source code. I think it could cause more confusion than helping anyone in case users use it like e.g.

const assert = require('assert'); const EventEmitter = require('events'); const e = new EventEmitter(); // Listen and verify the input e.once('hello', assert); // Emit some data e.emit('hello', null);

joyeecheung · 2018-01-31T09:35:02Z

lib/assert.js

-            closeSync(fd);
-        }
-      }
+      // TODO: fix the "generatedMessage property"


Can you put your handle in the TODO like TODO(BridgeAR)?

BridgeAR · 2018-01-31T14:48:24Z

Since the TODO was mentioned, I just went ahead and pushed another commit that fixed that as well. I did not want to open another PR to prevent rebasing all the time due to conflicting changes after one PR landed.

BridgeAR · 2018-01-31T14:49:27Z

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

BridgeAR · 2018-01-31T17:50:07Z

Rebased due to conflicts (that git did not realize?!).

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

joyeecheung · 2018-02-01T01:24:57Z

test/parallel/test-assert.js

+    e.emit('hello', false);
+  } catch (err) {
+    const frames = err.stack.split('\n');
+    const [, filename, line, column] = frames[1].match(/\((.+):(\d+):(\d+)\)/);


This might be affected by internal changes of events and assert...can you look for the frame that contains __filename instead of using hard-coded offsets?

Hm, I am not sure I can follow. I explicitly tried to write the test in a way that it will not be affected by internal changes. It should keep on working no matter if the lines are moved or if the specific code is moved to another file. It depends on the thrown error with the information about the filename etc.

Uh, so this frame is guaranteed to belong to where EventEmitter.emit is defined and that should be a safe target...never mind then, sorry for the noise.

joyeecheung · 2018-02-01T01:25:44Z

test/parallel/test-assert.js

  }
 );

+// Do not try to check Node.js modules.


This might deserve a separate test file since test-assert.js is already 1000+ lines long..

I personally would like to keep this test together with the other simple assert tests. I am fine to move all of those to another file though and I agree that this file is a bit long.

Prevent reading a Node.js module. That could theoretically lead to false errors being thrown otherwise.

The generatedMessage was wrong in case simple assert was used and a message was auto generated. This fixed the TODO.

BridgeAR · 2018-02-02T17:34:21Z

Rebased again due to conflicts.

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

mcollina

LGTM

Prevent reading a Node.js module. That could theoretically lead to false errors being thrown otherwise. PR-URL: nodejs#18322 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

The generatedMessage was wrong in case simple assert was used and a message was auto generated. This fixed the TODO. PR-URL: nodejs#18322 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

BridgeAR · 2018-02-06T10:54:56Z

Landed in 3e910fb and a27f48d

Trott · 2018-04-08T01:17:22Z

test/parallel/test-assert.js

+    const data = `${'\n'.repeat(line - 1)}${' '.repeat(column - 1)}` +
+                 'ok(failed(badly));';
+    try {
+      writeFileSync(filename, data);


Sorry to be so late to this, but can you explain why this is writing a file in project root? Is the idea to make sure that assert doesn't try to parse an events.js file when it needs a stack line or something from the internal events module? I don't completely understand what's going on here, but I'd like to.

Oh, I just saw this right now. What this test makes sure is that if assert.ok is triggered inside of Node.js core code and that Node.js core file has the same name as a user file in the root directory (in this example events.js) it would have read the users file and that is clearly wrong.

Prevent reading a Node.js module. That could theoretically lead to false errors being thrown otherwise. PR-URL: nodejs#18322 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

The generatedMessage was wrong in case simple assert was used and a message was auto generated. This fixed the TODO. PR-URL: nodejs#18322 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jan 23, 2018

BridgeAR added dont-land-on-v4.x labels Jan 23, 2018

jasnell reviewed Jan 24, 2018

View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR requested review from mcollina, jasnell, a team, addaleax, cjihrig, apapirovski and TimothyGu January 27, 2018 02:13

joyeecheung reviewed Jan 31, 2018

View reviewed changes

BridgeAR force-pushed the simple-assert-internal branch from f3e8895 to acb1cbd Compare January 31, 2018 17:44

joyeecheung reviewed Feb 1, 2018

View reviewed changes

BridgeAR added 2 commits February 2, 2018 18:08

assert: do not read Node.js modules

74bf73b

Prevent reading a Node.js module. That could theoretically lead to false errors being thrown otherwise.

assert: fix generatedMessage

16f1e48

The generatedMessage was wrong in case simple assert was used and a message was auto generated. This fixed the TODO.

BridgeAR force-pushed the simple-assert-internal branch from acb1cbd to 16f1e48 Compare February 2, 2018 17:22

jasnell approved these changes Feb 2, 2018

View reviewed changes

mcollina approved these changes Feb 2, 2018

View reviewed changes

joyeecheung approved these changes Feb 3, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 3, 2018

BridgeAR closed this Feb 6, 2018

Trott reviewed Apr 8, 2018

View reviewed changes

jeysal mentioned this pull request May 20, 2018

assert: filename may be undefined in filename.endsWith('.js') #20847

Closed

BridgeAR deleted the simple-assert-internal branch April 1, 2019 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: do not read Node.js modules #18322

assert: do not read Node.js modules #18322

BridgeAR commented Jan 23, 2018 •

edited

Loading

BridgeAR commented Jan 23, 2018

jasnell Jan 24, 2018

BridgeAR Jan 24, 2018

jasnell commented Jan 24, 2018

BridgeAR commented Jan 27, 2018

joyeecheung Jan 31, 2018

joyeecheung Jan 31, 2018 •

edited

Loading

BridgeAR Jan 31, 2018

joyeecheung Jan 31, 2018

BridgeAR commented Jan 31, 2018

BridgeAR commented Jan 31, 2018

BridgeAR commented Jan 31, 2018

joyeecheung Feb 1, 2018

BridgeAR Feb 1, 2018

joyeecheung Feb 3, 2018 •

edited

Loading

joyeecheung Feb 1, 2018

BridgeAR Feb 1, 2018

BridgeAR commented Feb 2, 2018

mcollina left a comment

BridgeAR commented Feb 6, 2018

Trott Apr 8, 2018

BridgeAR May 20, 2018

assert: do not read Node.js modules #18322

assert: do not read Node.js modules #18322

Conversation

BridgeAR commented Jan 23, 2018 • edited Loading

Checklist

Affected core subsystem(s)

BridgeAR commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jan 24, 2018

BridgeAR commented Jan 27, 2018

Choose a reason for hiding this comment

joyeecheung Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Jan 31, 2018

BridgeAR commented Jan 31, 2018

BridgeAR commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Feb 2, 2018

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Jan 23, 2018 •

edited

Loading

joyeecheung Jan 31, 2018 •

edited

Loading

joyeecheung Feb 3, 2018 •

edited

Loading