m1k1o / neko

A self hosted virtual browser that runs in docker and uses WebRTC.
https://neko.m1k1o.net/
Apache License 2.0
5.96k stars 449 forks source link

In app file transfer #221

Closed prophetofxenu closed 1 year ago

prophetofxenu commented 1 year ago

This adds uploads to and downloads from a specified directory when enabled by the server owner when launching the app or an admin in the app's settings.

Demo

Issues

I've tested this with large files locally (see demo) and smaller files across the Internet.

m1k1o commented 1 year ago

Thanks! I'll take a look at this over the weekend.

m1k1o commented 1 year ago

I am writing my comments here, if you would like to apply them, feel free. Alternatively I'll apply them over the weekend.

An admin can toggle admin only file transfer in the app's settings panel.

I would not allow admin to toggle filetransfer off or on. If I decide, I don't want to use filetransfer at all (because of security reason) in env, it should not be visible in neko at all. Admin could enable/disable it for users. For this I would reuse locks that we already have (for locking control/room). They would just lock file transfer.

Since there will be multiple features implemented in the future, i am thinking about adding project-wide some soft of feature gate, that can include/exclude part of code as needed.

 When downloading a file, a check is done to see if the path contains ".." to prevent a client from escaping the directory.

For this, I would suggest using filepath.Clean and similarly how container paths are implemented in neko-rooms.

prophetofxenu commented 1 year ago

I agree with your thoughts. I can work on the necessary changes starting tomorrow. I'll be busy Sunday but if I don't finish tomorrow, I'll push what I have and either pick it up later or, if you'd like to work on it, I'll leave it up to you.

m1k1o commented 1 year ago

I added error handling to transfers, made some lint fixes, moved unpriv_file_transfer to locks and file_transfer_enabled turn on/off whole feature. So once disabled in ENV it is not available (not even HTTP endpoints are registered, no file notify).

fsnotify is used to watch the directory and trigger updates. This leads to an update being sent for every write on a particular file. Sometimes, tens or hundreds of writes can happen for a single file, leading to tens or hundreds of events. It's annoying, but I haven't encountered any issues with this besides lag sometimes occurring in the browser console.

I listen only to create, delete and rename events. This means we get fewer events but when the file is being written, we do not get updates about increasing file size. We need to malually refresh that. In the future we could throttle those events to 1/sec and have reliable data.

Due to the possibility that the files panel may not be available due to an admin disabling it, if a user is on the file panel and refreshes the page, they will instead see the chat after the refresh. I've tried working around this but my solution does not work. I think this is due to a race condition between the setting of the panel and the receiving of the websocket message signaling the file transfer panel should be shown.

I added Watch() functions for fileTransferEnabled and tab it means, it hides/shows panel when needed.

prophetofxenu commented 1 year ago

Looks like you beat me to it :)

I like your changes. For the throttling of fsnotify write events, I can work on this some over the coming week or weekend. For now though, is that everything or are there other issues that needs resolving?

m1k1o commented 1 year ago

No additional issues, this can be merged. Maybe there will be some smaller enhancements over time, but first version is good to go. Thanks for your contribution!

alanmilinovic commented 1 year ago

Can this be used with google chrome docker image as well? Not sure if it will work, probably some policies would need to be changed in policy file.

m1k1o commented 1 year ago

It is available for all neko images. But yes, to be able to read local files from google chrome or download them, policies needs to be setup. Maybe you want to AllowFileSelectionDialogs or set up DownloadRestrictions and you might want to have file:// accessbile (you can just add file:///home/neko/Downloads). Or remove --disable-file-system.

alanmilinovic commented 1 year ago

It is available for all neko images. But yes, to be able to read local files from google chrome or download them, policies needs to be setup. Maybe you want to AllowFileSelectionDialogs or set up DownloadRestrictions and you might want to have file:// accessbile (you can just add file:///home/neko/Downloads). Or remove --disable-file-system.

Do you mean by overwriting supervisord.conf for the --disable-file-system (same like with policies,json)?

alanmilinovic commented 1 year ago

Ok so I did this in policy file:

  "DownloadRestrictions": 0,
  "AllowFileSelectionDialogs": true,
  "URLAllowlist": [
      "file:///home/neko/Downloads"
  ],

It looks good, but the window where I need to browse and select files that I want to upload is behind Chrome window. So I need to double click on chrome to resize the window. It looks like Chrome window is set to be on top.

I tested it on wetransfer.com site.

image

m1k1o commented 1 year ago

In openbox.xml we need to target only main browser window, not popups.

alanmilinovic commented 1 year ago

In openbox.xml we need to target only main browser window, not popups.

At the moment you cannot bring popup window to be on top at all, so you need to move main windows around to be able to select files for upload.

In case of downloading, it is working fine. Very strange.

m1k1o commented 1 year ago

Running obxprop | grep "^_OB_APP" inside docker exec -it neko bash leaves you with cursor, and you click on a window that you want to identify:

Clicking on popup:

# obxprop | grep "^_OB_APP"
_OB_APP_TYPE(UTF8_STRING) = "dialog"
_OB_APP_TITLE(UTF8_STRING) = "Open File"
_OB_APP_GROUP_CLASS(UTF8_STRING) = "Google-chrome"
_OB_APP_GROUP_NAME(UTF8_STRING) = "google-chrome"
_OB_APP_CLASS(UTF8_STRING) = "Google-chrome"
_OB_APP_NAME(UTF8_STRING) = "google-chrome"
_OB_APP_ROLE(UTF8_STRING) = "GtkFileChooserDialog"

Clicking on browser:

# obxprop | grep "^_OB_APP"
_OB_APP_TYPE(UTF8_STRING) = "normal"
_OB_APP_TITLE(UTF8_STRING) = "New Tab - Google Chrome"
_OB_APP_GROUP_CLASS(UTF8_STRING) = 
_OB_APP_GROUP_NAME(UTF8_STRING) = 
_OB_APP_CLASS(UTF8_STRING) = "Google-chrome"
_OB_APP_NAME(UTF8_STRING) = "google-chrome (/home/neko/.config/google-chrome)"
_OB_APP_ROLE(UTF8_STRING) = "browser"

Current match in openbox is <application class="Google-chrome*" name="google-chrome*">, what matches both of them and puts them to fullscreen <maximized>true</maximized>. We need to only match brwoser window, so when added role="browser" it makes it no more fullscreen.

But it still is paced under the brwoser window and you cannot bring it to foreground, even when removing whole openbox configuration.

m1k1o commented 1 year ago

@alanmilinovic fixed in latest version. Tested on all browsers, only on opera it does not seem to work. They have maybe own file dialogs that are not compatible.

alanmilinovic commented 1 year ago

Cool, tnx man.