negative icon caching #161

New Issue

OVERLORD · 2026-02-04T18:06:47+03:00

OVERLORD commented

2026-02-04 18:06:47 +03:00

Originally created by @tycho on GitHub (Dec 17, 2018).

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.

Originally created by @tycho on GitHub (Dec 17, 2018). 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.

OVERLORD closed this issue

2026-02-04 18:06:52 +03:00

OVERLORD commented

2026-02-04 18:07:03 +03:00

@tycho commented on GitHub (Dec 17, 2018):

How does something like this look?

1b1f8b4003 - add ICON_CACHE_TTL and ICON_CACHE_NEGTTL config variables
603dce2172 - 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.

@tycho commented on GitHub (Dec 17, 2018): 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.

OVERLORD commented

2026-02-04 18:07:10 +03:00

@mprasil commented on GitHub (Dec 17, 2018):

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.

@mprasil commented on GitHub (Dec 17, 2018): 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.

OVERLORD commented

2026-02-04 18:07:19 +03:00

@dani-garcia commented on GitHub (Dec 17, 2018):

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.

@dani-garcia commented on GitHub (Dec 17, 2018): 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.

OVERLORD commented

2026-02-04 18:07:31 +03:00

@mprasil commented on GitHub (Dec 17, 2018):

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)

@mprasil commented on GitHub (Dec 17, 2018): 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)

OVERLORD commented

2026-02-04 18:07:46 +03:00

@tycho commented on GitHub (Dec 17, 2018):

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

@tycho commented on GitHub (Dec 17, 2018): Well, rust *does* have a symlink API for Windows, it just uses a different name entirely: `std::os::windows::fs::symlink_file`

OVERLORD commented

2026-02-04 18:07:54 +03:00

@dani-garcia commented on GitHub (Dec 17, 2018):

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.

@dani-garcia commented on GitHub (Dec 17, 2018): 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.

OVERLORD commented

2026-02-04 18:08:00 +03:00

@tycho commented on GitHub (Dec 17, 2018):

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 commented on GitHub (Dec 17, 2018): 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.

OVERLORD commented

2026-02-04 18:08:03 +03:00

@tycho commented on GitHub (Dec 17, 2018):

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

@tycho commented on GitHub (Dec 17, 2018): Okay, I'm going to move this to a pull request for better review.

OVERLORD commented

2026-02-04 18:08:08 +03:00

@dani-garcia commented on GitHub (Dec 17, 2018):

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.

@dani-garcia commented on GitHub (Dec 17, 2018): 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.

OVERLORD commented

2026-02-04 18:08:12 +03:00

@tycho commented on GitHub (Dec 17, 2018):

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!

@tycho commented on GitHub (Dec 17, 2018): 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!

OVERLORD referenced this issue

2026-02-05 04:50:14 +03:00

[PR #161] [MERGED] `Attachment::save()` returns Result instead of bool #2616

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: starred/vaultwarden#161