fix(ses): makeError defaults to making passable errors #2200
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
|
Following the repro instructions at #2198 , without this PR I get the bug it describes on the console output of Brave (v8), Firefox (SpiderMonkey) and Safari (JSC). With this PR, I get the following screenshots, showing that the desired error is now successfully reported Brave: Firefox Safari |
erights
changed the title
erights
force-pushed
the
markm-sanitize-errors
branch
2 times, most recently
from
056e4ab to
c99f530
Compare
erights
force-pushed
the
markm-make-error-freezes
branch
from
fccab59 to
2ce3733
Compare
erights
force-pushed
the
markm-sanitize-errors
branch
from
c99f530 to
62b42ef
Compare
|
Now testing CI for Agoric/agoric-sdk#9201 with |
That CI run successful. All green. |
kriskowal
approved these changes
Apr 6, 2024
erights
force-pushed
the
markm-sanitize-errors
branch
from
62b42ef to
0284f08
Compare
erights
added a commit
that referenced
this pull request
Apr 19, 2024
This was referenced Apr 19, 2024
Open
mhofman
reviewed
Apr 19, 2024
| * - `sanitizeError` will ensure that any expected properties already | ||
| * added by the host are data | ||
| * properties, converting accessor properties to data properties as needed, | ||
| * such as `stack` on v8 (Chrome, Brave, Edge?) |
There was a problem hiding this comment.
It looks like stack own accessors are not modified by this helper, what am I missing?
Edit: never mind, I missed the second loop
erights
added a commit
that referenced
this pull request
Apr 22, 2024
closes: #2198 refs: #2230 #2200 https://chromium-review.googlesource.com/c/v8/v8/+/4459251 #2229 #2231 ## Description Alternative to #2229 that just hacks `harden` to directly repair a problematic error own stack accessor property, replacing it with a data property. ### Security Considerations Both before and after this PR, `passStyleOf` will reject errors with the v8 problematic error own stack accessor property, preventing the unsafety at stake here. However, this would mean that much existing code that used to be correct will break when run on a v8 with this problem. ### Scaling Considerations Avoids any extra overhead on platforms without this problem, including all platforms other than v8. ### Documentation Considerations probably none. This PR essentially avoids the need to document the v8 problem that it masks. ### Testing Considerations Only needed to repair one test to use `harden` rather than `freeze`, in a case where `harden` was more natural anyway. ### Compatibility Considerations This PR enables more errors to pass that check without further changes to user code. #2229 had similar goals, but would still require more changes to user code than this PR. This is demonstrated by all the test code in #2229 that needed to be fixed that does not need to be fixed in this PR. ### Upgrade Considerations none - ~[ ] Includes `*BREAKING*:` in the commit message with migration instructions for any breaking change.~ - ~[ ] Updates `NEWS.md` for user-facing changes.~
Merged
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: #2198
refs: #2156
Description
The symptoms of #2156 reveal that some hosts add "extraneous" or harmful own properties to errors
stackaccessor property, whose getter and setter functions can be used for mischief.fileNameandlineNumberproperties that are useful for diagnostics but should be redacted from public view.lineproperty, that likewise is useful for diagnostics, but should be redacted from public view.Note that the contents of the stack should also be redacted from public view, but SES already deals with that separately.
Prior to this PR, some code assumed that the error that
makeErrorreturns would be passable once frozen. But these extra own properties, or own accessor properties, added by the host beforemakeErrorreturns the error, caused it not to be passable, causing #2198 . As of this PR,makeErrorby default makes reasonable effort to return a passable error despite such host-added own properties. However, some code took advantage of the mutability of the error thatmakeErrorwas returning, so this PR adds asanitize: falseoption to suppress this cleaning and freezing of the returned error.makeErrordoes not guarantee that the returned error is passable, no matter whatcauseorerrorsoptions are provided. The purpose of this new sanitization behavior is to protect against host mischief. To test or coerce an error into a passable error, usetoPassablein @endo/pass-style.Security Considerations
Having
makeError, by default, return errors that are already better behaved and more discreet helps security. However, because the underlying platform makes errors that are not sanitized in this manner, this PR doesn't help security significantly.Scaling Considerations
none
Documentation Considerations
I added doc-comments to the
makeErroroptions doc-comment. However, the important added doccomment is on the newsanitizeErrorfunction which is encapsulated with SES's assert.js. I made aTODOthat I don't know how one doccomment can link to another.Testing Considerations
This problem was caught by accident. We should do enough browser-based testing early enough that problems like this would be caught first by purposeful testing.
Compatibility Considerations
This PR is staged on #2156. Like #2156, it presents a compat problem in theory but likely not in practice. #2156 stops exporting
isPassableErrorandassertPassableError, which it turns out were never correct but which no one imports or uses.This PR changes
makeErrorto return a "sanitized" and frozen error, where it used to return a non-sanitized and non-frozen error. I looked through every call tomakeErrorin endo and agoric-sdk, and found one site that made use of the non-frozen nature of that error. However,makeErroris much older, so it is more plausible that there are other such uses outside these two repos. To accommodate that one known use case and mitigate problems at possible others, this PR adds asanitize: falseoption tomakeErrorto recover the old behavior. That one known site uses this option.I leave it to my reviewers whether this PR as well as #2156 should be marked with a
!or not. In theory both should be. IMO, we should mark neither with!. Please let me know.Upgrade Considerations
Since this bug manifested for #2156 on unmarshalling, i.e., unserializing use the
@endo/marshalmechanisms, that bug raises the possibility of an upgrade hazard -- that an error saved to durable storage before an upgrade might cause another error on the attempt to read it from durable storage after the upgrade. This PR prevents that hazard.*BREAKING*:in the commit message with migration instructions for any breaking change.NEWS.mdfor user-facing changes.