guigrpa / docx-templates

Template-based docx report creation
MIT License
882 stars 145 forks source link

Retain original sandbox errors (from different JavaScript realms) without coercion #355

Closed davidjb closed 5 months ago

davidjb commented 6 months ago

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 because instanceof Error returns false for sandboxed errors when the prototype chain doesn't match; these errors are still Error objects, it's just that they can't be detected with instanceof.

For context, instanceof will 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 (or runJs calls out to a different environment - where I found this issue), err instanceof Error returns false, due to a different prototype chain. Likewise, when sandboxing is disabled err instanceof Error will 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 isError type guard which checks both instanceof Error and the mandatory fields of the Error type, and treats matching errors as Error thereafter. (The suggested fix for the same issue with instanceof Array is Array.isArray but 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 to CommandExeuctionError'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.

jjhbw commented 5 months ago

Awesome work and good find. PR looks great. I have one question but otherwise looks good to merge.