Disallow plugins from changing the teamID #381
Conversation
There was a problem hiding this comment.
Super awesome the docs you put out, absolutely great!
RE the actual task, LGTM. I'd generally avoid having a try catch for a user-defined throw, but I think it makes sense in this case.
I'm happy to merge as is. Do wonder though about an approach where we don't even expose team ID to plugins, therefore preventing plugin devs from shooting themselves in the foot by changing a prop that was indeed exposed to them.
Nevertheless, great stuff!
src/worker/plugins/run.ts
Outdated
| @@ -44,7 +44,8 @@ export async function runOnSnapshot(server: PluginsServer, event: PluginEvent): | |||
| } | |||
|
|
|||
| export async function runProcessEvent(server: PluginsServer, event: PluginEvent): Promise<PluginEvent | null> { | |||
| const pluginsToRun = getPluginsForTeam(server, event.team_id) | |||
| const team_id = event.team_id | |||
There was a problem hiding this comment.
Nit: I'd follow JS camelCase convention even if the prop uses snake_case
There was a problem hiding this comment.
Argh, how did I not notice everything else using JS camelCase lol. A python habit xD - is there a specific style guide we're following?
src/worker/plugins/run.ts
Outdated
| @@ -57,6 +58,10 @@ export async function runProcessEvent(server: PluginsServer, event: PluginEvent) | |||
|
|
|||
| try { | |||
| returnedEvent = (await processEvent(returnedEvent)) || null | |||
| if (returnedEvent.team_id != team_id) { | |||
| returnedEvent = null // don't try to ingest events with modified teamIDs | |||
| throw new Error('Changing team IDs is an illegal operation') | |||
There was a problem hiding this comment.
Nit: Can we devise an error message that will be more informative to a user of the plugin? Since plugin logs errors will now pop up on the UI so it'd be nice to do something like "Illegal operation: plugin tried to change team ID"
I was thinking about this too, and it felt very messy? Since plugins are inserted into the middle of the data flow, any data you remove needs to be managed outside the plugins: say, something like, the vm removes some event properties we don't want the users to see, or better yet, we remove it ourselves inside This was also a bit annoying, since some plugins might actually want to read these properties, for whatever reason, and then it becomes a question of exposing a readonly interface. At that point, I figured it was just easier to validate if no illegal operations occurred. Definitely worth revisiting though, if we have a lot of fields like these. |
…ugin-server#381) * add code walkthrough and prevent changing team IDs in plugins
Closes #363
Added a WIP walthrough section, describing how everything works together in plugin-server.
I'll fix the "stream of consciousness" sections in the README before I merge. Corrections welcome.
Checklist