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

Wonderful library! #31

Open
vanesca88 opened this issue Jan 27, 2023 · 9 comments
Open

Wonderful library! #31

vanesca88 opened this issue Jan 27, 2023 · 9 comments

Comments

@vanesca88
Copy link

vanesca88 commented Jan 27, 2023

Not really an issue, just a positive comment :)
A great pleasure to use + love how secure it is with the C++ execution.
Decided to give some back.

Your examples on the readme are pretty good, but it kinda lost me at the 'Host functions' part, because it's not fully explained.
So I did lots of testing myself. Please see the attached .zip file.

Perhaps you can replace/append the examples on the readme file with these simple tests?
Allows people to get up and running more quickly.
My test show how to use your excellent library + how to execute arbitrary code + that it retains its variables + how to call external functions/imported functions.
Better example.zip

I want your library to be successful, that's why I'm giving back :D
Have a wonderful day & thanks for sharing friend. ❤️

@Philipp122
Copy link

I do think a lot of that confusion comes from there not being any Architecture explanation and some problems with Terminology. For example it took me about 30 Minutes to understand that the v8 Sandbox is not run from the Main Process.

Maybe there should be some explanation that there is: Main Process > Child Process > v8 Sandbox.

Also it would be an option to change some parameter names in the Method and Constructors. Something like apiScript and initScript for require and template. Basically names to make it more clear what they are and from which process they are used in what way. I also noticed that there is no Documentation for SandboxCluster.

Just some Ideas on how to make it better. Otherwise this is an awesome Project, and I would love to see a Community grow around it! Maybe even a Contribution Guide.

Regards,
Philipp

@vanesca88
Copy link
Author

Yes, I completely agree. ✨
I had the luck of experience with other libraries in the past. So I did understand a lot, but still not all of it. The terminology alone is very confusing for beginners. Just reading the words main-process, host-functions and sandbox in close succession is enough to confuse anyone to understand the big picture.
I think a simple image illustration it would already be a big help.

Another thing that would be of great help is a use-case scenario!
Give people an example of what it can be used for.
Like: "If you have multiple projects/games, you can run them all on a single server.
Without any of them interrupting the other!"
That's our use-case at least. And only scale up when you need to, keeping costs down.

Or:
"As a teacher/admin, you can run multiple website/portals for your students/users. Keeping everyone safe from prying eyes, as well as accidental destruction or deletion of code."

SandboxCluster still has no documentation yes. I did check through the source randomly, and apparently it seems its a shortcut to enable this library in a cluster environment (just like the name suggest ofc), but I still have no idea how to use it.
For me it's not a problem though, as I'm used to create my own clusters anyhow.
I even got this library running in a cluster as we speak.

I also noticed a few minor bugs that I fixed for myself. Going to create a fork of this amazing library and send a pull request back to the owner, so it can get merged. I'm not great with Git in general yet (still new to it), but I'll do my best to contribute back :D

@vanesca88
Copy link
Author

Created a fork here, where I'll be posting any bug fixes I find. (Sent back as as pull request)
https://github.com/vanesca88/v8-sandbox-tweaks

@Philipp122
Copy link

For one Use-Case: My Company is using this Library to allow a REST API User to send JavaScript as a Form of Query to the Server, which gets executed in the Sandbox with a specified Context. The Sandbox only has access to some Database Methods, like Creating, Updating and Deleting Entries in the Users Collection as well as fetch for getting Data from the Web.

We have now decided to rewrite most of the Typescript Part of this Library, as the API is not verbose enough for us. One thing we are adding is the option to add a before and after function to every execute call, basically allowing for construction and/or deconstruction of environments within the Sandbox.

@vanesca88
Copy link
Author

Excellent use-case as well.
Hope the owner puts all of them into the readme file. Gives people a way better insight, in terms what it can do.
It does seem like this project is 97-ish% complete for production. Which is totally oke. Just missing a few things.
Luckily the tweaking is do-able. Be sure to check out my pull request as well.
Fixed a few things. I'll be posting more soon.

@zhm
Copy link
Member

zhm commented Feb 8, 2023

Thanks for the feedback! I hope to publish an architecture diagram some point soon to help.

@vanesca88 thanks for your PR. The behavior of the sandbox state not persisting across executions is intentionally not allowed. Each call to execute() guarantees a clean state within the sandbox. Perhaps that could be reworked or relaxed, but that's the intent.

As for SandboxCluster, it should be a drop-in replacement, so it's not documented (yet!) because the API is the same.

@zhm
Copy link
Member

zhm commented Feb 8, 2023

I added a basic diagram in the README

https://github.com/fulcrumapp/v8-sandbox#architecture

@Philipp122 what APIs are you wanting to be added, a before/after inside the sandbox'd env?

@vanesca88
Copy link
Author

@zhm Welcome!
Ah I see. From my experience with other sandbox libraries, I think all of them offer a way to continue re-executing.
It's their default behavior. Perhaps it could be optional when creating a new sandbox?
I'm using it for that reason as well. Of course you decide haha.
Just trying to say that it's a functionality people already expect. That's all.
The diagram already helps.

@zhm
Copy link
Member

zhm commented Feb 10, 2023

@vanesca88 👍 I'm inclined to agree with you that it should allow it, just have to check some places this library is being used to make sure it's properly cleaning things up and what the impact of the change would be. The reason the library intentionally doesn't allow it is so that one sandbox instance can be safely reused in a server environment to process multiple requests for different users. If the sandbox allows state to stick around it would require careful cleanup or making a new sandbox each time. I'll take a look!

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

No branches or pull requests

3 participants