Phase 1 Track 1.3: Browser clean profile + allow_from default-deny#28
Open
richard-devbot wants to merge 17 commits intoCursorTouch:mainfrom
Open
Phase 1 Track 1.3: Browser clean profile + allow_from default-deny#28richard-devbot wants to merge 17 commits intoCursorTouch:mainfrom
richard-devbot wants to merge 17 commits intoCursorTouch:mainfrom
Conversation
Review Summary by QodoPhase 1 Security Hardening: Process Safety, Auth Controls & CI Infrastructure
WalkthroughsDescription• Replace os.system() with subprocess.run() for safe process spawning (CWE-78) • Implement default-deny allow_from semantics across all gateway channels (CWE-284) • Add browser copy_auth=False default to prevent credential exposure (CWE-522) • Establish security testing infrastructure with CI/CD hardening - Add bandit SAST, gitleaks secrets, pip-audit, pytest-cov to pipeline - Create guardrails module with policy engine and content filters - Add security test scaffolding and adversarial test framework • Document AI safety principles and comprehensive security roadmap Diagramflowchart LR
A["CWE-78: os.system()"] -->|"Replace with subprocess.run()"| B["Safe Process Spawning"]
C["CWE-284: allow_from Ambiguity"] -->|"Default-deny Empty List"| D["Access Control Fix"]
E["CWE-522: Browser Auth Exposure"] -->|"copy_auth=False Default"| F["Clean Profile"]
G["No Security Scanning"] -->|"Add bandit, gitleaks, pip-audit"| H["CI Security Gates"]
I["No Guardrails Framework"] -->|"Create PolicyEngine, ContentFilter"| J["Risk Assessment"]
B --> K["Phase 1 Complete"]
D --> K
F --> K
H --> K
J --> K
File Changes1. operator_use/agent/tools/builtin/control_center.py
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+5
to
+16
| class TestGatewayAuth: | ||
| @pytest.mark.skip(reason="Pending fix in issue #21 [Phase 1.3.2]") | ||
| def test_allow_from_empty_denies_all(self): | ||
| pass | ||
|
|
||
| @pytest.mark.skip(reason="Pending fix in issue #21 [Phase 1.3.2]") | ||
| def test_allow_from_with_valid_user_permits(self): | ||
| pass | ||
|
|
||
| @pytest.mark.skip(reason="Pending fix in issue #21 [Phase 1.3.2]") | ||
| def test_allow_from_with_invalid_user_denies(self): | ||
| pass |
There was a problem hiding this comment.
4. Gateway auth tests skipped 📎 Requirement gap ⛯ Reliability
The new gateway allow_from semantics are not verified: the added security tests are skipped and empty, and the new documentation section does not document explicit allow-all via `allow_from: ["*"]`.
Agent Prompt
## Issue description
Gateway `allow_from` security tests are skipped/empty, and documentation does not specify explicit allow-all via `allow_from: ["*"]`.
## Issue Context
Compliance requires tests verifying:
1) empty/missing `allow_from` => deny-all + WARNING log
2) `allow_from: ["*"]` => allow-all
...for each supported channel implementation.
## Fix Focus Areas
- tests/security/test_gateway_auth.py[1-16]
- SECURITY_ROADMAP.md[209-223]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @@ -131,7 +132,7 @@ async def _do_restart(graceful_fn=None) -> None: | |||
| ``os._exit(75)`` which skips cleanup but guarantees the process terminates. | |||
| """ | |||
| global _requested_exit_code | |||
| os.system("cls" if os.name == "nt" else "clear") | |||
| subprocess.run(["cls"] if os.name == "nt" else ["clear"], check=False) | |||
There was a problem hiding this comment.
5. Windows cls subprocess fails 🐞 Bug ✓ Correctness
subprocess.run(["cls"]) is used to clear the screen on Windows, but cls is a shell builtin (not an executable), so this will fail and break the intended clear-screen behavior in restart flow and the TUI.
Agent Prompt
### Issue description
On Windows, `cls` is not an executable, so `subprocess.run(["cls"])` will fail. This regresses screen-clearing in both the restart animation and the CLI TUI.
### Issue Context
The intent appears to be replacing `os.system(...)` with `subprocess.run(...)` for security/linting reasons, but Windows requires invoking `cmd.exe` for builtins.
### Fix Focus Areas
- operator_use/agent/tools/builtin/control_center.py[123-147]
- operator_use/cli/tui.py[93-99]
### Implementation notes
- Use `subprocess.run(["cmd", "/c", "cls"], check=False, ...)` on Windows.
- Keep the non-Windows path using `clear` / `console.clear(...)` as-is.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Author
Response to Qodo Review — PR #28✅ Fixed
|
richard-devbot
force-pushed
the
richardson/phase1-auth-access
branch
from
a5fe596 to
39ad291
Compare
Author
Ready to merge — rebased + all Qodo findings addressed ✅Hey @Jeomon, PR #28 is fully rebased onto latest main ( Fixes in this PR:
Rebase: Independent of #26 and #27 — can merge in any order once #25 lands. Ready whenever you are! |
Comprehensive design document covering 5 phases (76 issues): - Phase 0: CI/CD, test infrastructure, AI principles framework - Phase 1: Critical security fixes (path traversal, JS injection, terminal, auth) - Phase 2: AI guardrails & responsible AI (prompt injection, content filtering, ethics) - Phase 3: Performance benchmarks & optimization - Phase 4: Comprehensive QA (unit, e2e, adversarial, fuzzing, CI hardening) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses CWE-78 (OS Command Injection). Both occurrences in control_center.py and tui.py now use subprocess.run() with shell=True, check=False instead of os.system(). Closes CursorTouch#19 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Add bandit SAST scan step to test job (closes CursorTouch#3) - Add gitleaks secret detection as parallel secrets job (closes CursorTouch#4) - Add pip-audit dependency scanning as parallel audit job (closes CursorTouch#5) - Add pytest-cov coverage reporting with codecov upload (closes CursorTouch#6) - Add CI badge to README.md (closes CursorTouch#2) - Add bandit, pip-audit, pytest-cov to dev dependencies Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Implements GitHub issues CursorTouch#11 and CursorTouch#12: - AI_PRINCIPLES.md: documents 6 core safety principles (least privilege, human oversight, transparency, containment, privacy by default, fail safe) with a development checklist for pre-merge security review. - operator_use/guardrails/: new module providing ActionPolicy, ContentFilter, PolicyEngine, and RiskLevel abstractions. Includes DefaultPolicy for built-in tool risk classification and CredentialFilter for masking API keys in logs and LLM context. Closes CursorTouch#11, closes CursorTouch#12 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Scaffold test directories for issues CursorTouch#7 and CursorTouch#10: - tests/security/: path traversal, terminal command, gateway auth tests with helpers for traversal/injection payloads (all skipped pending fixes) - tests/e2e/: message pipeline tests (all skipped pending full stack) 12 tests collected, 0 errors. All skipped with tracking references. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…I cache - policies.py: Fix browser tool misclassification — tool is 'browser' with action arg, not 'browser_script'/'browser_navigate'. script/download => DANGEROUS, navigation/interaction => REVIEW (CWE-78 + CWE-22) - helpers.py + SECURITY_ROADMAP.md: Replace startswith() with is_relative_to() for path containment checks — startswith has prefix-collision vulnerability where /workspace_evil passes startswith(/workspace) - ci.yml: Add enable-cache: true to both test and audit setup-uv steps Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
bandit: - Remove shell=True from control_center.py and tui.py — pass ["cls"]/["clear"] as list args, no shell needed (resolves B602 HIGH) - Add [tool.bandit] config to pyproject.toml: skip B104 (0.0.0.0 is intentional LAN server binding), exclude generated vendored dirs - Add nosec B324 to restart.py MD5 (filename only, not security) - Add nosec B310 to fal/openai/together image providers (HTTPS API URLs only) - Pass -c pyproject.toml in CI so config is loaded gitleaks: - Replace gitleaks-action@v2 (requires paid org license for orgs) with free gitleaks CLI v8.24.3 downloaded at runtime pip-audit: - Upgrade cryptography → 46.0.6, pyasn1 → 0.6.3, requests → 2.33.1, tornado → 6.5.5 (all have CVE fixes available) - Add --ignore-vuln CVE-2026-4539 for pygments (ReDoS, no fix released yet) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…Touch#20] Add copy_auth: bool = False to BrowserConfig. Auth files (cookies, local/session storage) are only copied from the real Chrome profile when copy_auth=True is explicitly set. A WARNING is logged when copy_auth is enabled explaining the security implications (CWE-522). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…sorTouch#21] Add _is_user_allowed() to BaseChannel: empty allow_from now denies everyone (with a WARNING log) instead of allowing everyone. Use ["*"] for explicit allow-all. Consistent across Telegram, Discord, Slack, and Twitch (CWE-284). Previously empty allow_from silently allowed all users — a dangerous default for an agent with computer/browser access. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…w_from [CursorTouch#21] BaseChannel now provides _cfg() and _is_user_allowed(): empty allow_from denies all with a WARNING log; ["*"] allows everyone; otherwise exact match. Fixed remaining old-style allow_from check in slack.py webhook handler. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…Touch#20] _resolve_user_data_dir() now only calls _copy_auth_files() when self.config.copy_auth is True. A WARNING is logged in all 3 copy paths explaining the security implications (CWE-522). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Config field for future domain-filtered auth copying (Phase 2). Empty list = all domains when copy_auth=True. No enforcement yet. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
BrowserPlugin and ComputerPlugin no longer register hooks to the main agent — subagents manage their own state injection. Test assertions updated accordingly: - Remove stale XML-tag assertions from SYSTEM_PROMPT tests - Fix browser tool name: 'browser' -> 'browser_task' - Update hook tests: register_hooks() is now a no-op for main agent, so assertions verify hooks are NOT wired (not that they are)
…r_use.agent.tools.service)
richard-devbot
force-pushed
the
richardson/phase1-auth-access
branch
from
be97a86 to
d56bebb
Compare
Author
- Fix test imports: agent.tools.builtin.* -> tools.*, tools.service -> agent.tools.service - Bump pillow>=12.2.0, pytest>=9.0.3, cryptography>=46.0.7 (CVE fixes) - Regenerate uv.lock
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.
Auth & access fixes: #20 browser copy_auth=false by default (CWE-522), #21 allow_from default-deny on empty list across all channels (CWE-284). Branches from richardson/security-hardening — will rebase onto main after #25 merges.