security(browser): validate the URL in capture_screenshot (#13204)

[mrveiss]

Thinking Path

#13204 lists four browser/fetch entry points and which ones run the DNS-resolving public-address guard before reaching the network. content_reach/backends/browser.py and research_browser_manager.py already did. api/browser_mcp.py::send_to_browser_vm did not at filing time, but that half was fixed separately (a7afad817, _reject_non_public_url + is_public_url_async) — its own docstring now documents the exemption for the admin surface that legitimately needs internal hosts. The remaining, unfixed half is services/playwright_service.py: capture_screenshot(url=...) posts a caller-supplied URL straight to the container's /test-frontend endpoint with no check at all.

Two callers reach it: browser_backends/container.py (the ADR-009 canonical-interface wrapper, whose own docstring says the registry validates the URL before dispatch — so callers arriving through get_browser() are already covered) and api/playwright.py's admin-gated /playwright/screenshot endpoint (calls it directly, bypassing the registry). Guarding inside capture_screenshot itself closes the gap for both paths at once — including any future direct caller — rather than only fixing the one HTTP endpoint, and it is a defense-in-depth check for the registry-routed path (a no-op there since that URL is already public by the time it arrives).

search_web and test_frontend/send_test_message were checked and excluded: search_web takes a query string, not a url — the container decides the destination, there is nothing here to validate. test_frontend/send_test_message default to ServiceURLs.FRONTEND_LOCAL and exist specifically to reach AutoBot's own internal frontend — guarding those would break their purpose, matching the same admin-surface exemption already documented in api/browser_mcp.py.

This does not attempt the larger ADR-009 migration (#13236 steps 5-6, routing tool_handler.py and api/playwright.py through get_browser() and retiring is_url_allowed) — that is real, separately-reviewable architectural work with its own production-behaviour-change implications. This PR is scoped to the one concrete, unguarded SSRF gap #13204 names.

What Changed

• autobot-backend/services/playwright_service.py: capture_screenshot now calls autobot_shared.url_safety.is_public_url_async(url) before touching the network, returning {"success": False, "error": "URL is not a public address", "url": url} on rejection — same guard function and same shape of check api/browser_mcp.py::_reject_non_public_url already applies to send_to_browser_vm.
• autobot-backend/services/playwright_service_test.py (new): asserts a non-public URL is rejected before is_ready() is even awaited, and that a public URL still reaches the container exactly as before (mocked http_client.post).
• changelog/unreleased/13204-playwright-screenshot-ssrf-guard.md.

Verification

Pre-push hook's own pytest run on the changed file, from the actual push:

[pre-push] running pytest on:
    autobot-backend/services/playwright_service_test.py
[pre-push OK] pytest: all relevant tests pass

Mutation argument (not run locally): removing the is_public_url_async check restores the exact pre-fix behaviour that test_non_public_url_is_rejected_before_any_http_call asserts against — the function would fall straight into the is_ready()/http_client.post path for http://169.254.169.254/..., and the test's is_ready.assert_not_awaited() would fail.

Risks

Low. The added check runs before any I/O and only tightens behaviour for non-public URLs, which were never a supported use case for this caller-facing screenshot function. browser_backends/container.py callers see no behaviour change — the registry already validated their URL first, so the added check is a no-op pass-through for them.

Model Used

Claude Sonnet 5 (claude-sonnet-5)

Issue Link

Closes #13204

Changelog fragment

• Added changelog/unreleased/13204-playwright-screenshot-ssrf-guard.md

Single-issue rationale

Single-issue rationale: this is the one concrete, unguarded call site #13204 names; #13236's remaining steps (5-6) are a separate architectural migration with their own review/rollout needs and are being tracked and scoped independently.

Checklist

• Code follows AutoBot patterns from CLAUDE.md
• Tests added or updated (or N/A with reason)
• Documentation updated if behavior changed (docstring only; no user-facing doc)
• Pre-commit hooks pass (git commit runs them automatically)
• PR targets main — the default branch since the 2026-09-12 rename (this checklist item's "Dev_new_gui" wording is stale template text)
• No secrets or credentials in the diff

🤖 Generated with Claude Code

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 79c7b784-ba52-4b82-afd7-490ecc89aecb

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

[github-actions]

Notice: 29 open PRs — past the runaway threshold (25)

There is no PR queue limit, and this is not a request to defer this PR. Work proceeds one issue at a time without a cap on open PRs; review capacity is the constraint.

This notice only means the count is high enough to be worth a glance for a runaway — something opening PRs in a loop, or a merge pipeline that has stalled so nothing is draining.

Currently open:

• fix(scripts): detect-environment.sh only selects files this repo tracks (#15143) #16553 fix(scripts): detect-environment.sh only selects files this repo tracks (bug(scripts): detect-environment.sh selects three .env files that do not exist #15143)
• fix(security): agent_config, sync and hardware-priority record the real caller, not "admin" (#16547, #16541) #16550 fix(agents): api/agent_config.py records who made the write (fix(agents): api/agent_config.py records the audit trail as created_by="admin" for every actor #16547)
• feat(llm): wire extractive prompt compression and enforce per-provider concurrency cap (#16526, #16527) #16546 feat(llm): wire extractive prompt compression and enforce per-provider concurrency cap (Wire the existing extractive prompt compressor into the live LLM completion path #16526, Enforce the existing per-provider LLM concurrency cap #16527)
• security(chat): scan/redact PII and flag injection risk on every raw chat message (#16529, #16530) #16545 security(chat): scan/redact PII and flag injection risk on every raw chat message (Add PII scanning to the main chat path #16529, Add a blanket prompt-injection guard on raw user chat input #16530)
• fix(perf): re-derive Multimodal processor startup's budget post-#15297 (#16535) #16537 fix(perf): re-derive Multimodal processor startup's budget post-fix(ml): drop resume_download, removed from from_pretrained in transformers v5 (#15054) #15297 (perf(test): Multimodal processor startup budget still calibrated against #15054's broken (work-skipping) baseline #16535)
• fix(llm-shared): remove dead SemanticLLMCache, keep live semantic cache (#16528) #16536 fix(llm-shared): remove dead SemanticLLMCache, keep live semantic cache (Consolidate duplicate semantic LLM-response cache implementations #16528)
• feat(tooling): check_run_status gains a pending-with-age helper (#16126) #16534 feat(tooling): check_run_status gains a pending-with-age helper (docs(process): two review findings on MEASUREMENT_DISCIPLINE.md — family order and D's missing mechanism #16126)
• DO NOT MERGE: mutation-check for #15931's AC4 #16525 DO NOT MERGE: mutation-check for bug(llc): the blocked-by transition rule has never fired — relation_svc is passed only by its own tests #15931's AC4
• fix(files): conversation uploads accept PDF and GIF, not the truncated '.pd' and '.gi' (#16521) #16523 fix(files): conversation uploads accept PDF and GIF, not the truncated '.pd' and '.gi' (bug(files): conversation uploads reject every PDF and GIF because the allowlist says '.pd' and '.gi' #16521)
• fix(infra): consolidate the last 3 private TLS-cert-generation copies (#16020) #16522 fix(infra): consolidate the last 3 private TLS-cert-generation copies (nginx cannot start on a node with no TLS cert: five roles consume the shared keypair, four provision it #16020)
• security(lint): classify test_first_remediation.py as production, fix the 2 real gaps it surfaced (#16514) #16518 security(lint): classify test_first_remediation.py as production, fix the 2 real gaps it surfaced (chore(lint): classify scripts/test_first_remediation.py as production and document the sync_orchestrator ssh gap #16514)
• docs(infra): record redis role tools as binary names, not units (#16060, #16071) #16511 docs(infra): record redis role tools as binary names, not units (Redis is managed by two unit names that do not exist, and a failed restore-stop is silent #16060, redis naming: five more sites name a unit the fleet does not have (#16060 follow-up) #16071)
• refactor(lifespan): split cleanup_services and _init_graph_rag_service into steps under the length limit (#16250) #16509 refactor(lifespan): split cleanup_services and _init_graph_rag_service into steps under the length limit (function-length-check pre-commit hook blocks any commit to lifespan.py: cleanup_services (267 lines) and _init_graph_rag_service (68 lines) exceed the 65-line ceiling #16250)
• security(routers): gate knowledge_search, knowledge_search_aggregator, knowledge_suggestions and voice_stream (#16507) #16508 security(routers): gate knowledge_search, knowledge_search_aggregator, knowledge_suggestions and voice_stream (security(routers): gate knowledge_search, knowledge_search_aggregator, knowledge_suggestions and voice_stream #16507)
• test(user-management): a user in org A gets no results from org B (#16279) #16506 test(user-management): a user in org A gets no results from org B (security(users): /user-management/users/search answers anonymous callers with names from every organisation #16279)
• security(settings): record the real caller, not the literal "admin" (#16501) #16504 security(settings): record the real caller, not the literal "admin" (security(settings): sync_config and update_hardware_priority record the literal "admin" instead of the real caller #16501)
• fix(chat-knowledge): contexts can be deleted, cascade with their chat, and orphans get cleaned (#16490) #16503 fix(chat-knowledge): contexts can be deleted, cascade with their chat, and orphans get cleaned (bug(chat-knowledge): contexts can be created but never deleted, so orphans accumulate #16490)
• fix(slm-frontend): give the service-log fetch one path and its real timeout (#16256) #16500 fix(slm-frontend): give the service-log fetch one path and its real timeout (tech-debt(slm-frontend): the two log viewers fetch the same journal through two paths with different timeouts #16256)
• fix(a2a): cooperative cancellation checkpoints in execute_a2a_task (#16174) #16498 fix(a2a): cooperative cancellation checkpoints in execute_a2a_task (Cancelling an A2A task does not stop its executor, so claims can only lapse rather than release #16174)
• fix(sdk): TypeScript SDK request/response parity + missing /api prefix (#15528, #16495) #16497 fix(sdk): TypeScript SDK request/response parity + missing /api prefix (bug(sdk): the TypeScript SDK still sends the parameters and parses the envelopes the Python SDK stopped sending #15528, bug(sdk): every TypeScript SDK request omits the /api prefix — 404 against a real backend #16495)
• refactor(agents): extract NPUCodeSearchAgent's pattern helpers, add declared_scopes (#16173) #16496 refactor(agents): extract NPUCodeSearchAgent's pattern helpers, add declared_scopes (NPUCodeSearchAgent cannot declare its write scopes: its file is at the size ceiling #16173)
• fix(frontend): converge the permission vocabulary onto the backend's real Permission enum, wire in v-permission (#16243) #16493 fix(frontend): converge the permission vocabulary onto the backend's real Permission enum, wire in v-permission (tech-debt(frontend): usePermissions' permission strings share nothing with the backend's Permission vocabulary #16243)
• security(research): treat fetched repos and pages as untrusted data (#16488) #16492 security(research): treat fetched repos and pages as untrusted data (security(research): treat fetched repos and pages as untrusted data: prompt-injection hardening for research, adopt and web-fetch #16488)
• feat(frontend): wire router meta.minRole into a real guard, ranked against the backend's role order (#16244) #16489 feat(frontend): wire router meta.minRole into a real guard, ranked against the backend's role order (tech-debt(frontend): router meta.minRole is declared and read by no guard #16244)
• chore(git): rename Dev_new_gui to main and main to release across the repo (#16479) #16487 chore(git): rename Dev_new_gui to main and main to release across the repo (infra(git): swap branch names across the repo for the rename (in-repo half of #16461) #16479)
• feat(secrets): wire in SecretAuditLog, consolidate SecretVault into SecretsManager (#16485) #16486 feat(secrets): wire in SecretAuditLog, consolidate SecretVault into SecretsManager (wire in SecretAuditLog, consolidate SecretVault into SecretsManager #16485)
• fix(frontend): retire four dead settings API paths, stop a fifth calling admin-gated settings from non-admin surfaces (#16481) #16484 fix(frontend): retire four dead settings API paths, stop a fifth calling admin-gated settings from non-admin surfaces (retire useSettingsApi/SystemRepository/services-api.ts/CacheService.ts dead settings paths #16481)
• feat(llm): cross-worker quota headroom store, persist real 429 headroom #16483 feat(llm): cross-worker quota headroom store, persist real 429 headroom
• chore(activity-tracking): retire the terminal/file/browser tracking hooks as dead code (#16466) #16475 chore(activity-tracking): retire the terminal/file/browser tracking hooks as dead code (chore(activity-tracking): wire in or retire terminal/file/browser activity tracking + secret_usage audit (backend) #16466)

If the queue is draining normally, ignore this. Otherwise:

1. Check whether CI is dispatching at all — see the ci-dispatch-watchdog status on these PRs
2. Merge the ones whose CI has finished and review has passed: gh pr merge <number> --squash --delete-branch
3. Look for a loop opening near-identical PRs

Warn-only runaway detector — .github/workflows/pr-queue-gate.yml. It never blocks a merge.

[github-actions]

AutoBot Phase Validation Results

System Maturity: 96.5%

Phase Status:

PASS Phase 1: Core Infrastructure: 100.0%
PASS Phase 2: Knowledge Base and Memory: 100.0%
IN PROGRESS Phase 3: LLM Integration: 66.7%
PASS Phase 4: Security and Authentication: 100.0%
PASS Phase 5: Agent Orchestration: 100.0%
PASS Phase 6: Enhanced UI/UX: 100.0%
PASS Phase 7: Testing and Validation: 100.0%
PASS Phase 8: Advanced Features: 100.0%
PASS Phase 9: Multi-Modal AI: 100.0%
PASS Phase 10: Production Readiness: 100.0%

Recommendations:

• 🟡 MEDIUM: Phase 3: LLM Integration requires attention (66.7% complete): Review and implement
• ✅ System is production-ready - consider advanced features and scaling: Review and implement

[mrveiss]

Independent review at aa297f77c: approved.

• capture_screenshot calls is_public_url_async before is_ready() and before any post, and test_non_public_url_is_rejected_before_any_http_call pins it with is_ready.assert_not_awaited(). That's the canonical url_safety entry point, not a parallel check.
• search_web carries no URL; search_engine is only a dict key.
• security(browser): send_to_browser_vm and playwright_service bypass the SSRF/DNS-rebind guard the other browser paths enforce #13204's concrete claims are resolved: send_to_browser_vm by fix(security): validate URLs at the browser transport, exempting the admin surface (#13204) #13310, capture_screenshot here, and the is_url_allowed regex by fix(security): resolve hosts instead of prefix-matching URLs in the browser allowlist (#13236) #13317. Closing refs are [13204].

Non-blocking:

• The changelog's "last unguarded entry point" overstates things, because POST /playwright/navigate is still unguarded (tracked in browser: migrate api/playwright.py onto get_browser(), retire is_url_allowed's regex allowlist (#13236 step 6) #16552).
• test_frontend/send_test_message are unguarded by design, and that deserves an in-code note like internal_ok has.
• The container re-resolves DNS and follows redirects after the one-time check. That's filed separately.

[github-actions]

AutoBot Phase Validation Results

System Maturity: 96.5%

Phase Status:

PASS Phase 1: Core Infrastructure: 100.0%
PASS Phase 2: Knowledge Base and Memory: 100.0%
IN PROGRESS Phase 3: LLM Integration: 66.7%
PASS Phase 4: Security and Authentication: 100.0%
PASS Phase 5: Agent Orchestration: 100.0%
PASS Phase 6: Enhanced UI/UX: 100.0%
PASS Phase 7: Testing and Validation: 100.0%
PASS Phase 8: Advanced Features: 100.0%
PASS Phase 9: Multi-Modal AI: 100.0%
PASS Phase 10: Production Readiness: 100.0%

Recommendations:

• 🟡 MEDIUM: Phase 3: LLM Integration requires attention (66.7% complete): Review and implement
• ✅ System is production-ready - consider advanced features and scaling: Review and implement

[mrveiss]

Delta aa297f77c..fc7c07adf: still approved. No content change. The net diff against main is identical to the approved aa297f77c: the same 3 files and 0 differing added or removed lines. The only new commit is the changelog fragment fc7c07adf (changelog/unreleased/13204-playwright-screenshot-ssrf-guard.md), which the approved net diff already contained. I did not re-check CI here.