Skip to content

Commit 22680b7

Browse files
committed
fix(webapp): address review feedback on session route hardening
Scope the batched session run-id lookup to the caller environment and project so a stale currentRunId pointer cannot resolve a run in another tenant. Escape the user-supplied segments of the append idempotency key so a colon in an externalId or X-Part-Id cannot collide and falsely suppress an append. Keep the waitpoint drain running on an idempotent retry: a duplicate append is skipped but still drains, so a retry whose first attempt died before waking the waitpoint can still recover it.
1 parent 3fc6d4d commit 22680b7

4 files changed

Lines changed: 52 additions & 36 deletions

File tree

apps/webapp/app/routes/api.v1.sessions.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,11 @@ export const loader = createLoaderApiRoute(
108108
environmentType: authentication.environment.type,
109109
organizationId: authentication.environment.organizationId,
110110
}) as Session
111-
)
111+
),
112+
{
113+
projectId: authentication.environment.projectId,
114+
runtimeEnvironmentId: authentication.environment.id,
115+
}
112116
);
113117

114118
return json<ListSessionsResponseBody>({

apps/webapp/app/routes/realtime.v1.sessions.$session.$io.append.ts

Lines changed: 32 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -151,49 +151,49 @@ const { action, loader } = createActionApiRoute(
151151
const partId = clientPartId ?? nanoid(7);
152152

153153
// Idempotency on client-supplied part ids: a retried POST whose first
154-
// attempt committed is acknowledged without a second append (which
155-
// would duplicate the record and double-fire the waitpoint drain).
156-
// The marker is only written after a successful append, so retries of
157-
// genuinely failed appends still go through. Server-generated ids are
158-
// per-request and carry no dedupe meaning.
159-
if (
160-
clientPartId &&
154+
// attempt committed skips the second append (which would duplicate the
155+
// record), but still falls through to the drain below — so a retry whose
156+
// first attempt died before waking the waitpoint can still recover it.
157+
const alreadyAppended =
158+
!!clientPartId &&
161159
(await wasSessionStreamPartAppended(
162160
authentication.environment.id,
163161
addressingKey,
164162
params.io,
165163
clientPartId
166-
))
167-
) {
168-
return json({ ok: true }, { status: 200 });
169-
}
164+
));
170165

171-
const [appendError] = await tryCatch(
172-
realtimeStream.appendPartToSessionStream(part, partId, addressingKey, params.io)
173-
);
166+
if (!alreadyAppended) {
167+
const [appendError] = await tryCatch(
168+
realtimeStream.appendPartToSessionStream(part, partId, addressingKey, params.io)
169+
);
174170

175-
if (appendError) {
176-
if (appendError instanceof ServiceValidationError) {
171+
if (appendError) {
172+
if (appendError instanceof ServiceValidationError) {
173+
return json(
174+
{ ok: false, error: appendError.message },
175+
{ status: appendError.status ?? 422 }
176+
);
177+
}
178+
logger.error("Failed to append to session stream", {
179+
sessionId: session.id,
180+
io: params.io,
181+
error: appendError,
182+
});
177183
return json(
178-
{ ok: false, error: appendError.message },
179-
{ status: appendError.status ?? 422 }
184+
{ ok: false, error: "Something went wrong, please try again." },
185+
{ status: 500 }
180186
);
181187
}
182-
logger.error("Failed to append to session stream", {
183-
sessionId: session.id,
184-
io: params.io,
185-
error: appendError,
186-
});
187-
return json({ ok: false, error: "Something went wrong, please try again." }, { status: 500 });
188-
}
189188

190-
if (clientPartId) {
191-
await markSessionStreamPartAppended(
192-
authentication.environment.id,
193-
addressingKey,
194-
params.io,
195-
clientPartId
196-
);
189+
if (clientPartId) {
190+
await markSessionStreamPartAppended(
191+
authentication.environment.id,
192+
addressingKey,
193+
params.io,
194+
clientPartId
195+
);
196+
}
197197
}
198198

199199
// Fire any run-scoped waitpoints registered against this channel. Best

apps/webapp/app/services/realtime/sessions.server.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,13 +134,20 @@ export async function serializeSessionWithFriendlyRunId(
134134
* can't use with `runs.retrieve(...)`.
135135
*/
136136
export async function serializeSessionsWithFriendlyRunIds(
137-
sessions: Session[]
137+
sessions: Session[],
138+
scope: { projectId: string; runtimeEnvironmentId: string }
138139
): Promise<SessionItem[]> {
139140
const runIds = [...new Set(sessions.map((s) => s.currentRunId).filter((id): id is string => !!id))];
140141

142+
// `currentRunId` is a plain string pointer (no FK), so scope the lookup to
143+
// the caller's tenant — a stale value must not resolve a run in another env.
141144
const runs = runIds.length
142145
? await $replica.taskRun.findMany({
143-
where: { id: { in: runIds } },
146+
where: {
147+
id: { in: runIds },
148+
projectId: scope.projectId,
149+
runtimeEnvironmentId: scope.runtimeEnvironmentId,
150+
},
144151
select: { id: true, friendlyId: true },
145152
})
146153
: [];

apps/webapp/app/services/sessionStreamWaitpointCache.server.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,12 @@ function buildAppendDedupeKey(
167167
io: "out" | "in",
168168
partId: string
169169
): string {
170-
return `${APPEND_DEDUPE_PREFIX}${environmentId}:${addressingKey}:${io}:${partId}`;
170+
// Encode the free-form segments — `addressingKey` (externalId) and `partId`
171+
// (X-Part-Id) are user-supplied and may contain `:`, which would otherwise
172+
// let different tuples collide and falsely suppress an append.
173+
return `${APPEND_DEDUPE_PREFIX}${encodeURIComponent(environmentId)}:${encodeURIComponent(
174+
addressingKey
175+
)}:${io}:${encodeURIComponent(partId)}`;
171176
}
172177

173178
/**

0 commit comments

Comments
 (0)