WPB-23659 Revoke cookie with same label on login/cookie renewal

[battermann]

Checklist

• Add a new entry in an appropriate subdirectory of changelog.d
• Read and follow the PR guidelines

[Copilot]

Pull request overview

This pull request implements automatic revocation of cookies with the same label when a new cookie is created or renewed. This ensures that only one active cookie exists per label per user, which is useful for device-specific authentication.

Changes:

• Automatic same-label cookie revocation on cookie creation
• Implementation of DeleteCookies in mock session store interpreter
• Addition of Ord instance to CookieId for test support
• Comprehensive property-based tests for the new behavior

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
libs/wire-subsystems/src/Wire/AuthenticationSubsystem/Cookie.hs	Adds revocation logic to automatically delete same-label cookies when creating a new cookie, introduces revokeCookiesMatchingExcept helper function
libs/wire-subsystems/test/unit/Wire/AuthenticationSubsystem/InterpreterSpec.hs	Adds two property-based tests verifying same-label revocation behavior and cross-user isolation
libs/wire-subsystems/test/unit/Wire/MockInterpreters/SessionStore.hs	Implements DeleteCookies operation in the mock interpreter using list filtering
libs/wire-api/src/Wire/API/User/Auth.hs	Adds Ord derivation to CookieId to support using it in Set for tests
changelog.d/2-features/WPB-23659	Documents the feature in the changelog

Comments suppressed due to low confidence (1)

libs/wire-subsystems/src/Wire/AuthenticationSubsystem/Cookie.hs:72

• When a labeled cookie is renewed in services/brig/src/Brig/User/Auth/Cookie.hs:renewCookie, it calls newCookie which revokes all same-label cookies including the cookie being renewed. The old cookie is then re-inserted with a successor link. This creates inefficiency (delete + re-insert) and a brief race condition window where the old cookie doesn't exist in the database. Consider adding a mechanism to exclude specific cookies from same-label revocation, or restructure the renewal flow to avoid this issue.

  for_ c.cookieLabel $ revokeCookiesMatchingExcept uid (Just c.cookieId) [] . (: [])

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

libs/wire-subsystems/src/Wire/AppSubsystem/Interpreter.hs:129

• This changes app cookies from having the explicit label Just "app" to Nothing. That alters cookie-label semantics for apps (including disabling same-label revocation because revocation only triggers when a label is present) and may break any behavior that relies on identifying app sessions by label. If this wasn’t intentional, keep the "app" label and only add the new SameLabelPolicy argument.

  c :: Cookie (Token U) <- newCookie u.id Nothing PersistentCookie Nothing RevokeSameLabel

libs/wire-subsystems/src/Wire/AppSubsystem/Interpreter.hs:224

• Same as above: newCookieLimited ... Nothing RevokeSameLabel drops the previous app cookie label. If app cookies should still be uniquely identifiable/revocable by label, pass Just "app" here as well (and keep the RevokeSameLabel policy if the goal is to ensure a single active app cookie).

  c :: Cookie (Token U) <-
    newCookieLimited appId Nothing PersistentCookie Nothing RevokeSameLabel
      >>= either throw pure

libs/wire-subsystems/test/unit/Wire/MockInterpreters/SessionStore.hs:39

• The in-memory DeleteCookies implementation removes cookies using full structural equality (notElem cc). In production (Cassandra), deletion is effectively keyed by (cookieExpires, cookieId) only. Using full equality can cause the mock to keep cookies that should be deleted if the caller passes an equivalent cookie record that differs in non-key fields. Consider filtering by cookieId and cookieExpires instead to better match real semantics.

  DeleteCookies uid cc -> modify $ Map.adjust (filter (`notElem` cc)) uid

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

newCookieImpl inserts the new cookie and then revokes same-label cookies by listing + deleting everything except c.cookieId. This is racy under concurrent logins for the same (user,label): one request can delete the other request’s freshly inserted cookie before it lists, causing the second request to return a cookie that is no longer persisted (and it may then delete the remaining cookie, potentially leaving none). Consider making the revocation step choose a deterministic “winner” from the currently stored cookies (e.g., newest by (cookieCreated,cookieId)) and delete all others, or otherwise ensure the cookie returned by this call cannot be concurrently deleted by another same-label issuance.

[battermann]

Multiple concurrent logins providing the same cookie label is extremely unlikely. And even if it does happen, the time window is extremely small. And even if it happens in the exact same moment, the worst thing that happens is that both login attempts fail, and have to be repeated. So, IMO, small impact + low probability makes the risk very small. I suggest to ignore this, WDYT @akshaymankar ?

[battermann]

merging now. if we decide that this is important, it will a follow up PR.