feat: Enforce CSP via middleware (OHE-2815) - #94
Merged
Conversation
- New SecurityHeadersMiddleware emits Content-Security-Policy-Report-Only plus X-Content-Type-Options, Referrer-Policy, Permissions-Policy and X-Frame-Options on every response. - POST /api/v1/security/csp-report accepts both application/csp-report and application/reports+json payloads and logs each violation. - Default policy uses 'unsafe-inline' on script-src/style-src so React Router's inline bootstrap does not flood the report; tighten before flipping to enforce mode. - New env vars: CONTENT_SECURITY_POLICY_REPORT_ONLY (override), CONTENT_SECURITY_POLICY_REPORT_URI, OH_FRAME_SRC_ALLOWLIST. Co-authored-by: openhands <openhands@all-hands.dev>
OHE-2815 Pen test remediation: Implement Content-Security-Policy (CSP) header
Source: All Hands Ai Web Application Penetration Test (June 2026), finding #1. Vanta: https://app.vanta.com/c/openhands.dev/tests/pen-test-remediation?tab=results Risk: Medium · OWASP A02 - Security Misconfiguration · CWE-693 Affected: https://staging.all-hands.dev/ (and presumably prod) Description: No Content-Security-Policy header is enforced. Leaves the app vulnerable to XSS and malicious content injection — no active XSS was found, but there's no defense-in-depth against it. Remediation:
|
tofarr
changed the title
…connect-src Staging surfaced three missing pieces in the default policy: - PostHog SDK scripts are loaded from us-assets.i.posthog.com, not just us.i.posthog.com (which is the event-ingest endpoint). Added the assets host to script-src and connect-src by default. - Runtime API calls (e.g. /api/conversations/.../events/count) and the VS Code iframe both target *.staging-runtime.all-hands.dev, so the runtime host set has to be in connect-src as well as frame-src. Renamed OH_FRAME_SRC_ALLOWLIST to OH_RUNTIME_HOSTS since it now drives both directives, and added coverage for wildcards and the posthog asset host. Co-authored-by: openhands <openhands@all-hands.dev>
- Remove openhands/app_server/security/ package and its router include in v1_router.py. Violations still surface in the browser DevTools / console under report-only mode, which is enough for the user to triage. - Drop report-uri directive from the default policy and the CONTENT_SECURITY_POLICY_REPORT_URI env var (now unused). - Trim the corresponding tests. Co-authored-by: openhands <openhands@all-hands.dev>
tofarr
changed the title
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||
Per reviewer feedback, drop OH_RUNTIME_HOSTS and reuse the existing WEB_HOST (the deployment's bare apex hostname): inject a single 'https://*.WEB_HOST' source into both frame-src and connect-src. CSP3 '*.host' matches the host itself plus any number of subdomains, so one entry covers '<sandbox>.<apex>' (REST + WS) and 'vscode-<sandbox>.<apex>' (iframe). Returns empty when WEB_HOST is unset, so local dev keeps frame-src 'self' without further config. Tolerate accidental 'https://WEB_HOST' or trailing '/' since operators set WEB_HOST inconsistently across the codebase. Co-authored-by: openhands <openhands@all-hands.dev>
Staging reported new connect-src violations: the public app lives at 'staging.all-hands.dev' (the WEB_HOST value), but the runtime lives at '<sandbox>.staging-runtime.all-hands.dev' - a *sibling* subdomain tree under 'all-hands.dev'. Wildcarding '*.staging.all-hands.dev' misses those siblings. Drop the leftmost DNS label so WEB_HOST is wildcarded at the registrable domain (eTLD+1): staging.all-hands.dev -> https://*.all-hands.dev app.all-hands.dev -> https://*.all-hands.dev openhands.example.com -> https://*.example.com app.example.co.uk -> https://*.example.co.uk CSP3 '*.host' matches the host itself plus any number of subdomains, so a single entry covers the API calls, the VS Code iframe, and any other sibling subdomain the runtime team spins up. WEB_HOST=localhost (single label) is skipped - the wildcard would either be invalid or too broad. Co-authored-by: openhands <openhands@all-hands.dev>
tofarr
changed the title
Staging report was clean after the registrable-domain fix, so flip the header from 'Content-Security-Policy-Report-Only' to 'Content-Security-Policy' in dispatch(). Violations are now blocked by the browser; DevTools still logs them for triage. Rename the override env var from CONTENT_SECURITY_POLICY_REPORT_ONLY to CONTENT_SECURITY_POLICY (it's now the policy itself, not a report-only variant). Detection uses os.getenv without a default so an explicit empty value disables CSP entirely - useful as a kill switch without a redeploy. Test changes: - _csp helper now reads 'Content-Security-Policy'. - New 'test_no_report_only_header' asserts the report-only header is not emitted and the enforced header is. - New 'test_explicit_empty_override_disables_csp' covers the kill switch. - Renamed test method that asserted on the old header name. Co-authored-by: openhands <openhands@all-hands.dev>
tofarr
changed the title
tofarr
changed the title
Failing test: test_explicit_empty_override_disables_csp asserted 'Content-Security-Policy' not in response.headers when CONTENT_SECURITY_POLICY='' (the kill-switch path). dispatch() was setting headers[Content-Security-Policy] = '' unconditionally, so the header was present with an empty value. Guard the assignment: only set the header when _policy() returns a non-empty string. That makes CONTENT_SECURITY_POLICY='' actually disable CSP, matching what the docstring promises. Co-authored-by: openhands <openhands@all-hands.dev>
tofarr
marked this pull request as ready for review
July 30, 2026 10:16
simonrosenberg
approved these changes
Jul 30, 2026
|
🚀 Released in 1.49.0. |
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
3 participants
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.
HUMAN:
Tested on staging and CSP is in place and app still works.
AGENT:
Why
OHE-2815 / pen-test finding #1 (Vanta): the SPA was served without a Content-Security-Policy header. Ship one in enforce mode so the browser actually blocks disallowed requests, scripts, frames and connections — matching what the pen-test finding asked for.
The PR went through a report-only phase on staging to surface every violation in DevTools before enforcing. Staging report came back clean (the runtime / PostHog / iframe cases below were all fixed during the report-only iteration), so this commit flips to enforce mode.
Summary
New
SecurityHeadersMiddleware(openhands/app_server/middleware.py) emitsContent-Security-Policy(enforce) plusX-Content-Type-Options,Referrer-Policy,Permissions-PolicyandX-Frame-Optionson every response. HSTS is intentionally left to the edge proxy.Default policy covers what the app actually needs and blocks everything else:
Notes:
'unsafe-inline'on script/style stays for now because React Router's SPA bootstrap emits inline<script>/<style>. Tightening to nonces or static hashes is the next follow-up before this PR is "fully done".WEB_HOSTby stripping the leftmost DNS label, yielding the registrable domain (eTLD+1).WEB_HOST=staging.all-hands.dev⇒https://*.all-hands.dev, which matches<sandbox>.staging-runtime.all-hands.dev,vscode-<sandbox>.staging-runtime.all-hands.dev, and any other sibling subdomain the runtime team spins up. Single-label hosts (e.g.localhost) skip the wildcard.us-assets.i.posthog.com) is inscript-src/connect-srcfor the SDK scripts. The event-ingest host (us.i.posthog.com) stays inconnect-srconly.CONTENT_SECURITY_POLICYenv var overrides the default policy string wholesale. An explicit empty value disables the header entirely — the kill switch if a directive ends up blocking something legitimate. The env var is only honoured when set (not when merely empty), so an unset variable doesn't accidentally disable CSP.Issue Number
OHE-2815 (pen-test remediation / Vanta)
How to Test
poetry run pytest tests/unit/app_server/test_security_headers_middleware.py -vmake run:curl -I http://localhost:3000/— every response carriesContent-Security-Policyplus the four companion headers. TheContent-Security-Policy-Report-Onlyheader must NOT be present.WEB_HOST=staging.all-hands.dev, the policy includeshttps://*.all-hands.devin bothframe-srcandconnect-src.CONTENT_SECURITY_POLICY="default-src 'none'"set, the response carries exactly that value (no derived directives).CONTENT_SECURITY_POLICY=""set, the response does NOT carry aContent-Security-Policyheader at all.The sandbox here could not run
pytestdirectly (the committed.venvis macOS-built, andbase62has no PyPI distribution). Lint/format were all verified locally, and the override / wildcard / disable paths were validated via an isolated FastAPI harness against a copy of the directive map.Video/Screenshots
Type
Rollout safety
CONTENT_SECURITY_POLICY=""to disable the header without a redeploy. Or paste a relaxed override (CONTENT_SECURITY_POLICY="default-src *"for "open it up entirely", or a partial override that drops the offending directive).Required follow-ups (separate PRs)
script-src/style-srcby either shipping nonces from React Router or pinning SHA-256 of the two inline scripts emitted by<Scripts />. Then drop'unsafe-inline'. This is the only directive keeping us from a strict policy.CONTENT_SECURITY_POLICYinenterprise/enterprise_local/convert_to_env.pyand its README so operators know about the kill switch.WEB_HOSTis set to the deployment's apex (e.g.staging.all-hands.devfor the staging cluster,app.all-hands.devfor prod).Doing those here would expand the blast radius of this PR.
Notes
vscode-tab.tsxalready points at the current host with the sandbox hostname replaced in. WithWEB_HOSTset,<sandbox>.<sibling>.<registrable>(REST/WS) andvscode-<sandbox>.<sibling>.<registrable>(iframe) are covered by*.<registrable>.app.pyso it wraps everything, including theSPAStaticFilesmount.Enterprise server image for this PR: