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

Adding FCLuaScriptItem and FCLuaScriptItems to RGP Lua and JetStream Controller #287

Open
rpatters1 opened this issue Jul 30, 2022 · 23 comments

Comments

@rpatters1
Copy link
Collaborator

I've been working on an idea that @Nick-Mazuk requested to be able to execute scripts from a different U.I. designed in Lua itself. To that we added the capability to add scripts on the fly. Today I suddenly realized this is going to address a JetStream Controller requirement as well.

Right now the program pops up an annoying dialog and enters a number which then executes a script. Well, with this new feature (coming in 0.64), your script will be able to fish its own item out of the list, modify the prefix on the fly, then re-execute itself with a different prefix. The details are a little more complicated than that, but I think it will work.

Sound interesting? If so, I'd be interested to know a little more detail on the design of the JetStream Controller now. The sequence is

  • KM or Applescript executes the dialog box and fills in the code specifying which item to run
  • Is that then a require statement within the script? (I suppose I can look at the source code now. Lol.)

@CJGarciaMusic @jwink75

@Nick-Mazuk
Copy link
Member

Nick-Mazuk commented Jul 30, 2022

This issue will probably answer your question of how the JetStream currently works in case you don't want to dig through all the source code: CJGarciaMusic/PowerUserWorkflow#11

In short, there isn't any require statements at the core of the JetStream. Just a big switch statement (built with if-thens) to execute the right function.

@rpatters1
Copy link
Collaborator Author

I have uploaded tentative documentation to the master branch of the PDK Framework Docs Repo. To review it:

  • Clone the repo to your local drive by downloading the zip file (easiest).
  • Open the first html file with your browser. (I believe it is called annotated.html.)
  • Navigate to FCLuaScriptItem.

Feedback is much appreciated before I get too much further into it.

@rpatters1
Copy link
Collaborator Author

On further reflection, I realize this does not address the fundamental challenge for JetStream Controller: that of passing a trigger value from the controller to the Lua script. But maybe this will be useful in a future refactoring.

@Nick-Mazuk
Copy link
Member

Ok, just to make sure I understand the docs, if I were to want to run a script I could do something like this?

local script_to_execute = "My script's name"
local script_items = finenv.CreateLuaScriptItems()
for item in each(script_items) do
    if item.GetMenuItemText() == script_to_execute then
        finenv.ExecuteLuaScriptItem(item.GetExecutableIndex())
        break
    end
end

This seems like a good API to me. Couple questions/thoughts, though:

  1. It looks like you can add a script using finenv.AddLuaScriptItem? I assume this would not add scripts to the menu bar and any added scripts would not persist between Finale restarts, but just want to double-check.
  2. Likewise, I assume methods like SetMenuItemText would not update the menu bar and the changes would not persist to the next session. I also assume it wouldn't update the source file.
  3. GetPrefix: what does this do? I can't think of what a script prefix is.
  4. AddLuaScriptItem seems like a security issue. Usually a user needs to download the script, and then they've implicitly consented to running that script. However, I could see someone creating a malicious script that adds a new script then immediately executes it. This other Lua script doesn't even necessarily need to be a script the user downloads. Now, would I think this ever would happen? No. Would it be easier to just write the malicious Lua code directly in your script? Yes. Would it be even easier to just execute an arbitrary terminal command (which is already possible)? Yep. But still, I can't think of a reason why someone would want to add a new script that's compelling enough to add another attack vector. With that said, I'm not against this feature since in reality it doesn't let a malicious user do anything new, but I think it's something to think about.
  5. SetFilePath this seems like a security issue. Let's say we have three scripts, A: "a.lua", B: "b.lua", and C: "c.lua". "c.lua" as some malicious code and may not even be a Finale Lua script. A uses SetFilePath to change the file of B to "c.lua". User tries to run "b.lua" by clicking on "B" and accidentally runs "c.lua". Whoops! They've now run malicious code. There are the same caveats as above, though.

@rpatters1
Copy link
Collaborator Author

  1. You can't change the menu bar, so no.
  2. See 1. SetMenuItemText doesn't have much use to Lua scripts.
  3. GetPrefix allows you to supply a Lua snippet that runs before the script does.
  4. AddLuaScriptItem is how you are going to execute a script on the fly. I don't see this as fundamentally more dangerous than a script that (as you say) does the malicious code without it.
  5. SetFilePath cannot change already-configured script items. Its primary use is for creating a new item to execute on the fly. CreateLuaScriptItems makes a copy of the already-existing items and it does not allow you to copy them back. Only add new ones.

@Nick-Mazuk
Copy link
Member

I see. I think that eases my concerns. LGTM.

@jwink75
Copy link
Collaborator

jwink75 commented Jul 30, 2022 via email

@rpatters1
Copy link
Collaborator Author

Actually, based on your feedback (@Nick-Mazuk ), I realized the Add and Delete APIs are probably unnecessary. All we really need is Create (collection) and Execute. The Execute method can execute one you create from scratch, which allows for on-the-fly additions.

FWIW: I can't see any increased security risk from Execute over what is already there with require. The Execute API just allows for the script to get its own Lua state.

@rpatters1
Copy link
Collaborator Author

With excellent feedback from this thread, I think I have zeroed in on a workable set of APIs that preserves a modicum of security. There will basically be two flavors of FCLuaScriptItem depending on how you construct them. (You will have to construct them with one of the two finenv functions. I've removed the Lua-accessible constructors.)

  • Items from Finale's plugin menu.
  • Adhoc items.

Items from Finale's plugin menu will be launch-and-forget and behave exactly as if you launched them from the menu. As a security precaution, your launcher script cannot see the file paths for them. (It occurred to me that a malicious script could rewrite third-party scripts if it knew where they were. This allows you to launch them without knowing anything about them other than their menu and other descriptive text.) Furthermore, the only thing that matters for these scripts is the (read-only) "execution index". No changes you make to the script item have any effect on the launching of these scripts. (Specifically, changing the prefix has no effect.)

Adhoc items will allow you to launch any file of your choosing. If it isn't proper Lua, of course, you'll just get an error. The big difference with these is your script has to stay alive as long as the launched script stays alive. For scripts with modeless dialogs and retained lua states, this is longer than just a single script invocation. Also, the properties on FCLuaScriptMenu do matter for these. Especially, the prefix, undo text, and booleans.

I've updated the docs at the doc repo.

If you would like to try out a prerelease version of it, you can download it here. No guarantees on how long the link stays up. But a few days at least. The Apple version is not notarized, so the usual rigamarole will be required to bless it.

@rpatters1
Copy link
Collaborator Author

P.S. I've also hooked up FCCtrlComboBox in the same version in the link above.

@rpatters1
Copy link
Collaborator Author

P.P.S. I removed the constructor on FCLuaScriptItems as well, but I now realize that was a mistake. It'll be restored in the next version.

@Nick-Mazuk
Copy link
Member

Awesome, thank you Robert!

@rpatters1
Copy link
Collaborator Author

rpatters1 commented Aug 4, 2022

I've been thinking that a script-launching script might like to have access to some of the other finaleplugin fields. The tradeoff is between security (not revealing too much about a script to 3rd parties) and usability of the launcher. The ones that occur to me are

Notes
Categories
ScriptGroupName
ScriptGroupDescription
required min max versions

Any others?

This was referenced Aug 6, 2022
@Nick-Mazuk
Copy link
Member

It would also be nice to also get the script's text contents, or at least the hash (SHA256?) of the contents. And the filename and path of the script (which I know I already argued against earlier).

I've been trying to figure out a good way of making it easy for users to update their scripts. After thinking about it, I put together a proposal in #322, but it would need these things. I'm open to another way of creating an updater. I just haven't figured out another way, yet.

@rpatters1
Copy link
Collaborator Author

I am fairly concerned about revealing a file path on the user's computer to a third party. A malicious third party could replace its contents, and if it were done carefully deflect the blame back to the script it modified.

Is there some way we could do this online? One of the finaleplugin attributes that Jari planned to enforce was a unique Id, unique to his repository. (Search for finaleplugin.Idon this page.) If we implemented that for every script, then you could find it at our site that way without knowing anything about the user's local disc. FCLuaScriptItem could provide a hash of the contents for comparison.

@rpatters1
Copy link
Collaborator Author

rpatters1 commented Aug 8, 2022

I've done some preliminary investigation into adding hashing to RGP Lua, and it is not completely straightforward or necessarily lightweight. Here is my proposal:

  1. Add an Id requirement for insertion in this repo. A human readable Id with a reverse url is fine with me. Alternatively we could use the filename itself, but an Id seems like the more reliable method. (And it is already a documented property for JW Lua scripts.)
  2. When the deployment program creates the dist version of the script, it inserts a hash into the plugindef() function. I would propose finaleplugin.Hash = "<string-value>".
  3. RGP Lua can report both the Id and this hash value back in the FCLuaScriptItem class.

The advantages of this approach are:

  • Minimal technical information supplied to 3rd party scripts.
  • The hash is only computed once.
  • The hash computation is guaranteed to be the accurate, because it only happens once.

Disadvantages are:

  • A version mismatch will not be noticed if the user edits the downloaded script.
  • (Others I haven't thought of, without doubt.)

@Nick-Mazuk
Copy link
Member

I like the idea of inserting the hash into the plugindef at bundle time. Definitely less error prone.

I am fairly concerned about revealing a file path on the user's computer to a third party. A malicious third party could replace its contents, and if it were done carefully deflect the blame back to the script it modified.

+1 to this.

My follow-up question would be then how to actually update the script. My proposal in #322 would require the script to get the location of the outdated script in order to automatically update it.

A stop-gap would be to just direct users to the website to download it themselves. While this would work, in my opinion updating scripts should be as painless and simple as possible for end users. I want to try and keep as many people as possible on the latest versions of everything.

An alternative is we could add a new method to RGP Lua that allows other scripts to update a script file. Script A will call this API to update script B and will tell RGP Lua B's new contents. RGP Lua will then ask the user whether script A should be allowed to update script B. If the user accepts, RGP Lua will overwrite script B's code with the new contents and return true to script A. If the user declines, RGP Lua will not update script B and will instead return false to script A.

Lastly, since a user will likely run the updater many times, and each time many scripts will be updated, it would be nice for allow the user to "trust" the updater script. And in the future, RGP Lua will not prompt for the user's consent in the future.

All this would have to be done inside the RGP Lua plugin. Thoughts?

@Nick-Mazuk
Copy link
Member

On another note, @ThistleSifter suggested in #145 of letting a script report a success/error case. I think that's a decent proposal. Perhaps the finenv.ExecuteLuaScriptItem function can return whatever the script itself returns?

Also clarification, does finenv.CreateLuaScriptItems() return individual scripts or menu items? I think the latter is preferred (and it shouldn't impact the updater at all).

@rpatters1
Copy link
Collaborator Author

rpatters1 commented Aug 10, 2022

Answers in no particular order:

  • finenv.CreateLuaScriptItems returns a collection of menu items, and the latest version of finenv.CreateLuaScriptItemsFromFilePath returns a collection of all the menu items in the script. (The version of RGP Lua I uploaded does not do this yet.)
  • Regarding the return value from ExecuteScriptItem: returning multiple values from C++ is challenging. The easiest approach would be to return a string if there is an error and null if it executed successfully. Right now, RGP Lua is displaying that string the same as if the script had been executed from the menu. Should it continue to so do? Should I try to figure out a way to return the Lua status as well? (Understanding that sometimes there is no Lua status, such as when you try to execute a script that has version restrictions.)
  • One of the challenges that both RGP Lua and a script will have in updating scripts is that they may not be in a directory that a program can write to without admin privilege. I'm thinking the update should be external to both. The most reliable solution would be an external installer that downloads and installs the update. There is no automatic way for RGP Lua to run under admin privilege, because it is running as part of Finale.
  • Adding some kind of privileged status to scripts to allow them to take actions that otherwise would cause user notifications is easy enough. But it is not very secure. A malicious script-author could figure out how to defeat it with a little digging. So I'd prefer not to rely on that.
  • Jan Angermüller has done considerable work with trying to download files from URLs in Lua, and he says it is fraught with difficulty due to OS restrictions around shelling out to do it. I'm probably going to add a function to get a string from a URL directly. I already added this to Patterson Plugins, and it's not much code. You get back whatever the http response is, so if the file doesn't exist you'll likely get the 404 message. I would be interested in brainstorming any security considerations around this.

@Nick-Mazuk
Copy link
Member

  • finenv.CreateLuaScriptItems returns a collection of menu items, and the latest version of finenv.CreateLuaScriptItemsFromFilePath returns a collection of all the menu items in the script. (The version of RGP Lua I uploaded does not do this yet.)

I see, thanks for the clarification!

  • Regarding the return value from ExecuteScriptItem: returning multiple values from C++ is challenging. The easiest approach would be to return a string if there is an error and null if it executed successfully. Right now, RGP Lua is displaying that string the same as if the script had been executed from the menu. Should it continue to so do? Should I try to figure out a way to return the Lua status as well? (Understanding that sometimes there is no Lua status, such as when you try to execute a script that has version restrictions.)

I think simply returning nil on success and a string on error is sufficient. Though I'm curious on @ThistleSifter's thoughts.

However, the next question is what that string should be. For instance, if a script puts return "hello" at the end of the file unconditionally, would that always be counted as an error?

One alternative is status codes and a message. One popular implementation of this pattern is absl::Status (https://abseil.io/docs/cpp/guides/status). So potentially the return value can always be a string that represents the status code. Even if the script doesn't run because of version restrictions, it can still return the "Unavailable" status code.

Of course, we may still wish to have a custom message, so perhaps a tuple of strings?

-- structure
return "status code", "message"

-- default if the return statement isn't included
return "ok", ""

Then from anywhere in the script you can return "ok" or an error.

We can easily abstract the details of this into a library function to ensure consistency across this repo.

-- status creation
status.ok() -- "ok", ""
status.ok("my message") -- "ok", "my message"

-- status parsing option 1
local status_code, message = invoke_script()
local is_ok = status.is_ok(status_code)

-- status parsing option 2 (which may be more forward compatible should we choose to change the format in the future)
local result = invoke_script()
local is_ok = status.is_ok(result)
local message = status.get_message(result)
  • One of the challenges that both RGP Lua and a script will have in updating scripts is that they may not be in a directory that a program can write to without admin privilege. I'm thinking the update should be external to both. The most reliable solution would be an external installer that downloads and installs the update. There is no automatic way for RGP Lua to run under admin privilege, because it is running as part of Finale.

Hmm… isn't this already an issue with the configuration library? How does the configuration library currently work if the script is in a protected directory?

While I'm not against for an external installer, I'd prefer keeping it in Lua if possible. That would just be much simpler to maintain since we already have a pipeline for that. A fallback is that if RGP Lua or the script cannot write the file, we can just prompt the user to download the script from the website manually.

  • Adding some kind of privileged status to scripts to allow them to take actions that otherwise would cause user notifications is easy enough. But it is not very secure. A malicious script-author could figure out how to defeat it with a little digging. So I'd prefer not to rely on that.

Ah true. Since the best place to store this is likely in the XML file, which is in a consistent place for every RGP Lua user.

  • Jan Angermüller has done considerable work with trying to download files from URLs in Lua, and he says it is fraught with difficulty due to OS restrictions around shelling out to do it. I'm probably going to add a function to get a string from a URL directly. I already added this to Patterson Plugins, and it's not much code. You get back whatever the http response is, so if the file doesn't exist you'll likely get the 404 message. I would be interested in brainstorming any security considerations around this.

That would be a useful function.

The main security issue I can think of is that now the host of the external URL is an attack vector. If we point to URLs we control (for instance, the website), it's likely not a huge deal. However, if someone were to ever take control of the external URL, they can potentially get arbitrary code to run on a user's computer. The two main ways I can think of are the following:

However, I'm not particularly worried about these.

loadstring itself is already a fairly large security risk (because if you own the string, why not just write the original Lua?). We can easily mitigate that by not using the loadstring function in our Lua scripts. And should anyone else use that function, I don't see an increased security risk since they are likely using that function with a string they don't control already.

For saving the contents of the string to the Lua script, that's the entire point of the operator. And the user needs to consent for this to happen. If we're worried about our website becoming compromised, the source code is open-source. So should anyone add malicious code, it'll be easy for us to replace it (or to move the site to a new domain). Also, all my accounts are secured with 2FA and the DNS has DNSSEC turned on, so it should be fairly difficult for an attacker to spoof the site. If someone does do something malicious with the site, I think that means we likely have larger problems since right now, there's practically no incentive to.

If the script points to a third-party site, I think one could make the case that the user assumed that risk at the time of downloading the script.

I think the largest consideration is that any HTTP call should be secured with HTTPS (this also goes for the images in #327). HTTP is very, very insecure.

If we were really concerned about the URL host being compromised, we could also have this be another popup generated by RGP Lua, saying "the script wants to access this URL", and potentially letting the script give some sort of reasoning like ("so we can check to see which scripts are out of date"). However, one could make the case that we'd need to do this for images as well, which could get very unwieldy very fast.

@rpatters1
Copy link
Collaborator Author

I don't see any way in the APIs to restrict downloads to HTTPS. I could force all URL strings to start with https however.

Jan Angermüller updates his code with an external script. This strikes me as a much more reliable update process than building it into RGP Lua. Writing the configs is not a permission problem because they are specifically being written to a location designated for writing 3rd party configuration parameters. But a lot of people are putting their Lua scripts in the Finale plugin directory, and this is definitely a protected location on both Mac and Win.

I'm thinking Execute will return the status code, and I'll think of a way to return the message as well, if any. I'm more interested in whether RGP Lua should display the message as normal.

@rpatters1
Copy link
Collaborator Author

Just following up, I figured out how to get Windows to refuse http links, and macOS was already doing it. (It may be possible to enable them somewhere in user settings, but disabled is the default.)

I have also figured out how to return multiple values to Lua from C++, so ExecuteLuaScriptItem will return a status and the message, if there is one.

Because the read-from-url function is a synchronous call, it is not going to be appropriate to download more than a few 10s of kilobytes with it.

@rpatters1
Copy link
Collaborator Author

I'm beginning work on finishing this up. The finenv.ExecuteLuaScriptItem will return a boolean success as its first return value and a string or nil as the second. One question remaining is, should ExecuteLuaScriptItem display the error string as RGP Lua does when executing directly from the menu? Right now it does, but it could be suppressed. Then it would incumbent on the calling script to display the message to the user.

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