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

Exception flag or similar to prevent exposure of backtrace #485

Closed
redbullmarky opened this issue May 31, 2022 · 4 comments · Fixed by #487
Closed

Exception flag or similar to prevent exposure of backtrace #485

redbullmarky opened this issue May 31, 2022 · 4 comments · Fixed by #487

Comments

@redbullmarky
Copy link
Collaborator

When using FLAG_PROPAGATE_PHP_EXCEPTIONS (which is the only way afaik to catch PHP exceptions in the JS) and catching an error thrown by PHP in your JS, you have full access to all of PHP's exception methods allowing you to view a full trace. So if you're running a sort of sandbox environment, it's probably exposing more than you'd like. Code:

<?php
class myv8 extends V8Js
{
	public function throwException(string $message) {
		throw new Exception($message);
	}
}

$v8 = new myv8();
$v8->executeString('
	try {
		PHP.throwException("Oops");
	}
	catch (e) {
		var_dump(e.getTrace());
	}

	print("done");
', null, V8Js::FLAG_PROPAGATE_PHP_EXCEPTIONS);

Unfortunately, it looks like PHP's Exception class methods are mostly final too so cannot be overridden to prevent access. The only way we've found is to (in boilerplate JS executed before custom JS) essentially wrap all functions available on the PHP object and wrapping them, with the addition of a try/catch that rethrows the string of an exception rather than the full thing. A bit like:

// inaccessible boilerplate JS that runs prior to usercode
let originalThrowException = PHP.throwException;
PHP.throwException = () => {
	try {
		return originalThrowException.call(PHP, ...arguments);
	}
	catch (e) {
		throw e.getMessage();
	}
}

So I think we need some way of changing this behaviour. Maybe an extra FLAG that gets the text from an exception and throws that in place of the PHP exception instance.

Thoughts?

@redbullmarky redbullmarky changed the title Custom exceptions Exception flag or similar to prevent exposure of backtrace May 31, 2022
@stesie
Copy link
Member

stesie commented May 31, 2022

Hej Mark,

first of all, I don't think this qualifies as a bug. After all it works as designed :-)
Default behaviour is to not expose exceptions, just silently abort execution. Years later the opt-in behaviour of exposing exceptions as-is was introduced. But I see your need, so ...

Providing (yet) another flag that just throws the result of getMessage feels a bit too specific to me. After all there might be reasons to have the exception type/class accessible or some kind of getCode, getKey etc. Contrary simply forbidding access to getTrace and getTraceAsString might be too little, as some might feel bad about getFile or getLine as well.

Therefore having some sort of proxy object would be what I'd strive for, that's automatically created by the extension.

Something like this:

<?php
class myv8 extends V8Js
{
	public function throwException(string $message) {
		throw new Exception($message);
	}
}

class ExceptionProxy {
	private $ex;

	public function __construct(Throwable $ex) {
		$this->ex = $ex;
	}

	public function getMessage() {
		return $this->ex->getMessage();
	}
}

$v8 = new myv8();
$v8->registerExceptionProxy(ExceptionProxy::class);

$v8->executeString('
	try {
		PHP.throwException("Oops");
	}
	catch (e) {
		var_dump(e.getMessage()); // calls ExceptionProxy::getMessage
		var_dump(e.getTrace()); // fails
	}
', null, V8Js::FLAG_PROPAGATE_PHP_EXCEPTIONS);

Then it just has to be decided what should happen if __construct throws an exception. That could mean that the exception shouldn't be exposed to JS. But that feels a bit sketchy.

@stesie stesie removed the bug label May 31, 2022
@redbullmarky
Copy link
Collaborator Author

The original title of this issue was 'Custom exceptions' where I thought adding an alternative would be better, I guess I just thought that if there's code in V8Js somewhere relying on checking that an object implements a PHP base exception, it'd no longer work. So I thought perhaps a flag such as FLAG_PROPAGATE_PHP_EXCEPTION_STRING (terrible name but you get the gist) might be an easy solve.

But to be honest, I could work with either approach - the most important thing is just not exposing internals to JS-land, but still allowing the user to catch errors that may have been thrown within one of a whole suite of provided functions.

@stesie
Copy link
Member

stesie commented Jun 27, 2022

Closed via #487

@stesie stesie closed this as completed Jun 27, 2022
@redbullmarky
Copy link
Collaborator Author

@stesie this one will need to go into php8 branch too, right?

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

Successfully merging a pull request may close this issue.

2 participants