feat(pass-style,exo): exo boundary only throws throwables

erights commented 2 months ago

closes: #XXXX refs: https://github.com/endojs/endo/pull/2223

Description

Modest step towards #2223 ,

bringing all the intended safety: the exo boundary only throws throwables, and that callWhen async exo methods only reject with throwable reasons.
but with less flexibility: the definition of passable errors and throwables is narrower than it need be.

Security Considerations

This is one of three necessary steps towards having the exo boundaries become the new intravat defensive security boundary, rather than treating every object boundary as a potential defensive security boundary. The rationale for this step is that the throw-control-path is too hard for reviewers to pay full attention to, so we wish to prevent caps from leaking over the exo boundary via the throw path. (E prevented caps from escaping via throws in general, but we're not in a position to be able to enforce that in general within Hardened JS.)

The other two steps are

Converting uses of Far for making far objects into making exos.
Addressing the leakage of promise settlements across exo boundaries due to the inability to synchronously enforce such a constraint on a promise passed through the boundary.

Scaling Considerations

Given that the exceptional pathways (throws and rejects) are only used for low frequency exceptional conditions, or at least exceptional conditions whose speed we don't care about, making this slow path a tiny bit slower should be inconsequential. Indeed, I expect the overall impact to be unmeasurable. (But we won't know until we measure!)

Documentation Considerations

This restriction on what can be throws across the exo boundary (or for exo async callWhen methods, what can be used as a rejection reason) needs to be documented. But it should not affect any normal code, and so documents an edge case.

Testing Considerations

tests included

Compatibility Considerations

If there are currently any throws or rejects that violate these constraints (likely) where other code depends on these violations (unlikely), then we have a compat problem.

Upgrade Considerations

None.

~[ ] Includes *BREAKING*: in the commit message with migration instructions for any breaking change.~
[x] Updates NEWS.md for user-facing changes.

erights commented 2 months ago

agoric-sdk compat testing underway at https://github.com/Agoric/agoric-sdk/pull/9201 , with

pound-endo-branch: markm-to-throwable-2 and force:integration label

erights commented 2 months ago

@kriskowal I hopefully marked this as next-release because it would be really nice for that to happen, and I also hope this is an easy enough review to not impose much delay. But if not, feel free not to include this in the next release.

erights commented 2 months ago

CI at https://github.com/Agoric/agoric-sdk/pull/9201 failed due to the presence of patches/@endo+exo+1.4.0.patch , which seems will need to disappear in the next sync anyway. So I just added to it a removal of that patch file to https://github.com/Agoric/agoric-sdk/pull/9201 and am trying again.

erights commented 2 months ago

CI is still failing a lot, but none of the failures seem to have anything to do with this PR.

erights commented 2 months ago

Restaged https://github.com/Agoric/agoric-sdk/pull/9201 on https://github.com/Agoric/agoric-sdk/pull/8774 and trying again. I see that https://github.com/Agoric/agoric-sdk/pull/8774 is not green either. But if https://github.com/Agoric/agoric-sdk/pull/9201 only has the same failures as https://github.com/Agoric/agoric-sdk/pull/8774 , I'll count that as a success.

erights commented 2 months ago

Reviewers, because it is possible in theory that some old code was depending on an exo throwing something that would now be classified as a non-throwable, this PR is in theory a compat break. So once again, please let me know whether you think this PR should be marked as a feat(pass-style,exo)!:. IMO if https://github.com/Agoric/agoric-sdk/pull/9201 comes out as clean as https://github.com/Agoric/agoric-sdk/pull/8774 , I hope the answer will be no, we do not need the !.

Speaking of which, https://github.com/Agoric/agoric-sdk/pull/9201 CI now has novel failures such as

Error: src/storage-test-utils.js(2,1): error TS9006: Declaration emit for this file requires using private name 'PASS_STYLE' from module '"/home/runner/work/agoric-sdk/agoric-sdk/node_modules/@endo/pass-style/src/types"'. An explicit type annotation may unblock declaration emit.

at https://github.com/Agoric/agoric-sdk/actions/runs/8956132010/job/24597592447?pr=9201#step:6:2640

These still seem unrelated to this PR, so I don't understand why they are different from https://github.com/Agoric/agoric-sdk/pull/8774 . OTOH, it does seem to have something to do with the @endo/pass-style module, so perhaps it is related to this PR in a way I don't understand.

kriskowal commented 2 months ago

You are down to a couple ts-expect-errors that can be removed in integration. You can do me a favor by adding a commit to endo-integration-master on Agoric SDK so it gets picked up by CI and eventually by me for the sync. https://github.com/Agoric/agoric-sdk/pull/9201#issuecomment-2096909380

endojs / endo