Skip to content

Let Content document readers play and comment on Clips uploaded to the document - #8090

Merged
3mdistal merged 54 commits into
mainfrom
clips-content-bindings
Oct 27, 2026
Merged

3mdistal merged 54 commits into
mainfrom
clips-content-bindings

Conversation

@3mdistal

@3mdistal 3mdistal commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

A Clip recorded or uploaded inside an Agent-Native Content document should play for everyone who can read that document, and those readers should be able to comment on it, without the Clip's owner sharing it with each reader. Today Clips knows only its own sharing. A private Clip placed in a document cannot play for the document's readers, and Content has no way to ask Clips, on a reader's behalf, for playback or comments.

This is the Clips server side of "Clips videos embedded natively in Content docs" (step A2). It has no UI. It builds on #8059 and #8088, both now on main.

Framework changes

A secret only Content and Clips hold. Clips takes Content's word for who can read a document, so it has to know a request really came from Content. Core's A2A tokens are signed with the deployment-wide A2A_SECRET or a workspace's own secret, and every app in the deployment holds those, so a token signed with either could claim to be any Content app. Core gains a server config field, clips.contentPeerSecret (env CONTENT_CLIPS_PEER_SECRET), and one resolver both apps read it through: resolveContentClipsPeerSecret(getAppConfig().clips, getGlobalA2ASecret()). It returns null while the secret is unset, and throws AppConfigurationError when the value is shorter than 32 characters or equal to A2A_SECRET, because a secret every app holds would prove nothing. Content signs with it (A3); Clips verifies with it and nothing else. signShortLivedToken and verifyShortLivedToken also take an optional { signingKey } that replaces the app's server secret with no fallback; Clips uses it for the media grants below.

Signed-out host access resolvers are opt-in. Core lets an app register a host access resolver for a resource type: a function that grants a role, denies (null), or has no opinion (undefined). Core asks it only when someone is signed in. A signed-out reader of a public Content document has to reach Clips' resolver, so registerHostAccessResolver(type, resolver, { includeSignedOut: true }) also asks that resolver about signed-out requests, with ctx.userEmail unset. Without the option a resolver is still asked only about signed-in requests, so Design's Builder resolver, and any other app's, behaves exactly as before. Only Clips sets it.

Both ship as patch changesets.

Approach

Clips now records which Content documents a Clip is placed in. A Clip uploaded into a document counts as a viewer share for whoever Content vouches can read that document, for as long as that placement is active. Content vouches on every call with a one-minute token, signed with the secret only Content and Clips hold, that names one document and one recording.

Everything else stays Clips' own rules. Trash, takedown, expiry, a share password and the comments-off setting apply to a document reader exactly as they do to any non-owner viewer, and the owner keeps the owner's exceptions:

  • A trashed or taken-down Clip is a 404 for document readers, and an expired one is a 410. A password-protected Clip does not play through a document, because a document cannot supply the password; a media URL already handed out stops working when a password is added, whatever unlock it carries.
  • With comments off, a reader, signed in or out, gets no comments and cannot add, reply, edit, react, resolve or delete. That is stricter than Clips' own delete-comment, which lets an author delete with comments off.
  • When every placement of a Clip is trashed, document readers lose access. The owner never does.
  • A pasted Clip (a link placement) grants nothing; the Clip's own sharing decides. A2 writes no link rows, since nothing needs them yet.

How access is granted. Rather than teach each comment action about documents, the Content routes put the document's placement in scope for the request, and a host access resolver registered for recordings turns an active upload placement into a viewer role for that one recording. resolveAccess, assertCommentAccess and the existing comment actions then run unchanged. The resolver only adds: it grants viewer only when the reader has no access of their own, so a reader with an editor share keeps it, and it never denies. It applies only when the request's identity is the reader Content vouched for, so a nested request for someone else cannot inherit the grant. A signed-in reader acts in their active Clips workspace, as their own Clips session would, so a Clip shared with that workspace opens too.

Why routes, not actions. A peer can call actions with an A2A token through invokeAgentAction, but that path is read-only, maps the token to a signed-in user, and has no per-recording fence or signed-out caller. These calls need all three. They follow the private-icon precedent: a dedicated route that accepts a scoped A2A token (templates/content/server/lib/private-icon-authority.ts, packages/dispatch/src/server/lib/private-icon-assets.ts).

What changed

  • recording_content_bindings (migration 95; main took 93 for meeting capture and 94 for agent frames): one row per recording, Content app and document, with block id, origin upload | link, state active | trashed, creator and timestamps. state has no CHECK constraint, so the lifecycle step (A6) can add states without a migration, and state_changed_at lets it ignore a late transition. A duplicated document gets its own row.
  • shared/content-clip-contract.ts: the token contract Content mints against, including the exact signA2AToken call Content makes with CONTENT_CLIPS_PEER_SECRET. Audience <Clips URL>/content-clips, one scope, and document_id + recording_id claims. Scopes: content-clip:read (signed-in: playback and comment list), content-clip:comment (signed-in: comment writes), and content-clip:public-read for a signed-out reader of a public document, whose subject must be the fixed content-public-doc-reader. It never carries an email and never writes.
  • Routes: GET /api/content-clips/:recordingId/playback, GET …/comments, POST …/comments (add, reply, edit, delete, resolve, react). Every token problem is the same bare 401. A Clip the reader cannot open is a 404, never a 403. While Clips has no usable peer secret, every route answers 503 with code: "content_clips_not_configured" and reads or writes nothing.
  • Playback returns duration, dimensions, poster, the comments-enabled flag, and Clips media URLs carrying a five-minute signed grant that names the reader. The grant is signed with a key derived from CONTENT_CLIPS_PEER_SECRET, not the app's own server secret, which a workspace derives from A2A_SECRET. /api/video and /api/thumbnail use that reader in place of the browser's session and re-run every gate on each request. Trashing the placement or the Clip stops a URL that was already handed out. Under a grant, a Clip the reader cannot open is a 404, like a grant that does not verify for that recording; a 403 with code: "content_grant_expired" means only that the grant ran out.
  • Refresh contract for Content's player (A3), in shared/content-clip-contract.ts: when playback.expiresAt passes or that code comes back, ask Content for playback again. Content re-checks the reader's document access before it mints the token, so a reader who loses the document stops playing within five minutes. A YouTube-backed Clip is the one exception: its URL is YouTube's own embed, checked only when playback answers.
  • Earlier-screen-time footage attached to a Clip is a 404 through these routes; it plays only through its Clip.
  • Core: the clips.contentPeerSecret field and its resolver, and the includeSignedOut option; see the framework changes above. docs/environment-variables.md is regenerated.
  • The Clips video-sharing skill gains a short section; the agent's raw-SQL tool denylists the new table.

Safety and operations

  • Schema: additive table and indexes only; no backfill. Rolling back the code leaves an unused table.
  • Trust: only a token signed with CONTENT_CLIPS_PEER_SECRET is accepted. Tokens signed with the deployment A2A_SECRET or a workspace's A2A secret are refused, so no other app in the deployment can pose as Content. The media grants are bound the same way, so an A2A_SECRET holder cannot mint a grant that plays a Clip as its owner or as a document reader. The token's issuer must still equal the Content app stored on an active placement for that document and recording, and only Clips writes placements. The issuer-bound credential that earlier drafts listed as a core follow-up is done here for Content and Clips. One value binds one Content-Clips pairing: a Content deployment that holds it can name any Content issuer, so give each pairing its own value.
  • Deploy: set CONTENT_CLIPS_PEER_SECRET to the same value (at least 32 characters, not A2A_SECRET) on both the Content and the Clips deployments before the feature works anywhere but locally. Until then these routes refuse with the typed 503, and Content's Clip UI stays behind the content.clip-embed flag.
  • Exposure: the comment list includes author and reaction emails, as Clips' public share page already does. A signed-out reader of a public document receives them through Content.
  • Rate limits: per-instance and in memory, like view-event: 600 reads and 30 writes a minute per reader, document and Clip. Clips counts every signed-out reader of a public document as one reader, so they share one 600-read budget per Clip. Before A6 turns content.clip-embed on, Content limits signed-out reads per visitor and signs an opaque visitor key into content-clip:public-read tokens, and Clips keys signed-out budgets on that key.
  • Upload tickets (A4) are untouched; these routes add no CORS headers.
  • Four related issues found along the way are tracked separately and not changed here: core's A2A verifier accepts HS384 and HS512 tokens signed with the shared secret, and three edge cases in existing comment and workspace-lookup code.

Verification

  • server/lib/content-clips.test.ts (new, 76 cases) runs real sharing, real token verification and the real media routes against an in-memory Postgres. It covers active, missing and trashed placements; trashed, expired and password-protected Clips, including a password added after a URL was handed out; the owner's exceptions; duplicated documents; pasted Clips falling back to their own sharing, by person, workspace or public link; a reader's own editor role surviving a placement; a reader in several workspaces, using core's real active-workspace lookup on both the Content and media routes; comments off for signed-in and signed-out readers; anonymous tokens that try to write or reach another recording or document; empty, swapped and expired media grants; a reader who lost the document getting no fresh grant; footage ids; comment ids from another recording for every comment write; wrong-audience, wrong-scope, missing-claim, expired, future-dated and long-lived tokens; missing or mistyped exp, iat and nbf; on a fake clock, one second either side of the token's one-minute expiry, its five-minute cap, its one-minute issue-time skew and the media grant's five minutes; tokens signed with the deployment A2A_SECRET or a workspace's secret; a peer secret that is unset, too short or equal to A2A_SECRET, which answers 503 on every route and writes no comment; and media grants signed with any key but the one derived from the peer secret. Each case was written before the code that makes it pass. Accepting the deployment secret again fails 2 cases, signing media grants with the app's server secret fails 1 (a forged grant plays the Clip as its owner), and dropping Clips' includeSignedOut fails 3. Before this change, breaking the resolver failed 24 cases, accepting a media grant that fails verification failed 3, and ignoring a placement's state failed 4.
  • Core: sharing/access.spec.ts covers a resolver without the opt-in never being asked about a signed-out request, and one with it being asked; breaking the opt-in check fails 1 case. app-config/clips.spec.ts covers the env alias, unset, empty and whitespace values counting as unset, and refusing a value that is too short or equal to A2A_SECRET. short-lived-token.spec.ts covers a token signed with an explicit key failing against the server secret and the other way round, and an empty explicit key throwing. Core's a2a, app-config and sharing specs plus the short-lived-token spec: 910 pass.
  • Clips and core typechecks pass.
  • Full Clips suite: 4,125 tests pass. The video and thumbnail route tests stub every core entry their imports reach, so they now also stub @agent-native/core/a2a. Two video-frame.test.ts cases fail on Windows only, because Windows cannot spawn their shell-script ffmpeg stub; they don't touch this change.
  • pnpm guards: all 93 checks pass, as do guard:i18n-catalogs and guard:i18n-changed-copy. On Windows, dead-settings-keys needs GNU grep on PATH.
  • Independent review, four rounds (Codex Sol). Round 1 found six issues; five are fixed with a test each (delete with comments off, an editor share lowered to viewer, workspace shares ignored, a password unlock on media URLs, an empty media grant falling back to the browser's session), and the sixth is the HS384/HS512 item above. Round 2 found three, all fixed: the video route answered 403 where the thumbnail answered 404 for a Clip a grant cannot open, footage ids were admitted at the document boundary but judged differently by the media routes, and the YouTube exception was undocumented. Round 3 reviewed the peer secret and found two, both fixed with a test: an explicitly empty value in app config threw instead of counting as unset, and media grants were still signed with the app's server secret, which a workspace derives from A2A_SECRET, so a holder of that secret could forge one. Round 4 found nothing. Builder's review bot asked for a shorter media grant (done), questioned the shared secret (see Trust) and asked that signed-out host resolution be opt-in (done).

Also on this branch

Review focus

  • verifyPeer (server/lib/content-clip-peer.ts): it refuses with the 503 before reading the token when the peer secret is unusable, then verifies with that secret alone. Also the scope per route, the fixed anonymous subject, the claim fences and the five-minute lifetime cap.
  • The media grant key (server/lib/content-peer-secret.ts, server/lib/content-playback-grant.ts): derived from the peer secret, and a grant arriving while the secret is unusable is a 503, never a fallback to the browser's session.
  • The resolver (server/lib/content-doc-grant.ts): its identity check and the nested lookup of the reader's own access with the placement switched off. In core, the includeSignedOut check in packages/core/src/sharing/access.ts.
  • The five-minute media grant and its refresh contract, which Content's player (A3) will implement.

🤖 Generated with Claude Code

builder-io-integration[bot]

This comment was marked as outdated.

@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Here's a visual recap of what changed:

Visual recap

Open the full interactive recap

@3mdistal

Copy link
Copy Markdown
Contributor Author

A second independent review round (Codex Sol) found three issues. All are fixed in 174a4c5, with tests:

  • Under a Content grant, the video route answered 403 for a Clip the reader could not open, while the thumbnail answered 404. Both now answer 404, and a 403 under a grant means only that the grant expired.
  • Earlier-screen-time footage ids got in at the document boundary, but the media routes judged them by a different Clip's rules. Content routes now refuse footage ids; footage plays only through its Clip.
  • A YouTube-backed Clip's videoUrl is YouTube's own embed URL, with no grant. The contract now documents that exception, and a test pins it.

Also added: a cross-recording check for every comment write (reply, edit, delete, resolve, react), plus a token issued in the future.

Not changed: the review bot's note that video range requests repeat the workspace and placement lookups. That is about two small indexed reads per range request, on top of the per-request session and org lookups the signed-in path already does. Caching them would let a revoked placement keep serving until the cache expires.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

@3mdistal

3mdistal commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor Author

The review's LOW items, about core's process-wide workspace caches carrying over between tests, are fixed test-side only, in 954b784 and c59ca97:

  • Each test that gives a reader workspaces now gets a new reader email at the moment the memberships are written (readerInWorkspaces). Core keys its cached memberships and its cached active-workspace choice by email, so no entry can carry over into another test. The per-test invalidateMemberOrgCaches() from 954b784 is gone: it didn't clear the active-workspace cache, and fresh readers cover both caches.
  • The tests set the active workspace through core's putUserSetting, not raw SQL, so a switch partway through a test drops core's cached choice the way a real switch does.

Today's routes don't read through those caches: the reader's workspace comes from resolveOrgIdForEmail, whose caches last one request, and a query log of the workspace tests shows core's cached membership query never runs. A throwaway probe confirmed the gap and the fix. It read core's active-workspace cache across two tests using the workspace tests' reader email: on 954b784 the second test got the first test's choice, and on c59ca97 each test saw only its own. The workspace tests also passed when run twice in one process, the second time in reverse order.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

…cess opt-in

Clips' Content routes now verify tokens with CONTENT_CLIPS_PEER_SECRET
alone, a core config field read through resolveContentClipsPeerSecret.
Tokens signed with the deployment A2A_SECRET or a workspace secret are
refused, and an unset or unusable secret answers a typed 503
(content_clips_not_configured) without reading or writing anything.
Media grants are signed with a key derived from the peer secret, through
a new signingKey option on core's short-lived tokens.

Host access resolvers are asked about signed-out requests only when they
register with { includeSignedOut: true }; Clips is the only one that does.
builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

Base automatically changed from t3/clips-comment-visibility to main October 27, 2026 12:34
builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

@3mdistal

Copy link
Copy Markdown
Contributor Author

Recap of Builder's review of ff32560:

  • LOW, SSRF-blocked media URL answers an uncoded 403 under a Content grant: fixed in cf5ed5f. Under a grant, the video route now answers a media URL the server blocks with 502, the same status any other source it can't fetch gets. Both 403 paths are covered: the thrown SSRF error and the private-address check in the provider fetch. A 403 under a grant again means only an expired grant. The new case in content-clips.test.ts points a bound Clip at http://127.0.0.1/…. It answered 403 before the fix and 502 after.

  • The shared anonymous-reader rate-limit bucket: unchanged since the earlier thread. That thread explains why it stays as it is.

  • Claude Code AI

builder-io-integration[bot]

This comment was marked as outdated.

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder reviewed your changes and found 2 potential issues 🟡

Review Details

Incremental review summary

This update adds a regression test and adjusts the video proxy to translate upstream 403 responses to 502 when a Content media grant is present, preserving 403 for the explicit expired-grant signal. I assessed the PR as high risk because it introduces cross-app authentication and document-scoped access. The implementation of the new remap is sound, and the previous upstream-403 finding is now fixed; I resolved its review thread before this submission. Three independent review passes identified two further concerns: anonymous comment readers can receive commenter email addresses, and the process-local rate-limit bucket cap rejects every new key while the table is full. The previously reported shared anonymous-reader bucket remains open and unchanged, so it is not repeated. No tests were run independently; the PR has no UI changes, so browser testing is skipped.

Findings

  • 🟡 MEDIUM: Public-document readers receive raw comment rows, including commenter email addresses.
  • 🟡 MEDIUM: Once 5,000 unexpired rate-limit buckets exist, new keys across tenants receive 429 until a bucket expires.

🧪 Browser testing: Skipped — PR only modifies backend/config/docs/tests, no UI impact.

Comment thread templates/clips/server/lib/content-clip-peer.ts
Comment thread templates/clips/server/lib/content-clip-peer.ts

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder reviewed your changes and has a few items to flag 🟡

Review Details

Incremental review summary

This update adjusts the Clip binding migration to version 95 after main added migration 94, adds a regression test for provider-side 403 handling, and ensures a provider 403 under a Content grant is treated as a 502 rather than an expired grant. I assessed the PR as high risk because it changes authentication and media authorization. The dedicated peer secret, document-scoped grant checks, and comment actions remain sound in the reviewed paths; the prior upstream-403 finding is fixed and its thread was resolved. One low-severity consistency issue remains: the peer path normalizes the reader email, but the existing delete action compares comment author email case-sensitively, unlike edit/resolve. The two already-open findings (anonymous commenter email exposure and full rate-map rejection) remain unchanged and are not reposted. No tests were run independently.

🧪 Browser testing: Skipped — PR only modifies backend/config/docs/tests, no UI impact.

@3mdistal
3mdistal merged commit 1a39024 into main Oct 27, 2026
49 of 50 checks passed
@3mdistal
3mdistal deleted the clips-content-bindings branch October 27, 2026 15:14
@3mdistal

Copy link
Copy Markdown
Contributor Author

The last review's body-only finding (LOW: delete-comment compared the author's email case-sensitively, unlike edit and resolve) is fixed in #8147, in b090b44: it now uses sameOwnerEmail, with a test that fails before the fix. I kept it out of this PR so its checks didn't restart just before merging.

  • Claude Code AI

3mdistal added a commit that referenced this pull request Oct 27, 2026
…anage-draft

Main's #8090 fixed the prompt-surface snapshot this branch had repaired,
so the conflict takes main's version and the file leaves this diff.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants