kee-org / KeeFox

Legacy browser and XUL application integration with KeePass Password Safe. See https://github.com/kee-org/browser-addon for the new version for Firefox 57+
https://forum.kee.pm
418 stars 48 forks source link

Don't automatically edit and save database upon first opening #513

Closed elieux closed 4 years ago

elieux commented 9 years ago

I recently started using a shared read-only database and every time I open it, KeePass:

  1. complains it can't save it
  2. after acknowledging the error, the database is marked as changed (with an asterisk in the title and all)

Comparing a XML dump of the database from my KeePass with a dump from an installation that doesn't do this, it seems the difference is in KeePassRPC.KeeFox.configVersion being added.

I'm not entirely sure why exactly this happens (I suspect a migration takes place unless the right configVersion is found), but could KeePassRPC be changed so that the value isn't saved unless actually necessary? AFAIK, there's no KPRPC data in the database.

KeePassRPC v1.5.3.0 (2015-07-08T11:15:57Z), KeePass v2.29

luckyrat commented 9 years ago

KeeFox only works with databases that have the latest config version (your suspicion is correct) so various config updates occur when migrating from 1.4.x to 1.5.x including the configVersion being updated to version 2. That's the same process as with a previous migration (from essentially v0 to v1) so if the configVersion string was never in the read-only database, this shouldn't be an issue new to the v1.5 beta.

There might be problems if different people have different KeeFox versions for the same shared database but otherwise the solution would be to get the shared database updated with the latest KPRPC config version and make it read-only after that.

I don't think there's any way we can avoid the config update and save procedure because it is not related to the presence (or not) of any KPRPC specific data. The only idea I've got now is if we can somehow mark specific databases as being unavailable to KeeFox but I'm not keen to have that sort of power to break KeeFox available only in the database config file so we'd also need a GUI to allow for selection of the databases that should be blacklisted (I think database URI is the only option here despite its fragility) and/or maybe some way to highlight blacklisted databases in the KeePass UI.

I don't think I'll have time to do any work on this myself (at least for a year or so) but if it sounds like this approach would be worthwhile for your situation, feel free to create an issue and/or pull request about it.

elieux commented 9 years ago

this shouldn't be an issue new to the v1.5 beta

That's probably the case. The shared database is a new thing.

idea I've got now is if we can somehow mark specific databases as being unavailable to KeeFox

I'm not sure this is worth the effort. I think it's more logical to have the database itself know if it's allowed to be used with KeePassRPC, but I haven't thought about the proposal much yet.

Anyway, I have a counter-proposal (in the spirit of my original request). The plugin would have an internal in-memory storage with configVersion stored for each database. When a database is loaded, the configVersion from the database file is taken as the initial value for the in-memory configVersion. The plugins then optionally runs the migration by first updating the configVersion value in memory and then upgrading all the data that needs upgrading. If and when some data is changed in the database (during migration or otherwise), the configVersion is then also stored in there, otherwise it stays in the memory.

If you've already considered and rejected this idea, could you describe your reasoning? If it's okay, I could try to implement this.

luckyrat commented 9 years ago

That approach could cause the database to be displayed in a permanently "save needed" state (even if the initial KeeFox implementation does not, other plugins or even built-in KeePass operations might cause the database state to be re-evaluated and therefore displayed as modified to the user). That would lead to an equally annoying situation where you would be unable to exit / lock KeePass without confirming that you want to discard changes every time. In that situation I think we've just made it worse for some people and not really made huge headway on solving the original problem.

However, if we can reliably detect whether a database is read-only we could skip the upgrade process in that situation. I suspect that would be possible but it might be difficult to do it at the right time (e.g. if KeePass still has the file open on disk while our plugin tries to examine its writeable state).

With a bit of luck the read-only state will be already available to us following the initial load from KeePass but I've not got time to investigate at the moment.

If that works then all is good for any shared database that does not need to be used with KeeFox but for situations where the user needs access to the read-only database in the latest version of KeeFox, we've still got a problem. I still think we'd have to develop an unobtrusive way of telling the user that there is a reason (and corresponding solution) for why they can't see their database in KeeFox and that might be challenging without the ability to record any state specific to that database.

github-actions[bot] commented 4 years ago

Following the recent announcement of the end of critical security patch support for this old software - https://forum.kee.pm/t/keefox-critical-security-support-ends-30th-september-2020-kee-is-unaffected/3219 - this issue has been automatically marked as stale. We will soon close this issue and then archive this repository in early October 2020.

If you think that the issue contents may still be relevant to the actively maintained Kee project, the successor of KeeFox, please search the community forum for help and post a new topic if appropriate: https://forum.kee.pm

Please do not reply to this comment / notification - it won't be seen.