Retain original sandbox errors (from different JavaScript realms) without coercion #355
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
…hout coercion This PR retains original errors being returned from different realms. Currently, this module [wraps any such errors](https://github.com/guigrpa/docx-templates/blob/master/src/jsSandbox.ts#L54) as `new Error(`${err}`)`, coercing their name/message into a string, losing these as separate fields, and losing the stack trace entirely (it gets effectively replaced when the error is re-thrown). The reason it does this is because `instanceof Error` returns false for sandboxed errors when the prototype chain doesn't match; these errors are still Error objects, just that they can't be detected with `instanceof`. This change affects the formatting of `CommandExeuctionError`, since the string coercion no longer happens automatically, so the `${err.name}` has been added back in to its message. Most tests continue to work unchanged, but several have required updating now that this messaging is consistent between sandboxing and unsandboxed execution.
|
Awesome work and good find. PR looks great. I have one question but otherwise looks good to merge. |
jjhbw
reviewed
Mar 21, 2024
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.
This PR retains original errors being returned from different realms. Currently, this module wraps any such errors as
new Error(`${err}`), coercing their name/message into a string, losing these as separate fields, and losing the stack trace entirely (it gets effectively replaced when the error is re-thrown). The reason it does this is becauseinstanceof Errorreturnsfalsefor sandboxed errors when the prototype chain doesn't match; these errors are still Error objects, it's just that they can't be detected withinstanceof.For context,
instanceofwill return false if an object is from a different realm (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof#instanceof_and_multiple_realms), which affects use of sandboxing of JavaScript. When sandboxing is enabled (orrunJscalls out to a different environment - where I found this issue),err instanceof Errorreturns false, due to a different prototype chain. Likewise, when sandboxing is disablederr instanceof Errorwill always return true since it's the same execution environment.To fix this issue, this PR retains the original error without wrapping. To do so, it adds an
isErrortype guard which checks bothinstanceof Errorand the mandatory fields of the Error type, and treats matching errors asErrorthereafter. (The suggested fix for the same issue withinstanceof ArrayisArray.isArraybut this doesn't exist for Errors, hence duck-typing). Relevant tests have been added and any other non-errors thrown (e.g. strings) are still treated in the same way.This change affects the formatting of
CommandExeuctionError, since the string coercion no longer happens automatically, so${err.name}has been added back in toCommandExeuctionError's message. Most tests continue to work unchanged, but several have required updating. The benefit is that this messaging is now consistent between sandboxing and unsandboxed execution and double-wrapping is avoided.