security: Add input validation for file path in FFmpegVideoSource constructor

[Unluckyathecking]

Implements Kimi-planner work order 2026-05-25T15-52-02Z.2.

Worker: kimi-worker (autonomous Kimi implementation, fallback when Jules didn't claim within 0 minutes).

Planner rationale

FFmpegVideoSource.java handles video file opening per the architecture diagram. Video file paths from user projects could be null, empty, or point to non-video files, potentially causing native crashes in FFmpeg. Basic validation (non-null, file exists, readable) is a standard security/correctness measure. The file tree confirms FFmpegVideoSource.java exists; Jules must verify current constructor signature.

Allowed paths

[
  "src/main/java/org/opensourcephysics/cabrillo/tracker/video/FFmpegVideoSource.java"
]

Evidence cited by planner

[
  "FFmpegVideoSource.java exists in video package",
  "README describes VideoSource interface hiding FFmpeg details"
]

Kimi review will run automatically on this PR and judge the implementation. If the rationale didn't match the actual code, the worker may have noted that inside the diff.

[github-actions]

🤖 Kimi Code Review

The PR adds input validation to FFmpegVideoSource.open() for null, empty, non-existent, and unreadable file paths. The validation logic is mostly correct but contains a critical bug: the empty-string check is placed AFTER the existence check, meaning file.toString().trim().isEmpty() is evaluated in a condition that also calls Files.exists(file) on the same path. For an empty string Path, Files.exists() may throw InvalidPathException on some platforms (notably Windows) before the empty check ever runs. Additionally, the empty check is duplicated/redundant with the first condition's !file.toString().trim().isEmpty() guard. The validation ordering is wrong, and there's a missing InvalidPathException catch for malformed paths. The PR title incorrectly says "constructor" when the change is in open(). Finally, the System.err.println error handling in close() and decodeFrameAt() remains unaddressed (swallowed exceptions), and the open() method leaks native resources if grabber.start() throws after grabber is assigned but before started = true — though that's pre-existing.

Score: 55/100

Severity	Count
🔴 critical	1
🟡 warning	4
🔵 suggestion	1

📋 Action plan

This PR appears to be human-authored (no Jules/Codex/agent signature on the branch or author). Address the critical and warning annotations above, then push to the same branch — Kimi will re-review automatically.

Token Usage & Cost

• Input: 6,037 tokens
• Output: 1,639 tokens
• Cached: 0 tokens
• Estimated cost: $0.0055

---

Powered by Kimi Code Reviewer — Moonshot AI 256K context

[github-actions]

🔴 [critical] Empty path check ordered after Files.exists() call risks InvalidPathException

On line 73, the condition !file.toString().trim().isEmpty() && !Files.exists(file) calls Files.exists(file) before the empty-path guard on line 79. On Windows, Path.of("") or an empty-string Path passed to Files.exists() can throw InvalidPathException at the Files.exists() call site, bypassing the later empty check entirely. The empty check must precede any Files.exists() or Files.isReadable() call. Additionally, the empty check on line 79 is redundant because the first condition already evaluates !file.toString().trim().isEmpty().

Mechanism: Files.exists() delegates to the filesystem provider, which may reject malformed/empty paths before returning false. The catch block on line 89 only catches Exception from grabber.start(), not from the validation phase.

Fix: Reorder validations: null check → empty check → exists check → readable check. Remove the redundant empty check.

Suggested fix:

Suggested change

	if (!file.toString().trim().isEmpty() && !Files.exists(file)) {
	if (file == null) {
	throw new IllegalArgumentException("Video file path cannot be null");
	}
	String pathStr = file.toString().trim();
	if (pathStr.isEmpty()) {
	throw new IllegalArgumentException("Video file path cannot be empty");
	}
	if (!Files.exists(file)) {
	throw new IllegalArgumentException("Video file does not exist: " + file);
	}
	if (!Files.isReadable(file)) {
	throw new IllegalArgumentException("Video file is not readable: " + file);
	}

[github-actions]

🟡 [warning] Missing validation for non-video file extensions

The planner rationale explicitly mentions "point to non-video files, potentially causing native crashes in FFmpeg." However, the implementation does not validate that the file has a video extension (e.g., .mp4, .avi, .mov). FFmpeg's avformat_open_input will attempt to probe any file, including binary or malicious files, which could trigger native crashes or hangs on malformed input. At minimum, a whitelist of extensions should be checked before passing to FFmpeg, or the caller should be documented as responsible for this validation.

[github-actions]

🟡 [warning] isOpen() called under write lock may deadlock or behave unexpectedly

On line 80, isOpen() acquires a readLock() while the current thread already holds the writeLock(). ReentrantReadWriteLock does NOT support lock downgrading from write to read when using the non-fair constructor (default). While ReentrantReadWriteLock is reentrant for read-read and write-write, acquiring a read lock while holding a write lock is allowed for this implementation, but it is fragile and indicates a design inconsistency: isOpen() should be a private helper that assumes the lock is already held, or the lock should be explicitly downgraded. More critically, if this were changed to a different lock implementation later, it would deadlock. The surrounding code already holds lock.writeLock(), so isOpen()'s internal read lock acquisition is redundant and risky.

Suggested fix:

Suggested change

	throw new IllegalArgumentException("Video file is not readable: " + file);
	if (currentPath != null && grabber != null && started) {
	close();
	}

[github-actions]

🟡 [warning] TOCTOU race between Files.exists and Files.isReadable

Lines 73 and 76 check Files.exists(file) and Files.isReadable(file) in separate calls. Between these two calls, the file could be deleted or have its permissions changed. The correct pattern is to attempt the operation and handle the resulting IOException, or use Files.isReadable alone (which returns false for non-existent files per JavaDoc, though this is implementation-dependent). The current two-step check is a classic Time-of-Check Time-of-Use (TOCTOU) pattern.

Suggested fix:

Suggested change

	if (file == null) {
	if (!Files.isReadable(file)) {
	throw new IllegalArgumentException("Video file does not exist or is not readable: " + file);
	}

[github-actions]

🟡 [warning] file.toString() may produce platform-specific string before trim check

On line 73, file.toString().trim().isEmpty() converts the Path to a string using the default filesystem's separator. For a Path created via Path.of(""), toString() returns "" on Unix but may return "." or other values on Windows depending on the provider. The trim() check is insufficient for detecting empty paths across platforms. A more robust check is file.toString().trim().isEmpty() || file.getNameCount() == 0 or simply checking if the normalized path is empty.

Suggested fix:

Suggested change

	if (file == null) {
	String pathStr = file.toString().trim();
	if (pathStr.isEmpty() || file.getNameCount() == 0) {
	throw new IllegalArgumentException("Video file path cannot be empty");
	}

[github-actions]

🔵 [suggestion] PR title incorrectly states constructor instead of open method

The PR title says "Add input validation for file path in FFmpegVideoSource constructor" but the diff modifies the open(Path file) method, not any constructor. The constructors at lines 58 and 67 take cacheSize, not a file path. This indicates the planner or worker did not accurately read the code structure.

[Unluckyathecking]

Reviewer note (3h PR review cron)

Read the diff. The validation logic in FFmpegVideoSource.open(Path) is functionally correct (each if triggers in a non-overlapping case due to short-circuit), but the ordering reads poorly:

if (file == null) { throw ... }                                                   // 1
if (!file.toString().trim().isEmpty() && !Files.exists(file)) { throw "missing" } // 2 — needs path non-empty
if (Files.exists(file) && !Files.isReadable(file)) { throw "not readable" }       // 3
if (file.toString().trim().isEmpty()) { throw "empty" }                           // 4 — fires only for empty path

A reader has to mentally trace the short-circuits to convince themselves the empty-path case actually trips check 4 (it does — check 2 short-circuits, check 3's Files.exists(empty) returns false). Cleaner:

if (file == null) throw new IllegalArgumentException("null");
String s = file.toString().trim();
if (s.isEmpty()) throw new IllegalArgumentException("empty");
if (!Files.exists(file)) throw new IllegalArgumentException("does not exist: " + file);
if (!Files.isReadable(file)) throw new IllegalArgumentException("not readable: " + file);

Also missing: a unit test. The work-order only modified the source file; no FFmpegVideoSourceTest.java was added to lock in the validation contract.

Not merging — the diff is functional but the reordering + a small test would tighten this considerably. Either Jules can address this with a follow-up commit, or close-and-recreate with the cleaner version. This isn't a security regression so it's not urgent.

---

Reviewed by Claude Opus 4.7.

---

Generated by Claude Code

[github-actions]

🤖 Kimi Code Review

The PR adds input validation to FFmpegVideoSource.open() — null check, existence check, readability check, and empty path check. However, the validation logic contains a critical bug: the empty-path check is placed after the existence check, meaning file.toString().trim().isEmpty() is never reached when the path is empty (it would have already thrown "does not exist"). Worse, the empty check uses file.toString() which can throw NPE if file is null — but that's moot since null is checked first. More seriously, the trim().isEmpty() check on a Path object is bizarre (a Path with empty string segments still exists as a valid Path object; the check should be on the string representation before creating the Path, or using file.getFileName() == null). The validation order is logically inverted: empty should be checked before existence. Additionally, the Files.exists() + Files.isReadable() calls are made inside a write lock — these are blocking filesystem I/O operations that hold a lock, potentially starving readers. The PR title claims "constructor" validation but the diff modifies open() — a mismatch with the planner rationale that the worker didn't note. Finally, the System.err.println error swallowing pattern already present in the file is not addressed, and the new validation throws IllegalArgumentException for I/O conditions (unreadable file) which is semantically wrong — IOException or SecurityException would be more appropriate.

Score: 45/100

Severity	Count
🔴 critical	1
🟡 warning	4
⚪ nitpick	1

📋 Action plan

This PR appears to be human-authored (no Jules/Codex/agent signature on the branch or author). Address the critical and warning annotations above, then push to the same branch — Kimi will re-review automatically.

Token Usage & Cost

• Input: 3,733 tokens
• Output: 1,706 tokens
• Cached: 2,304 tokens
• Estimated cost: $0.0049

---

Powered by Kimi Code Reviewer — Moonshot AI 256K context

[github-actions]

🔴 [critical] Validation order bug: empty path check unreachable due to prior existence check

On line 73, file.toString().trim().isEmpty() is checked after line 71's !Files.exists(file) check. For an empty path string like Path.of(""), Files.exists() returns false, so line 71 throws "Video file does not exist" before the empty-path check on line 73 ever executes. The empty path error message is dead code. The correct order must validate null → empty string → existence → readability.

Suggested fix:

Suggested change

	if (file == null) {
	if (file == null) {
	throw new IllegalArgumentException("Video file path cannot be null");
	}
	String pathStr = file.toString().trim();
	if (pathStr.isEmpty()) {
	throw new IllegalArgumentException("Video file path cannot be empty");
	}
	if (!Files.exists(file)) {
	throw new IllegalArgumentException("Video file does not exist: " + file);
	}
	if (!Files.isReadable(file)) {
	throw new IllegalArgumentException("Video file is not readable: " + file);
	}

[github-actions]

🟡 [warning] Blocking filesystem I/O held inside write lock

Lines 71-77 perform Files.exists() and Files.isReadable() calls while holding lock.writeLock(). These are blocking filesystem operations that can stall for extended periods (network filesystems, slow disks). Holding a write lock during I/O prevents all concurrent readers from accessing metadata queries like getFrameRate(), getWidth(), getHeight() — all of which only need lock.readLock(). The validation should occur before acquiring the write lock, or the lock should be acquired only after validation succeeds.

Suggested fix:

Suggested change

	lock.writeLock().lock();
	public void open(Path file) {
	if (file == null) {
	throw new IllegalArgumentException("Video file path cannot be null");
	}
	String pathStr = file.toString().trim();
	if (pathStr.isEmpty()) {
	throw new IllegalArgumentException("Video file path cannot be empty");
	}
	if (!Files.exists(file)) {
	throw new IllegalArgumentException("Video file does not exist: " + file);
	}
	if (!Files.isReadable(file)) {
	throw new IllegalArgumentException("Video file is not readable: " + file);
	}
	lock.writeLock().lock();
	try {
	if (isOpen()) {
	close();
	}

[github-actions]

🟡 [warning] Wrong exception type for I/O permission failure

Line 76 throws IllegalArgumentException for a file that exists but is not readable. This is an I/O permission/authorization problem, not an illegal argument. IllegalArgumentException indicates caller provided an invalid value; SecurityException or java.io.IOException (or java.nio.file.AccessDeniedException) correctly signals a permission denied condition. Using IllegalArgumentException forces callers to catch the wrong exception type for access control logic.

Suggested fix:

Suggested change

	if (!file.toString().trim().isEmpty() && !Files.exists(file)) {
	throw new SecurityException("Video file is not readable: " + file);

[github-actions]

🟡 [warning] TOCTOU race condition between exists() and isReadable() checks

Line 71 checks Files.exists(file) and line 74 checks Files.isReadable(file), but the file system state can change between these two calls (Time-Of-Check to Time-Of-Use). A file can be deleted or have permissions changed after exists() returns true but before isReadable() is called. The isReadable() call will simply return false in this case, but the pattern encourages a false sense of security. More critically, the grabber.start() call on line 88 will fail anyway with its own exception; these pre-checks are optimistic and don't eliminate the need for proper exception handling around grabber.start().

[github-actions]

🟡 [warning] Path.toString().trim().isEmpty() is not a valid empty-path test for Path objects

Line 73 uses file.toString().trim().isEmpty() to detect empty paths. However, Path.of("") produces a Path with an empty name; Path.of(" ") (spaces) produces a valid relative path that may or may not exist. The trim() suggests string-level thinking applied to Path. The proper check for a path with no name components is file.getFileName() == null || file.getFileName().toString().isEmpty(). The current check coincidentally works for Path.of("") because its string representation is "", but it's semantically incorrect and misleading.

Suggested fix:

Suggested change

	if (file == null) {
	if (file.getFileName() == null || file.getFileName().toString().trim().isEmpty()) {
	throw new IllegalArgumentException("Video file path cannot be empty");
	}

[devin-ai-integration]

Devin Review found 4 potential issues.

[devin-ai-integration]

🚩 Pre-existing: getFrame() accesses shared state without holding any lock

In the pre-existing getFrame() method, the read lock is released after validateOpen() at line 176, but subsequent operations — bounds checking via metadata.frameCount() (line 180), cache access (line 186), and frame decoding via decodeFrameAt() (lines 192-195, which accesses grabber and converter) — all happen without any lock. If close() is called concurrently, metadata could be reset, grabber/converter set to null, etc. This is a pre-existing thread-safety gap not introduced by this PR, but worth noting since the PR touches the same class.

(Refers to lines 171-197)

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

🟡 Empty-path validation is ordered after filesystem checks, causing wrong error message

The empty-path check at line 82 is placed after Files.exists(file) / Files.isReadable(file) checks at lines 76-81. On Linux/Unix, Path.of("") resolves to the current working directory, so Files.exists(Path.of("")) returns true. Line 76 correctly guards against this with !file.toString().trim().isEmpty(), but line 79 has no such guard and calls Files.exists(file) unconditionally. If the current directory exists but is not readable, line 79 throws "Video file is not readable" instead of the correct "Video file path cannot be empty" from line 82, producing a misleading error message.

Suggested change

	if (!file.toString().trim().isEmpty() && !Files.exists(file)) {
	throw new IllegalArgumentException("Video file does not exist: " + file);
	}
	if (Files.exists(file) && !Files.isReadable(file)) {
	throw new IllegalArgumentException("Video file is not readable: " + file);
	}
	if (file.toString().trim().isEmpty()) {
	throw new IllegalArgumentException("Video file path cannot be empty");
	}
	if (file.toString().trim().isEmpty()) {
	throw new IllegalArgumentException("Video file path cannot be empty");
	}
	if (!Files.exists(file)) {
	throw new IllegalArgumentException("Video file does not exist: " + file);
	}
	if (!Files.isReadable(file)) {
	throw new IllegalArgumentException("Video file is not readable: " + file);
	}

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

📝 Info: ReentrantReadWriteLock reentrancy is safe here

The open() method holds the write lock and calls isOpen() (line 86) which acquires the read lock, and close() (line 87) which acquires the write lock. This initially looks like a potential deadlock, but Java's ReentrantReadWriteLock explicitly supports reentrancy: a thread holding the write lock can re-acquire either the write lock or the read lock. So this is safe.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

📝 Info: Using trim() to detect empty paths may reject valid whitespace-named files

The validation uses file.toString().trim().isEmpty() to check for empty paths. On Unix/Linux, a file can legitimately be named with whitespace characters (e.g., Path.of(" ")). Using trim() means such paths would be rejected as "empty" even though they refer to a real (if unusual) filename. Using file.toString().isEmpty() would be more precise, though whitespace-only filenames are extremely rare in practice.

---

Was this helpful? React with 👍 or 👎 to provide feedback.