err on invalid feature flag

[stefan0xC]

As mentioned in the PR #4168 (comment) we have to err! to disallow unknown flags otherwise all flags are allowed.

I've just tested it and the warning is not even displayed. Might be the same reason as

vaultwarden/src/config.rs

Line 128 in 890e668

// We can't use warn! here because logging isn't setup yet.

[BlackDex]

The problem with that is, if we remove a flag, we will break those setups.

[stefan0xC]

Let me quote myself:

I think it's better to return an error so that users can't enable unsupported flags (or at least immediately notice that they have misspelled a feature flag when changing the flag via the admin panel - which I'm not sure would happen if this were a mere warning) and if we remove a feature flag from KNOWN_FLAGS (because it's not supported anymore or was added prematurely) I'd probably want this validation to fail. Furthermore I don't think that it's an issue enabling feature flags that are not in use anymore (like display-kdf-iteration-warning), so we won't have to remove them as soon as possible. (Users might also want to clean up removed feature flags that they have enabled to reduce the size of the returned /api/config - without such a drastic feedback I don't think this will happen.)

And also

In order not to break too many installations we probably should not remove features from KNOWN_FLAGS that we have enabled by default (like fido2-vault-credentials) or only if there's an important reason to do so or we really want to remove it. (I think the other flags, which we don't enable by default, would be fair game as users would be responsible if they change the defaults).

We could also inform our users in the release notes about removed flags if we do remove them.

[stefan0xC]

Let me put it in other words: Without this change, it makes no difference if we remove flags from KNOWN_FLAGS and we could also get rid of the validation. (IMHO it would still be better to validate the spelling to get immediate feedback and have a growing list of known flags, if we never want to break someones configuration instead of accepting every string.)

[BlackDex]

I still do not like using err! here. My main point is.
If a user auto updates Vaultwarden, and a flag has been removed from the known flags list, it will break that setup and the instance is not reachable anymore. So, for me, that is a no-go.

What is happening currently is also not ok i think.
It should filter out the unknown flags and return only a list of valid items and warn/println about the unknown ones.

Currently the latest Self-Hosted is returning:

"trusted-device-encryption": true,
"fido2-vault-credentials": true

So the actions i think we need to take to fix this are:

1. Still warn/println about unknown flags
2. Filter those unknown flags, and only return valid flags
3. Maybe have a default list like fido2-vault-credentials, and always add that, unless it is excluded with ^fido2-vault-credentials for example.
   That way we are still able to add new flags by default, and allow users to override them if they do not want it.

[stefan0xC]

Why do we have to remove something from the KNOWN_FLAGS?

[BlackDex]

Because, if those flags are not needed anymore, we probably also do not want them to be added.
It might even cause troubles in the future or something if they are still in there.
What if they re-use a flag for some reason. They know when they removed a flag and were not using it for a while, and can just re-use a previous flag for any other reason.

[stefan0xC]

I'd say in those cases it would be fine to break a user's invalid configuration, is it not? 🤨

I don't see the point in making sure it doesn't break (by filtering out unknown flags) and somehow still warn a user (which doesn't currently work). I mean, why would anyone fix their invalid config, if it just keeps working?

edit: I mean, I see the point. I just think that it's better to force users to keep their config up-to-date than to risk unwanted side effects because they did not notice a spelling error or because they have kept a reused feature flag enabled.

[BlackDex]

The only way i would see err! work is if we do not remove known features, and when we do, we need to filter those out, and have at least those a warn, and no err.

Also, best to also parse all flags, and create an err with all invalid flags instead of break on the first invalid one.

[stefan0xC]

Okay, I've rewritten the check so it prints all invalid flags and also tells users what flags are supported:

Error loading config:
  Unrecognized experimental client feature flags: ["passwordless-login", "bla"].

Please ensure all feature flags are spelled correctly and that they are supported in this version.
Supported flags: ["autofill-overlay", "autofill-v2", "browser-fileless-import", "fido2-vault-credentials"]

Regarding the other change, you've suggested: I've added a comment so that we don't forget about dealing with removed flags once we do decide to remove a flag.