BTCTaras/kristscape

SCRIPT tags and sandboxing discussion

dmarcuse opened this issue · 29 comments

To prevent cluttering #16. @Luca0208

Here we are

You are added to my repo, I already added the basic sandbox + execution code, really basic tho. @apemanzilla

Alright, so: We should use a whitelist for previously stated reasons (to prevent problems with future CC versions)

On top of that, we should make sure that all tables are hard copied, or use protected metatables like so:

Bad

local env = {}
env.colors = colors

Good

local env = {}
env.colors = setmetatable({}, {__index = colors, __metatable = "protected"})

This is to prevent scripts from modifying functions that may later be called by kristscape or CraftOS or other programs.

This is to prevent scripts from modifying functions that may later be called by kristscape or CraftOS or other programs.

Clever, didn't thought of that, but yeah that's what we should do

Also, to prevent breaking out of the sandbox, functions like setfenv, and more dangerously, getfenv should not be whitelisted.

I knew of that. I wouldn't put anything where theres no reason of adding it, like direct print()
An api like JS document would be something good to interact with the content

print itself should be alright as long as the scripts are redirected to prevent modifying the top level windows.

Functions like term.native should also be either overwritten with a 'safe' version or left out entirely for this reason.

print was just an example, but it isn't needed, so leaving it out might be senseful so that you can't annoy the user by printing in e.g. the address bar

EDIT: Maybe replacing term with window.create(windowSpecsOfContentInKristscape) would be a good idea

print obeys terminal redirection, so if the terminal is redirected properly, the only place print could write to would be the viewpane.

then we could add print without making any website being able to fake something(e.g. by just printing in the address bar)

Also, maybe having a kristscape.request function to allow scripts to request access to non-whitelisted APIs would be useful - requiring the user to allow or deny the request:

local fs = kristscape.request "fs"
if fs then
  -- user allowed request
else
  -- user denied request
end

I like how firewolf handles fs, it uses a download function. But yes a request function would be nice too.

I'm going for a more Chrome-style approach. Scripts are started with minimal permissions, requiring them to request access to the lower level stuff.

that is probably the best solution

I tried to add the mt thing, I'm not 100% sure how it works, so it would be nice if you could check https://github.com/Luca0208/kristscape/commit/b39d0deae0132ac4ab9c1f9a92ec049663624885

That should be OK. The __metatable field is just there to prevent people getting access to the other metamethods.

Should the sandboxing stuff be moved to its own file?

Should the sandboxing stuff be moved to its own file?

Maybe, if he's online @BTCTaras might wanna give his opinion here

Ok, Taras is not online or can't respond right now, but the whitelist could go relatively large, so an external file would prevent that the ksml.lua file get's too big

Hold on, I'll get him online.

Nice, thanks!

I'm going to sleep now, 23:47 here, do you know what time zone Taras lives in? I am in CEST.

I had intended for the argument in the script tag to specify which interpreter to use (i.e. sandboxed lua would be [SCRIPT:LUA]) and yes I do think it should be in a separate file

I put it in another file, I can add you to the Repo @BTCTaras if you want

There's a problem when enabling term api for use and making a window. As soon as we create a window it will override everything already on the term

Try window.create(term.current(), x, y, w, h, false)

I actually tried it now, but the whole screen turns black

So we now have scripts 😄
what about global functions tho?

If we do 2 languages they should be similar so I reverted some of the progress to use systems like Inquire, but it doesn't work if someone could take a look at 766efd7

Now that this is further ahead than expected, we can go ahead and slate it for 1.0