Bitwarden_rs: negative icon caching

[tycho]

I notice that bitwarden_rs doesn't remember failing to download icons which don't exist. Every time I refresh my vault in a web browser, I notice it logs 404 errors on the server-side, trying to fetch icons via https://icons.bitwarden.com. I would expect these requests to fail until the domains in question add icons. It seems bad to retry downloading these every time a user refreshes their vault page, and we could reasonably do negative caching for a long time. I'd probably cache the negative responses for 3 days or longer.

Also it's possible for icons to change on the upstream pages, so it'd probably be nice to have a time-to-live on the cached icons in ICON_CACHE_FOLDER.

To avoid DoS-ing icons.bitwarden,com, the TTL should probably have a bit of variance to it as well.

[tycho]

How does something like this look?

https://github.com/tycho/bitwarden_rs/commit/1b1f8b400398c26f3078a5a6bd439e7716316bb7 - add ICON_CACHE_TTL and ICON_CACHE_NEGTTL config variables
https://github.com/tycho/bitwarden_rs/commit/603dce21723130af96507de7c88cf328ce241d40 - implement icon cache TTLs

Be forewarned, this is my first time touching Rust code since before the first version of Rust was released and I'm a little bit concerned with whether my error handling patterns and coding style are sane or not.

I'm also not sure if people want to run bitwarden_rs on non-Unixy systems where std::os::unix::fs::symlink is unavailable, or std::fs::Metadata::modified could fail. If we do want to support such systems, the icon cache expiration timestamps should probably be stored in a database table instead of based on symlinks and filesystem timestamps like I use in these patches.

[mprasil]

Yeah something like that could work. I kinda like the symlink approach, it leaves no temporary data in the DB. There should be some way to turn off negative caching completely and then we can provide that as fallback on systems that might have issues with symlinks? Do a PR so we can reason about it a bit better.

[dani-garcia]

I'd prefer to keep compatibility with as many systems as possible unless completely necessary. While I like the idea behind the symlinks, and storing that data in the database is a bit more clunky, I don't think the benefits are worth dropping support for non-unix.

[mprasil]

I wonder if this is actually an issue on non-unix systems. From what I remember, windows can handle symlinks too. On the other hand, it might be file system specific issue on Linux systems even, so yeah maybe the DB would be better solution overall. (and as a bonus, it's definitely going to be faster)

[tycho]

Well, rust does have a symlink API for Windows, it just uses a different name entirely: std::os::windows::fs::symlink_file

[dani-garcia]

I remember hearing that windows symlinks were somewhat buggy and they needed admin privileges, but this was around Windows 7, so maybe those issues were fixed in Windows 10?

If they do work, I guess we could do with the symlinks for now, and we can talk more about an alternative if any problems occur.

[tycho]

It looks like there's also a fairly simple crate for handling cross-platform symlinking:

https://gitlab.com/chris-morgan/symlink/blob/master/README.md

I'm going to try using that instead of the unix-specific symlink API. I'll have to also figure out how to get a Windows rust dev environment up and running so I can see how well this stuff works.

[tycho]

Okay, I'm going to move this to a pull request for better review.

[dani-garcia]

If it's just those two functions, unless the crate is doing something special you can probably just create two functions:

#[cfg(target_os = windows))]
fn symlink(...) {}

#[cfg(target_os = unix))]
fn symlink(...) {}

I have an old windows 10 laptop laying around that I could test it on, if you want.

[tycho]

Yeah, your approach would probably work. I used the crate for the moment but I can switch over to using the #[cfg... stuff if we think that's better!