From d288e7049bc7c5c5cf9c3c2eb62d9d4300682c7c Mon Sep 17 00:00:00 2001 From: Cristen Jones Date: Fri, 8 Nov 2024 11:05:03 -0500 Subject: [PATCH] feat(Plugs.SecureHeaders): add secure headers to errors and static files (#2216) * chore(ControllerHelpers): consolidate rendering 404s and 500s * feat(Plugs.SecureHeaders): add even more headers * feat(DotcomWeb.Router): handle errors with secure headers * reorg * typo * avoid compile vs runtime check --- config/config.exs | 1 + config/runtime.exs | 66 ++++++------- lib/dotcom_web/controllers/cms_controller.ex | 5 +- lib/dotcom_web/controllers/helpers.ex | 10 +- .../controllers/transit_near_me_controller.ex | 5 +- lib/dotcom_web/plugs/secure_headers.ex | 97 +++++++++++++++++++ lib/dotcom_web/plugs/static.ex | 10 +- lib/dotcom_web/redirector.ex | 2 +- lib/dotcom_web/router.ex | 35 ++++--- lib/dotcom_web/views/error_view.ex | 29 ------ test/dotcom_web/controllers/helpers_test.exs | 12 --- test/dotcom_web/plugs/secure_headers_test.exs | 28 ++++++ 12 files changed, 190 insertions(+), 110 deletions(-) create mode 100644 lib/dotcom_web/plugs/secure_headers.ex create mode 100644 test/dotcom_web/plugs/secure_headers_test.exs diff --git a/config/config.exs b/config/config.exs index 73387783f4..54b74281f4 100644 --- a/config/config.exs +++ b/config/config.exs @@ -4,6 +4,7 @@ config :elixir, ansi_enabled: true config :dotcom, :aws_client, AwsClient.Behaviour +config :dotcom, :content_security_policy_definition, "" config :dotcom, :cms_api_module, CMS.Api config :dotcom, :httpoison, HTTPoison diff --git a/config/runtime.exs b/config/runtime.exs index 6c3b7c74a3..f115253b46 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -144,6 +144,33 @@ end if config_env() == :prod do config :dotcom, alerts_bus_stop_change_bucket: System.get_env("S3_PREFIX_BUSCHANGE") + # Extract the host fron the sentry dsn + sentry_dsn_host = + case Regex.run(~r/@(.*)\//, System.get_env("SENTRY_DSN", ""), capture: :all_but_first) do + nil -> "" + [match | _] -> match + end + + static_host = System.get_env("STATIC_HOST", "") + + config :dotcom, + :content_security_policy_definition, + DotcomWeb.Plugs.SecureHeaders.base_csp_directives() + |> Enum.map(fn + {:connect, directive} -> + directive ++ ["wss://#{host}", sentry_dsn_host] + + {:img, directive} -> + directive ++ [static_host, System.get_env("CMS_API_BASE_URL", "")] + + {key, directive} when key in [:font, :script, :style] -> + directive ++ [static_host] + + {_, directive} -> + directive + end) + |> Enum.map_join("; ", &Enum.join(&1, " ")) + config :dotcom, DotcomWeb.Endpoint, http: [ # Enable IPv6 and bind on all interfaces. @@ -205,42 +232,3 @@ if System.get_env("LOGGER_LEVEL") in ~w(emergency alert critical error warning n config :logger, level: String.to_atom(System.get_env("LOGGER_LEVEL")) config :logger, :console, level: String.to_atom(System.get_env("LOGGER_LEVEL")) end - -# Extract the host fron the sentry dsn -sentry_dsn_host = - case Regex.run(~r/@(.*)\//, System.get_env("SENTRY_DSN", ""), capture: :all_but_first) do - nil -> "" - [match | _] -> match - end - -# Set the content security policy -case config_env() do - :prod -> - config :dotcom, - :content_security_policy_definition, - Enum.join( - [ - "default-src 'none'", - "img-src 'self' cdn.mbta.com #{System.get_env("STATIC_HOST", "")} #{System.get_env("CMS_API_BASE_URL", "")} px.ads.linkedin.com www.linkedin.com www.facebook.com *.google.com *.googleapis.com *.gstatic.com *.s3.amazonaws.com data: i.ytimg.com www.googletagmanager.com *.arcgis.com", - "style-src 'self' 'unsafe-inline' www.gstatic.com #{System.get_env("STATIC_HOST", "")} cdn.jsdelivr.net", - "script-src 'self' 'unsafe-eval' 'unsafe-inline' #{System.get_env("STATIC_HOST", "")} insitez.blob.core.windows.net snap.licdn.com connect.facebook.net www.instagram.com www.google-analytics.com *.google.com www.gstatic.com www.googletagmanager.com *.googleapis.com data.mbta.com *.arcgis.com", - "font-src 'self' #{System.get_env("STATIC_HOST", "")}", - "connect-src 'self' wss://#{host} #{sentry_dsn_host || ""} *.googleapis.com analytics.google.com www.google-analytics.com www.google.com px.ads.linkedin.com stats.g.doubleclick.net *.arcgis.com *.s3.amazonaws.com", - "frame-src 'self' data.mbta.com www.youtube.com www.google.com cdn.knightlab.com livestream.com www.instagram.com *.arcgis.com", - "worker-src blob: ;" - ], - "; " - ) - - # Dev is only used for local development, so we don't need, and in - # fact actively do not want, a restrictive CSP - :dev -> - config :dotcom, :content_security_policy_definition, "" - - :test -> - config :dotcom, :content_security_policy_definition, "" - - # Unknown env, reject all - _ -> - config :dotcom, :content_security_policy_definition, "default-src 'none'" -end diff --git a/lib/dotcom_web/controllers/cms_controller.ex b/lib/dotcom_web/controllers/cms_controller.ex index a2e83126ba..e8c73797f5 100644 --- a/lib/dotcom_web/controllers/cms_controller.ex +++ b/lib/dotcom_web/controllers/cms_controller.ex @@ -92,10 +92,7 @@ defmodule DotcomWeb.CMSController do end defp handle_page_response({:error, _}, conn) do - conn - |> put_status(500) - |> put_view(DotcomWeb.ErrorView) - |> render("500.html", []) + render_500(conn) end @spec render_page(Conn.t(), Page.t()) :: Conn.t() diff --git a/lib/dotcom_web/controllers/helpers.ex b/lib/dotcom_web/controllers/helpers.ex index d84d362455..7c7f50d6cf 100644 --- a/lib/dotcom_web/controllers/helpers.ex +++ b/lib/dotcom_web/controllers/helpers.ex @@ -2,7 +2,7 @@ defmodule DotcomWeb.ControllerHelpers do @moduledoc false import Plug.Conn, only: [halt: 1, put_resp_content_type: 2, put_status: 2] - import Phoenix.Controller, only: [render: 3, put_view: 2, put_layout: 2] + import Phoenix.Controller, only: [render: 3, put_view: 2] alias Alerts.{Alert, InformedEntity, Match, Repo} alias DotcomWeb.CMSController @@ -39,13 +39,11 @@ defmodule DotcomWeb.ControllerHelpers do |> halt() end - def render_not_found(conn) do + def render_500(conn) do conn - |> put_status(:not_found) - |> put_layout({DotcomWeb.LayoutView, "root.html"}) + |> put_status(:internal_server_error) |> put_view(DotcomWeb.ErrorView) - |> render("404.html", []) - |> halt() + |> render("500.html", []) end def return_internal_error(conn), diff --git a/lib/dotcom_web/controllers/transit_near_me_controller.ex b/lib/dotcom_web/controllers/transit_near_me_controller.ex index 7dd5688973..36311d6669 100644 --- a/lib/dotcom_web/controllers/transit_near_me_controller.ex +++ b/lib/dotcom_web/controllers/transit_near_me_controller.ex @@ -16,10 +16,7 @@ defmodule DotcomWeb.TransitNearMeController do |> flash_if_error() |> case do :error -> - conn - |> put_status(:internal_server_error) - |> put_view(DotcomWeb.ErrorView) - |> render("500.html", []) + render_500(conn) %Conn{} = loaded_conn -> render(loaded_conn, "index.html", breadcrumbs: [Breadcrumb.build("Transit Near Me")]) diff --git a/lib/dotcom_web/plugs/secure_headers.ex b/lib/dotcom_web/plugs/secure_headers.ex new file mode 100644 index 0000000000..1f75c25c10 --- /dev/null +++ b/lib/dotcom_web/plugs/secure_headers.ex @@ -0,0 +1,97 @@ +defmodule DotcomWeb.Plugs.SecureHeaders do + @moduledoc """ + Configures secure browser headers for Dotcom requests, including specifying + content security policy directives at funtime. + """ + + @base_csp_directives %{ + connect: ~w[ + connect-src + 'self' + *.arcgis.com + *.googleapis.com + *.s3.amazonaws.com + analytics.google.com + px.ads.linkedin.com + stats.g.doubleclick.net + www.google-analytics.com + www.google.com + ], + default: ~w[default-src 'none'], + font: ~w[font-src 'self'], + frame: ~w[ + frame-src + 'self' + *.arcgis.com + cdn.knightlab.com + data.mbta.com + livestream.com + www.youtube.com + www.google.com + www.instagram.com + ], + img: ~w[ + img-src + 'self' + *.arcgis.com + *.google.com + *.googleapis.com + *.gstatic.com + *.s3.amazonaws.com + cdn.mbta.com + data: + i.ytimg.com + px.ads.linkedin.com + www.linkedin.com + www.facebook.com + www.googletagmanager.com + ], + script: ~w[ + script-src + 'self' + 'unsafe-eval' + 'unsafe-inline' + *.arcgis.com + *.google.com + *.googleapis.com + connect.facebook.net + data.mbta.com + insitez.blob.core.windows.net + snap.licdn.com + www.instagram.com + www.google-analytics.com + www.gstatic.com + www.googletagmanager.com + ], + style: ~w[style-src 'self' 'unsafe-inline' www.gstatic.com], + worker: ~w[worker-src blob: ;] + } + + @default_secure_headers %{ + "strict-transport-security" => "max-age=31536000", + "x-content-type-options" => "nosniff", + "x-frame-options" => "DENY", + "x-xss-protection" => "1; mode=block" + } + + @behaviour Plug + + @impl Plug + def init(opts), do: opts + + @impl Plug + @spec call(Plug.Conn.t(), any()) :: Plug.Conn.t() + def call(conn, _) do + Phoenix.Controller.put_secure_browser_headers(conn, default_secure_headers()) + end + + def base_csp_directives, do: @base_csp_directives + + def default_secure_headers, + do: + @default_secure_headers + |> Map.put( + "content-security-policy", + Application.get_env(:dotcom, :content_security_policy_definition, "") + ) +end diff --git a/lib/dotcom_web/plugs/static.ex b/lib/dotcom_web/plugs/static.ex index 57c1564d99..c4574b4d84 100644 --- a/lib/dotcom_web/plugs/static.ex +++ b/lib/dotcom_web/plugs/static.ex @@ -1,5 +1,11 @@ defmodule DotcomWeb.Plugs.Static do + @moduledoc """ + Configure Plug.Static for Dotcom + """ + use Plug.Builder + + import DotcomWeb.Plugs.SecureHeaders, only: [default_secure_headers: 0] import Phoenix.Controller, only: [redirect: 2] plug(:check_if_apple_touch_icon_request) @@ -9,8 +15,8 @@ defmodule DotcomWeb.Plugs.Static do at: "/", from: :dotcom, gzip: Mix.env() == :prod, - headers: %{"access-control-allow-origin" => "*"}, - cache_control_for_etags: "public, max-age=86400", + headers: default_secure_headers() |> Map.put("access-control-allow-origin", "*"), + cache_control_for_etags: "max-age=86400", only: DotcomWeb.static_paths(), only_matching: ~w(favicon apple-touch-icon) ) diff --git a/lib/dotcom_web/redirector.ex b/lib/dotcom_web/redirector.ex index 6eb2eea9d3..8895a85097 100644 --- a/lib/dotcom_web/redirector.ex +++ b/lib/dotcom_web/redirector.ex @@ -16,7 +16,7 @@ defmodule DotcomWeb.Redirector do @spec call(Plug.Conn.t(), Keyword.t()) :: Plug.Conn.t() def call(%Plug.Conn{params: %{"id" => id}} = conn, to: to) when to not in ["/projects"] do case find_record(id, to) do - :not_found -> DotcomWeb.ControllerHelpers.render_not_found(conn) + :not_found -> DotcomWeb.ControllerHelpers.render_404(conn) record -> redirect_to_show(conn, to, record) end end diff --git a/lib/dotcom_web/router.ex b/lib/dotcom_web/router.ex index d70e4a1b5e..919868ebc6 100644 --- a/lib/dotcom_web/router.ex +++ b/lib/dotcom_web/router.ex @@ -2,6 +2,24 @@ defmodule DotcomWeb.Router do @moduledoc false use DotcomWeb, :router + use Plug.ErrorHandler + + alias DotcomWeb.ControllerHelpers + + @impl Plug.ErrorHandler + def handle_errors(conn, %{reason: reason}) do + conn + |> DotcomWeb.Plugs.SecureHeaders.call([]) + |> then(fn conn -> + case reason do + %Phoenix.Router.NoRouteError{plug_status: 404} -> + ControllerHelpers.render_404(conn) + + _ -> + ControllerHelpers.render_500(conn) + end + end) + end alias DotcomWeb.StaticPage @@ -16,16 +34,16 @@ defmodule DotcomWeb.Router do plug(:fetch_session) plug(:fetch_flash) plug(:fetch_cookies) - plug(:put_secure_browser_headers_runtime, %{}) plug(:put_root_layout, {DotcomWeb.LayoutView, :root}) - plug(DotcomWeb.Plugs.CanonicalHostname) plug(DotcomWeb.Plugs.Banner) + plug(DotcomWeb.Plugs.CanonicalHostname) + plug(DotcomWeb.Plugs.ClearCookies) + plug(DotcomWeb.Plugs.Cookies) plug(DotcomWeb.Plugs.CommonFares) plug(DotcomWeb.Plugs.Date) plug(DotcomWeb.Plugs.DateTime) plug(DotcomWeb.Plugs.RewriteUrls) - plug(DotcomWeb.Plugs.ClearCookies) - plug(DotcomWeb.Plugs.Cookies) + plug(DotcomWeb.Plugs.SecureHeaders) plug(:optional_disable_indexing) end @@ -321,13 +339,4 @@ defmodule DotcomWeb.Router do Plug.Conn.put_resp_header(conn, "x-robots-tag", "noindex") end end - - defp put_secure_browser_headers_runtime(conn, default_headers) do - runtime_headers = %{ - "content-security-policy" => - Application.get_env(:dotcom, :content_security_policy_definition) - } - - put_secure_browser_headers(conn, Map.merge(default_headers, runtime_headers)) - end end diff --git a/lib/dotcom_web/views/error_view.ex b/lib/dotcom_web/views/error_view.ex index dbc317013b..1d9520dc54 100644 --- a/lib/dotcom_web/views/error_view.ex +++ b/lib/dotcom_web/views/error_view.ex @@ -29,35 +29,6 @@ defmodule DotcomWeb.ErrorView do ) end - # Stubbed out, but untested - # def render("403.html", assigns) do - # render(__MODULE__, "error_layout.html", Map.merge(assigns, %{ - # error_code: "403", - # error_type: "Access denied", - # error_title: "This stop is closed.", - # error_description: "Unfortunately, you do not have access to this page.", - # error_instructions: "Try searching for what you're looking for below." - # })) - # end - # def render("404_missing_file.html", assigns) do - # render(__MODULE__, "error_layout.html", Map.merge(assigns, %{ - # error_code: "404", - # error_type: "File not found", - # error_title: "Uh oh! We can't find the file.", - # error_description: "The file you were looking for cannot be found.", - # error_instructions: "We may have deleted it. Try searching for what you're looking for below." - # })) - # end - # def render("503.html", assigns) do - # render(__MODULE__, "error_layout.html", Map.merge(assigns, %{ - # error_code: "503", - # error_type: "Maintenance", - # error_title: "Service change in effect.", - # error_description: "This page is down for maintenance.", - # error_instructions: "Please check back after at least 24 hours..." - # })) - # end - # In case no render clause matches or no # template is found, let's render it as 500 def template_not_found(_template, assigns) do diff --git a/test/dotcom_web/controllers/helpers_test.exs b/test/dotcom_web/controllers/helpers_test.exs index c27d680be5..e072b3a44c 100644 --- a/test/dotcom_web/controllers/helpers_test.exs +++ b/test/dotcom_web/controllers/helpers_test.exs @@ -24,18 +24,6 @@ defmodule DotcomWeb.ControllerHelpersTest do end end - describe "render_not_found/1" do - test "renders the 404 bus page", %{conn: conn} do - rendered = - conn - |> put_private(:phoenix_endpoint, DotcomWeb.Endpoint) - |> render_not_found() - |> html_response(404) - - assert rendered =~ "This page is no longer in service." - end - end - describe "return_internal_error/1" do test "renders an 'Internal error' 500 json response" do response = diff --git a/test/dotcom_web/plugs/secure_headers_test.exs b/test/dotcom_web/plugs/secure_headers_test.exs new file mode 100644 index 0000000000..aea210fc67 --- /dev/null +++ b/test/dotcom_web/plugs/secure_headers_test.exs @@ -0,0 +1,28 @@ +defmodule DotcomWeb.Plugs.SecureHeadersTest do + use DotcomWeb.ConnCase, async: true + + import DotcomWeb.Plugs.SecureHeaders, only: [call: 2] + import Plug.Conn, only: [get_resp_header: 2] + + describe "call/2" do + test "puts the secure headers", %{conn: conn} do + refute has_security_headers?(conn) + conn = call(conn, []) + assert has_security_headers?(conn) + end + end + + defp has_security_headers?(conn) do + [ + "content-security-policy", + "strict-transport-security", + "x-content-type-options", + "x-frame-options", + "x-xss-protection" + ] + |> Enum.all?(&(get_resp_header(conn, &1) |> has_value())) + end + + defp has_value([_ | _]), do: true + defp has_value(_), do: false +end