search

macstories / obsidian-shortcut-launcher

Trigger shortcuts in Apple's Shortcuts app from Obsidian with custom commands.
MIT License
187 stars 10 forks source link

Review #1

Closed lishid closed 2 years ago

lishid commented 2 years ago

https://github.com/macstories/obsidian-shortcut-launcher/blob/b704ddb8321e8af6cb544ff0f2dac0a4b2a1b423/src/main.ts#L8 Doesn't seem like you need this, and it wouldn't work anyway on mobile.

https://github.com/macstories/obsidian-shortcut-launcher/blob/b704ddb8321e8af6cb544ff0f2dac0a4b2a1b423/src/main.ts#L48 Maybe use some kind of name here. When users assign hotkeys, it's identified by the ID, so if somehow the list gets shifted then assigned hotkeys will be put to the wrong command.

https://github.com/macstories/obsidian-shortcut-launcher/blob/b704ddb8321e8af6cb544ff0f2dac0a4b2a1b423/src/main.ts#L169 Any chance you can get the URL working on the desktop app as well instead of using shell commands? This doesn't seem sanitized and you have a vulnerability risk of arbitrary command execution by using child_process with unsanitized input.

https://github.com/macstories/obsidian-shortcut-launcher/blob/b704ddb8321e8af6cb544ff0f2dac0a4b2a1b423/src/main.ts#L189 You can keep an array of all the registered commands before you register them so you don't have to go and look for it in the commands list. This would save you in the future in case your command ID changes or in case we have to change the way command IDs work inside the app.

finnvoor commented 2 years ago

@lishid thanks for the review!

  1. Fixed, think it was an auto-import
  2. Fixed, switched to using launcher name as ID
  3. child_process is used instead of the URL scheme on macOS so that shortcuts can be run in the background without opening the shortcuts app for a better experience. I have made the following changes: https://github.com/macstories/obsidian-shortcut-launcher/blob/14396a82ee16d98c3dedae17bc4f09b3d219c3ac/src/main.ts#L184-L208 Now the input is saved to a file and the file is passed to the shortcuts command, preventing arbitrary code execution there. I guess it could still be possible to have a shortcut name that could result in code execution, but that seems unlikely to me. The URL scheme does work on macOS if you think this is unsafe to use.
  4. I'm not really sure what you mean by this, it seems like the only way to remove or update a command is to remove it by name (e.g. "obsidian-shortcut-launcher:launcher_name", so I don't see how I could avoid using the plugin's command ID. I guess I could avoid using this.app.commands.listCommands() by saving a list of all the commands added, but that seems unnecessary.
lishid commented 2 years ago

3) Given you're using node apis (child_process) I recommend using fs to generate a temp file directly (perhaps using os.tmpdir()). Doing so in the vault causes other obsidian components (sync, other plugins) to react to the new file.

4) Yeah that's what I recommend, because you are making an assumption of obsidian internal implementation that the command IDs are encoded as pluginid + something. I don't think we will change this but in the off change it does get changed, keeping a reference of your commands and calling removeCommand(command.id) is less likely to be broken by internal API changes.

finnvoor commented 2 years ago
  1. Great idea, changed
  2. Ah I see, for some reason I was thinking I would have to serialize the references across launches but obviously I'm adding the commands on each launch. Changed to keep references. (side note, it would be nice to have an official API for removing or modifying existing commands 🙂)