Repository navigation
Let Content document readers play and comment on Clips uploaded to the document - #8090
Conversation
|
Here's a visual recap of what changed: Open the full interactive recap |
|
A second independent review round (Codex Sol) found three issues. All are fixed in 174a4c5, with tests:
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. |
|
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:
Today's routes don't read through those caches: the reader's workspace comes from |
…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.
# Conflicts: # docs/environment-variables.md # packages/core/src/server/index.ts # templates/clips/server/plugins/db.ts
|
Recap of Builder's review of ff32560:
|
# Conflicts: # templates/clips/server/plugins/db.ts
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
The last review's body-only finding (LOW:
|
…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>

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_SECRETor 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(envCONTENT_CLIPS_PEER_SECRET), and one resolver both apps read it through:resolveContentClipsPeerSecret(getAppConfig().clips, getGlobalA2ASecret()). It returns null while the secret is unset, and throwsAppConfigurationErrorwhen the value is shorter than 32 characters or equal toA2A_SECRET, because a secret every app holds would prove nothing. Content signs with it (A3); Clips verifies with it and nothing else.signShortLivedTokenandverifyShortLivedTokenalso 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, soregisterHostAccessResolver(type, resolver, { includeSignedOut: true })also asks that resolver about signed-out requests, withctx.userEmailunset. 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:
delete-comment, which lets an author delete with comments off.linkplacement) grants nothing; the Clip's own sharing decides. A2 writes nolinkrows, 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
viewerrole for that one recording.resolveAccess,assertCommentAccessand the existing comment actions then run unchanged. The resolver only adds: it grantsvieweronly 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;maintook 93 for meeting capture and 94 for agent frames): one row per recording, Content app and document, with block id, originupload|link, stateactive|trashed, creator and timestamps.statehas no CHECK constraint, so the lifecycle step (A6) can add states without a migration, andstate_changed_atlets 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 exactsignA2ATokencall Content makes withCONTENT_CLIPS_PEER_SECRET. Audience<Clips URL>/content-clips, one scope, anddocument_id+recording_idclaims. Scopes:content-clip:read(signed-in: playback and comment list),content-clip:comment(signed-in: comment writes), andcontent-clip:public-readfor a signed-out reader of a public document, whose subject must be the fixedcontent-public-doc-reader. It never carries an email and never writes.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 withcode: "content_clips_not_configured"and reads or writes nothing.CONTENT_CLIPS_PEER_SECRET, not the app's own server secret, which a workspace derives fromA2A_SECRET./api/videoand/api/thumbnailuse 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 withcode: "content_grant_expired"means only that the grant ran out.shared/content-clip-contract.ts: whenplayback.expiresAtpasses 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.clips.contentPeerSecretfield and its resolver, and theincludeSignedOutoption; see the framework changes above.docs/environment-variables.mdis regenerated.video-sharingskill gains a short section; the agent's raw-SQL tool denylists the new table.Safety and operations
CONTENT_CLIPS_PEER_SECRETis accepted. Tokens signed with the deploymentA2A_SECRETor 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 anA2A_SECRETholder 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.CONTENT_CLIPS_PEER_SECRETto the same value (at least 32 characters, notA2A_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 thecontent.clip-embedflag.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 turnscontent.clip-embedon, Content limits signed-out reads per visitor and signs an opaque visitor key intocontent-clip:public-readtokens, and Clips keys signed-out budgets on that key.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 mistypedexp,iatandnbf; 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 deploymentA2A_SECRETor a workspace's secret; a peer secret that is unset, too short or equal toA2A_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'includeSignedOutfails 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.sharing/access.spec.tscovers 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.tscovers the env alias, unset, empty and whitespace values counting as unset, and refusing a value that is too short or equal toA2A_SECRET.short-lived-token.spec.tscovers a token signed with an explicit key failing against the server secret and the other way round, and an empty explicit key throwing. Core'sa2a,app-configandsharingspecs plus the short-lived-token spec: 910 pass.@agent-native/core/a2a. Twovideo-frame.test.tscases 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 doguard:i18n-catalogsandguard:i18n-changed-copy. On Windows,dead-settings-keysneeds GNUgreponPATH.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
agent-chat-plugin-surface.spec.tssnapshot was missing the attachment rule Send attached text files and SVGs to the model in full #8129 added, because Keep framework runtime tools out of delegated app calls #8140 regenerated it on an older base, so every PR's core fast tests failed. The snapshot now has that one line.sharing/index.tsno longer re-exportsHostAccessResolverOptions. Nothing imported it from there, and the mutation job refuses a changed file no spec imports.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.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.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, theincludeSignedOutcheck inpackages/core/src/sharing/access.ts.🤖 Generated with Claude Code