fix(ses): error stack consistency

mhofman commented 11 months ago

closes: #1810

Description

This change adds some rough normalizing of the shape of stack strings:

Includes error details in the first line, using the captured Error.prototype.toString
Stack frame lines are indented by at least one space (the shim uses 2 when it inserts them)
No trailing new line in stack strings

Furthermore it brings consistency to the various stack properties:

On non-v8, when error taming is safe, the stack prototype getter returns the empty string instead of the error details, like it already does on v8

Because getStackString now always includes the error details, I made the defaultGetStackString defined by asserts's loggedErrorHandler to no longer remove the first line. Accordingly I modified the console taming logic to only remove the first line of the stack string if it includes the error details, as those are already logged explicitly.

Security Considerations

None

Scaling Considerations

There will be slightly more processing of non-v8 error stacks when accessed, but it shouldn't impact most operations

Documentation Considerations

TBD

Testing Considerations

We don't have a good way to test cross engines. I manually tested the stack string shape normalization with eshost Added a test for getStackString

Upgrade Considerations

The stack string changes may be observable. In particular we're no longer using an own toString to print the details.

erights commented 11 months ago

I see that your PR is green under CI.

When I check it out, any idea why I get

error › tame-v8-error › lockdown Error is safe

  test/error/test-tame-v8-error.js:10

    9:   t.is(typeof Error.stackTraceLimit, 'number');
   10:   t.is(Error().stack, '');                     
   11: });                                            

  Difference (- actual, + expected):

  - `Error␊
  -   at file:///Users/markmiller/src/ongithub/endojs/endo/packages/ses/test/error/test-tame-v8-error.js:10:8␊
  -   at Test.callFn (file:///Users/markmiller/src/ongithub/endojs/endo/node_modules/ava/lib/test.js:523:21)␊
  -   at Test.run (file:///Users/markmiller/src/ongithub/endojs/endo/node_modules/ava/lib/test.js:536:23)␊
  -   at Runner.runSingle (file:///Users/markmiller/src/ongithub/endojs/endo/node_modules/ava/lib/runner.js:285:33)␊
  -   at Runner.runTest (file:///Users/markmiller/src/ongithub/endojs/endo/node_modules/ava/lib/runner.js:367:30)␊
  -   at processTicksAndRejections (node:internal/process/task_queues:96:5)␊
  -   at async Promise.all (index 0)␊
  -   at async file:///Users/markmiller/src/ongithub/endojs/endo/node_modules/ava/lib/runner.js:532:21␊
  -   at async Runner.start (file:///Users/markmiller/src/ongithub/endojs/endo/node_modules/ava/lib/runner.js:540:15)`
  + `

  › file://test/error/test-tame-v8-error.js:10:5

  error › tame-v8-error › lockdown Error in Compartment is safe

  test/error/test-tame-v8-error.js:27

   26:   t.is(c.evaluate('typeof Error.stackTraceLimit'), 'undefined');
   27:   t.is(c.evaluate('Error().stack'), '');                        
   28: });                                                             

  Difference (- actual, + expected):

  - `Error␊
  -   at Object.eval (eval at <anonymous> (eval at makeEvaluate (file:///Users/markmiller/src/ongithub/endojs/endo/packages/ses/src/make-evaluate.js:92:27)), <anonymous>:1:1)␊
  -   at Object.eval (eval at makeEvaluate (file:///Users/markmiller/src/ongithub/endojs/endo/packages/ses/src/make-evaluate.js:92:27), <anonymous>:12:22)␊
  -   at safeEvaluate (file:///Users/markmiller/src/ongithub/endojs/endo/packages/ses/src/make-safe-evaluator.js:78:14)␊
  -   at compartmentEvaluate (file:///Users/markmiller/src/ongithub/endojs/endo/packages/ses/src/compartment-evaluate.js:90:10)␊
  -   at Compartment.evaluate (file:///Users/markmiller/src/ongithub/endojs/endo/packages/ses/src/compartment.js:105:12)␊
  -   at file:///Users/markmiller/src/ongithub/endojs/endo/packages/ses/test/error/test-tame-v8-error.js:27:10␊
  -   at Test.callFn (file:///Users/markmiller/src/ongithub/endojs/endo/node_modules/ava/lib/test.js:523:21)␊
  -   at Test.run (file:///Users/markmiller/src/ongithub/endojs/endo/node_modules/ava/lib/test.js:536:23)␊
  -   at Runner.runSingle (file:///Users/markmiller/src/ongithub/endojs/endo/node_modules/ava/lib/runner.js:285:33)␊
  -   at Runner.runTest (file:///Users/markmiller/src/ongithub/endojs/endo/node_modules/ava/lib/runner.js:367:30)`
  + `

  › file://test/error/test-tame-v8-error.js:27:5

  error › tame-v8-error › lockdown Error in nested Compartment is safe

  test/error/test-tame-v8-error.js:34

   33:   t.is(c.evaluate('typeof Error.stackTraceLimit'), 'undefined');
   34:   t.is(c.evaluate('Error().stack'), '');                        
   35: });                                                             

  Difference (- actual, + expected):

  - `Error␊
  -   at Object.eval (eval at <anonymous> (eval at makeEvaluate (file:///Users/markmiller/src/ongithub/endojs/endo/packages/ses/src/make-evaluate.js:92:27)), <anonymous>:1:1)␊
  -   at Object.eval (eval at makeEvaluate (file:///Users/markmiller/src/ongithub/endojs/endo/packages/ses/src/make-evaluate.js:92:27), <anonymous>:12:22)␊
  -   at safeEvaluate (file:///Users/markmiller/src/ongithub/endojs/endo/packages/ses/src/make-safe-evaluator.js:78:14)␊
  -   at compartmentEvaluate (file:///Users/markmiller/src/ongithub/endojs/endo/packages/ses/src/compartment-evaluate.js:90:10)␊
  -   at Compartment.evaluate (file:///Users/markmiller/src/ongithub/endojs/endo/packages/ses/src/compartment.js:105:12)␊
  -   at file:///Users/markmiller/src/ongithub/endojs/endo/packages/ses/test/error/test-tame-v8-error.js:34:10␊
  -   at Test.callFn (file:///Users/markmiller/src/ongithub/endojs/endo/node_modules/ava/lib/test.js:523:21)␊
  -   at Test.run (file:///Users/markmiller/src/ongithub/endojs/endo/node_modules/ava/lib/test.js:536:23)␊
  -   at Runner.runSingle (file:///Users/markmiller/src/ongithub/endojs/endo/node_modules/ava/lib/runner.js:285:33)␊
  -   at Runner.runTest (file:///Users/markmiller/src/ongithub/endojs/endo/node_modules/ava/lib/runner.js:367:30)`
  + `

  › file://test/error/test-tame-v8-error.js:34:5

  ─

  3 tests failed
  2 known failures
  2 tests skipped
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
(1):ses(mhofman/1810-error-stack-consistency)$

mhofman commented 11 months ago

When I check it out, any idea why I get

It looks like those stacks are not censored. Do you have any env that would cause error taming to be unsafe?

erights commented 10 months ago

@mhofman is this really not yet merged? Did something else get merged in its place? If not, please merge before the next endo release. Thx.

mhofman commented 10 months ago

@mhofman is this really not yet merged? Did something else get merged in its place? If not, please merge before the next endo release. Thx.

Sorry, things came up on the agoric-sdk side. I plan on picking this back up later this week.

erights commented 10 months ago

Please before the next endo release? (attn @kriskowal )

erights commented 8 months ago

ping re upcoming endo release ;)

mhofman commented 8 months ago

ping re upcoming endo release ;)

I need to reevaluate this. I remember there were issues before, which is why I didn't merge it. I marked the PR back as draft to reflect that.

endojs / endo