fix: validate recording output paths - #1755
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens recording/replay-buffer startup validation by rejecting unsafe output directories (e.g., symlink/junction paths) using os_is_path_safe, aligning behavior with upstream OBS safety expectations.
Changes:
- Add
os_is_path_safevalidation for simple recording output directory before starting recording. - Add
os_is_path_safevalidation for simple and advanced replay buffer output directory before starting replay buffer. - Add
os_is_path_safevalidation for advanced recording output directory before building the final output file path.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| obs-studio-server/source/osn-simple-replay-buffer.cpp | Rejects unsafe/empty replay buffer directory before output update/start. |
| obs-studio-server/source/osn-simple-recording.cpp | Rejects unsafe/empty recording directory before constructing the final recording file path. |
| obs-studio-server/source/osn-advanced-replay-buffer.cpp | Rejects unsafe/empty replay buffer directory before output update/start. |
| obs-studio-server/source/osn-advanced-recording.cpp | Rejects unsafe/empty recording directory before constructing the final recording file path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (!recording->path.size()) { | ||
| if (!recording->path.size() || !os_is_path_safe(recording->path.c_str())) { | ||
| PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Invalid recording path."); |
There was a problem hiding this comment.
Done. Rather than reword the shared message, the check is now split so each branch says what actually tripped:
if (!recording->path.size()) {
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Invalid recording path.");
}
if (!os_is_path_safe(recording->path.c_str())) {
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Unsafe recording path: symbolic links and junctions are not allowed.");
}PRETTY_ERROR_RETURN puts the string in both the IPC return value and the log, so this reaches the desktop error and the support log. Applied consistently across all four files.
|
|
||
| obs_output_set_video_encoder(recording->GetOutput(), recording->videoEncoder); | ||
| if (!recording->path.size() || !os_is_path_safe(recording->path.c_str())) { | ||
| PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Invalid recording path."); |
There was a problem hiding this comment.
Done. Rather than reword the shared message, the check is now split so each branch says what actually tripped:
if (!recording->path.size()) {
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Invalid recording path.");
}
if (!os_is_path_safe(recording->path.c_str())) {
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Unsafe recording path: symbolic links and junctions are not allowed.");
}PRETTY_ERROR_RETURN puts the string in both the IPC return value and the log, so this reaches the desktop error and the support log. Applied consistently across all four files.
|
|
||
| if (!replayBuffer->path.size()) { | ||
| if (!replayBuffer->path.size() || !os_is_path_safe(replayBuffer->path.c_str())) { | ||
| PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Invalid recording path."); |
There was a problem hiding this comment.
Done. Rather than reword the shared message, the check is now split so each branch says what actually tripped:
if (!recording->path.size()) {
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Invalid recording path.");
}
if (!os_is_path_safe(recording->path.c_str())) {
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Unsafe recording path: symbolic links and junctions are not allowed.");
}PRETTY_ERROR_RETURN puts the string in both the IPC return value and the log, so this reaches the desktop error and the support log. Applied consistently across all four files.
|
|
||
| if (!replayBuffer->path.size()) { | ||
| if (!replayBuffer->path.size() || !os_is_path_safe(replayBuffer->path.c_str())) { | ||
| PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Invalid recording path."); |
There was a problem hiding this comment.
Done. Rather than reword the shared message, the check is now split so each branch says what actually tripped:
if (!recording->path.size()) {
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Invalid recording path.");
}
if (!os_is_path_safe(recording->path.c_str())) {
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Unsafe recording path: symbolic links and junctions are not allowed.");
}PRETTY_ERROR_RETURN puts the string in both the IPC return value and the log, so this reaches the desktop error and the support log. Applied consistently across all four files.
Splitting the empty-path and os_is_path_safe() checks lets the IPC error and the log line say which one tripped, so a rejected symlink/junction directory is diagnosable instead of just "Invalid recording path." Also switch osn-encoders.hpp to a quoted include, matching every other consumer of that header. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
014dd0f to
0387e73
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (8)
obs-studio-server/source/osn-simple-recording.cpp:406
os_is_path_safealso returns false when a path component is missing, inaccessible, malformed, or not a directory, so this message can incorrectly blame a symbolic link/junction for ordinary path failures. Make the message cover those failure modes as well.
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Unsafe recording path: symbolic links and junctions are not allowed.");
obs-studio-server/source/osn-advanced-recording.cpp:291
os_is_path_safealso returns false when a path component is missing, inaccessible, malformed, or not a directory, so this message can incorrectly blame a symbolic link/junction for ordinary path failures. Make the message cover those failure modes as well.
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Unsafe recording path: symbolic links and junctions are not allowed.");
obs-studio-server/source/osn-simple-replay-buffer.cpp:152
os_is_path_safealso returns false when a path component is missing, inaccessible, malformed, or not a directory, so this message can incorrectly blame a symbolic link/junction for ordinary path failures. Make the message cover those failure modes as well.
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Unsafe recording path: symbolic links and junctions are not allowed.");
obs-studio-server/source/osn-advanced-replay-buffer.cpp:210
os_is_path_safealso returns false when a path component is missing, inaccessible, malformed, or not a directory, so this message can incorrectly blame a symbolic link/junction for ordinary path failures. Make the message cover those failure modes as well.
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Unsafe recording path: symbolic links and junctions are not allowed.");
obs-studio-server/source/osn-simple-recording.cpp:406
- The simple-recording integration suite exercises
start()with normal paths, but no automated case verifies that this new branch rejects a Windows symlink/junction path and propagates the intended error. Add a regression case so this security boundary cannot silently disappear.
This issue also appears on line 406 of the same file.
if (!os_is_path_safe(recording->path.c_str())) {
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Unsafe recording path: symbolic links and junctions are not allowed.");
obs-studio-server/source/osn-advanced-recording.cpp:291
- The advanced-recording integration suite exercises
start()with normal paths, but no automated case verifies that this new branch rejects a Windows symlink/junction path and propagates the intended error. Add a regression case so this security boundary cannot silently disappear.
This issue also appears on line 291 of the same file.
if (!os_is_path_safe(recording->path.c_str())) {
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Unsafe recording path: symbolic links and junctions are not allowed.");
obs-studio-server/source/osn-simple-replay-buffer.cpp:152
- The simple replay-buffer integration suite exercises
start()with normal paths, but no automated case verifies that this new branch rejects a Windows symlink/junction path and propagates the intended error. Add a regression case so this security boundary cannot silently disappear.
This issue also appears on line 152 of the same file.
if (!os_is_path_safe(replayBuffer->path.c_str())) {
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Unsafe recording path: symbolic links and junctions are not allowed.");
obs-studio-server/source/osn-advanced-replay-buffer.cpp:210
- The advanced replay-buffer integration suite exercises
start()with normal paths, but no automated case verifies that this new branch rejects a Windows symlink/junction path and propagates the intended error. Add a regression case so this security boundary cannot silently disappear.
This issue also appears on line 210 of the same file.
if (!os_is_path_safe(replayBuffer->path.c_str())) {
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Unsafe recording path: symbolic links and junctions are not allowed.");
Summary
companion to obs-studio): reject unsafe recording/replay directories at start via
os_is_path_safe.Covers simple/advanced recording and both replay buffers.
Test plan