Repository navigation
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.
…o clips-content-bindings
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Review summary
Reviewed PR #8090 at 04d6cf0f1. The latest branch update includes share-route changes that stop polling after a missing-recording response; I found the private access-denied state and session-loading behavior remain distinct, and the new tests cover missing/trashed and private states. I also reviewed the Clips-side Content integration. The dedicated peer secret now prevents tokens signed with the deployment-wide A2A secret from impersonating Content, so I resolved the existing A2A review thread.
Risk: High because the PR changes authentication and access-control paths. One medium-severity availability issue remains: anonymous readers of a public Content document share one per-document/Clip read-rate-limit bucket, so ordinary traffic can exhaust it for everyone. The general design is sound in keeping Content access scoped to a single recording and rechecking media grants. No tests were run in this review. Browser verification was applicable to the share route, but the browser-test-planner agent is unavailable in this environment.
🧪 Browser testing: Skipped — browser-test-planner agent unavailable.
| const key = [ | ||
| kind, | ||
| peer.contentApp, | ||
| peer.documentId, | ||
| peer.recordingId, | ||
| peer.viewerEmail ?? CONTENT_CLIP_PUBLIC_READER, |
There was a problem hiding this comment.
🟡 Signed-out readers of a public doc share one 600/min read budget
All signed-out readers for the same Content app, document, and Clip share one read-rate-limit bucket because the key substitutes the same public-reader subject for every anonymous viewer. At 600 requests per minute, ordinary traffic of roughly 300 page views (each making playback and comments reads) can return 429 to every anonymous reader of that document until the window resets; consider per-viewer/IP limiting or a higher, separate anonymous-read budget.
There was a problem hiding this comment.
Leaving the code as is in this PR. The finding is right, and the fix is now named in Safety and operations.
- Why it's shared: Clips can't tell signed-out readers apart. Content sends no visitor identity for them, only the fixed
content-public-doc-readersubject, so the budget for "this reader, document and Clip" covers everyone reading that page. - Why not a bigger, separate signed-out budget here: it would be a second number to keep in step with Content. Either way, one visitor could still spend it for everyone.
- The fix, before A6 turns
content.clip-embedon:- Content limits signed-out reads per visitor, which was already an A6 prerequisite.
- Content signs an opaque visitor key into
content-clip:public-readtokens, never an email. - Clips keys signed-out budgets on that key, so a busy public page no longer spends one bucket.
- Today: the flag is off, so no signed-out reader reaches these routes. The limits are also per instance and in memory, like
view-event, so the shared bucket isn't global across instances.
…o clips-content-bindings
…o clips-content-bindings
…o clips-content-bindings
There was a problem hiding this comment.
Builder reviewed your changes and has a few items to flag 🟡
Review Details
Incremental review summary
Reviewed PR #8090 at head b7565d8fea. The PR’s diff against its base remains the Clips server/core integration for Content-scoped playback and comments, including peer-secret verification and short-lived media grants. The latest PR diff contains no additional UI changes; the current open issue about anonymous readers sharing a rate-limit bucket is unchanged and remains in its existing thread, so I did not repost it. The dedicated peer secret and explicit signed-out resolver opt-in remain in place.
I reviewed the latest branch updates as well. They concern separately merged share-route, reaction UI, and test changes; reviewer comments on those files are outside this PR’s diff and were not submitted here. I found no new actionable issue within the PR diff. Risk: High because the PR changes authentication and access-control paths. The browser-testing step was skipped because the current PR diff has no UI impact. Focused tests were not run in this review; a reviewer noted pnpm was unavailable in the environment.
🧪 Browser testing: Skipped — PR diff has no UI impact.

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. Stacked on #8059 and #8088; it will be rebased onto
mainonce those merge.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 93): 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).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