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

Websocket backend #40

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Websocket backend #40

wants to merge 20 commits into from

Conversation

qti3e
Copy link
Collaborator

@qti3e qti3e commented Jun 3, 2018

A simple WebSocket server to move toward supporting Node.js backend...
depends on #21
(this patch starts from 223532d)
to test:

./tools/build.js
cd bin
ts-node bin.ts

@qti3e qti3e requested review from ry and piscisaureus June 3, 2018 08:58
@piscisaureus
Copy link
Member

piscisaureus commented Jun 5, 2018

Very nice! There are a few things:

  • https://propelml.org/notebook?nbId=tWdWpdiM1o3uixg4sQRb -> when I apply this PR, this notebook throws an error in the console after 2 seconds.
  • The initial page uses the node-based sandbox, however when I refresh the page it uses the iframe sandbox again.
  • The node worker process isn't actually sandboxed. Would it be possible to use the vm module to actually make it secure. I do agree with creating a separate process for every sandbox, please keep that.
  • Is it possible to not use the same server for the sandbox and the web server. E.g. the website is served from localhost:8080, and the sandbox is on localhost:8081.
  • Use the iframe sandbox by default, and add a button "connect to local sandbox" if there is a server running there. (this may be out of scope for this PR, we could do it later. In that case it would be nice to pass the address of the local sandbox as part of the url, e.g. http://localhost/#notebook/anonymous?vm=8081.

it's still not safe at this point, **process** should be removed from the sandbox
@qti3e
Copy link
Collaborator Author

qti3e commented Jun 5, 2018

@piscisaureus Thanks for the review, I've fixed the errors (the reason was that TextDecoder is not present in Node.js, and it is used inside fetchString() in sandbox.ts), and I'm using VM module now.
I prefer to make this patch small and I won't include any UI related stuff right now and will do that in a follow-up pr : )

@piscisaureus
Copy link
Member

I prefer to make this patch small and I won't include any UI related stuff right now and will do that in a follow-up pr : )
That's ok - but then can you make it so that I can connect to the local sandbox by URL and stay on that URL (so it doesn't use the web sandbox after a page reload)?

@qti3e
Copy link
Collaborator Author

qti3e commented Jun 6, 2018

@piscisaureus Now you can use node-based sandbox by passing server address in your url, like:
http://localhost:8081/#/notebook/anonymous?ws=ws://localhost:8081

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

Successfully merging this pull request may close these issues.

2 participants