Repository navigation
Session revocation support - #759
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
fb4748f to
ebf9d14
Compare
0704c76 to
2fd44f3
Compare
ebf9d14 to
86e0b19
Compare
dowski
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 7 files
This PR changes authentication, authorization, or input validation. Ultrareviews find 2.4x more serious bugs than standard reviews. Comment @cubic-dev-ai ultrareview to run one.
Re-trigger cubic
2fd44f3 to
78fe425
Compare
86e0b19 to
1053d64
Compare
78fe425 to
dfc4870
Compare
1053d64 to
3f34310
Compare
3f34310 to
ff30c70
Compare
dfc4870 to
a8fadc6
Compare
ff30c70 to
2f66a76
Compare
commit: |
There was a problem hiding this comment.
3 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/lib/types.ts">
<violation number="1" location="packages/core/src/lib/types.ts:369">
P3: The doc says this returns 0 only when not authenticated, but it also returns 0 without revoking anything when the current session has already been revoked. Document that case so callers can tell it apart from having no other sessions.</violation>
</file>
<file name="packages/core/src/component/public.ts">
<violation number="1" location="packages/core/src/component/public.ts:805">
P2: This collects every session and queries its refresh tokens one session at a time; repeated sign-ins can grow this work without bound because session creation has no per-user cap. Cap sessions when issuing them or batch/paginate this listing and revocation.</violation>
<violation number="2" location="packages/core/src/component/public.ts:810">
P2: This treats the newest issued token's expiry as the session expiry, but refresh races can leave several issued tokens valid at once. After `refreshTokenTtlSeconds` changes, an older token can expire later, so `listSessions` reports the session as expired and `revokeOtherSessions` undercounts active sessions; use the maximum expiry across issued tokens.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| for (const session of sessions) { | ||
| // Issued rows have no `retiredAt`, so the index orders them by | ||
| // `_creationTime`: the last one is the newest. | ||
| const newestIssued = await ctx.db |
There was a problem hiding this comment.
P2: This treats the newest issued token's expiry as the session expiry, but refresh races can leave several issued tokens valid at once. After refreshTokenTtlSeconds changes, an older token can expire later, so listSessions reports the session as expired and revokeOtherSessions undercounts active sessions; use the maximum expiry across issued tokens.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/core/src/component/public.ts, line 810:
<comment>This treats the newest issued token's expiry as the session expiry, but refresh races can leave several issued tokens valid at once. After `refreshTokenTtlSeconds` changes, an older token can expire later, so `listSessions` reports the session as expired and `revokeOtherSessions` undercounts active sessions; use the maximum expiry across issued tokens.</comment>
<file context>
@@ -755,3 +759,112 @@ export const signOut = mutation({
+ for (const session of sessions) {
+ // Issued rows have no `retiredAt`, so the index orders them by
+ // `_creationTime`: the last one is the newest.
+ const newestIssued = await ctx.db
+ .query("refreshTokens")
+ .withIndex("by_session_state", (q) =>
</file context>
There was a problem hiding this comment.
I'll address this separately.
| .query("sessions") | ||
| .withIndex("by_user", (q) => q.eq("userId", userId)) | ||
| .order("desc") | ||
| .collect(); |
There was a problem hiding this comment.
P2: This collects every session and queries its refresh tokens one session at a time; repeated sign-ins can grow this work without bound because session creation has no per-user cap. Cap sessions when issuing them or batch/paginate this listing and revocation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/core/src/component/public.ts, line 805:
<comment>This collects every session and queries its refresh tokens one session at a time; repeated sign-ins can grow this work without bound because session creation has no per-user cap. Cap sessions when issuing them or batch/paginate this listing and revocation.</comment>
<file context>
@@ -755,3 +759,112 @@ export const signOut = mutation({
+ .query("sessions")
+ .withIndex("by_user", (q) => q.eq("userId", userId))
+ .order("desc")
+ .collect();
+ const result = [];
+ for (const session of sessions) {
</file context>
There was a problem hiding this comment.
Will prune sessions at issuance (see TODO comment above).
| /** | ||
| * Revokes every session of the signed-in user other than the current one | ||
| * (the JWT's subject). Returns the number of active (non-expired) | ||
| * sessions revoked; returns 0 when not authenticated. Expired sessions are |
There was a problem hiding this comment.
P3: The doc says this returns 0 only when not authenticated, but it also returns 0 without revoking anything when the current session has already been revoked. Document that case so callers can tell it apart from having no other sessions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/core/src/lib/types.ts, line 369:
<comment>The doc says this returns 0 only when not authenticated, but it also returns 0 without revoking anything when the current session has already been revoked. Document that case so callers can tell it apart from having no other sessions.</comment>
<file context>
@@ -363,6 +363,18 @@ export type BoundAuthHelpers<Profile> = {
+ /**
+ * Revokes every session of the signed-in user other than the current one
+ * (the JWT's subject). Returns the number of active (non-expired)
+ * sessions revoked; returns 0 when not authenticated. Expired sessions are
+ * deleted as well, but not counted.
+ *
</file context>
| * sessions revoked; returns 0 when not authenticated. Expired sessions are | |
| * sessions revoked. Returns 0 when not authenticated, or when the current | |
| * session no longer exists (then nothing is revoked). Expired sessions are |
There was a problem hiding this comment.
It already says returns the number of active sessions revoked. That could be 0.
8740231 to
1c5ba2c
Compare
afe8173 to
fcde1bf
Compare
| * NOTE: Callers should only invoke this function after verifying that the | ||
| * logged in user truly owns the session, via confirming an authentication | ||
| * method. Otherwise, the request might be originating from a stolen session, | ||
| * which shouldn't be able to revoke other sessions. |
There was a problem hiding this comment.
What is this comment for, since this method receives no parameter?
There was a problem hiding this comment.
The implementation looks up the user via the current session. So even though there's no argument, the caution here is to confirm that the session user can authenticate as proof that they're the real user.
| */ | ||
| resolveUserId(providerAccountId: string): Promise<string | null>; | ||
| /** | ||
| * Revokes every session of the signed-in user other than the current one |
There was a problem hiding this comment.
It sounds good to add this method to the method authoring APIs, it allows us to improve the implementation later if necessary
| sessionIds: others.map(({ sessionId }) => sessionId), | ||
| }), | ||
| ); | ||
| // Expired sessions are deleted too, but only the active ones count. |
There was a problem hiding this comment.
Shouldn't we automatically exclude expired sessions from listSessions? Logically they don’t exist, whether an expired session has been deleted by the batch worker or not should be an implementation detail of the component that is not exposed to the app
There was a problem hiding this comment.
In order to know whether a session is expired, you need the current time. I didn't want to put a Date.now() call in the listSessions query since that's an anti-pattern.
Once we have the batch worker (actually pending in a draft up stack) the window for including expired sessions will be small, but possible. So I think it makes sense to handle them here.
| ): Promise<TokenBundle> { | ||
| const now = Date.now(); | ||
| // TODO: cap the sessions a user may have (e.g. 100), deleting the oldest | ||
| // when a new one would exceed it. That bounds `listSessions`, which reads |
There was a problem hiding this comment.
Maybe as an alternative we could consider paginating listSessions?
There was a problem hiding this comment.
I don’t know how feasible it would be to have methods revoke sessions asynchronously (e.g. through a self-scheduling mutation), so that it supports revoking sessions for a user that has lots of them.
Maybe it would make sense to keep the revokeOtherSessions() method @internal (even after we expose BoundAuthHelpers for custom method authors) if we consider that this API is a little bit footgunny
There was a problem hiding this comment.
Yeah I'd like to explore/discuss what a more reactive/scheduled deletion approach might look like.
I'm fine marking the function as @internal for now.
| continue; | ||
| } | ||
| result.push({ | ||
| sessionId: session._id as string, |
There was a problem hiding this comment.
nit: Is this cast necessary? Id<> is string & { … } so I would expect this cast to look more dangerous than what it is
1c5ba2c to
f77ca58
Compare
fcde1bf to
201b3bd
Compare
201b3bd to
957d13b
Compare
f77ca58 to
242704a
Compare
242704a to
56bf5c5
Compare
957d13b to
520e0ba
Compare
520e0ba to
2a03112
Compare
Replace the component's single-session revokeSession with revokeSessions, which takes a list of session ids and returns the ids it revoked. The app-side revokeOtherSessions now lists the user's sessions, filters out the current one, and revokes the rest in a single component call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2a03112 to
adf014c
Compare
Auth mutations now have access to a
ctx.convexAuth.revokeOtherSessionsfunction. It will revoke every session for the user except the current one.It's built from new
listSessionsandrevokeSessionscomponent functions.The session revocation functions have documentation noting that they should be used with care (e.g. use along with proof that a user possesses credentials).