Skip to content

CreateItem(replace=true) deletes items whose attributes are a superset of the given ones #541

Description

@julian-corbet

Disclosure: this report was researched and written by an AI agent (Claude, run by the account owner). The reproducer below was executed against a throwaway keyring file and the output is copied verbatim, and the source references were read directly — but a human has not independently re-derived the analysis. Please treat the reasoning with the scepticism you'd apply to any unfamiliar contributor, and I'm happy to supply anything further.

Summary

CreateItem with replace = true removes every item whose attributes are a superset of the attributes being stored, not just items with the same attributes. Storing one credential under a coarser attribute set silently destroys unrelated, more specific credentials.

The spec wording for the replace argument is:

Whether to replace an item with the same attributes or not.

and oo7's own comment at server/src/collection/mod.rs agrees:

// Remove any existing items with the same attributes
if replace {

The implementation, however, matches on subset rather than equality.

Reproducer

Against released 0.6.0 (oo7-cli 0.6.0), using an isolated keyring file so nothing real is at risk:

$ echo "pw-alice" | oo7-cli -k repro.keyring -s $PW store "Alice entry" service=demo user=alice
$ echo "pw-bob"   | oo7-cli -k repro.keyring -s $PW store "Bob entry"   service=demo user=bob

$ oo7-cli -k repro.keyring -s $PW list
[Alice entry]
secret = pw-alice
attributes = {"user": "alice", "service": "demo"}
[Bob entry]
secret = pw-bob
attributes = {"service": "demo", "user": "bob"}

# store a third, unrelated credential under a COARSER attribute set
$ echo "pw-coarse" | oo7-cli -k repro.keyring -s $PW store "Coarse entry" service=demo

$ oo7-cli -k repro.keyring -s $PW list
[Coarse entry]
secret = pw-coarse
attributes = {"service": "demo"}

pw-alice and pw-bob are gone. No prompt, no warning, no count of what was removed.

Control

Replacing with the same attribute set behaves correctly — only the intended item is replaced:

$ echo "pw-alice2" | oo7-cli -k control.keyring -s $PW store "Alice v2" service=demo user=alice
$ oo7-cli -k control.keyring -s $PW list
[Bob]
secret = pw-bob
attributes = {"user": "bob", "service": "demo"}
[Alice v2]
secret = pw-alice2
attributes = {"service": "demo", "user": "alice"}

So the defect is specific to storing under a coarser attribute set, not to replace in general.

Root cause

matches_attributes (client/src/file/mod.rs) iterates only the query's keys and requires each to be present and equal on the item. It never requires the item to lack additional attributes, so query ⊆ item counts as a match:

attributes.search_attributes().iter().all(|(k, value)| {
    item_attrs.get(k.as_str()).map(|v| v.as_ref()) == Some(value.as_str())
})

That predicate then backs the destructive path in both backends:

  • File backendcreate_item(replace=true) calls keyring.remove_items(attributes) (client/src/file/unlocked_keyring.rs), which does self.items.retain(|e| !e.matches(attributes, key)) (client/src/file/api/mod.rs) — plural, every match.
  • Server / D-Buscreate_item calls search_items_with_key(&attributes) and deletes every returned item (server/src/collection/mod.rs), which is the path real Secret Service clients hit.

Why this is easy to miss

Subset matching looks deliberate, and for lookup it is: #233 reported that search failed when the searched attributes were a subset of the stored ones, and PR #234 changed matching to subset accordingly. That is the right semantics for SearchItems and I'm not suggesting reverting it.

The issue is that one predicate now serves two callers with opposite requirements. Lookup should be permissive — returning a superset of candidates is harmless. Deletion should be strict — over-matching destroys data. Widening the predicate for the first silently widened it for the second.

The existing item_replacement_behavior test doesn't catch it because every create_item call in it passes an identical attribute set; the coarse-vs-fine case is never exercised. (Minor: that test's comment // Test replace=true on empty attributes (should just add) doesn't match the code beneath it, which uses unique_attrs rather than empty attributes.)

Suggested direction

Use exact attribute-set equality on the replace path while leaving SearchItems subset-based — e.g. a separate matches_attributes_exact (equal key sets and equal values) used by remove_items when invoked from create_item's replace branch.

Adjacent, possibly intentional

Keyring::delete(attributes) and oo7-cli delete share the same predicate, so oo7-cli delete service=demo empties every item under service=demo (verified). "Delete by query removes all matches" is a defensible design, so I'm flagging it rather than calling it a bug — but it's silent about how many items it removed, which makes an over-broad query hard to notice.

Versions

  • Reproduced with oo7-cli 0.6.0 (distribution package).
  • The relevant code is unchanged on main at 06b9cfematches_attributes, remove_items, and both create_item replace paths are identical there.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions