Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SES.confine() throws away stack traces and line numbers #172

Closed
dckc opened this issue Jan 29, 2019 · 6 comments
Closed

SES.confine() throws away stack traces and line numbers #172

dckc opened this issue Jan 29, 2019 · 6 comments

Comments

@dckc
Copy link
Contributor

dckc commented Jan 29, 2019

In Agoric/SES#9, @warner writes Aug 9:

Another thing to keep in mind is debuggability and sourcemaps. At present, each SES.confine() is a barrier beyond which stack traces and line numbers get thrown away. It might be interesting to ...

he goes on to discuss solutions, but meanwhile, I think it's worth promoting the problem to its own issue.

@erights
Copy link
Contributor

erights commented Feb 6, 2019

Added the 1.0-blocker label. But not that we do not need a fully function debugging experience to unblock 1.0. Progress could unblock even if this bug remains open for remaining problems.

@warner
Copy link
Contributor

warner commented Feb 7, 2019

There are two things which might help:

  • we just landed an option named errorStackMode: "allow" (or rather we fixed it to stop breaking exceptions quite so badly). When you enable this, Error objects ought to retain their name and stack data. But it also might open up a confinement leak, as it leaves some non-standard properties on the Error prototype (which V8 needs to produce stack traces).
  • we're looking at a patch to the Realms shim that would attach the source of each evaluated string as a "source map". This may or may not help give more information to a debugger.. we haven't been able to test it properly yet.

The patch would look like this:

diff --git a/shim/src/evaluators.js b/shim/src/evaluators.js
index 8b4fe9a..0e0fc32 100644
--- a/shim/src/evaluators.js
+++ b/shim/src/evaluators.js
@@ -94,6 +94,8 @@ export function createSafeEvaluatorFactory(unsafeRec, safeGlobal) {
     const safeEval = {
       eval(src) {
         src = `${src}`;
+        const sourceURL = `data:,${encodeURIComponent(src)}`;
+        src += `\n//# sourceURL='${sourceURL}'\n`;
         rejectImportExpressions(src);
         scopeHandler.allowUnsafeEvaluatorOnce();
         let err;

It wants to live in Realms, but we might also be able to add it to SES instead. Also it kinda wants to be provided as a third argument to the r.evaluate() or SES.confine() call (the first is the source code, the second is the endowments, and the third would be an options bundle, of which sourceURL: or source: could include this stuff).

What we learned about errorStackMode is that our default (which deletes the non-standard properties from Error) breaks exception rendering pretty badly: Node.js at least doesn't show anything, not even the name and message of the exception. Your process just terminates abruptly with a line that says undefined. So my current advice is that if your process is exiting this way, change your code to use SES.makeSESRootRealm({errorStackMode: 'allow'}) until you fix the problem, then turn that back off again before you let it run untrusted code.

The other thing we learned is that there are a lot of eval() going on, so the stack traces usually point at line numbers inside anonymous strings. If you happen to stash a copy of the string you pass into r.evaluate(), then the line number ought to match up with that, but in a lot of cases that string is thrown away. This will hopefully get better when we implement more of a module loader, so you can pass a module or a filename into evaluate() or confine(), in which case the eval'ed source code can point to a file, from which line numbers can come. The tricky bit is that we must make sure this doesn't lead to a confinement leak: a process should not be able to learn the contents of an external file by throwing an exception, and an exception on one side of a trust boundary should not reveal excess information to a caller on the other side.

And debugging, in general, is a special power, which should come from the top-most "primal" realm, and get virtualized as you make new realms inside that. We have some old E work on this to draw from, but it'll be a while before we get this figured out in the JS context.

@dckc
Copy link
Contributor Author

dckc commented Feb 7, 2019

Monte draws from that E work: exceptions are sealed and a special debugging power, unsealException, is passed to the top-level main entry point.

Monte's trace facility also has the power to unseal exceptions. (that's perhaps more relevant to #148 on console.log ...)

@dckc
Copy link
Contributor Author

dckc commented Sep 28, 2019

@michaelfig does the technique in Agoric/SwingSet#158 apply here, by chance?

@michaelfig
Copy link
Member

It does, insofar as the filename is attached via a sourceURL to the string you're evaluating. This is the "module loader provides URLs" solution.

@jfparadis jfparadis transferred this issue from Agoric/SES Feb 21, 2020
@kriskowal
Copy link
Member

SES now carries sourceURL and has taming options for errors. Please reopen if this issue is still relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants