Skip to content

Per-user isolation untested for play history and API key operations #451

Description

@forkwright

Finding

The play-history and API-key repository tests never create two distinct users, so cross-user isolation is unverified at the DB layer. No test asserts that user A cannot read user B's play history, that list_api_keys_for_user returns only the calling user's keys, or that revoke_api_key rejects a key owned by another user. Compounding this, revoke_api_key has no user_id predicate in its WHERE clause, so the query layer does not enforce ownership even in principle.

Evidence

crates/apotheke/src/repo/play_history/tests.rs:10

fn make_user_id() -> UserId {
    UserId::new()

Every test in this file constructs exactly one user via make_user_id. No test seeds a second user's data and asserts that recent_sessions, top_items, get_active_sessions, or get_pending_scrobbles filter by user_id. The user.rs repository has no test covering list_api_keys_for_user scoping, and revoke_api_key(pool, id) keys revocation on id alone with no user_id column in its WHERE clause, so the DB layer does not enforce ownership.

Why this matters

The per-user SQL filters (WHERE user_id = ?) are the sole isolation mechanism for all play-history analytics. A dropped or mis-bound user_id predicate would pass every single-user test while leaking every user's listening history. On a sovereign phone OS, listening history is a behavioral fingerprint that a capable adversary can use for deanonymization and pattern-of-life analysis; an isolation regression here is a direct counter-surveillance failure, not a cosmetic data leak. The revoke_api_key gap is a DB-layer IDOR: any caller bug that bypasses the HTTP-layer authorization can revoke another user's API key, and no query-level guard stops it.

Desired correction

Add two-user isolation tests to play_history/tests.rs: create user A and user B, seed each with distinct sessions and stats, then assert that recent_sessions(user_a), top_items(user_a), and get_active_sessions(user_a) contain none of user B's rows, and symmetrically for user B. Add a user.rs test confirming list_api_keys_for_user returns only the requested user's keys when a second user's keys exist. Enforce ownership at the query level for revocation by adding WHERE user_id = ? AND id = ? to revoke_api_key, or by introducing a revoke_api_key_for_user variant, and cover it with a test asserting user A cannot revoke user B's key.

Done when: at least one two-user isolation test exists for each of recent_sessions, top_items, and get_active_sessions; list_api_keys_for_user has a two-user scoping test; and revoke_api_key enforces ownership at the query level with a test confirming cross-user revocation fails.

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions