Skip to content

Commit 558703b

Browse files
d-csclaude
andcommitted
fix(webapp): mollifier buffered-path auth checks + drop high-cardinality envId from realtime counter
Three CodeRabbit findings from #3709, re-raised on #3757: - resources.taskruns.$runParam.debug.ts: buffered fallback returned the run's queue / concurrencyKey / queueTimestamp from the snapshot without verifying org membership. Any authenticated user who knew a friendlyId could read those fields across orgs. Now joins through orgMember the same way the PG path does and 404s on miss. - resources.runs.$runParam.logs.download.ts: same shape — the buffered placeholder leaked runId existence to non-members on direct URL access. Same orgMember check now gates the buffered branch. - mollifierTelemetry.server.ts: recordRealtimeBufferedSubscription was attaching envId (a UUID) as an OTEL counter dimension, violating the project's "no high-cardinality IDs in metric attributes" guideline. Dropped the parameter; the call site's logger.info still emits envId. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 0ac3244 commit 558703b

4 files changed

Lines changed: 24 additions & 8 deletions

File tree

apps/webapp/app/routes/realtime.v1.runs.$runId.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ export const loader = createLoaderApiRoute(
7979
typeof bufferedDwellMs === "number" &&
8080
isInitialBufferedSubscriptionRequest(request.url)
8181
) {
82-
recordRealtimeBufferedSubscription(authentication.environment.id);
82+
recordRealtimeBufferedSubscription();
8383
logger.info("mollifier.realtime.buffered_subscription", {
8484
runId: run.friendlyId,
8585
envId: authentication.environment.id,

apps/webapp/app/routes/resources.runs.$runParam.logs.download.ts

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,15 +33,21 @@ export async function loader({ params, request }: LoaderFunctionArgs) {
3333
if (!run || !run.organizationId) {
3434
// Buffered run has no events to package yet. Return a small gzipped
3535
// placeholder file so the dashboard's "Download logs" button doesn't
36-
// 404 mid-burst. We don't enforce org membership here because the
37-
// buffer entry's envId/orgId fields aren't bound to the requesting
38-
// user — that's checked by the calling page's loader already (this
39-
// route is only reachable from a page the user has visited).
36+
// 404 mid-burst. Re-check org membership against the buffer entry's
37+
// orgId so direct URL access can't confirm runId existence across
38+
// orgs (loader-based check on the calling page doesn't apply here).
4039
const buffer = getMollifierBuffer();
4140
if (buffer) {
4241
try {
4342
const entry = await buffer.getEntry(parsedParams.runParam);
4443
if (entry) {
44+
const member = await prisma.orgMember.findFirst({
45+
where: { userId: user.id, organizationId: entry.orgId },
46+
select: { id: true },
47+
});
48+
if (!member) {
49+
return new Response("Not found", { status: 404 });
50+
}
4551
const placeholder = new Readable({
4652
read() {
4753
this.push(

apps/webapp/app/routes/resources.taskruns.$runParam.debug.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,16 @@ export async function loader({ request, params }: LoaderFunctionArgs) {
5454
try {
5555
const entry = await buffer.getEntry(runParam);
5656
if (entry) {
57+
// Same org-membership gate as the PG path above. Without it,
58+
// any authenticated user who knows a runId could read the
59+
// buffered run's queue/concurrencyKey snapshot across orgs.
60+
const member = await $replica.orgMember.findFirst({
61+
where: { userId, organizationId: entry.orgId },
62+
select: { id: true },
63+
});
64+
if (!member) {
65+
throw new Response("Not Found", { status: 404 });
66+
}
5767
const snapshot = deserialiseSnapshot<{
5868
taskIdentifier?: string;
5969
queue?: string;

apps/webapp/app/v3/mollifier/mollifierTelemetry.server.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,9 @@ export const realtimeBufferedSubscriptionsCounter = meter.createCounter(
3131

3232
// No `envId` attribute — `envId` is a banned high-cardinality metric
3333
// label per the repo's OTel rules. The structured warn log emitted
34-
// alongside the counter tick (in `mollifierStaleSweep.server.ts`)
35-
// carries the envId / orgId / runId for forensic drill-down; the
36-
// metric stays an aggregate.
34+
// alongside the counter tick (in `mollifierStaleSweep.server.ts` and
35+
// the realtime route's `logger.info`) carries the envId / orgId /
36+
// runId for forensic drill-down; the metric stays an aggregate.
3737
export function recordRealtimeBufferedSubscription(): void {
3838
realtimeBufferedSubscriptionsCounter.add(1);
3939
}

0 commit comments

Comments
 (0)