endojs / endo

Endo is a distributed secure JavaScript sandbox, based on SES
Apache License 2.0
804 stars 71 forks source link

fix(ses): use prepareStackTrace for error stack string #1799

Open mhofman opened 11 months ago

mhofman commented 11 months ago

closes: #1798

Description

Fixes Error.prepareStackTrace with the following behavior changes:

Security Considerations

This change defers to a prepareStackTrace set in the start compartment to generate SES stack strings. The assumption is that such overrides are vetted.

Scaling Considerations

The new implementation uses try/catch which is often more expensive, but it likely dwarfs in comparison to the call sites wrapping we are already doing.

Documentation Considerations

How should this change be documented? I somewhat expected this to be the behavior in the first place

Testing Considerations

Passes existing unit test.

Upgrade Considerations

None

mhofman commented 11 months ago

For reference, I think v8 does its own changes to Error for prepareStackTrace:

erights commented 11 months ago

For reference, I think v8 does its own changes to Error for prepareStackTrace:

Did you mean Node?

kriskowal commented 10 months ago

@mhofman You have a stamp and a request from @turadg to include this in the next release.

mhofman commented 10 months ago

This does not do what it's supposed to do, back to draft

erights commented 2 months ago

Hi @mhofman , after this had several approvals, you wrote:

This does not do what it's supposed to do, back to draft

Do you remember what the problems are?

As a draft this PR was not assigned. I went ahead and assigned myself, since I want to look at reviving it. I also left myself on as a reviewer FWIW.

mhofman commented 2 months ago

Do you remember what the problems are?

Yeah it didn't solve the problem. I think because Node.js verified that the prepareStackTrace function was not modified from its own.