fix(pass-style): fix #2700 ignore more safe async_hook extra properties #2701
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
erights
force-pushed
the
markm-fix2700-ignore-safe-asynchook-extras
branch
from
10aac43 to
76012d0
Compare
erights
commented
Jan 27, 2025
| t.throws(() => passStyleOf(harden(p3)), { | ||
| message: 'Own @@toStringTag value must be a string: 3', | ||
| }); | ||
| t.is(passStyleOf(harden(p3)), 'promise'); |
There was a problem hiding this comment.
The change to this test case demonstrates the loosening of the @@toStringTag tests
erights
commented
Jan 27, 2025
| message: | ||
| 'Own @@toStringTag must not be enumerable: {"configurable":false,"enumerable":true,"value":"Promise p4","writable":false}', | ||
| }); | ||
| t.is(passStyleOf(harden(p4)), 'promise'); |
There was a problem hiding this comment.
The change to this test case demonstrates the loosening of the @@toStringTag tests
|
Because the observable change of #2700 that this PR allegedly fixes is only observed interactively when running the vscode debugger, this PR does not add a test case verifying the fix. Rather, I manually verified that this PR fixes #2700 by running the steps it documents. That run got well past the point where the previous problem was encountered, but then causes #2702 , which I suspect and hope is unrelated. |
|
I'm gonna have to look at this closer because the async hooks taming was supposed to handling these things |
mhofman
requested changes
Jan 31, 2025
There was a problem hiding this comment.
I believe the orignal checks are fine, it's just that they rely on @endo/init to have applied the node async_hooks patch that tames things.
See #2700 (comment)
erights
force-pushed
the
markm-fix2700-ignore-safe-asynchook-extras
branch
from
76012d0 to
08e4fc0
Compare
kriskowal
added a commit
that referenced
this pull request
Mar 19, 2025
Closes: #2700 Refs: #2701 ## Description Implements exactly @mhofman 's suggestion at #2700 (comment) Alternative to #2701 Changes some occurrences of `import 'ses';` to `import '@endo/init';` so that the async hooks are properly prepared so that things work in an interactive debug session -- at least in vscode. See #2700 However, there are many other bare `import 'ses';` occurrences in endo. How many for of these need to `import '@endo/init';` instead in order to avoid this same problem? For now, I conservatively looked for other occurrences of the common marking comment "Establish a perimeter". I found two additional ones that this PR currently just annotates with a TODO. ***Reviewers, please advise.*** If we take this approach, can we avoid needing to teach developers not to `import 'ses';` so they avoid this problem? Or should we move the async-hook preparation somehow from `@endo/init` in `ses`, perhaps as a repair, so all existing `import 'ses';` occurrences become correct? Or should we instead proceed with #2701 after all? ### Security Considerations none ### Scaling Considerations none ### Documentation Considerations Weird to need to teach developers to avoid `import 'ses';`. I hope we can avoid needing to teach this. ***Reviewers*** please advise. ### Testing Considerations The underlying issue #2700 occurs during an interactive debug session. If this is not fixed, it can impede the run-debug-loop, especially for running tests. In particular, ses-ava currently `import 'ses';` rather than `import '@endo/init';`. This would perhaps be the highest next priority to change. ### Compatibility Considerations Perhaps not all occurrences of `import 'ses';` can or should be switched to `import '@endo/init';` because of the other things bundled into `@endo/init`. ### Upgrade Considerations I think none. ***Reviewers: please double check me on this.***
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: #2700
Refs: #2708
Description
#2700 explains how
passStyleOf's criteria for judging a promise to be passable happens to cause some failures when run under the vscode debugger. This both loosens the criteria to accept those properties that async_hooks might add to an individual promise instance, and tightens the criteria to enforce that these properties are symbol-named non-configurable non-writable own data properties with values that are obviously safe, such as primitives.Because
@@toStringTagwould normally satisfy this more general criteria, and doing so would guarantee it safe enough for all known purposes, this PR no longer makes a special case for it. By folding it into the more general check above, we no longer enforce that this property is non-enumerable, nor that the value of this property is a string.This is an alternate approach to #2708
Security Considerations
The tightened criteria above enhance safety. But the loosenings above endanger safety, and so must be carefully considered. This loosening allows properties with any symbol name, whether or not async_hooks would ever add them. Further, it loosens the restrictions on the
@@toStringTag-named property in ways that code might conceivably find confusing.Reviewers, please form your own judgement about whether such loosening actually matters. I will tighten back up whatever you request.
Scaling Considerations
none
Documentation Considerations
none for normal programmers.
For use of a debugger, this PR only removes an unpleasant surprise that would otherwise need to be explained.
Testing Considerations
none for non-debugger-based testing.
Compatibility Considerations
Both the tightenings and the loosenings are conceivably compat breaks, but are highly unlikely to matter in practice.
Upgrade Considerations
none.