fix: security and correctness issues found in #4514 review#4590
Merged
Conversation
…ly ignoring it The CLI previously accepted the env var as a fallback; this PR dropped it without a migration path, breaking CI/CD pipelines that set it as a secret. Restore backwards-compat by checking the env var after config.json and printing a deprecation warning with the migration command.
…inux Downloads to a temp file, fetches the .sha256sum file Cloudflare publishes alongside each release, and verifies before moving to the install destination. Protects against MITM/CDN tampering. Temp file is cleaned up on failure.
…ta loss
A SIGKILL mid-write truncates config.json; read_config() catches
json.JSONDecodeError and returns {}, silently wiping the API key and
all other settings. Mirror the pattern already used by _write_state():
write to a sibling temp file, fsync, chmod 600, then os.replace() into
place — which is atomic on POSIX and effectively atomic on Windows.
sauravpanda
force-pushed
the
fix/pr-4514-review-fixes
branch
from
27a53db to
ea99055
Compare
Agent Task Evaluation Results: 2/2 (100%)View detailed results
Check the evaluate-tasks job for detailed task execution logs. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="browser_use/skill_cli/commands/cloud.py">
<violation number="1" location="browser_use/skill_cli/commands/cloud.py:87">
P2: This reintroduces `BROWSER_USE_API_KEY` fallback for `api_key`, which violates the CLI policy to keep the config file as the single source of truth.
(Based on your team's feedback about treating the CLI config as the single source of truth for configuration values.) [FEEDBACK_USED]</violation>
</file>
<file name="browser_use/skill_cli/commands/setup.py">
<violation number="1" location="browser_use/skill_cli/commands/setup.py:246">
P2: Using `Path.rename()` for the temp download can fail across filesystems (e.g., `/tmp` -> `/usr/local/bin`), causing false installation failures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
…move - cloud.py: remove BROWSER_USE_API_KEY env var fallback (violates CLI policy of config.json as single source of truth); instead detect the env var in the error path and print a targeted migration hint - setup.py: replace Path.rename() with shutil.move() so the temp file can be moved across filesystems (e.g. /tmp -> /usr/local/bin)
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.
Summary
Three security and correctness issues found during review of #4514, which has since been merged. All three affect the new
skill_clilayer introduced by that PR.BROWSER_USE_API_KEYenv var silently ignored (commands/cloud.py): another big cli update #4514 dropped the env var fallback in_get_api_key()without a migration path, breaking CI/CD pipelines that set it as a secret. Restored with a deprecation warning directing users tobrowser-use config set api_key._install_cloudflared()downloads binary without integrity check (commands/setup.py, Linux only): Rawurllib.request.urlretrievewrote directly to the install destination with no verification. Now downloads to a temp file, fetches the.sha256sumCloudflare publishes alongside each release, verifies SHA256 before installing, and cleans up on failure. macOS (brew) and Windows (winget) were already safe — they verify internally.write_config()not atomic (config.py): Directpath.write_text()truncatesconfig.jsononSIGKILLmid-write;read_config()catchesjson.JSONDecodeErrorand returns{}, silently wiping the API key and all settings. Now usestempfile.mkstemp(dir=same_dir)+fsync+os.replace()— the same pattern_write_state()indaemon.pyalready uses correctly.Test plan
BROWSER_USE_API_KEY=sk-xxx browser-use cloud connectprints deprecation warning and still authenticatesbrowser-use config set api_key sk-xxx, commands work without the env var setSIGKILLduringbrowser-use config setleavesconfig.jsonintact or absent, never truncated🤖 Generated with Claude Code