self-identification for errors

[boneskull]

What is the Problem Being Solved?

@endo/compartment-mapper and ses both throw many exceptions (mostly Error and a dusting of other intrinsics).

As a consumer and/or a developer of these libraries, when an exception is thrown, I want to:

• Easily identify the subsystem(s) involved, whether debugging or no
• Have some notion of intent, at-a-glance
• Safely trap errors and programmatically take actions depending on the type of error
• Grep better

Description of the Design

I know of two (2) strategies for this, which are not mutually exclusive:

1. Subclassing builtin Error classes
2. Setting a code (typically a string constant, e.g., ENO_TEA) and/or errno (integers; probably negative) property on an Error object prior to throw.

I prefer using a subclass and providing a code field. instanceof has gotchas, but a subclass communicates intent for the Endo developer and enables further customization of behavior (URLs to docs; redaction; other formatting; additional metadata). A code field is common in the ecosystem, but errno is mostly a node-ism.

The name field should also be set for these subclasses (as intrinsic subclasses of Error do):

class GooseError extends Error {}

try {
  throw new GooseError()
} catch (err) {
  err.name // Error
}

class GanderError extends Error { name = 'GanderError' }
try {
  throw new GanderError()
} catch (err) {
  err.name // GanderError
}

In the weeds here:

The signature constructor signatures should probably be compatible with the constructor signature of Error, for the sake of familiarity and ease of implementation:

interface ErrorConstructor {
  new (message?: string, options?: ErrorOptions): Error;
}

interface ErrorOptions {
  cause?: unknown;
}

If we wish to accept additional metadata, we can just stuff it in the second parameter by extending ErrorOptions.

I'm not sure if we want to set code on the prototype or just use a class field. It is handy to have code available as a static field as well (e.g. GooseError.code === 'EHORRIBLE_GOOSE').

Unsure if we should be using getters or Object.defineProperty on these fields; it doesn't seem necessary to me, but I'm open to suggestions!

Implementation does not need to happen all at once; we don't need to define all the possible errors and codes up-front and then do a full sweep through the codebase looking for replacement candidates. For this issue, I'm hoping we can settle on a design.

[!IMPORTANT] Remember that class demands use of new (sorry @erights):

class GooseError extends Error {}

try {
  throw GooseError()
} catch (err) {
  err.name // TypeError
  err.message // Class constructor GooseError cannot be invoked without 'new'
}

I suppose we could avoid class and define them the old-fashioned way, though!

Security Considerations

I know there are some concerns in ses around the information exposed in Error objects. I don't know about it otherwise; for all I know, it could be the reason that exceptions are somewhat murky. That said, I don't think @endo/compartment-mapper faces or should face the same restrictions.

Scaling Considerations

Throwing in a hot code path has negative performance impacts, but I am not suggesting we throw more exceptions—only that we throw better ones.

The usual warnings about deep inheritance trees apply.

Test Plan

Depends on implementation, but we could certainly update extant tests to inspect error codes instead of error messages.

Compatibility Considerations

AFAIK subclassing Error is pretty straightforward in modern JS.

Upgrade Considerations

We may want to notify users that the code or class is part of the public API and thus will adhere to SemVer, but the error message may not. This also makes i18n/l10n more easily achievable, if that ever wants to happen.

[kriskowal]

Off the cuff, I can tell you I strongly disprefer error subclassing because of eval twin problems and strongly prefer structural error classification. We should continue to discourage callers to classify errors based on message name. I do not hate the code string property for classification. But, we should also assess programmatic error classification on a case-by-case basis, because it’s often better to reorient the API around better non-error response shapes, like we do with maybeRead.

[boneskull]

If we stick to structural composition or whatever, let's just be sure to have some sort of nice factory functions and strong typing. 😄

[erights]

Hi @boneskull , thanks for this! As you suspect, it intersects a lot of error issues I'm wrestling with, so I assigned myself. A key issue is to find an Error semantics that

• meets all our security constraints
• does everything we need
• is suitable to propose for ocapn
• enables out-of-band gathering of suitable diagnostics

Some relevant links: https://github.com/endojs/endo/blob/master/packages/ses/src/error/README.md https://github.com/endojs/endo/pull/2429 https://github.com/endojs/endo/pull/2223 https://github.com/endojs/endo/pull/2445 , which your observation about class is grounds for closing. We should instead always use new.

https://github.com/endojs/endo/pull/1814 https://github.com/endojs/endo/issues/1583

[boneskull]

@erights Is this specific to ses, or would it be workable to take another approach in @endo/compartment-mapper since (I'm assuming) it does not have the same constraints?

[erights]

Why would @endo/compartment-mapper not have the same constraints as ses? Not rhetorical. I genuinely am not oriented enough to have a strong opinion, but I am skeptical.

[erights]

We should continue to discourage callers to classify errors based on message name.

@kriskowal , I have been encouraging exactly this. Clearly this is another error issue we need to resolve.

maybeRead is only mentioned by compartment-mapper. I don't yet know what it is.

[erights]

That is, I encourage <some error>.name. I don't know if this is what you mean by "message name". I certainly discourage treating <some error>.message as semantic.

[boneskull]

@erights I'm not sure if I can answer your question about @endo/compartment-mapper; I don't know much about how Agoric consumes it other than I know it's used to generate archives ostensibly containing untrusted code. I would assume it was then used at a later point to recreate the compartment map from the archive prior to execution (compartment.import()).

In @lavamoat/node, @endo/compartment-mapper is consumed prior to execution of 3rd-party code, so any exceptions it throws will necessarily abort execution before a stack could be exposed to the untrusted code.

To that end, I'm not sure exactly what SES is redacting from exceptions or why (but I'd love to know!).

[boneskull]

@erights maybeRead is a function that reads a file and returns Promise<bytes> if it is successful, and Promise<undefined> if it is unsuccessful. Compare to read, which returns a Promise<bytes> if successful and rejects if unsuccessful. So I think @kriskowal is just getting at "let's throw fewer exceptions!"

[erights]

To that end, I'm not sure exactly what SES is redacting from exceptions or why (but I'd love to know!).

Primarily two things:

Redaction of sensitive user data

Fail`${300} must be less than ${q(10)}`

under production lockdown settings (no 'unsafe*') the error that's thrown has a .message of

(a number) must be less than "10"

whereas when this error is logged to the ses console, you see the unredacted message

"300" must be less than "10"

Thus, you should use q or b or qp to unredact data that is appropriate to reveal to untrusted callers, and to be sent over the network to untrusted peers. Data that is not so quoted is redacted under the assumption that it may carry sensitive user data. But it still shows up in local logs, under the presumption that only authorized parties can look at the logs.

Redaction of stack

Under production lockdown settings (no 'unsafe*') the error that's thrown has a .stack of ''

whereas when this error is logged to the ses console, you see the stacks. This is because stacks are notorious as a side channel for leaking data adequate to infer secrets.

See https://github.com/endojs/endo/blob/master/packages/ses/src/error/README.md for much more on these topics.