fix(ses): temp patch in Endo 2355,2359 line-number workaround #9711
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deploying agoric-sdk with
|
| Latest commit: |
8558636
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://db4215b1.agoric-sdk.pages.dev |
| Branch Preview URL: | https://markm-patch-endo-linenumber.agoric-sdk.pages.dev |
erights
force-pushed
the
markm-patch-endo-linenumber-sensitivity
branch
2 times, most recently
from
b22f8ae to
d4dc4cc
Compare
This was referenced Jul 14, 2024
|
In light of the review suggestions on endojs/endo#2355 , I'm putting this one into Draft until the form of endojs/endo#2355 is at least settled, and ideally merged. |
erights
added a commit
to endojs/endo
that referenced
this pull request
Jul 16, 2024
Closes: #2348 Refs: Agoric/agoric-sdk#9711 #1798 #1799 Agoric/agoric-sdk#8662 Agoric/agoric-sdk#9700 ## Description Prior to this PR, when you ran on Ava on Node a test written in TypeScript, you'd see something like the following in your stack traces. ``` boot/test/bootstrapTests/stack-linenumbers.test.ts:1:104 ``` This is because the TypeScript compiler compiles a TypeScripy file into one line of JavaScript with a sourceMap that should map back into original source positions. Node specifically makes use of that sourceMap to produce original line-numbers. However, Node does this in a way that resists virtualization, so the normal SES error taming cannot use this sourceMap info. By default, this PR does not change this behavior. However it recognizes a new `SUPPRESS_NODE_ERROR_TAMING` environment variable. With the `SUPPRESS_NODE_ERROR_TAMING` environment variable absent or set to `'disabled'`, you should still see stack traces as shown above However, if you also set the `SUPPRESS_NODE_ERROR_TAMING` environment variable `'enabled'`, for example by doing ```sh $ export SUPPRESS_NODE_ERROR_TAMING=enabled ``` at a bash shell, then when you run this test you should instead see something like ``` boot/test/bootstrapTests/stack-linenumbers.test.ts:40:32 ``` At Agoric/agoric-sdk#9711 I both - turn this PR into an agoric-sdk patch of endo, in order to emulate this fix until the next endo-release-agoric-sdk-sync cycle, and - add a test case that emits an error stack trace from an Ava test case written in TypeScript, to test that it works. ### Security Considerations This new behavior only applies when `errorTaming: 'unsafe'`, on v8, and with this new environment variable enabled. Setting `errorTaming: 'unsafe'` already flags to sacrifice some security for a better debugging experience. But the loss of security is moderate enough --- mostly confidentiality rather than integrity --- that some may chose this setting for some production purposes. The new behavior is a more severe loss of security that really should be used ***only during development***, not production, when even a severe loss of security is usually not an issue. ### Scaling Considerations none ### Documentation Considerations The behavior prior to this PR or without this environment variable enabled is an unpleasant debugging experience. However, developers won't know how to repair it, or even that it can be repaired, without explanation. Even then, the difficultly of discovery in a problem. The names `SUPPRESS_NODE_ERROR_TAMING` and the settings `'enabled'` and `'disabled'` are by no means clear expressions of what this does. Reviewers, ***better names would be appreciated!*** ### Testing Considerations The point. As developers write and run tests written in TypeScript, they need to iterate with problems revealed by the tests, for which they need good line numbers, including into the test code. When the environment variable is enabled, the new behavior broke some SES tests written specifically to test the old behavior. This would not happen under CI because the environment variable is not set by default, and so may not have been noticed. But it was revealed in local testing. To repair this, this PR also sets those tests up to set `process.env.SUPPRESS_NODE_ERROR_TAMING` to `'disabled'` before lockdown, protecting those tests from the external environment variable setting. Awkwardly, at the moment Agoric/agoric-sdk#9711 serves as the only test of this PR. This is because I failed to figure out how to configure things so I can run TypeScript tests under Ava, like Agoric/agoric-sdk#9711 does. I tried cargo culting the configs that seemed relevant, but it didn't work. Reviewers, if you let me know how to do this, I'll duplicate the test case from Agoric/agoric-sdk#9711 here, which would be good. ### Compatibility Considerations With the environment variable absent or disabled, there should be zero difference in behavior, so none. In a development environment where this environment variable is enabled, some stack traces will be different. But outside of SES itself, nothing should depend on the contents of stack traces, so again none. ### Upgrade Considerations No upgrade considerations. Nothing BREAKING. - [x] Update `NEWS.md` for user-facing changes.
This was referenced Jul 16, 2024
erights
added a commit
to endojs/endo
that referenced
this pull request
Jul 17, 2024
*BREAKING*: Importing `@endo/lockdown/commit-debug.js` will now endanger integrity as well, in order to get better line numbers. `@endo/lockdown/commit-debug.js` should normally be used only for development/debugging/testing scenarios, where it is not potentially in contact with any genuinely malicious code. For those cases, there is no BREAKING change here. But we're flagging a potentially breaking change in case anyone is importing `@endo/lockdown/commit-debug.js` in scenarios where potentially malicious code is a concern. Such uses should be revised to avoid setting `errorTaming: 'unsafe-debug'`. Closes: #XXXX Refs: #2355 Agoric/agoric-sdk#9711 ## Description In #2355 , we introduced a new `errorTaming` setting, `'unsafe-debug'`, to sacrifice more security for a better debugging experience. In many ways it would have been more convenient to modify `'unsafe'` to do this, rather than introduce a new setting. But we did not because the `'unsafe'` setting was already documented as threatening only confidentiality, not integrity. Many production scenarios don't need the `'safe'` level of confidentiality defense, and so have been using `errorTaming: 'unsafe'` in production, as shown below. Thus, a less-safe form had to be a new mode, so the loss of safety was purely opt-in. However, for uses that were clearly development-only uses, especially those that are explicitly about testing/debugging, they often inherit these lockdown settings from a long chain of imports bottoming out in ```js import '@endo/lockdown/commit-debug.js'; ``` Since this import is explicitly named something-debug, and since none of the production uses of `errorTaming: 'unsafe'` that we are aware of inherit it from `endo/lockdown/commit-debug.js`, it seems we could recover the convenience for those testing/debugging uses by modifying this file in place, with an acceptable security risk. This PR does so. ### Some existing uses of `errorTaming: 'unsafe'` Some of these are in production code https://github.com/LavaMoat/LavaMoat/blob/1cb057b281d46d2564872f53c2769bf4c4cb0ba5/packages/webpack/src/plugin.js#L54 https://github.com/LavaMoat/LavaMoat/blob/1cb057b281d46d2564872f53c2769bf4c4cb0ba5/packages/core/src/kernelTemplate.js#L60 https://github.com/LavaMoat/LavaMoat/blob/1cb057b281d46d2564872f53c2769bf4c4cb0ba5/packages/webpack/src/plugin.js#L54 https://github.com/MetaMask/metamask-extension/blob/7b3450a294a2fe5751726a9c33d6fa0b564f03dd/app/scripts/lockdown-run.js#L6 https://github.com/MetaMask/snaps/blob/0a265dcf9de73a16a7b50cc47681fe6da179383a/packages/snaps-utils/src/eval-worker.ts#L14 https://github.com/MetaMask/snaps/blob/0a265dcf9de73a16a7b50cc47681fe6da179383a/packages/snaps-execution-environments/src/common/lockdown/lockdown.ts#L15 From the [psm.inter.trade](https://psm.inter.trade/) console  ### Security Considerations As explained above. If there are any production uses that get their lockdown settings by importing `@endo/lockdown/commit-debug.js`, they will experience a silent loss of security when they upgrade to depend on this more recent version. We are not aware of any such cases. Because of the explicit "debug" in the name, we expect such cases to be rare. But we have no way to confirm they do not exist. Reviewers, please let me know if you'd like me to change from a `fix:` to a `fix!` because of this silent loss of security on upgrading the dependency. ### Scaling Considerations none ### Documentation Considerations #2355 documents the differences between `'unsafe'` and `'unsafe-debug'` for both security and functionality. - [ ] Somewhere we need to explain that `endo/lockdown/commit-debug.js` has changed in place, explaining the difference. ### Testing Considerations Most of our tests inherit their `lockdown` settings from importing `@endo/lockdown/commit-debug.js`, so these test would experience both the security loss --- which should not matter for testing --- and functionality gain of seeing correct line-numbers into transpiled code, such as TypeScript sources. ### Compatibility Considerations For development purposes, practically none, since the loss of security in question is unlikely to matter. The improvement in line numbers will help developers looking at stack traces, but is unlikely to affect any code. This change is not compatible with production code that imports `@endo/lockdown/commit-debug.js`. But as the name indicates, production code should not have been importing this file anyway. ### Upgrade Considerations No upgrade issues - [ ] Include `*BREAKING*:` in the commit message with migration instructions for any breaking change. - [x] Update `NEWS.md` for user-facing changes.
erights
force-pushed
the
markm-patch-endo-linenumber-sensitivity
branch
from
99fa2c4 to
af82dde
Compare
erights
changed the title
|
For the current state of this PR, I locally verified that the output of the test case contains
|
|
Ready for Review! |
kriskowal
approved these changes
Jul 18, 2024
erights
added
automerge:squash
|
@Mergifyio refresh |
✅ Pull request refreshed |
mhofman
added
automerge:squash
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes: #8662
refs: endojs/endo#2355 endojs/endo#2348
Description
endojs/endo#2355 makes it possible to see accurate line numbers into TypeScript Ava tests run under Node, which would fix #8662 as of the next endo-release-agoric-sdk-sync cycle. To get this benefit before then, this PR turns endojs/endo#2355 into an agoric-sdk patch of endo. This PR also adds a test case to demonstrate that the fix works, and updates the
env.mdfile to document the new environment variable for enabling this behavior.Security Considerations
See Security Consideration of endojs/endo#2355 . In short, this feature sacrifices security for a better test-debug experience, which is fine for development only.
Scaling Considerations
none
Documentation Considerations
See Documentation Considerations of endojs/endo#2355
Testing Considerations
This PR effectively serves as a test both for endojs/endo#2355 and for the corresponding patch in this PR.
Upgrade Considerations
none.