Skip to content

fix(video-surfer): apply extract_audio path guards to save_screenshot - #8200

Open
Dev Oskii (Oskii) wants to merge 1 commit into
microsoft:mainfrom
Oskii:fix/video-surfer-save-screenshot-path
Open

fix(video-surfer): apply extract_audio path guards to save_screenshot#8200
Dev Oskii (Oskii) wants to merge 1 commit into
microsoft:mainfrom
Oskii:fix/video-surfer-save-screenshot-path

Conversation

@Oskii

Copy link
Copy Markdown

What

VideoSurfer's extract_audio already refuses a URL video_path and an audio_output_path that resolves outside the current working directory. save_screenshot takes the same kinds of paths and did not run those checks. An agent-chosen output_path such as ../escaped.png (or an absolute path outside cwd) is passed straight to cv2.imwrite. A URL video_path is passed straight to cv2.VideoCapture.

On main at 027ecf0:

# extract_audio
if re.match(r"^[a-zA-Z][a-zA-Z0-9+\-.]*://", video_path):
    raise ValueError("video_path must be a local file path, not a URL.")
# ...
if not output_real.startswith(cwd + os.sep) and output_real != cwd:
    raise ValueError("audio_output_path must be within the current working directory.")

# save_screenshot
cap = cv2.VideoCapture(video_path)
# ...
cv2.imwrite(output_path, frame)

Default VideoSurfer includes save_screenshot in its tool list. After this change, save_screenshot uses the same two helpers as extract_audio.

Why

I was reading VideoSurfer's tools because extract_audio already documents an SSRF and path-traversal guard. I expected save_screenshot to share that jail. It does not. I confirmed the three new save_screenshot tests fail on 027ecf0 (they never raise the cwd/URL ValueError) and pass with this patch.

How

Move the URL and cwd checks into video_surfer/_path_guards.py. Call them from extract_audio (same behaviour, same error strings) and from save_screenshot before OpenCV runs.

Testing

python3 -m pytest python/packages/autogen-ext/tests/agents/test_video_surfer_path_guards.py -q

5 passed. The three save_screenshot cases fail on 027ecf0 and pass with the patch. The two extract_audio cases already passed on 027ecf0.

The test file stubs cv2, ffmpeg, and whisper so it runs without the video-surfer extra.

extract_audio already rejects URL video paths and writes outside the
working directory. save_screenshot did not, so an agent-chosen output
path could leave the cwd. Share the same checks.
@Oskii

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@Oskii

Copy link
Copy Markdown
Author

CLA is signed. GitHub Actions is still waiting on maintainer approval for first-time contributor workflows (Checks and the other pull_request jobs). I cannot approve those from this account.

Once the workflows run I can fix any failures. A review would also unblock merge: save_screenshot now uses the same URL and cwd checks as extract_audio, with tests for both.

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