-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(pass-style,exo): exo boundary only throws throwables #2266
Conversation
17dae07
to
4e19ba7
Compare
agoric-sdk compat testing underway at Agoric/agoric-sdk#9201 , with pound-endo-branch: markm-to-throwable-2 |
@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. |
CI at Agoric/agoric-sdk#9201 failed due to the presence of |
CI is still failing a lot, but none of the failures seem to have anything to do with this PR. |
Restaged Agoric/agoric-sdk#9201 on Agoric/agoric-sdk#8774 and trying again. I see that Agoric/agoric-sdk#8774 is not green either. But if Agoric/agoric-sdk#9201 only has the same failures as Agoric/agoric-sdk#8774 , I'll count that as a success. |
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 Speaking of which, Agoric/agoric-sdk#9201 CI now has novel failures such as
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 Agoric/agoric-sdk#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. |
19b5817
to
8966b06
Compare
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 |
8966b06
to
ed8e427
Compare
closes: #XXXX
refs: #2223
Description
Modest step towards #2223 ,
callWhen
async exo methods only reject with throwable reasons.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
Far
for making far objects into making exos.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.NEWS.md
for user-facing changes.