feat: protect manually-edited Bitwarden items on re-run (#30)#31
Merged
Conversation
kp2bw is KeePass-authoritative, so a re-run overwrote any difference on an existing item -- silently reverting edits made in Bitwarden (a fixed title, added note, corrected URI). Now every written item carries a KP2BW_SYNC plain-text content signature; a re-run that finds an item's managed content no longer matching the stamp knows a user edited it (kp2bw's own writes restamp, so they never self-trip) and preserves the edit, reporting it as 'protected' instead of clobbering. --force-update (KP2BW_FORCE_UPDATE) makes KeePass win regardless. Signature mechanism, not a revisionDate/timestamp compare: the server bumps revisionDate after kp2bw generates a stamp, so a timestamp compare would false-trip every kp2bw-written item. _content_differs and the stamp share _content_signature, so protection can't drift from the diff definition. - bw_serve: KP2BW_SYNC_FIELD_NAME + item_kp2bw_sync; strip_field_from_items takes *field_names so --strip-ids drops KP2BW_SYNC alongside KP2BW_ID. - convert: _content_signature/_login_signature/_is_user_modified; stamp on write; force_update flag; 'protected' outcome + summary counter. - snapshot normalizer drops KP2BW_SYNC too (goldens stay deterministic). - tests/protect_edits_test.py covers the full decision matrix.
📦 Test this PR (archived)
This PR has been merged. You can still test the final state: uvx -p 3.14 --from 'git+https://github.com/kjanat/kp2bw@aa80c2cb3441a306321d170e0dd3a176d84b11ef' kp2bw --help📋 PR Details
Merge commit: aa80c2c 📋 Example usage# Show version
uvx -p 3.14 --from 'git+https://github.com/kjanat/kp2bw@aa80c2cb3441a306321d170e0dd3a176d84b11ef' kp2bw --version
# Migrate KeePass DB (interactive password prompts)
uvx -p 3.14 --from 'git+https://github.com/kjanat/kp2bw@aa80c2cb3441a306321d170e0dd3a176d84b11ef' kp2bw vault.kdbx
# Non-interactive with env vars
KP2BW_KEEPASS_PASSWORD=kppass KP2BW_BITWARDEN_PASSWORD=bwpass uvx -p 3.14 --from 'git+https://github.com/kjanat/kp2bw@aa80c2cb3441a306321d170e0dd3a176d84b11ef' kp2bw vault.kdbx -y🤖 Archived on merge |
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
🚫 Excluded labels (none allowed) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL py/clear-text-logging-sensitive-data flagged 10 sites (convert + bw_serve) after the protection stamp landed. Root cause: _content_signature did repr((... _login_signature(login) ...)), materialising the cleartext password into a string; CodeQL then classed the whole item dict as clear-text-sensitive and flagged every read of it (name, item_id, attachment_id, even bw_serve request internals) -- none of which actually log the password. Fix at the source: _login_signature now incorporates the password as its own SHA-256 digest, never in clear text. A password edited in Bitwarden still flips the digest (detection unchanged, deterministic), but no cleartext credential enters the signature string or the stored stamp -- the stamp is derived from a password digest. Defense-in-depth bonus: the KP2BW_SYNC value no longer fingerprints the raw password.
The digest attempt did not clear the clear-text-logging alerts (CodeQL taints through the hash -- it is the password-derived KP2BW_SYNC stamp *stored on the item*, not its cleartext-ness, that marks the dict sensitive) and it added a new py/weak-sensitive-data-hashing alert (sha256 flagged as weak password hashing). Back to comparing the raw password value, which is only used for the content fingerprint and equality checks, never logged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #30.
Problem
kp2bw is KeePass-authoritative:
_reconcile_existing_itemoverwrote any difference on an existing item with the KeePass value, silently reverting manual edits made in Bitwarden (a fixed title, an added note, a corrected URI). The only escape was--no-update, which freezes every item.Approach — content signature, not a timestamp
Every item kp2bw writes carries a
KP2BW_SYNCplain-text field holding_content_signature(item)— a sha256 over exactly the surface_content_differscompares (name, notes, custom fields with managed stamps excluded, login creds/totp/uris). On re-run, when content differs,_is_user_modified(existing)checksstamp != _content_signature(existing):protectedin the summary.--force-update(envKP2BW_FORCE_UPDATE) makes KeePass win regardless.Why not
revisionDate?The issue's "catch" warns kp2bw's own writes bump
revisionDate. A client-stamped timestamp compared against the server'srevisionDatewould false-trip every kp2bw-written item, because the server setsrevisionDateafter kp2bw generates the stamp. The content signature sidesteps all clock/timezone/race issues, and_content_differs+ the stamp share_content_signatureso protection can never drift from the diff definition.Idempotency / safety
KP2BW_SYNCis in_MANAGED_FIELD_NAMES, excluded from the content signature → unchanged re-runs stayskipped, neverprotected.type=0(not "hidden") — likeKP2BW_IDit's metadata, not a secret, and a hidden field is only UI-masked. The digest leaks nothing not already in cleartext on the same item.--strip-idsnow removesKP2BW_SYNCalongsideKP2BW_ID(strip_field_from_items(*field_names)).KP2BW_SYNCtoo, so e2e goldens stay deterministic.Tests
tests/protect_edits_test.pycovers the full matrix: signature excludes the stamp; legacy/own-write not modified; user edit detected; protected vs--force-update; KeePass-only change updates; matching content skips.Gates: ruff ✓, ty ✓, basedpyright 0/0/0, 18 passed/4 skipped, dprint ✓.