quisquous / cactbot

FFXIV TypeScript Raiding Overlay
Apache License 2.0
793 stars 379 forks source link

Refactor cactbot data loading so it can be merged into OverlayPlugin #1767

Open quisquous opened 4 years ago

quisquous commented 4 years ago

ngld: I've disabled the Cactbot integration for now since there were some changes to Cactbot's C# code which I have to pick up first. I also have to solve the problem around file loading (static files like timelines & triggers as well as the user folder) before I can actually release the Cactbot integration.

I had a conversation with @ngld about this a long while ago on discord, and (at the time) the conclusion was something like:

ngld: So to summarize:

  • Add a new API WSServer.RegisterStaticPath(string path, string localDirectory) that allows addons (and OverlayPlugin itself) to register directories from which the web server serves local files.
  • Add a new setting to select the public_html folder (or w/e I'm going to call it) and call WSServer.RegisterStaticPath("/static", Config.WSServerPublicHtmlDir); (or something like that)
  • Limit config and data file access through Cactbot's handler to the user dir
  • Modify user_config.js to load as much as possible through fetch
  • Add a new cmdline option to HtmlRenderer.Renderer to allow local files to access other local stuff

Well... that's a lot but it sounds like a doable long-term plan.

Other constraints:

Other thoughts:

I think my favorite conclusion for solving the raid emulator data file issue was to use webpack to bundle it together, although it does have the downsides of making life a lot harder for anybody who is modifying files locally (whyyy) and making life a tiny bit harder for developers. (Arguably maybe the c# plugin with developer options could auto-run webpack??) If we go that approach, then that solves data files, but not user files.

CC @ngld @valarnin

ngld commented 4 years ago

Sorry for the later reply (over a month later... whoops).

Bundling files (through Webpack or otherwise) would make my life much easier. Downloading a single (or a few) bundle files is much easier (and faster!) than the dozen request I need right now. I'll build a quick PoC to demonstrate how Cactbot+Webpack could look. I'm not sure if OverlayPlugin should handle Webpack but I'll think about it. IMO having a .bat or .ps1 script that installs Node, NPM and everything else into a subfolder (similar to fetch_deps.py) would be a better solution. Regarding subdirectories and the user folder in general, what do you think about adding the following handlers?

Each Cactbot overlay could call callOverlayHandler('getConfigFolderFiles', 'cactbot', (files) => ...) on load and would receive the expected config files. Since the files object would contain both files directly in the selected folder and those contained in subfolders, adding support for those subfolders shouldn't be too hard.

The button to change the user directory in the Cactbot config would call callOverlayHandler('requestConfigFolder', 'Please select a user folder for Cactbot', (success) => ...); which would then trigger the folder select dialog. This shouldn't require too many changes to Cactbot's code, solves the issue around reading local files (only files inside the user folder are accessible to overlays) and seems like a fairly simple and clean interface.

The new handlers and Webpack together would make the fetch() changes and the static web server in WSServer unnecessary since Webpack would already handle the static web server (through webpack-dev-server) and the resource requests (through bundling).

EDIT: Read through the previous comments from @valarnin again and I think adding an exclude filter would be a good idea. Not sure if the overlay page should specify it, if I should hardcode reasonable defaults (i.e. .git, *.bak, *~, etc.) or if a combination of both would be best. Also sorry for not responding earlier @valarnin, that ping got lost in the long list of unread stuff (RL kept me away for too long).

valarnin commented 4 years ago

Probably two checks:

  1. If the canonical path (via File.GetFullPath or some other mechanism) does not case-insensitive match the original path, don't read those files (prevent directory traversal and such)
  2. Exclude all dot-files (./.*)
quisquous commented 4 years ago

Re: data files. I put up a proof of concept here: https://github.com/quisquous/cactbot/commits/dataloaderv2, back before @panicstevenson changed from CJS to ESM, but I think the concept is the same. I think the followups are to turn all the triggers into modules so they can be imported directly rather than eval'd as strings, but I need to think a little more about what that means for copy+pasted user files.

I think this approach sounds great to me. I think it would be very little change on the cactbot side, as you say. I think the fact that you can only set the folder via a dialog box from requestConfigFolder means that overlay code can't change this value and request an arbitrary folder. I think from the cactbot side, I'd also want to remove the default user folder lookup (since that won't make sense in the future anyway) and only use the explicit folder.

I am not sure about allowlist vs blocklist (or both). I think that cactbot could provide a list of extensions it cared about to requestConfigFolder (right now only .js? maybe I'd support a timeline .txt file too in the future), and that it would probably also good to exclude obvious bad patterns. It would certainly be helpful if OverlayPlugin added console messages for any skipped files so that users would be less surprised if they fell into that.

If you added such a thing to OverlayPlugin, I'd be happy to release a cactbot version with it, so we can at least get that stage out of the way.

quisquous commented 4 years ago

The other question I had is: how do you feel about adding plugin code to convert the existing cactbot setting to be an OverlayPlugin setting if the OverlayPlugin setting for the cactbot directory hasn't been set? In other words, some mechanism where everybody doesn't have to re-set this if they have already set the directory.

quisquous commented 3 years ago

@ngld I asked this in discord, but maybe better to ask here in a more permanent medium, but what process are you looking for here? Are you planning to implement OverlayPlugin functions that I can then have cactbot depend on? Are you expecting that cactbot implement some version of these handlers that then OverlayPlugin can provide in the future?

quisquous commented 2 years ago

I think the remaining work here is to refactor GetUserConfigDirAndFiles to be more secure. It does only load js/css/txt files now, however it can still be any directory. There likely needs to be more restrictions here. (ngld has also seemed busy, so it's not clear to me that this merging work is really proceding?)