endo icon indicating copy to clipboard operation
endo copied to clipboard

Hardened JavaScript interferes with Node.js 14 Error construction

Open kriskowal opened this issue 4 years ago • 5 comments

@warner reports:

Then I used fs.openReadStream(undefined) (not intentionally, I forgot to provide argv). The error I get is TypeError#1: Cannot assign to read only property 'name' of object 'TypeError: The "path" argument must be of type string or an instance of Buffer or URL. Received undefined' the stack trace includes:

 at addCodeToName (internal/errors.js:329:12)
 at new NodeError (internal/errors.js:294:7)
 at internal/fs/utils.js:634:11

The relevant code in Node.js 14.17.4 is:

function addCodeToName(err, name, code) {
  // Set the stack
  if (excludedStackFn !== undefined) {
    ErrorCaptureStackTrace(err, excludedStackFn);
  }
  // Add the error code to the name to include it in the stack trace.
  err.name = `${name} [${code}]`;
  // Access the stack to generate the error message including the error code
  // from the name.
  err.stack; // eslint-disable-line no-unused-expressions
  // Reset the name to the actual name.
  if (name === 'SystemError') {
    ObjectDefineProperty(err, 'name', {
      value: name,
      enumerable: false,
      writable: true,
      configurable: true
    });
  } else {
    delete err.name;
  }
}

And the culprit is the property override hazard, upon assigning to name of an instance descended from a frozen error constructor.

kriskowal avatar Aug 24 '21 21:08 kriskowal

Although #867 fixes the issue revealed here, it won't actually change this specific occurrence. The problematic Node code might override the name property of any error by assignment. #867 extends the default overrideTaming: 'moderate' setting to enable override-by-assignment of name for all error types. But it was already enabled for Error and TypeError, so this symptom would not have been triggered at the 'moderate' setting. We only became aware of it by debugging at the overrideTaming: 'min' setting. At that setting, this problem will still occur.

erights avatar Aug 24 '21 22:08 erights

@kriskowal Can you please put an estimate on this?

Tartuffo avatar Feb 02 '22 15:02 Tartuffo

@erights I’m not clear on whether or what the next steps for this issue would be. Is there fix we could stomach that would address this issue at all overrideTaming settings?

kriskowal avatar Feb 02 '22 23:02 kriskowal

Well, we could trivially copy this moderate change so it applies to min too. I don't like it, but Node....

Have we told Node about this? Have we filed an issue? Is it still a problem on Node 16?

erights avatar Feb 03 '22 01:02 erights

@kriskowal This does not have an area label that is covered by our weekly tech / planning meetings. Can you assign the proper label? We cover: agd, agoric-cosmos, amm, core economy, cosmic-swingset, endo, getrun, governance, installation-bundling, metering, run-protocol, staking, swingset, swingset-runner, token economy, wallet, zoe contract,

Tartuffo avatar Feb 03 '22 16:02 Tartuffo

We no longer support Node.js 14.

kriskowal avatar Jan 10 '24 01:01 kriskowal