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

Eval with bindings ignoring allowExitFunctions(false) settings #67

Closed
everestbt opened this issue Sep 10, 2018 · 10 comments
Closed

Eval with bindings ignoring allowExitFunctions(false) settings #67

everestbt opened this issue Sep 10, 2018 · 10 comments
Labels

Comments

@everestbt
Copy link
Contributor

everestbt commented Sep 10, 2018

While trying to wire this up I found an issue with allowExitFunctions(false). When this is set and then eval(String js,Bindings bindings) is called, for example for the script "quit()" it will exit the JVM, counterposed to the intent of the setting.

This appears to be due to the bindings overwriting the original settings. I am not very familiar with Nashorn so my assumptions of the impact of Bindings and the persistence of these methods could be false. For usability I believe it would be helpful that, regardless of Bindings the settings are applied.

everestbt pushed a commit to everestbt/delight-nashorn-sandbox that referenced this issue Sep 10, 2018
…karound.

This will run the assertScriptEngine() anytime bindings are sent in.
@everestbt
Copy link
Contributor Author

Opened #68.

everestbt pushed a commit to everestbt/delight-nashorn-sandbox that referenced this issue Sep 12, 2018
@everestbt
Copy link
Contributor Author

everestbt commented Sep 12, 2018

This appears to impact all function limitations enforced by bindings, i.e. allowPrintFunctions(false) is not enforced when custom bindings are passed in.

mxro added a commit that referenced this issue Nov 3, 2018
#67:Added tests to expose bindings problem. Introduced workaround.
@mxro
Copy link
Collaborator

mxro commented Nov 4, 2018

Thanks heaps for the pull request. This is all merged now and published with version 0.1.17! Is the issue resolved now?

@mxro mxro added the solved label Nov 4, 2018
@everestbt
Copy link
Contributor Author

@mxro Thanks for merging it in. I have been working on optimising the fix a little more, I have a bit more testing to do on this but by Tuesday I should have an improved fix on a pull request.

@mxro
Copy link
Collaborator

mxro commented Nov 4, 2018

@everestbt That sounds awesome! Looking forward to the fix - will do my best to merge it in faster this time 😃

everestbt pushed a commit to everestbt/delight-nashorn-sandbox that referenced this issue Nov 5, 2018
Changed the bindings protection to cache an initial Secure set of bindings, which are then put into custom bindings, and also used to check whether the engine bindings need to be reset. This does not protect from malicious actors!
@everestbt
Copy link
Contributor Author

@mxro I have introduced a pull request #69 which I think improves things, by removing the extra evaluations by instead caching a set of secured bindings. Take a look and let me know your thoughts, happy to address any comments.

everestbt pushed a commit to everestbt/delight-nashorn-sandbox that referenced this issue Nov 6, 2018
mxro added a commit that referenced this issue Nov 15, 2018
#67-2: Remove extra evaluations for bindings by introducing a cached set of bindings
@mxro
Copy link
Collaborator

mxro commented Nov 20, 2018

Version 0.1.18 released to Maven Central including this fix. Let me know if there is anything else to be done for this issue, otherwise we can close. Thank you for fixing this 👍

@everestbt
Copy link
Contributor Author

Great, do you want me to open another issue in relation to ScriptContext having the same issue, there is an ignored test indicating this in the testExit:
@Test @Ignore("This will fail as there is no confirmation on Script Contexts") public void testQuitWithScriptContext() throws ScriptCPUAbuseException, ScriptException { final NashornSandbox sandbox = NashornSandboxes.create(); sandbox.eval("quit();", new SimpleScriptContext()); }

@mxro
Copy link
Collaborator

mxro commented Dec 4, 2018

Yes please :)

@everestbt
Copy link
Contributor Author

I have opened #70. Closing this as resolved.

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

No branches or pull requests

2 participants