search

estroBiologist / pluralchum

PluralKit integration for BetterDiscord
MIT License
41 stars 11 forks source link

Reworking Servernames #29

Closed ariagivens closed 10 months ago

ariagivens commented 11 months ago

Motivation

The servername has a ton of issues that keep popping up. (See: #4, #8, #15, #25) Unicode is really complicated and trying to munge the author name to separate the server name and system tag is kind of nightmarish. Really it would be much easier and better to query the PK API for the servername. However, for safety reasons you need the user's token in order to query that endpoint. So, we need to store the user's tokens.

The Security Issues

PK Tokens are Inherently Insecure

There's many potential issues with storing the user's PK token. Unfortunately tokens are an all or nothing proposition: either you get full access or no access. This makes it impossible at the moment to implement the Principle of Least Privilege. Compromised tokens cannot be revoked per application, the whole token must be regenerated. Together, this means we are taking on a big responsibility by storing tokens.

Encryption

We should at least try to encrypt the tokens while they are on disk so it can't just be read by anyone with read access to the pluralchum config file. The question is: where is the encryption key stored? It has to be accessible at runtime! The best solution as far as I can tell is to use Electron's safeStorage module. It uses the users "secret key" to encrypt/decrypt data. On Mac and Linux this the keychain/keyring. On Windows this is DPAPI.

However, these services are not always available. For instance if the user dismisses the keychain prompt. (Personally, I could not for the life of me get electron to recognize my gnome-keyring installation on arch).

Additionally, if we have access to decryption anybody else locally can probably decrypt too. So it doesn't eliminate risk entirely.

It's important to note too that a BetterDiscord plugin can in principle run arbitrary PK commands. So a lot of the risks kinda exist anyways.

Proposal

I propose that gate the servername feature behind two conditions:

  1. safeStorage encryption is available.
  2. A valid PK token is entered/stored Then we can use the PK API to retrieve the servernames, tags, etc. without issue.

    Implementation

    I've started work on implementing this feature, but wanted to get feedback early.

    Alternatives

    Do Nothing

    We could decide that leaving servernames in the current experimental string munging state is better than having to deal with storing tokens.

    Get rid of tag coloring for servernames

    This would probably fix ?most? of the servername issues but also kinda defeats the point of the plugin and makes things weirdly inconsistent.

estroBiologist commented 11 months ago

Yeah, this pretty much matches my thoughts on the matter.

Another reason I held off on this for so long is that I talked to the PK devs about this back in the day, and I recall them mentioning plans to implement a proper tiered access solution - though I'm not holding my breath at this point.

Assuming we can indeed get an acceptable encryption solution set up, I'm on board.

ariagivens commented 11 months ago

I have a proof of concept working for encryption. https://github.com/ariagivens/pluralchum/commit/bd0fea3ec93916f97f8262ce02c794d8f15912ec image

ariagivens commented 11 months ago

I've got the bulk of the servername via API feature done. The UX isn't quite there yet but other than that it seems to be working! https://github.com/ariagivens/pluralchum/tree/7991c8d33fbc8b4e99bc25429091ace617033cf0

ariagivens commented 11 months ago

I've hit a bit of a roadblock. The API-based system works great... as long as you are talking to yourself 🤦‍♀️. Having the token only allows you to see your guild settings. Along the way I've made some nice refactors and improved the general responsiveness. (e.g. fixing #6 by using React hooks). I'm considering ripping out the servername and token parts and submitting what I have as a PR.