fix(ipc): allow media in configured recordings directory#755
Open
RyderAsKing wants to merge 1 commit into
Open
fix(ipc): allow media in configured recordings directory#755RyderAsKing wants to merge 1 commit into
RyderAsKing wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughMedia-path authorization now allowlists files beneath the runtime-configured recordings directory using canonical paths, while retaining the existing local-read fallback. A Vitest case verifies approval for a configured recordings file. ChangesRuntime recordings authorization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ipc/project/manager.ts`:
- Around line 103-112: Update the recordings-directory check in the
path-allowance flow to canonicalize candidatePath before evaluating
isPathInsideDirectory, ensuring symlinks resolve within the configured
recordings root rather than bypassing isAllowedLocalReadPath’s canonical guard.
Preserve the existing fallback behavior for unavailable real paths, and add a
regression test covering a symlink under the recordings directory targeting a
file outside it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0376398a-295a-4cb4-96ef-57e722021899
📒 Files selected for processing (2)
electron/ipc/project/manager.test.tselectron/ipc/project/manager.ts
Comment on lines
+103
to
+112
| // The recordings location can be changed in settings, so the compile-time | ||
| // default RECORDINGS_DIR alone is not sufficient for media created there. | ||
| // Canonicalize the configured root as well as the candidate to retain the | ||
| // symlink protection enforced by isAllowedLocalReadPath. | ||
| const recordingsDir = await getRecordingsDir(); | ||
| const canonicalRecordingsDir = await fs | ||
| .realpath(recordingsDir) | ||
| .catch(() => normalizePath(recordingsDir)); | ||
| if (isPathInsideDirectory(normalizedCandidatePath, canonicalRecordingsDir)) { | ||
| return true; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant files and nearby context.
git ls-files 'electron/ipc/project/*' 'electron/**/*manager*' 'electron/**/*media*' | sed -n '1,120p'
printf '\n--- manager.ts outline ---\n'
ast-grep outline electron/ipc/project/manager.ts --view expanded || true
printf '\n--- manager.ts relevant lines ---\n'
sed -n '1,220p' electron/ipc/project/manager.ts
printf '\n--- search for helper definitions/usages ---\n'
rg -n "isAllowedLocalMediaPath|isAllowedLocalReadPath|resolveApprovedLocalMediaPath|getRecordingsDir|isPathInsideDirectory|normalizePath" electron -SRepository: webadderallorg/Recordly
Length of output: 18310
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' electron/ipc/project/manager.ts
printf '\n---\n'
rg -n "isAllowedLocalMediaPath|isAllowedLocalReadPath|resolveApprovedLocalMediaPath" electron -SRepository: webadderallorg/Recordly
Length of output: 11154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- manager.test.ts symlink-related cases ---'
sed -n '1,220p' electron/ipc/project/manager.test.ts
printf '\n%s\n' '--- filesystem probe for the fast-path behavior ---'
node - <<'JS'
const fs = require('node:fs');
const path = require('node:path');
const os = require('node:os');
function normalizePath(p) {
return path.normalize(p);
}
function isPathInsideDirectory(candidatePath, directoryPath) {
const normalizedCandidatePath = normalizePath(candidatePath);
const normalizedDirectoryPath = normalizePath(directoryPath);
return (
normalizedCandidatePath === normalizedDirectoryPath ||
normalizedCandidatePath.startsWith(`${normalizedDirectoryPath}${path.sep}`)
);
}
const base = fs.mkdtempSync(path.join(os.tmpdir(), 'recordly-probe-'));
const recordings = path.join(base, 'recordings');
const outside = path.join(base, 'outside');
fs.mkdirSync(recordings);
fs.mkdirSync(outside);
const target = path.join(outside, 'track.wav');
fs.writeFileSync(target, 'x');
const symlink = path.join(recordings, 'track.wav');
fs.symlinkSync(target, symlink);
const canonicalRecordingsDir = fs.realpathSync(recordings);
const normalizedCandidatePath = normalizePath(symlink);
const candidateReal = fs.realpathSync(normalizedCandidatePath);
console.log(JSON.stringify({
recordings,
outside,
symlink,
target,
canonicalRecordingsDir,
normalizedCandidatePath,
candidateReal,
fastPathResult: isPathInsideDirectory(normalizedCandidatePath, canonicalRecordingsDir),
realPathInsideRecordings: isPathInsideDirectory(candidateReal, canonicalRecordingsDir),
}, null, 2));
JSRepository: webadderallorg/Recordly
Length of output: 8878
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the existing tests around media authorization and symlinks.
sed -n '1,220p' electron/ipc/project/manager.test.tsRepository: webadderallorg/Recordly
Length of output: 8265
Canonicalize candidatePath before the recordings-dir fast path. A symlink inside the configured recordings directory can satisfy the lexical prefix check here while resolving outside the allowlist, so this bypasses isAllowedLocalReadPath’s canonical-path guard. Add a regression test for a symlink under the recordings directory that points to a file outside it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/ipc/project/manager.ts` around lines 103 - 112, Update the
recordings-directory check in the path-allowance flow to canonicalize
candidatePath before evaluating isPathInsideDirectory, ensuring symlinks resolve
within the configured recordings root rather than bypassing
isAllowedLocalReadPath’s canonical guard. Preserve the existing fallback
behavior for unavailable real paths, and add a regression test covering a
symlink under the recordings directory targeting a file outside it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Allow media in configured recordings directory
Description
Fixes Recordly blocking audio/media files stored in a user-configured recordings directory.
Motivation
The editor rejected recording audio files as “disallowed” when the recordings location was changed in settings. This prevented microphone and system audio tracks from loading.
Type of Change
Related Issue(s)
N/A
Screenshots / Video
N/A, this is a backend media-path validation fix with no UI changes.
Testing Guide
Automated validation:
Checklist
Summary by CodeRabbit