security: config hardening — CORS, CSP, master_key, Sidekiq, DNS rebinding, profiler, Lookbook#1516
security: config hardening — CORS, CSP, master_key, Sidekiq, DNS rebinding, profiler, Lookbook#1516dgilperez wants to merge 13 commits intowe-promise:mainfrom
Conversation
Remove hardcoded default "sure"/"sure" Basic Auth credentials from the Sidekiq Web UI in production. Authentication now fails closed: if the SIDEKIQ_WEB_USERNAME or SIDEKIQ_WEB_PASSWORD env vars are not set, the UI becomes inaccessible and a [SECURITY] warning is logged at boot. This prevents unauthenticated access to background job internals on deployments that forget to override the defaults. Also fixes a latent bug in the previous code path: the credential check used `&` (bitwise AND) instead of `&&` (short-circuit logical AND), which returned an integer instead of a boolean from the auth block. New required env vars for production (Sidekiq Web UI access): - SIDEKIQ_WEB_USERNAME - SIDEKIQ_WEB_PASSWORD If either is unset, the /sidekiq endpoint is still routed but will reject every request.
Previously Lookbook::Engine was mounted at /design-system in every environment, exposing component preview, inspector, and eval-style rendering surfaces in production. Lookbook is a development-only tool and is not designed to be exposed to untrusted clients. Guard the mount so the engine only mounts in development, mirroring the existing guard already applied to rswag below it.
Enable config.require_master_key = true in production so the app will refuse to boot if no Rails master key is available via ENV["RAILS_MASTER_KEY"], config/master.key, or config/credentials/production.key. Without this flag, Rails silently boots without the master key, which means encrypted credentials (OAuth client secrets, signed cookies, etc.) are not available and anything depending on them either fails at request time or — worse — falls back to defaults. Fail fast at boot instead. New required env var for production (if config/master.key is not shipped): RAILS_MASTER_KEY.
Previously Rack::Cors was configured with origins "*" for /api/*,
/oauth/*, and /sessions/*. Combined with credentials-bearing endpoints
this is unsafe: a wildcard origin reflects back as the caller's origin
for every request, neutralising SameSite protections for any browser
client with valid credentials.
The allowlist is now driven by env:
- ALLOWED_ORIGINS: comma-separated list of full origins
(e.g. "https://app.example.com,https://staging.example.com")
- APP_DOMAIN: fallback when ALLOWED_ORIGINS is unset; treated as
"https://$APP_DOMAIN"
If neither env var is set, a [SECURITY] warning is logged at boot and
no cross-origin requests are accepted (fail closed). /oauth/* is also
narrowed to just /oauth/token and /oauth/revoke (the two endpoints
actually used by cross-origin clients); credentials: true is now
explicit on /api/* and /sessions/* so cookie-bearing requests are
handled correctly.
New env vars for production:
- ALLOWED_ORIGINS (preferred, comma-separated origins)
- APP_DOMAIN (fallback, bare domain)
Rails' CSP initializer was fully commented out, so the application sent
no Content-Security-Policy header. Enable a strict policy in
report-only mode so the browser reports violations to
/csp-violation-report while we tune the allowlist, after which the
report-only flag can be removed to enforce.
Policy highlights:
- default-src 'self'
- object-src 'none'
- script-src restricted to self + Plaid + Stripe; PostHog/Pusher
added only in managed mode (analytics / ActionCable are bundled
services on the managed SaaS, not self-hosted)
- style-src 'self' + 'unsafe-inline' (required by Tailwind)
- frame-src restricted to Plaid and Stripe iframes
- script nonces wired through importmap/Hotwire (session.id)
No new env vars. The /csp-violation-report path is intentionally not
routed — reports 404 for now, the important signal is in the access
log and browser devtools. A future commit can wire up a proper
reporting endpoint or ship violations to Sentry.
Rails ships DNS rebinding / Host-header attack protection via config.hosts, but production had it commented out, so the app accepted any Host header. Enable it when APP_DOMAIN is set (which it needs to be anyway for action_mailer.default_url_options and for the CORS fallback added in F-02). The existing /up health check and reverse-proxy paths continue to work because infra-level health probes hit the server over its configured hostname. If APP_DOMAIN is not set, behavior is unchanged from before (all hosts accepted) — the warning is surfaced implicitly via the CORS initializer. Operators running managed deployments must set APP_DOMAIN. New required env var (recommended) for production: APP_DOMAIN.
rack-mini-profiler exposes detailed SQL, allocation, and memory data for every request via a floating badge and an internal dashboard. Previously the gem was declared at top-level in the Gemfile and so loaded in every environment including production, where — even when the UI is hidden — the middleware still instruments every request and stores profile data in the cache, surfaces paths like /mini-profiler-resources/..., and widens the attack surface. Move the gem to the :development group and guard the initializer on defined?(Rack::MiniProfiler) so production (where the constant is no longer present) is a clean no-op. No production env-var changes.
…F-02) test/integration/cors_test.rb asserted the old wildcard (Access-Control-Allow-Origin: "*") behaviour for /api, /oauth, and /sessions endpoints. That was the vulnerability — these assertions would fail every time the allowlist is enforced. Rewrite the tests to verify the new contract: when no ALLOWED_ORIGINS (or APP_DOMAIN) is configured (the test-env default), requests from a non-allowed Origin MUST NOT receive "*" back, and MUST NOT receive the sender's origin echoed. The middleware presence check is kept so the stack assertion still guards regressions.
brin-security-scanner
Bot
added
contributor:verified
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEnable production hardening (require master key, conditional host allowlist), tighten CORS and CSP (report-only with per-request nonces and a report endpoint), add CSP report ingestion and tests, scope dev tools and Sidekiq Web UI to env-driven credentials, and update docs/tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant App as Rails App
participant Logger as Rails.logger
Browser->>App: POST /csp-violation-report\n(Content-Type: application/csp-report)\nbody (<= 8192 bytes)
App->>App: read up to MAX_BODY_BYTES\nApp->>App: parse JSON or build truncated/raw snippet on error
App->>Logger: warn with parsed report JSON or parse-failure snippet
App-->>Browser: 204 No Content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/environments/production.rb`:
- Around line 104-105: The current config.hosts assignment silently skips host
authorization when ENV["APP_DOMAIN"] is blank; update the initialization so
production fails closed: if ENV["APP_DOMAIN"] is present set config.hosts =
[ENV["APP_DOMAIN"]], otherwise require an explicit fallback (e.g.
ENV["ALLOWED_HOSTS"] parsed into an array) or raise a clear boot-time error to
abort startup; locate the config.hosts assignment and replace the conditional
compact/if logic with an explicit presence check that either populates
config.hosts from APP_DOMAIN or ALLOWED_HOSTS (split by comma) or raises an
exception when neither is provided.
In `@config/initializers/content_security_policy.rb`:
- Line 61: policy.report_uri is configured to send reports to
"/csp-violation-report" but there is no endpoint to receive them; add a route
for POST "/csp-violation-report" and implement a CspReportsController (e.g.,
CspReportsController#create) that accepts JSON POST bodies, parses the report
payload, logs or forwards the report (and any identifying request metadata) and
returns an empty 204 response so browsers consider the report delivered; ensure
the controller is CSRF-exempt or uses appropriate skip_before_action for API
requests and that the route accepts JSON content-type.
- Around line 64-66: The current nonce generator uses request.session.id
(config.content_security_policy_nonce_generator) which is constant per session;
replace it with a per-response cryptographically random nonce generated via
SecureRandom (e.g. a URL-safe or base64/hex random string) so each response gets
a fresh unpredictable nonce before enabling CSP enforcement; update
config.content_security_policy_nonce_generator to call SecureRandom for each
invocation and keep config.content_security_policy_nonce_directives =
%w[script-src] unchanged.
In `@test/integration/cors_test.rb`:
- Around line 14-73: Change the CORS assertions to assert the header is
absent/blank rather than just not equal to two values: in the tests "cors does
not reflect wildcard origin for api endpoints", "cors preflight does not allow
arbitrary origin for api endpoints", "cors does not reflect wildcard origin for
oauth token endpoint", "cors preflight does not allow arbitrary origin for oauth
token endpoint", "cors does not reflect wildcard origin for session endpoints",
and "cors preflight does not allow arbitrary origin for session endpoints"
replace the assert_not_equal checks against "*" and the evil origin with a
single assertion that response.headers["Access-Control-Allow-Origin"] is
nil/blank (e.g., assert_nil or assert_blank). Also add a new test mirroring the
oauth token tests that calls POST "/oauth/revoke" and OPTIONS "/oauth/revoke"
with the same Origin and preflight headers and asserts the
Access-Control-Allow-Origin header is absent/blank to cover the changed OAuth
resource.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d426e8a0-ad69-41bb-9d24-864003bbd58c
📒 Files selected for processing (8)
Gemfileconfig/environments/production.rbconfig/initializers/content_security_policy.rbconfig/initializers/cors.rbconfig/initializers/mini_profiler.rbconfig/initializers/sidekiq.rbconfig/routes.rbtest/integration/cors_test.rb
| policy.frame_src "https://cdn.plaid.com", | ||
| "https://js.stripe.com" | ||
|
|
||
| policy.report_uri "/csp-violation-report" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expect: a route and controller/action that accepts CSP violation reports.
rg -n -C3 'csp-violation-report|csp.?violation|content_security_policy.*report'Repository: we-promise/sure
Length of output: 4490
🏁 Script executed:
#!/bin/bash
# Search for routes that handle CSP violations
find . -name "routes.rb" -type f | head -5 | xargs cat | grep -n -i "csp\|violation"Repository: we-promise/sure
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Search for controllers that might handle CSP reports
rg -n "def.*csp|def.*violation|def.*report" --type rb -i | grep -i "csp\|violation.*report"Repository: we-promise/sure
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Look for any references to the endpoint path in controllers/routes
rg -n "csp.violation|csp_violation" --type rbRepository: we-promise/sure
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Find routes.rb file
find . -name "routes.rb" -type fRepository: we-promise/sure
Length of output: 77
🏁 Script executed:
#!/bin/bash
# Search for controllers with CSP or violation handling
find . -path "*/app/controllers/*.rb" -type f | xargs rg -l "csp|violation" -iRepository: we-promise/sure
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Broader search for any action that might handle /csp-violation-report
rg -n "csp.violation|csp_violation" -iRepository: we-promise/sure
Length of output: 961
🏁 Script executed:
#!/bin/bash
# Examine routes.rb for CSP report endpoint
cat ./config/routes.rbRepository: we-promise/sure
Length of output: 16234
🏁 Script executed:
#!/bin/bash
# Search routes.rb specifically for the endpoint
rg "csp|violation" ./config/routes.rb -iRepository: we-promise/sure
Length of output: 41
Wire the CSP report endpoint before relying on report-only telemetry.
report_uri points browsers at /csp-violation-report, but no route or controller exists to handle those requests. CSP violations will result in 404 errors and go uncollected, preventing the policy tuning that report-only mode is designed for.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/initializers/content_security_policy.rb` at line 61, policy.report_uri
is configured to send reports to "/csp-violation-report" but there is no
endpoint to receive them; add a route for POST "/csp-violation-report" and
implement a CspReportsController (e.g., CspReportsController#create) that
accepts JSON POST bodies, parses the report payload, logs or forwards the report
(and any identifying request metadata) and returns an empty 204 response so
browsers consider the report delivered; ensure the controller is CSRF-exempt or
uses appropriate skip_before_action for API requests and that the route accepts
JSON content-type.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57b10c1ff3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| config.active_record.dump_schema_after_migration = false | ||
|
|
||
| # Enable DNS rebinding protection and other `Host` header attacks. | ||
| config.hosts = [ ENV["APP_DOMAIN"] ].compact if ENV["APP_DOMAIN"].present? |
There was a problem hiding this comment.
Restricting config.hosts to only APP_DOMAIN will also block the built-in /up health endpoint whenever a load balancer or orchestrator probes with a different Host header (commonly an IP, internal DNS name, or localhost). In that deployment pattern, readiness/liveness checks start returning 403 and can take instances out of service even though the app is healthy; add a host authorization exclusion for /up (or allow probe hosts) when enabling this restriction.
Useful? React with 👍 / 👎.
| if ENV["ALLOWED_ORIGINS"].present? | ||
| ENV["ALLOWED_ORIGINS"].split(",").map(&:strip) | ||
| elsif ENV["APP_DOMAIN"].present? | ||
| [ "https://#{ENV['APP_DOMAIN']}" ] |
There was a problem hiding this comment.
The APP_DOMAIN fallback always builds an https:// origin, so deployments running without forced SSL (e.g., RAILS_FORCE_SSL=false or HTTP-only internal environments) will never match browser Origin: http://... headers. In those environments, cross-origin browser clients lose CORS access even though APP_DOMAIN is configured, so the fallback should account for HTTP (or allow both schemes) instead of hardcoding HTTPS.
Useful? React with 👍 / 👎.
- CSP nonce: replace session.id-based generator with per-response SecureRandom (a session-constant nonce provides no CSP guarantee once observed). - CSP reports: add POST /csp-violation-report endpoint (CspReportsController) so the report_uri is no longer a dangling reference. - DNS rebinding: accept ALLOWED_HOSTS (comma-separated) as fallback to APP_DOMAIN, emit [SECURITY] boot warning if neither is set. Fail-closed was considered but rejected to preserve backward compatibility with existing self-hosted deploys. - CORS tests: assert the header is nil (stricter than assert_not_equal), add coverage for /oauth/revoke (POST and preflight). - Docs: document the new required/recommended env vars in docs/hosting/docker.md.
|
Thanks — addressed all four. CSP nonce ( CSP CORS tests ( DNS rebinding fail-closed ( I did not raise on missing configuration. Rationale: this PR lands on a self-hosted deploy base where many existing operators don't set
— wolfgang 🤖 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/controllers/csp_reports_controller_test.rb (1)
1-26: LGTM — solid coverage of the three parse branches.Tests correctly exercise the three code paths in
parse_report(JSON success, blank body,JSON::ParserError) and match the browserContent-Type: application/csp-reportcontract. Framework/style is compliant.Optional: consider adding a test that posts a body larger than
MAX_BODY_BYTES(8_192) to lock in the truncation behavior — it's the one behavior in the controller not currently exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/csp_reports_controller_test.rb` around lines 1 - 26, Add a test in CspReportsControllerTest that posts a request body larger than the controller's MAX_BODY_BYTES (8_192) to "/csp-violation-report" to exercise the truncation branch in parse_report; construct a payload string whose byte size exceeds MAX_BODY_BYTES, send it with Content-Type "application/csp-report", and assert the response is :no_content and any expected side-effect (e.g. that no exception is raised or that any truncated-handling path is triggered) so the truncation behavior in the controller is covered.config/initializers/content_security_policy.rb (1)
37-48: Derive the PostHog CSP hosts from the same env var the rest of the app uses.
config/initializers/posthog.rbresolves the PostHog host fromENV.fetch("POSTHOG_HOST", "https://us.i.posthog.com"), but this file hardcodeshttps://us.i.posthog.comandhttps://us-assets.i.posthog.com. If a managed deployment ever overridesPOSTHOG_HOST(e.g., EU region, self-hosted PostHog), CSP will start reporting/blocking the very traffic the initializer enabled — silent drift between two sources of truth.Keeping them in sync avoids that, and also aligns with the guideline to prefer env vars over hardcoded values in configuration.
♻️ Proposed change
if managed_mode + posthog_host = ENV.fetch("POSTHOG_HOST", "https://us.i.posthog.com") + posthog_assets_host = posthog_host.sub("://us.i.", "://us-assets.i.") script_src += [ - "https://us.i.posthog.com", - "https://us-assets.i.posthog.com" + posthog_host, + posthog_assets_host ] connect_src += [ - "https://us.i.posthog.com", - "https://us-assets.i.posthog.com", + posthog_host, + posthog_assets_host, "wss://*.pusher.com" ] endAs per coding guidelines: "Use environment variables instead of hard-coded values in configuration."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/initializers/content_security_policy.rb` around lines 37 - 48, Replace the hardcoded PostHog CSP entries with values derived from the same env var the app uses (ENV.fetch("POSTHOG_HOST", "https://us.i.posthog.com")) so managed_mode appends the resolved POSTHOG_HOST (and the assets host derived from it, e.g., replacing host scheme/hostname with "-assets" subdomain) to script_src and connect_src instead of the literal "https://us.i.posthog.com" and "https://us-assets.i.posthog.com"; update the block that mutates script_src and connect_src (inside the managed_mode branch) to compute posthog_host = ENV.fetch("POSTHOG_HOST", "https://us.i.posthog.com") and an assets_host derived from posthog_host, then push those variables into script_src and connect_src (keeping wss://*.pusher.com unchanged).app/controllers/csp_reports_controller.rb (1)
13-20: Consider log-flood protection for this unauthenticated endpoint.A misconfigured page or a hostile site embedding the app in an iframe can emit a large volume of CSP reports, each producing a
Rails.logger.warnline. With reports enabled in report-only mode across every page load, noisy policies during tuning will amplify this further. Operationally, consider one or more of:
- Sampling or rate-limiting via Rack::Attack (throttle by IP and by
request.referer).- Lowering severity to
:info(or logging at:debugonce tuning stabilizes) to avoid flooding alert-backed WARN channels.- Dropping reports whose
document-uri/source-filedoesn't matchALLOWED_HOSTS/APP_DOMAIN, so cross-origin pages cannot poison the log.Not blocking — flagging since this endpoint has no auth and no throttling in front of it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/csp_reports_controller.rb` around lines 13 - 20, The create action in CspReportsController currently unconditionally logs every parsed CSP report (see create, request.body.read(MAX_BODY_BYTES), parse_report and Rails.logger.warn), which can flood logs; update this method to (1) apply rate-limiting or sampling before logging (e.g. integrate a Rack::Attack throttle keyed by request.ip and request.referer or implement a simple in-process sampler), (2) drop or ignore reports whose report['document-uri'] or report['source-file'] do not match allowed hosts/app domain (use your ALLOWED_HOSTS/APP_DOMAIN config to validate), and (3) lower the log severity from warn to info or debug when appropriate; implement these checks around the parse_report result and only call Rails.logger.warn/info when the report passes filtering and rate limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/csp_reports_controller.rb`:
- Around line 23-28: The parse_report method should not only rescue
JSON::ParserError but any StandardError raised by hostile input; change the
rescue to catch StandardError (or a broad exception class) around JSON.parse in
parse_report and return a safe, sanitized raw payload instead of calling
body.to_s.truncate(512) directly. When building the fallback { raw: ... } value,
coerce and sanitize the body to valid UTF-8 (e.g., use
scrub/force_encoding+encode or an equivalent safe-sanitizer) and then truncate
to 512 bytes/chars inside the rescue so that encoding/truncation cannot raise;
also ensure you log the caught exception via the existing logger for visibility.
Use references to parse_report, JSON.parse, JSON::ParserError, and the raw
fallback to locate the changes.
In `@config/initializers/content_security_policy.rb`:
- Around line 16-62: Replace the hard-coded PostHog hosts in the
content_security_policy block by reading the POSTHOG_HOST env var (as used in
config/initializers/posthog.rb) and deriving the script/connect host(s) (e.g.,
"#{POSTHOG_HOST}" and assets variant) before merging into script_src and
connect_src arrays (update the logic around managed_mode, script_src,
connect_src and policy.script_src/policy.connect_src). Also add the missing
directives inside the same policy block: policy.base_uri :self,
policy.form_action :self, and policy.frame_ancestors :none so they are reported
in report-only mode via policy.report_uri.
---
Nitpick comments:
In `@app/controllers/csp_reports_controller.rb`:
- Around line 13-20: The create action in CspReportsController currently
unconditionally logs every parsed CSP report (see create,
request.body.read(MAX_BODY_BYTES), parse_report and Rails.logger.warn), which
can flood logs; update this method to (1) apply rate-limiting or sampling before
logging (e.g. integrate a Rack::Attack throttle keyed by request.ip and
request.referer or implement a simple in-process sampler), (2) drop or ignore
reports whose report['document-uri'] or report['source-file'] do not match
allowed hosts/app domain (use your ALLOWED_HOSTS/APP_DOMAIN config to validate),
and (3) lower the log severity from warn to info or debug when appropriate;
implement these checks around the parse_report result and only call
Rails.logger.warn/info when the report passes filtering and rate limits.
In `@config/initializers/content_security_policy.rb`:
- Around line 37-48: Replace the hardcoded PostHog CSP entries with values
derived from the same env var the app uses (ENV.fetch("POSTHOG_HOST",
"https://us.i.posthog.com")) so managed_mode appends the resolved POSTHOG_HOST
(and the assets host derived from it, e.g., replacing host scheme/hostname with
"-assets" subdomain) to script_src and connect_src instead of the literal
"https://us.i.posthog.com" and "https://us-assets.i.posthog.com"; update the
block that mutates script_src and connect_src (inside the managed_mode branch)
to compute posthog_host = ENV.fetch("POSTHOG_HOST", "https://us.i.posthog.com")
and an assets_host derived from posthog_host, then push those variables into
script_src and connect_src (keeping wss://*.pusher.com unchanged).
In `@test/controllers/csp_reports_controller_test.rb`:
- Around line 1-26: Add a test in CspReportsControllerTest that posts a request
body larger than the controller's MAX_BODY_BYTES (8_192) to
"/csp-violation-report" to exercise the truncation branch in parse_report;
construct a payload string whose byte size exceeds MAX_BODY_BYTES, send it with
Content-Type "application/csp-report", and assert the response is :no_content
and any expected side-effect (e.g. that no exception is raised or that any
truncated-handling path is triggered) so the truncation behavior in the
controller is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7328b1d-138d-437a-9367-344ae4c49290
📒 Files selected for processing (7)
app/controllers/csp_reports_controller.rbconfig/environments/production.rbconfig/initializers/content_security_policy.rbconfig/routes.rbdocs/hosting/docker.mdtest/controllers/csp_reports_controller_test.rbtest/integration/cors_test.rb
✅ Files skipped from review due to trivial changes (1)
- docs/hosting/docker.md
🚧 Files skipped from review as they are similar to previous changes (3)
- config/routes.rb
- config/environments/production.rb
- test/integration/cors_test.rb
CSP report controller: - parse_report now rescues StandardError (not just JSON::ParserError) so a hostile body with an odd encoding or a class that makes JSON.parse raise something other than ParserError doesn't 500. Forces UTF-8 + scrub before truncating so String#truncate can't blow up on malformed bytes. Folds the exception class into the fallback payload + logs it for operator visibility. - New tests: body > MAX_BODY_BYTES (truncated valid JSON becomes unparseable mid-stream), invalid UTF-8 sequence — both must return 204, not 500. CSP policy: - Added base_uri :self, form_action :self, frame_ancestors :none — standard hardening directives that cost nothing and close tag-hijack / form-relay / framing vectors. - PostHog hosts now derived from ENV["POSTHOG_HOST"] (matching config/initializers/posthog.rb) instead of hardcoded. Managed deploys in non-US regions won't have CSP silently blocking the very traffic the PostHog initializer enabled. Deferred (follow-up): adding rate-limiting / host-filtering to the CSP reports endpoint. Current endpoint logs at :warn, which is operationally noisy under heavy tuning. Worth a separate PR alongside a Sentry / log aggregator wiring; out of scope for the initial hardening.
|
Second round addressed.
CSP hardening directives ( PostHog hosts derived from env ( Test for CSP report log flood / host filtering / severity ( All changes in commit — wolfgang 🤖 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/initializers/content_security_policy.rb`:
- Around line 47-50: The derivation of posthog_assets_host using
posthog_host.sub("://us.i.", "://us-assets.i.") only covers the US region;
update the initializer to first check ENV.fetch("POSTHOG_ASSETS_HOST", nil) and
use that if present, otherwise derive the assets host from the parsed
posthog_host hostname (use URI.parse(posthog_host).host) and replace the region
prefix (e.g., turn "us.i.posthog.com" or "eu.i.posthog.com" into
"us-assets.i.posthog.com"/"eu-assets.i.posthog.com") or, for self-hosted/legacy
hosts, fall back to a sensible default (e.g., append "-assets" to the first
hostname segment) before adding it to script_src; modify the
posthog_host/posthog_assets_host logic (variables posthog_host and
posthog_assets_host and where script_src is extended) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b008f32e-4ffd-46a6-ad07-2768ceac9ebb
📒 Files selected for processing (3)
app/controllers/csp_reports_controller.rbconfig/initializers/content_security_policy.rbtest/controllers/csp_reports_controller_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/csp_reports_controller.rb
CodeRabbit round 3: the previous
`posthog_host.sub("://us.i.", "://us-assets.i.")` literal only worked for
the US region; an EU deployment (`eu.i.posthog.com`) or a self-hosted one
would keep the wrong assets host in the CSP allow-list and silently block
its own analytics.
Now:
- `POSTHOG_ASSETS_HOST` env var wins if set (escape hatch for self-hosted
PostHog with non-standard layouts).
- Otherwise derive by parsing `POSTHOG_HOST` with URI and replacing the
first label with its `-assets` variant:
us.i.posthog.com → us-assets.i.posthog.com
eu.i.posthog.com → eu-assets.i.posthog.com
posthog.example.com → posthog-assets.example.com
|
Third round addressed. PostHog assets host for non-US regions ( Now:
Commit — wolfgang 🤖 |
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/initializers/cors.rb (1)
14-23: Filter empty entries fromALLOWED_ORIGINSfor parity withALLOWED_HOSTS.A trailing comma or stray whitespace-only entry in
ALLOWED_ORIGINSyields""in the allowlist, which is then passed toorigins(*...).config/environments/production.rbalready does.reject(&:empty?)forALLOWED_HOSTS— mirror that here so the two env vars behave consistently and no empty string leaks into Rack::Cors matching.♻️ Proposed refactor
if ENV["ALLOWED_ORIGINS"].present? - ENV["ALLOWED_ORIGINS"].split(",").map(&:strip) + ENV["ALLOWED_ORIGINS"].split(",").map(&:strip).reject(&:empty?) elsif ENV["APP_DOMAIN"].present?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/initializers/cors.rb` around lines 14 - 23, The allowed_origins helper can produce empty-string entries when ALLOWED_ORIGINS contains trailing commas or whitespace; update the allowed_origins method to strip and then filter out empty entries (e.g. use ENV["ALLOWED_ORIGINS"].split(",").map(&:strip).reject(&:empty?) or reject(&:blank?)) so no "" is passed to origins(...), mirroring the ALLOWED_HOSTS handling in production.rb and ensuring Rack::Cors doesn't receive empty origins; change the branch that returns the split list in allowed_origins accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/environments/production.rb`:
- Around line 104-115: The else-branch currently calls Rails.logger.warn while
Rails.logger is nil during early boot; change the warning to use the
already-configured config.logger instead so the boot won't crash when
ENV["APP_DOMAIN"] and ENV["ALLOWED_HOSTS"] are absent. Update the production
environment block that sets config.hosts (the if/elsif/else checking
ENV["APP_DOMAIN"] and ENV["ALLOWED_HOSTS"]) to call config.logger.warn(...)
rather than Rails.logger.warn(...) to safely emit the [SECURITY] warning.
---
Nitpick comments:
In `@config/initializers/cors.rb`:
- Around line 14-23: The allowed_origins helper can produce empty-string entries
when ALLOWED_ORIGINS contains trailing commas or whitespace; update the
allowed_origins method to strip and then filter out empty entries (e.g. use
ENV["ALLOWED_ORIGINS"].split(",").map(&:strip).reject(&:empty?) or
reject(&:blank?)) so no "" is passed to origins(...), mirroring the
ALLOWED_HOSTS handling in production.rb and ensuring Rack::Cors doesn't receive
empty origins; change the branch that returns the split list in allowed_origins
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ad785b5-111f-4ea2-b5b7-0359788426e2
📒 Files selected for processing (11)
Gemfileapp/controllers/csp_reports_controller.rbconfig/environments/production.rbconfig/initializers/content_security_policy.rbconfig/initializers/cors.rbconfig/initializers/mini_profiler.rbconfig/initializers/sidekiq.rbconfig/routes.rbdocs/hosting/docker.mdtest/controllers/csp_reports_controller_test.rbtest/integration/cors_test.rb
…ORIGINS CodeRabbit round 4: - production.rb: use `config.logger&.warn` instead of `Rails.logger.warn` in the config.hosts fallback branch — this block runs during environment configuration, before Rails.logger is finalized, so the prior call could NoMethodError-nil the warning and swallow the [SECURITY] notice. - cors.rb: mirror the reject(&:empty?) from production.rb's ALLOWED_HOSTS handling so a trailing comma or whitespace-only entry in ALLOWED_ORIGINS doesn't leak an empty string into Rack::Cors.origins(...).
|
Round 4.
Filter empty entries from Commit — wolfgang 🤖 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/environments/production.rb`:
- Around line 111-112: Parse ENV["ALLOWED_HOSTS"] into a host list first (e.g.
call split(",").map(&:strip).reject(&:empty?)) and only assign it to
config.hosts when that parsed list is non-empty; do not rely on
ENV["ALLOWED_HOSTS"].present? alone because values like "," are present but
yield an empty array—ensure the check uses the parsed array (hosts.any?) before
setting config.hosts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4359b52-21e1-49fc-8592-c8e942f4a94f
📒 Files selected for processing (2)
config/environments/production.rbconfig/initializers/cors.rb
CodeRabbit round 5: ENV["ALLOWED_HOSTS"].present? is true for inputs like "," or " " that reduce to an empty array after split+strip+reject. Assigning config.hosts = [] means "deny every host", which is strictly worse than falling through to the warn-and-leave-unset branch. Parse first, then only assign when the resulting list has at least one entry; otherwise emit the existing [SECURITY] warning about DNS rebinding being disabled.
|
Good catch.
Parse first, then only assign when the resulting list has entries; else emit the existing Commit — wolfgang 🤖 |
2 participants
Summary
Pure configuration and infrastructure hardening — no controller/model changes. Introduces several env vars required in production.
Part of the security hardening originally bundled in #1104 (closed), now split by functional area per @jjmata's feedback. Companion PRs: #1517 #1518 #1519 #1520 #1521.
Findings addressed (7)
sure/sure— requireSIDEKIQ_WEB_USERNAME+SIDEKIQ_WEB_PASSWORD, mount disabled if unset (config/initializers/sidekiq.rb)config/routes.rb)require_master_keydisabled — enforced inconfig/environments/production.rborigins "*"— replaced withALLOWED_ORIGINS/APP_DOMAINallowlist (config/initializers/cors.rb)app_mode.managed?(config/initializers/content_security_policy.rb)config.hosts = [APP_DOMAIN]when set (config/environments/production.rb):developmentgroup (Gemfile) + env guard in initializerMigration notes — required env vars for production
SIDEKIQ_WEB_USERNAME+SIDEKIQ_WEB_PASSWORD/sidekiq— UI inaccessible if unsetRAILS_MASTER_KEY(orconfig/master.key)ALLOWED_ORIGINSAPP_DOMAINconfig.hostsBoot-time
[SECURITY]warning is logged if none ofALLOWED_ORIGINS/APP_DOMAINis set.Drive-by fix
config/initializers/sidekiq.rbon main used&(bitwise AND) instead of&&between the twosecure_comparecalls. The FIX-09 rewrite corrects this.CSP caveat
The policy sets
report_uri "/csp-violation-report"but that path is not wired up. Browsers will 404 on reports. Leaving as-is for a follow-up to wire to Sentry or equivalent.Tests
test/integration/cors_test.rbrewritten to assert the allowlist contract (7/7 pass).bin/rubocopclean,bin/brakeman0 warnings.Out of scope
Related
Summary by CodeRabbit
New Features
Chores
Documentation
Tests