search

sachaos / todoist

Todoist CLI Client. I ❤️ Todoist and CLI.
MIT License
1.48k stars 104 forks source link

fix(main.go): resync if cached token is not equal to the configured token #229

Closed HacDan closed 1 year ago

HacDan commented 1 year ago

Updated the Before call to check if the configured token matches the cached token. If it doesn't, resync and reload the cache before performing the requested operation

kenliu commented 1 year ago

One thing I was thinking about this is that there really isn't a good reason for the token to be stored in the cache. It seems bad from a security perspective to store another copy of the token in a file where it's not really expected by the user to be. What do you think about updating this PR so that the cache instead stores a hash of the token and that hash is used to compare against the configured token?

kenliu commented 1 year ago

fixes #212

HacDan commented 1 year ago

I don't see any issue with that, but I don't see the gain from a security perspective. The file with the configured token is already stored on the system. Software to decrypt the token is provided, if not freely available.

What is the gain here outside of having the token no longer stored in two different places in the user's home folder?

kenliu commented 1 year ago

I think the benefit is following the principle of least surprise. The token is a highly sensitive piece of data -- it really shouldn't be copied anywhere without the user's knowledge. I was surprised to learn that it was stored in the cache, because I didn't really expect it to be there. While all of the user's data is already stored in the cache, the token actually is even more sensitive because it gives the bearer access to all of the user's future data as well.

It's true that the storing the token on the filesystem in the config file isn't really great either -- but the user knows that it's there and that it needs to be protected and it has to be stored somewhere.

It hard to know what could go wrong with this, but I could see someone accidentally copying the cache file somewhere (maybe as a backup) without knowing that the token is embedded in it. I figure if it's simple enough to hash the token, it's probably worth the effort to do it just to reduce the possible surprise.

HacDan commented 1 year ago

I understand the premise here. The argument could be made, however, that the would-be attacker already has access to the user's data as it's contained in the same json file.

There are plenty of other examples that users are not made aware of with software that store potentially compromising credentials on the filesystem. Cookies come to mind with their session tokens. The average user has no idea they're there.

Another great example would be the official Todoist app. While I don't have a Linux box in front of me at the moment, on my Mac the token is currently stored, plain text, in the ~/Library directory in two different files currently, but I'm guessing that's due to some old cache. I would assume under a fresh install, it's only in one file. Either way, searching my home directory for that file look less than a minute with a tool like ripgrep.

Keep in mind it isn't copied over to the ~/.cache/todoist/ directory. It's synced down from the API with the configured token and the user object.

I think if this project is going to go down this path, we should look at the bigger picture of all of the user's data that is stored on the filesystem instead of just looking at tokens. Tokens can be made invalid. User data is forever, or something cheesy like that.

kenliu commented 1 year ago

Fair enough. I don't feel that strongly about this so happy to merge the change as is -- LMK what you think.