Skip to content

fix(channels): keep foreign canvases out of me - #3888

Open
puemos wants to merge 5 commits into
mainfrom
posthog-code/fix-personal-canvas-ownership
Open

fix(channels): keep foreign canvases out of me#3888
puemos wants to merge 5 commits into
mainfrom
posthog-code/fix-personal-canvas-ownership

Conversation

@puemos

@puemos puemos commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Problem

The personal #me space classifies canvases with missing creator metadata as owned by the viewer, allowing foreign legacy canvases to appear there.

Changes

  • Decide ownership solely by the stable creator uuid (authorUuid), uniformly for canvases and tasks.
  • Remove display-name matching from ownership: a shared/auto-generated name is not an identity, and using it to gate the private #me space could leak canvases between two users with the same name.
  • Fail closed — items with no creator uuid don't appear in anyone's #me. authorName/authorUser are kept for display only.

Depends on PostHog/posthog#74282 for creator UUIDs in file-system responses. Safe to merge first: until it lands, uuid-less canvases fail closed (they're excluded from #me rather than mis-attributed).

How did you test this?

  • pnpm --filter @posthog/core exec vitest run src/canvas/channelItems.test.ts src/canvas/dashboardsService.test.ts
  • pnpm --filter @posthog/core typecheck
  • pnpm --filter @posthog/ui typecheck
  • pnpm build
  • Biome check on all changed files

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

Created with PostHog Code

Generated-By: PostHog Code
Task-Id: d8debcda-8a0a-4287-a5aa-ee0f5f6ce8d9
@puemos puemos self-assigned this Jul 28, 2026
@trunk-io

trunk-io Bot commented Jul 28, 2026

Copy link
Copy Markdown

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown

React Doctor found no issues in the changed files. 🎉

Reviewed by React Doctor for commit bdeb095.

@puemos
puemos marked this pull request as ready for review July 28, 2026 16:47
puemos added 3 commits July 28, 2026 20:05
isOwnedBy now compares the stable authorUuid uniformly for both canvases
and tasks, instead of a separate authorUser branch for tasks. authorUser
becomes a display-only field; display-name matching stays as a fallback for
rows with no creator uuid. Removes the canvas/task ownership asymmetry with
no behavior or UI change.

Generated-By: PostHog Code
Task-Id: e9aaba50-9b88-43d5-8a62-dd7adf4c76f1
Deciding ownership of the private #me space by display-name equality could
leak canvases between two users who share a name. Ownership now requires a
stable creator uuid match; items without one fail closed. authorName stays
for display only. Adds a regression test that a name match alone never grants
ownership.

Generated-By: PostHog Code
Task-Id: e9aaba50-9b88-43d5-8a62-dd7adf4c76f1
The personal-channel hook test relied on name-based ownership, which no
longer grants membership; give the viewer's canvas a matching creator uuid.

Generated-By: PostHog Code
Task-Id: e9aaba50-9b88-43d5-8a62-dd7adf4c76f1
@puemos puemos added the Create Release This will trigger a new release label Jul 28, 2026
@adamleithp

Copy link
Copy Markdown
Contributor

Reviewed on the assumption that PostHog/posthog#74282 lands first (checked it: UserBasicSerializer on FileSystemSerializer, so created_by rides every list row and is null only when the creator was deleted; the queryset already does select_related("created_by") at file_system.py:201, so no N+1).

Direction is right — a display name isn't an identity, and gating #me on it was a real leak. Findings, roughly by weight:

  • Three parallel author fields where the backend now gives one canonical user. ChannelItemModel carries authorUser + authorName + authorUuid. Post-#74282 canvas rows ship a full UserBasic, the same shape tasks already use. Carrying it whole makes isOwnedBy a single item.authorUser?.uuid === owner.uuid for both kinds with no per-kind mapping to keep in sync, and removes the "which field is identity" ambiguity this PR exists to fix rather than adding one more field to it. Would mean dashboardSummarySchema carrying the user object instead of the createdBy/createdByUuid scalars.

  • Canvases still can't render an avatar. ChannelItemRow.tsx:171 shows <UserAvatar> only when authorUser is set, so canvas rows fall back to a letter initial while task rows show the real avatar. The data to fix that arrives with #74282 and this PR routes past it — same root cause as the point above.

  • viewerKnown gate is now stale. useChannelItems.tsx:77 is meUuid != null || meName != null. Since a name no longer establishes identity, a name-only viewer passes the gate and then filters against owner.uuid === null, so isOwnedBy is false for everything and #me goes silently empty. Should be meUuid != null.

  • ChannelItemOwner.name is dead. isOwnedBy was its only reader — traced me out of the hook through ChannelSidebar into filterChannelItems, nothing else touches .name. Worth dropping so name matching doesn't get rewired later.

  • toRecord still prefers the stale stamped name. Line 795 is meta.createdBy ?? creatorName(entry.created_by), but the schema comment this PR rewrites now says the meta stamp is compat-only for pre-API rows. As written the stamp wins forever, so anyone who changes their display name shows stale on every canvas they've made. Flipping to creatorName(entry.created_by) ?? meta.createdBy matches the new comment.

  • "Created by: others" claims creator-deleted rows. filterChannelItems derives both buckets from one isOwnedBy, so authorUuid == null lands in "others". The backend returns created_by: null for deleted creators — their test_desktop_list_returns_null_for_deleted_creator covers it — so this is reachable, not theoretical. Consider excluding null-uuid from both me and others.

  • No service-level test for created_by: null. dashboardsService.test.ts covers only the happy mapping. That null branch is the whole fail-closed path, and the backend tests both sides of it.

  • Self-hosted skew. apiHost is configurable (posthog-client.ts:1347), so instances behind that release return uuid-less rows and get a blank #me with no upgrade signal. Stamping createdByUuid into create()'s meta alongside the existing name stamp would cover it. Skip if cloud-only is the working assumption.

Addresses review feedback on uuid-only ownership:

- filterChannelItems: an item with no creator uuid (the backend sends
  created_by: null once a creator is deleted) is unknown, not "someone
  else", so exclude it from both the "me" and "others" buckets.
- Drop the now-unused ChannelItemOwner.name; isOwnedBy was its only
  reader, so nothing can rewire name matching by accident.
- Gate #me on meUuid alone, since a display name never established
  identity. No behavior change, but the condition no longer encodes the
  premise this PR removed.
- Cover the created_by: null mapping at the service level, which is the
  fail-closed path.

Generated-By: PostHog Code
Task-Id: e9aaba50-9b88-43d5-8a62-dd7adf4c76f1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Create Release This will trigger a new release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants