Skip to content

fix(ipc): confine browser recordings to the recordings directory#738

Merged
meiiie merged 2 commits into
mainfrom
fix/ipc-recording-path-confinement
Jul 10, 2026
Merged

fix(ipc): confine browser recordings to the recordings directory#738
meiiie merged 2 commits into
mainfrom
fix/ipc-recording-path-confinement

Conversation

@meiiie

@meiiie meiiie commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Description

Confines store-recorded-video writes to app-generated recording names under the configured
recordings directory. The IPC handler now treats fileName as untrusted runtime input, validates it
before the target write, and resolves the accepted path as a strict descendant of the recordings
root.

The accepted forms match the current primary and webcam recorder callers:

  • recording-<timestamp>.webm|mp4
  • recording-<timestamp>-webcam.webm|mp4

Motivation

The handler previously passed a renderer-supplied filename directly to path.join and then
fs.writeFile. Traversal components could therefore select a target outside the configured
recordings directory, while nested and platform-specific path forms had no explicit contract.
Main-process IPC boundaries must validate the renderer data they use for filesystem writes.

Type of Change

  • New Feature
  • Bug Fix
  • Refactor / Code Cleanup
  • Documentation Update
  • Other

Related Issue(s)

Architecture remediation trust-boundary finding. PR #737 establishes the standard typecheck/lint/test
workflow and contains the one unrelated main typecheck baseline fix.

Testing Guide

npx vitest --run electron/ipc/recording/storagePath.test.ts electron/ipc/project/manager.test.ts
npx biome lint electron/ipc/recording/storagePath.ts electron/ipc/recording/storagePath.test.ts electron/ipc/register/recording.ts
npm test

Results:

  • path-policy and project-manager tests: 35/35 passed;
  • focused Biome lint: passed;
  • full Vitest rerun: 97/97 files and 866/866 tests passed;
  • the existing exporter fallback test also passed 5/5 in isolation after one known full-suite import
    timeout;
  • changed code typechecks with --noUnusedLocals false; standard typecheck reaches only the unrelated
    unused import on current main, already removed by ci: add pull-request quality checks #737.

Risk and non-goals

The change preserves the existing result/error contract and both live caller filename forms. It is
limited to renderer-controlled filename traversal. It does not redesign preload capabilities, change
valid-name overwrite behavior, or claim protection against a hostile same-user process that can
pre-place filesystem symlinks.

Checklist

  • I have performed a self-review of my code.
  • Screenshots/videos are not applicable.
  • I added focused regression coverage for POSIX and Windows path spellings.
  • The change is limited to one IPC source-to-sink path.

Summary by CodeRabbit

  • Bug Fixes

    • Improved recorded video storage validation to reject invalid, malformed, absolute, and path-traversal filenames.
    • Prevented untrusted filename inputs from writing recordings outside the designated storage directory.
  • Tests

    • Added coverage for valid recording filenames and a wide range of invalid input types and patterns.

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d8d564bd-9ea6-4c37-b0cb-fc29f9e99cb7

📥 Commits

Reviewing files that changed from the base of the PR and between aea12b6 and e7283f0.

📒 Files selected for processing (3)
  • electron/ipc/recording/storagePath.test.ts
  • electron/ipc/recording/storagePath.ts
  • electron/ipc/register/recording.ts

📝 Walkthrough

Walkthrough

Changes

Recorded video path handling

Layer / File(s) Summary
Path validation and coverage
electron/ipc/recording/storagePath.ts, electron/ipc/recording/storagePath.test.ts
Adds strict filename validation, traversal checks, absolute path resolution, and tests for valid, unsafe, malformed, and non-string inputs.
Recording IPC integration
electron/ipc/register/recording.ts
Updates the store-recorded-video handler to accept unknown filenames and use the validated resolver before writing video data.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: confining recorded browser videos to the recordings directory.
Description check ✅ Passed The description covers the required purpose, motivation, change type, issue reference, testing guide, and checklist, with only screenshots/video left non-applicable.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/ipc-recording-path-confinement

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@meiiie
meiiie marked this pull request as ready for review July 10, 2026 19:55
@meiiie
meiiie merged commit 6889765 into main Jul 10, 2026
4 checks passed
@meiiie
meiiie deleted the fix/ipc-recording-path-confinement branch July 10, 2026 20:00
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.

1 participant