Fix private REST inbox teammate lastSeenAt context#157
Fix private REST inbox teammate lastSeenAt context#157seemayr wants to merge 1 commit intocossistantcom:mainfrom
Conversation
|
@seemayr is attempting to deploy a commit to the cossistant Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR fixes the private REST
Confidence Score: 4/5Not safe to merge as-is — the required: false guard has a gap that can 403 linked private keys on teamless websites. There is one P1 finding: the inbox route can now return 403 for previously-valid linked private keys when the website has no teamId, because resolvePrivateApiKeyActorUser only short-circuits to null when candidateUserId is absent. The fix is narrowly scoped and well-tested for the happy path, but this edge case is neither tested nor guarded against. apps/api/src/rest/routers/conversation.ts — the requirePrivateConversationActor call around line 2046 needs a fallback or guard for the linked-key + teamless-website case. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant InboxRoute as GET /inbox
participant ActorResolver as requirePrivateConversationActor
participant PlanFetcher as getPlanForWebsite
participant HeadersQuery as listConversationsHeaders
Client->>InboxRoute: GET /inbox (private key)
InboxRoute->>InboxRoute: requirePrivateControlContext()
Note over InboxRoute: NEW: resolve actor before query
InboxRoute->>ActorResolver: resolvePrivateApiKeyActorUser(required: false)
alt linked key + no teamId
ActorResolver-->>InboxRoute: throws 403 (regression)
else unlinked key / no actor
ActorResolver-->>InboxRoute: null
else linked key + teamId present
ActorResolver-->>InboxRoute: { userId, member, source }
end
InboxRoute->>PlanFetcher: getPlanForWebsite()
InboxRoute->>HeadersQuery: listConversationsHeaders(userId: actor?.userId ?? null)
HeadersQuery-->>InboxRoute: { items, nextCursor }
InboxRoute-->>Client: 200 inbox payload
Reviews (1): Last reviewed commit: "Fix REST inbox teammate last-seen contex..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const actor = await requirePrivateConversationActor({ | ||
| c, | ||
| db: extracted.db, | ||
| apiKey: privateContext.apiKey, | ||
| organizationId: privateContext.organization.id, | ||
| websiteTeamId: privateContext.website.teamId, | ||
| required: false, | ||
| }); |
There was a problem hiding this comment.
required: false doesn't prevent 403 for linked keys on teamless websites
resolvePrivateApiKeyActorUser only short-circuits to null when candidateUserId is falsy (unlinked key + no explicit actor header). When a key has linkedUserId set — or when an X-Actor-User-Id header is present — candidateUserId becomes truthy, and the next check if (!params.websiteTeamId) throws a 403 unconditionally, regardless of required. So any linked private key used against a website with teamId: null | undefined will now get a 403 from the inbox, where previously it always returned 200 with userId: null.
This contradicts the PR's stated goal of keeping actorless / no-team-context integrations working. The relevant path in resolvePrivateApiKeyActorUser (apps/api/src/lib/private-api-key-actor.ts lines 54-58):
if (!params.websiteTeamId) {
throw new AuthValidationError(403, ...); // no required check here
}A catch around the call in the inbox handler, or a separate guard, is needed to handle this gracefully.
| const actor = await requirePrivateConversationActor({ | ||
| c, | ||
| db: extracted.db, | ||
| apiKey: privateContext.apiKey, | ||
| organizationId: privateContext.organization.id, | ||
| websiteTeamId: privateContext.website.teamId, | ||
| required: false, | ||
| }); | ||
|
|
||
| const [planInfo, result] = await Promise.all([ | ||
| getPlanForWebsite(privateContext.website), | ||
| listConversationsHeaders(extracted.db, { | ||
| organizationId: privateContext.organization.id, | ||
| websiteId: privateContext.website.id, | ||
| userId: null, | ||
| userId: actor?.userId ?? null, | ||
| limit: extracted.query.limit, | ||
| cursor: extracted.query.cursor ?? null, | ||
| }), |
There was a problem hiding this comment.
requirePrivateConversationActor is awaited sequentially before the Promise.all, but actor resolution and getPlanForWebsite are fully independent of each other. Parallelizing them — e.g. const [actor, planInfo] = await Promise.all([requirePrivateConversationActor(...), getPlanForWebsite(...)]) — would reduce inbox latency on every request at the cost of zero correctness risk.
1 participant
Summary
/conversations/inboxroute resolve the acting teammate when availableuserIdintolistConversationsHeaders(...)so dashboard inbox items can surface teammate-specificlastSeenAtuserId: nullProblem
The backend already models two distinct seen concepts:
visitorLastSeenAtconversation_seen.userId -> lastSeenAtThe shared dashboard header query already supports teammate-specific
lastSeenAt, and the tRPC dashboard path passes the actinguser.idinto that query.However, the private REST inbox route currently hardcodes
userId: nullwhen callinglistConversationsHeaders(...). That means REST dashboard clients never receive teammate-specificlastSeenAtfrom inbox payloads, even when the private key resolves to a valid actor user.Reasoning
This PR assumes the private REST inbox is intended to behave like the dashboard surface, not like the public/widget surface.
Why this looks like a bug rather than an intentional design:
lastSeenAtas user-specific when availablelistConversationsHeaders(...)computes teammatelastSeenAtalreadyuser.idinto that same queryPOST /conversations/{id}/readexplicitly writes read state for the acting teammateuserId: nullSo the narrow fix here is to let the private REST inbox use the resolved actor user when one exists.
Assumptions
This PR is only correct if these assumptions are correct:
lastSeenAton private inbox items is teammate-specific, not visitor-specificlastSeenAtlastSeenAt: nullIf any of those assumptions are wrong, the change should be reconsidered.
Scope
This PR intentionally does not:
markReadresponse shapeIt only fixes actor propagation for the private REST inbox route.
Testing
bun test apps/api/src/rest/routers/conversation-auth.test.ts