Skip to content
This repository has been archived by the owner on Nov 7, 2022. It is now read-only.

[WIP] Implement Custom Command #79

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

Conversation

kevincox
Copy link

@kevincox kevincox commented Oct 16, 2017

This is a very basic implementation but it is actually pretty close. (It turns out calling eval on a string isn't too difficult).

What rocks:

  • Simple
  • Powerful.

What sucks:

  • Custom function key is hidden in the command name. This is because currently commands carry not config at all. It would probably be more "proper" to give each command an argument object so that they can be configured. If this is done then the id can be passed in the argument and the string split hack can be removed.
  • Command has access to a lot more then indended. Right now I want to expose "event" and the standard web globals to the code but as it stands I can't think of a way to whitelist things it has access to. I could try putting it in it's own module but IIUC the bundler basically prevents that from acutally restricting access at all. I might be able to use "let global_eval = eval; global_eval(code)" but I will have to take another look at the bundler to see what globals it leaks.
  • Content eval only. Mozilla won't let us eval stuff in the background page so we can't do stuff there. I guess we will just have to expose functionality manually overtime (which really nullifies the security benifit anyways 😢) maybe we can start making a self-hosted build that can do fun things, but that is largely independant of this feature.

What needs to be done:

  • Source the commands from the UI. Right now I have just hardcoded one command for testing. This works but the functionality is pointless unless the user can acutally configure it.
  • Allow a dynamic set of commands to be configured. Right now I have just hardcoded one into the options.
  • Improve appearance of the UI.
  • Document how custom commands work as well as the provided api.
  • Add link to documentaiton from the config UI.
  • Report errors to user nicely. (Likely catching the exception and posting it back to the privledged process so that it can display a notification/dialog letting the user know they should take a peak at the browser console.)
  • Consider cleaning up the trie structure a little now that it can't assume that objects are an internal structure.
  • Consider/add user-configurable "labels" for custom commands, allowing easier scanning.
  • Implement command-colision checking.

Fixes #45

It's mostly UI and some wiring which is left.
This prevents the user from seeing our internals, allowing us to provide
a more stable API.
@eejdoowad
Copy link
Member

eejdoowad commented Oct 22, 2017

This is definitely a great start for evaluating custom code. Sorry for the delayed response. Creating a GUI for this and deciding how commands should be stored will be challenging.

My current idea is to add a new Options Category to the Options Page labeled "Custom Commands." This section would contain a single CustomCommand widget. The CustomCommand Widget will provide the GUI that lets users add, edit, and remove custom commands.

For each command, users will have to provide:

  1. A label (using an <input>)
  2. A unique key (using an <input>)
  3. The command's Javascript (using a <texarea>)

The CustomCommand Widget will be similar to the Keybindings widget in that the user can add or delete entries by pressing a single button. Error checking will be done on the key field to ensure user-defined command keys don't conflict.

The value of the CustomCommand Widget will be stored just like any other widget type. Look at this example to see how I imagine custom commands will be stored.

When the user modifies a custom command entry, the underlying options in storage will change. This will trigger a rerender with a new set of options (e.g. new keybinding widgets representing new commands).

Currently, pages are messages their options on load as well as when the user changes an option. See https://github.com/lusakasa/saka-key/blob/master/src/client/index.js#L44, https://github.com/lusakasa/saka-key/blob/master/src/client/modes.js#L6,7 and https://github.com/lusakasa/saka-key/blob/master/src/background_page/modes.js#L87. I'll just add a new field "customCommands" that will contain the function text to the options object sent to every client.

It will be important to adjust the extension update logic so that custom commands don't break Saka Key.

@kevincox
Copy link
Author

Thanks for the pointers, they sped things up a lot. Right now I am making progress on the options but it seems to assume that the number of options are a compile-time construct. I am working around this by making the option type an array of custom commands but this feels like going against the grain. Would you recomend a different approach?

It's not wired up at all but it's a start. It is missing keybindings
which look like they will take some work.
return (
<li>
<p>This is a command</p>
<TextArea
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, Widgets are meant to be standalone containers representing a single option. Although code reuse is nice, I think it's important to be able to change the TextArea Widget independently of the CustomCommand Widget.

@eejdoowad
Copy link
Member

eejdoowad commented Oct 24, 2017

Good start. These are the exact steps I would take. Please refer to the storage example for my vision on the underlying storage.

  1. Ignore the user-defined options and code execution aspects and just try to get the widget rendering correctly and storing its value. If the user makes a change, the widget's appearance and underlying value will change, but nothing else. The value in storage should look something like the block below. You'll eventually want to add a Custom Commands category, but just to get started, you can stick the widget in General.
 [
      {
        "key": "switchToHTTPS",
        "label": "Switch To HTTPS",
        "function": "()=>{document.location=document.location.href.replace(/^http:/,'https:')}"
      },
      {
        "key": "toggleYoutubePlay",
        "label": "Pause/Play Youtube",
        "function": "()=>{document.querySelector('.ytp-play-button').click()}"
      }
]
  1. Create a Custom Commands category and stick the widget inside. You'll want to add some configuration that disables the category's profile select menu GUI. Custom Commands will have exactly one profile default.

  2. Define and implement

    1. a generic storage format for custom options of ANY type, not just keybindings (see example block below)
    2. generic APIs for adding, renaming, and deleting user-defined Options The block below is how custom options (in this case custom keybindings specifically) will be stored.
    3. A scheme for updating storage upon extension updates and importing options, which may introduce conflicting keys. You'll also have to make sure storage is correctly persisted on updates. You also have to modify all existing profiles when an option is renamed or deleted.
  "customOptions_General": ["minFontSize"],
  "customOptions_Keybindings": ["switchToHTTPS", "toggleYoutubePlay"],
  "customOptions_Hints": [],
  "customOptions_Custom Commands": [],
  "customOptions": {
    "minFontSize": {
      "type": "number",
      "label": "Minimum Font Size",
       "key": "minFontSize",
       "default": 12
    }
    "switchToHTTPS": {
      "type": "keybinding",
      "label": "Switch To HTTPS",
      "key": "switchToHTTPS",
      "default": []
    },
    "toggleYoutubePlay": {
      "type": "keybinding",
      "label": "Pause/Play Youtube",
      "key": "toggleYoutubePlay",
      "default": []
    }
  }
  1. Modify the Options Page's render, state, and update logic so that

    1. Custom Options are rendered at the end of the appropriate category
    2. Changes to the CustomCommands Widget's value trigger changes to the available options in storage. This should be implementing using the generic APIs defined in 3.2. The hard part will be keeping options in sync with values. For example, if an option's key is renamed from 'cat' to 'dog', you've got to make sure that the value for key 'cat' is simultaneously transferred over to key 'dog'. The Custom Command's transform function is my first guess at where this logic would be placed, but it may be necessary to change other sections.
  2. Send the custom command javascript to every page. After steps 1-4, this should be a piece of cake. You'll just have to modify the clientOptions() functions in the background page and client.

  3. Execute the strings with eval in each client. But only on clients that aren't privileged (e.g. not the background page or the popup). For those pages, when a custom command is triggered, render a popup that explains that they don't work for security reasons.

In summary, it's a lot of work. You can probably come up with something simpler, but it probably won't be as user-friendly or configurable. And this infrastructure is necessary for the plugin system I've been designing.

image

This is now feature complete, however there is a lot of cleanup to be
done.
@kevincox
Copy link
Author

Ok, I now have a feature-complete implementation. There is clearly more work to be done though. I'l going to update the first comment with a checklist of work to be done. However I will likely want feedback on a lot of the points.

1-4 had already been done. I have updated the trie structure to include the custom commands as well as pre-defined commands.

Execute the strings with eval in each client. But only on clients that aren't privileged

How do I tell if a client isn't privledged? I figured that all clients could use eval since they don't have a CSP applied? If not I suppose the most reliable way to do this is catch an exception from the user code, or eval call and report this nicely.

@kevincox
Copy link
Author

kevincox commented Oct 24, 2017

Also I took out the TextArea widget but put the KeyBinding one right in. The TextArea was simple enough that code sharing isn't really required but there is no way the KeyBinding code should be copied, so we need to decide upon a plan for code reuse there.

@eejdoowad
Copy link
Member

I gave your branch a shot and it looks promising. Here's some quick feedback. I'll replay again later with something more detailed.

  • Custom Command keybindings shouldn't be part of the Custom Commands category. They should be added to (and stored in) the Keybindings category. This is to ensure users can easily switch between different keybinding profiles by changing only one setting. At some point I'll add a keyboard command for switching keybinding profiles.

  • Check for the existence of APIs only available on privileged pages. const isPrivileged = chrome.tabs !== undefined. At some point in the future, it might be worth the effort to generate separate privileged and unprivileged clients at compile time.

  • I don't fully understand why the trie code needed to change, but I'll take a look later.

@kevincox
Copy link
Author

kevincox commented Nov 9, 2017

Hi, thanks for all the feedback. But it is unlikely that I will continue work on this. Anyone can feel free to take over or you can close this PR and forget about it.

@eejdoowad
Copy link
Member

Thanks for the effort @kevincox. I'll keep this open since I (or someone else) might use it as a base for an implementation in the future.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bind JS scripts/bookmarklets? (for pocket integration)
2 participants