Repository navigation
feat(cli): inspect and trim a local Studio timeline - #3604
richardokonicha wants to merge 2 commits into
Conversation
|
@richardokonicha is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![superagent-security[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
| } | ||
| Ok(()) | ||
| } | ||
| DeepLinkAction::TrimClipEdge { |
There was a problem hiding this comment.
P2: New deep-link trim action lets any URL writer modify an open Studio project without app-side confirmation
New TrimClipEdge deep link applies and saves a trim without any app-side auth; the CLI-only confirmed flag can be bypassed.
Authenticate trims at the desktop boundary with a single-use nonce or in-app confirmation instead of trusting the caller-provided confirmed flag.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="apps/desktop-gpui/src/deeplink.rs">
<violation number="1" location="apps/desktop-gpui/src/deeplink.rs:650">
<priority>P2</priority>
<title>New deep-link trim action lets any URL writer modify an open Studio project without app-side confirmation</title>
<evidence>The PR adds a `TrimClipEdge` variant to the OS-supplied `DeepLinkAction` grammar and an execute arm that calls `apply_clip_trim_edge(...)` directly (lines 650-669), with no confirmation, authentication, or nonce check. `apply_clip_trim_edge` only canonicalizes the path, locates the matching open editor, and calls `view.apply_agent_clip_trim(...)` (lines 335-368). `apply_agent_clip_trim` then calls `edit_timeline_project`, which records history, schedules the debounced save, and writes the project (editor_window.rs lines 2448-2483 and 2528-2574). The `confirmed` boolean exists only in the CLI MCP tool (apps/cli/src/mcp.rs lines 1082-1090), so it cannot constrain a direct deep-link invocation. The deep-link module itself documents that this input arrives from the OS on behalf of whatever wrote the URL, meaning any local process or web page is the actor. The only integrity check is the caller-supplied `expected_current_seconds`, which is not a secret and is commonly 0.0 for the first clip's start edge.</evidence>
<recommendation>Authenticate the trim request at the desktop boundary rather than trusting the CLI-supplied confirmation. For example, have the desktop mint a single-use nonce that the CLI obtains through a privileged local channel and include it in the action, or require an in-app user confirmation for `TrimClipEdge`. Validate that the action came from the intended caller before mutating and saving the project.</recommendation>
</violation>
</file>
![greptile-apps[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (resolved - current).abs() > 0.001 { | ||
| persisted = Some(resolved); | ||
| break; |
There was a problem hiding this comment.
The disk check accepts any edge movement greater than 0.001 seconds as proof that this request succeeded. A pending manual edit can reach disk first, making the CLI return success without reading the trim's receipt—even if the editor later rejects it. Conversely, a valid 0.0005-second trim is saved but reported as EDITOR_NOT_PERSISTED.
Require this request's applied receipt, then compare the saved edge with its applied_seconds using a tolerance that supports accepted trim sizes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/cli/src/mcp.rs
Line: 1186-1188
Comment:
**Trim results can be wrong**
The disk check accepts any edge movement greater than 0.001 seconds as proof that this request succeeded. A pending manual edit can reach disk first, making the CLI return success without reading the trim's receipt—even if the editor later rejects it. Conversely, a valid 0.0005-second trim is saved but reported as `EDITOR_NOT_PERSISTED`.
Require this request's applied receipt, then compare the saved edge with its `applied_seconds` using a tolerance that supports accepted trim sizes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| let outcome = apply_clip_trim_edge( | ||
| &project_path, | ||
| clip_index, | ||
| edge, | ||
| time_seconds, | ||
| expected_current_seconds, | ||
| cx, | ||
| ); |
There was a problem hiding this comment.
The new TrimClipEdge URL edits and saves an open project without checking who sent it or asking the user. If its sender knows the open project path, a crafted cap-desktop://action link can bypass studio_timeline_trim's confirmation; an untrimmed clip's start is commonly 0. The current-edge check is not permission to edit.
Require a trusted local request token or show an editor confirmation before applying URL-driven trims.
How this was verified: OS-supplied URLs reach TrimClipEdge, which calls the editor's write path without a consent or caller check.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/deeplink.rs
Line: 658-665
Comment:
**Links edit projects without approval**
The new `TrimClipEdge` URL edits and saves an open project without checking who sent it or asking the user. If its sender knows the open project path, a crafted `cap-desktop://action` link can bypass `studio_timeline_trim`'s confirmation; an untrimmed clip's start is commonly 0. The current-edge check is not permission to edit.
Require a trusted local request token or show an editor confirmation before applying URL-driven trims.
**How this was verified:** OS-supplied URLs reach `TrimClipEdge`, which calls the editor's write path without a consent or caller check.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| let segment = timeline | ||
| .segments | ||
| .get(index) | ||
| .ok_or_else(|| format!("Clip index {index} is out of range"))?; | ||
| let current = match edge { | ||
| ClipTrimEdge::Start => segment.start, | ||
| ClipTrimEdge::End => segment.end, | ||
| }; | ||
| if (current - expected_current).abs() > 0.001 { |
There was a problem hiding this comment.
check_clip_trim_edge checks a position and one edge value, not which clip the caller inspected. If the user deletes an earlier clip or reorders clips after inspection, a different clip can occupy that position. Multiple recording clips start at 0, so a stale start-edge request can pass both checks and trim the wrong clip.
Carry an inspected timeline revision or clip identity through the request and check it in the editor before changing anything.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/editor_edits.rs
Line: 2117-2125
Comment:
**Stale requests trim another clip**
`check_clip_trim_edge` checks a position and one edge value, not which clip the caller inspected. If the user deletes an earlier clip or reorders clips after inspection, a different clip can occupy that position. Multiple recording clips start at 0, so a stale start-edge request can pass both checks and trim the wrong clip.
Carry an inspected timeline revision or clip identity through the request and check it in the editor before changing anything.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if timescale > 0. { | ||
| self.set_preview_time(clip_start + (resolved_seconds - current_seconds) / timescale); |
There was a problem hiding this comment.
The preview uses the edge's movement rather than its position in the updated clip. For an end trim from 20 to 18 seconds, this seeks to two seconds before the clip's start instead of its new end. Start trims also add their movement after the new start has already been committed.
Compute the preview from the updated clip's start and duration, with separate positions for the two edges.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/editor_window.rs
Line: 2478-2479
Comment:
**Preview shows the wrong frame**
The preview uses the edge's movement rather than its position in the updated clip. For an end trim from 20 to 18 seconds, this seeks to two seconds before the clip's start instead of its new end. Start trims also add their movement after the new start has already been committed.
Compute the preview from the updated clip's start and duration, with separate positions for the two edges.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.The review found four things worth fixing before this lands. A deep link is open to anything on the machine that can write a URL, so the CLI now leaves a single-use secret behind before it asks for anything: the app only trims a request that presents it, and the secret is spent with the request. A crafted link on its own changes nothing. A request named a position, not a clip, so a timeline that changed underneath it could put another clip at that position - and several clips start at 0. The clip's identity now travels with the request, and the editor refuses a clip that is not the one that was inspected. The CLI accepted any movement on disk as proof that its own trim had landed, so a manual edit that reached the file first could make it report success for an edit the editor refused. The receipt is now required, and the saved edge is compared with the value the editor reports. The preview seeked by the edge's movement rather than by where the edge ended up, so an end trim showed a frame before the clip instead of its new end. It now uses the same mapping the drag handles scrub with. The stale agent-installer expectation (2 changes) no longer matches the installer, which has shipped the cap-demo companion skill for a while: 15 files, one per file it writes.
Cap's CLI can already read a recording's metadata and export it. It cannot
change a timeline:
project-config.jsonbelongs to the editor, which rewritesthat file on every save, so an external write is lost and leaves the timeline on
screen disagreeing with the file.
This adds two local MCP tools for the editor that is already running:
studio_timeline_inspect- the clips of a.capproject that is open inStudio, with their bounds in recording seconds and an identity per clip.
studio_timeline_trim- moves one edge of one clip, after explicitconfirmation, with the inspected clip and its current edge value as guards.
The edit is applied by the editor, through
edit_timeline_project()- the samepath a manual drag takes - so the clamps, the undo history, the preview and the
debounced save all behave as they do for a person dragging the handle. The CLI
never writes the project file.
The deep-link channel has no reply path, so the editor leaves a receipt in the
app-data dir and the CLI reads it back: the caller learns whether the trim was
applied or refused, and why ("Clip edge is stale: expected 19.000s, current
value is 20.000s", "Finish the active timeline drag before applying a trim").
Changes
apps/cli/src/mcp.rs:1132-studio_timeline_inspect, and the sharedstudio_timeline_clips()helper that reads the clips, their bounds and theiridentity.
apps/cli/src/mcp.rs:1152-studio_timeline_trim: it checks the requestagainst the timeline it inspected (
validate_studio_trim(),apps/cli/src/mcp.rs:182), hands the edit to the running app, and onlyreports success once the app's receipt and the saved edge agree.
apps/cli/src/mcp.rs:186-apps/cli/src/mcp.rs:203- the request the CLIleaves behind: a single-use secret, one project, one trim.
crates/utils/src/agent_trim.rs:191- the request and receipt protocol theCLI and the app share, including
consume_request_in()(
crates/utils/src/agent_trim.rs:262), which spends the secret with therequest.
apps/desktop-gpui/src/deeplink.rs:144- thetrim_clip_edgeaction, withthe clip's identity and the secret it needs.
apps/desktop-gpui/src/deeplink.rs:376-authorize_trim_request(): a linkthat cannot present the CLI's secret changes nothing.
apps/desktop-gpui/src/editor_edits.rs:2121-check_clip_trim_edge(), whichrefuses a request whose clip is not the one that was inspected, and
trim_clip_edge()(apps/desktop-gpui/src/editor_edits.rs:2164), the clampsthe drag handles use.
apps/desktop-gpui/src/editor_window.rs:2418-apply_agent_clip_trim():the editor applies the edit, scrubs to the frame under the moved edge and
records the edit in the undo history.
crates/project/src/configuration.rs:1028-TimelineSegment::identity(),so a request names a clip and not only a position.
apps/cli/tests/cli.rs:979- the agent-installer's dry-run expectation, whichstill counted two changes while the installer writes fifteen files.
Testing
cargo test -p cap- 89 unit tests and 65 CLI tests pass.cargo test -p cap-utils- 95 pass, 3 ignored.cargo test -p cap-project- 204 pass, 4 ignored.cargo test -p cap-desktop-gpui- 1567 pass, 31 ignored. One pre-existingfailure,
devices::tests::paired_listings_agree_with_the_plain_ones, alsofails on an unmodified checkout: it reads the live window server.
cargo clippy -p cap -p cap-utils -p cap-project --all-targets -- -D warnings- clean.cargo clippy -p cap-desktop-gpui --all-targets --no-deps- no findings inthe changed files.
cargo fmt --check- clean for the root workspace; the changed desktopfiles are formatted with the toolchain that workspace pins.
Verification
moved in the editor, the project file held the new value, the undo history
had the edit, and the receipt said the editor had applied it.
refusal, and a request for a clip that had changed comes back as stale.
cap-desktop://actionlink without the CLI's secret is refused andchanges nothing.