fix(bright_data): confine get_screenshot output to a configurable directory - #522
fix(bright_data): confine get_screenshot output to a configurable directory#522yonib05 wants to merge 4 commits into
Conversation
…ectory Resolve the get_screenshot output_path and require it to be the current working directory (or the directory named by STRANDS_BRIGHT_DATA_OUTPUT_DIR) or a location inside it. Reject a final path component that is a symlink. Add regression tests covering relative traversal, absolute paths outside the root, and symlink targets.
|
@strandly-the-agent can you review this PR please? |
strandly-the-agent
left a comment
strandly-the-agent
left a comment
There was a problem hiding this comment.
🔴 Changes requested: the pathname check and later write are not atomic, and the model-facing schema still omits the new confinement contract.
✅ Reviewed 582f2099 against merge-base ee184bcb (current-base diff is unchanged and merges cleanly).
✅ Focused Bright Data tests: 21 passed; git diff --check passed; exact-head Linux/Windows/macOS, lint, and dependency CI is green.
❓ API sign-off needed: this intentionally changes the directly importable BrightDataClient.get_screenshot() path/return behavior. This repo has no API-review labels, so please record owner approval on the PR.
Review passes
- Correctness/safety: one reproduced confinement escape and one request-order issue.
- API/DevX: the restriction may be justified, but the direct-client compatibility trade-off needs explicit approval.
- Test quality: 21 focused tests pass; add regressions for the race-safe creation and generated schema.
- LLM context/docs: the new description is absent from
tool_spec; configured-root errors are inaccurate; operator configuration is missing from setup docs. - Suppressed: static symlink test variants, absolute-path exposure without a stated threat-model rule, and test-hardening-only suggestions.
Advanced API/adversarial tier was unavailable in this environment; API was rerun on the default tier. I independently reproduced the safety issue twice.
…re request - Bind screenshot writes to a descriptor of the trusted root (openat-style traversal with O_NOFOLLOW per component) so a directory swapped for a symlink after validation cannot redirect the write; add a deterministic regression that swaps a validated directory mid-request - Validate output_path lexically before the remote request so an invalid destination no longer performs billable work - Fix Args indentation in the tool docstring so the generated tool_spec carries the output_path confinement description; add a tool_spec regression - Correct the rejection message when STRANDS_BRIGHT_DATA_OUTPUT_DIR is set to name only the configured directory - Document STRANDS_BRIGHT_DATA_OUTPUT_DIR in the module setup block and the README environment table
|
@strandly-the-agent can you re-review this PR please? |
strandly-the-agent
left a comment
strandly-the-agent
left a comment
There was a problem hiding this comment.
🔴 Changes requested: the prior five findings are fixed, but the new secure-open path still has two reproduced confinement escapes.
✅ Re-reviewed exact head d5286466; focused tests: 25 passed; git diff --check passed.
✅ Latest ordinary exact-head CI is green across Linux/Windows/macOS, dependencies, lint, and CI Gate.
pull_request_target checkout guard—not a Bright Data test failure; a superseded run was concurrency-cancelled.
🔴 Root-swap and fallback check/open escapes reproduced twice each.
❓ API sign-off still needed: can a Bright Data/codeowner approve the intentional BrightDataClient.get_screenshot() behavior change? No approval is visible yet.
Delta review details
- The previous schema, configured-root error, setup-docs, lexical pre-request rejection, and original child-path findings are resolved; I did not repeat them.
- Correctness/adversarial: retained two new confinement bypasses and the remaining billable-request behavior.
- Test quality: retained the race regression because it passes against known-vulnerable
582f2099; suppressed lower-value nested-success/fd-cleanup gaps. - LLM context/docs: generated
output_pathschema is now correct; implementation claims remain too broad only where folded into the security findings. - Advanced API/adversarial tiers were unavailable due retention validation; both were rerun on the default tier.
No safe small suggestion exists for the handle-lifetime/platform security changes; they need an architectural fix rather than another pathname check.
| symlink_errnos = {errno.ELOOP, errno.ENOTDIR, getattr(errno, "EMLINK", errno.ELOOP)} | ||
|
|
||
| if os.open in os.supports_dir_fd: | ||
| dir_fd = os.open(str(root), os.O_RDONLY | os.O_DIRECTORY) |
There was a problem hiding this comment.
🔴 The configured root can be replaced before this pathname is reopened. root was resolved at line 131, but after the remote request this call follows whatever now occupies that pathname. I made requests.post rename the configured root and replace it with an outside-pointing symlink; twice, the method returned allowed/shot.png while the original root stayed empty and outside/shot.png contained b"ATTACK".
Please securely acquire and pin the intended root handle before the request, retain it through the response, and create the file relative to that handle while rejecting root-level symlink/reparse substitution. There is no safe small suggestion here—the handle lifetime and API between validation/request/write need to change together.
| probe = probe / part | ||
| if probe.is_symlink(): | ||
| raise _confinement_error(root, f"output_path must not contain a symlink: {final_path}.") | ||
| fd = os.open( |
There was a problem hiding this comment.
🔴 The non-dir_fd fallback still has the original check/open race. Lines 191–195 inspect components, then this separately opens the full pathname. In a forced-fallback repro I replaced images with an outside-pointing symlink immediately after its successful is_symlink() check; twice, the returned path stayed under allowed/images, but b"ATTACK" was written outside.
O_NOFOLLOW may be unavailable here and, when present, only protects the final component. Because supported Windows uses this branch, please use handle-based/no-reparse traversal there or fail closed and explicitly scope the platform limitation. No small suggestion is safe for this platform-specific handle change.
| Raises: | ||
| ValueError: If output_path resolves outside the allowed output directory. | ||
| """ | ||
| # Validate the destination before making the (billable) remote request. |
There was a problem hiding this comment.
🟡 Only lexical containment is validated before the billable request. With an existing final symlink, the current code made one mocked request and only then raised ValueError in _open_confined_output_file(); I repeated that twice. So this comment and the method docstring’s “destination is validated” claim overstate the behavior.
Prefer securely acquiring/validating the root and destination parent before the request—this also supports the root-handle fix above. Otherwise narrow the wording to say that only lexical containment is checked before requesting.
| outside = tmp_path / "outside" | ||
| outside.mkdir() | ||
|
|
||
| def swap_then_respond(*args, **kwargs): |
There was a problem hiding this comment.
🟡 This regression passes against the known-vulnerable implementation. I copied this current test to old head 582f2099; it passed (1 passed) because that version validates only after requests.post, sees the already-installed symlink, and never reaches its check/open race.
Synchronize the swap after validation and immediately before the final open/write in the replacement design. The security regression should fail on 582f2099 and pass only when validation and creation are bound safely.
3 participants
Summary
get_screenshotpreviously wrote the downloaded screenshot bytes to the rawoutput_pathwithout any validation, so the file could land anywhere on the filesystem.This change resolves
output_pathand confines it to a single root directory:STRANDS_BRIGHT_DATA_OUTPUT_DIRenvironment variable when set.ValueErroris raised.Behavior change
get_screenshotnow writes within the working directory by default. To write somewhere else, setSTRANDS_BRIGHT_DATA_OUTPUT_DIRto the desired directory (it may point anywhere the operator chooses, including a broad location); relativeoutput_pathvalues are taken from that directory and absolute paths inside it are allowed. The docstring, tool parameter docs, and the rejection error message now state this so the override is discoverable.Testing
../../etc/x) rejected, an absolute path outside the root rejected, a symlink target rejected, default-root (no env var) traversal/absolute-outside rejection, an actionable rejection message, and an operator-chosen output directory accepting an absolute path inside it.pytest tests/test_bright_data.pypasses (21 tests).ruff format --checkandruff checkpass on the changed files.