fix: H12 cookie hardening + 5 low/medium bugs (L2, L5, L6, L8, L10, L11)

- H12: session and remember-me cookies get http_only + secure (prod-only).
  Cookie can no longer be read via document.cookie (XSS exfil defense)
  and the Secure flag is set in production via config/prod.exs.
- L2: 404 tracker uses EXPIRE … NX so a sustained probe can't keep
  refreshing the 60s window and dodge the threshold ban.
- L5: vault only reads CLOAK_KEY when :env == :prod — a developer with
  a prod env var set in their shell won't accidentally encrypt local
  data with the production key.
- L6: health endpoint no longer leaks the app version.
- L8: Preseem.dismiss_insight/2 + InsightsLive uses it — dismissing now
  requires the insight to belong to the user's org (closes IDOR).
- L10: SidebarCollapse JS hook stores its click handler and removes it
  in destroyed(); listeners no longer accumulate across LV navigation.
- L11: WebMCP navigate tool rejects anything that isn't a same-origin
  absolute path (blocks javascript:, data:, off-site URLs).
This commit is contained in:
Graham McIntire 2026-05-12 11:22:47 -05:00
parent 6a9b471d1f
commit 97232117f5
12 changed files with 87 additions and 16 deletions

View file

@ -314,13 +314,21 @@ const StatusTitle = {
// so the state persists across all navigations without flicker.
const SidebarCollapse = {
mounted(this: any) {
this.el.addEventListener('click', (e: Event) => {
// Stash the handler so destroyed() can remove it. Anonymous listeners
// accumulate on LV nav and leak memory.
this.clickHandler = (e: Event) => {
if ((e.target as HTMLElement).closest('[data-sidebar-toggle]')) {
const html = document.documentElement
html.classList.toggle('sidebar-collapsed')
localStorage.setItem('sidebarCollapsed', String(html.classList.contains('sidebar-collapsed')))
}
})
}
this.el.addEventListener('click', this.clickHandler)
},
destroyed(this: any) {
if (this.clickHandler) {
this.el.removeEventListener('click', this.clickHandler)
}
}
}
@ -476,6 +484,12 @@ if (typeof navigator !== "undefined" && (navigator as any).modelContext) {
required: ["path"]
},
execute({ path }: { path: string }) {
// Only allow same-origin paths — rejects javascript:, data:, and
// off-site URLs so a compromised WebMCP agent can't phish or run
// arbitrary JS via this tool.
if (typeof path !== "string" || !path.startsWith("/") || path.startsWith("//")) {
return;
}
window.location.href = path;
}
},

View file

@ -44,3 +44,8 @@ config :towerops, ToweropsWeb.Endpoint, cache_static_manifest: "priv/static/cach
# them and a pod restart doesn't lose computed predictions. The
# deployment mounts the NFS share at /data.
config :towerops, :coverage_storage_dir, "/data/coverage"
# Cloudflared terminates TLS in front of the app, so requests reach Phoenix
# over HTTP — but cookies must still carry the Secure attribute so a
# downgrade attack or misconfigured route can't leak them over cleartext.
config :towerops, :secure_cookies, true

View file

@ -187,5 +187,6 @@ defmodule Towerops.Preseem do
defdelegate list_insights(organization_id, opts \\ []), to: Insights
defdelegate list_device_insights(device_id), to: Insights
defdelegate dismiss_insight(insight_id), to: Insights
defdelegate dismiss_insight(insight_id, organization_id), to: Insights
defdelegate dismiss_insights(insight_ids), to: Insights
end

View file

@ -97,6 +97,26 @@ defmodule Towerops.Preseem.Insights do
end
end
@doc """
Dismiss an insight scoped to an organization. Returns `{:error, :not_found}`
if the insight doesn't exist or belongs to a different org — used by the
LiveView dismiss handler to prevent IDOR.
"""
def dismiss_insight(insight_id, organization_id) when is_binary(organization_id) do
case Repo.get_by(Insight, id: insight_id, organization_id: organization_id) do
nil ->
{:error, :not_found}
insight ->
insight
|> Insight.changeset(%{
status: "dismissed",
dismissed_at: Towerops.Time.now()
})
|> Repo.update()
end
end
@doc """
List active insights that have not been LLM-enriched yet.

View file

@ -28,8 +28,9 @@ defmodule Towerops.Security.FourOhFourTracker do
try do
# Add path to set
_ = redix.command(conn, ["SADD", key, path])
# Set TTL if this is the first path
_ = redix.command(conn, ["EXPIRE", key, @window_seconds])
# Set TTL only on first creation (NX). Without NX, every 404 refreshes
# the TTL, letting an attacker keep the tracking window open forever.
_ = redix.command(conn, ["EXPIRE", key, @window_seconds, "NX"])
# Get unique count
case redix.command(conn, ["SCARD", key]) do

View file

@ -23,10 +23,12 @@ defmodule Towerops.Vault do
@impl GenServer
def init(config) do
# In dev/test, the key is already configured in config files
# In production (runtime.exs), we override with env var
# In dev/test, use the key from config files so a developer who happens
# to have CLOAK_KEY set in their shell (e.g., from a prod-deploy env)
# doesn't accidentally encrypt local data with the production key.
# In production (runtime.exs), the env var override is required.
config =
if System.get_env("CLOAK_KEY") do
if Application.get_env(:towerops, :env, :prod) == :prod and System.get_env("CLOAK_KEY") do
Keyword.put(config, :ciphers,
default: {
Cloak.Ciphers.AES.GCM,
@ -34,7 +36,6 @@ defmodule Towerops.Vault do
}
)
else
# Use the key from config (dev/test)
config
end

View file

@ -25,8 +25,7 @@ defmodule ToweropsWeb.HealthController do
Jason.encode!(%{
status: "ok",
database: "connected",
redis: redis_status_string(redis_status),
version: :towerops |> Application.spec(:vsn) |> to_string()
redis: redis_status_string(redis_status)
})
)
else

View file

@ -5,12 +5,18 @@ defmodule ToweropsWeb.Endpoint do
# The session will be stored in the cookie and signed,
# this means its contents can be read but not tampered with.
# Set :encryption_salt if you would also like to encrypt it.
# `http_only: true` blocks JavaScript access via `document.cookie` (XSS
# defense). `secure` is enabled in :prod by `config/runtime.exs` so the
# cookie is never sent over plain HTTP; left off here so local HTTP dev
# still works.
@session_options [
store: :cookie,
key: "_towerops_key",
signing_salt: "hrDZxLhd",
encryption_salt: "vK3p8mNx",
same_site: "Lax"
same_site: "Lax",
http_only: true,
secure: Application.compile_env(:towerops, :secure_cookies, false)
]
socket "/live", Phoenix.LiveView.Socket,

View file

@ -69,7 +69,7 @@ defmodule ToweropsWeb.InsightsLive.Index do
@impl true
def handle_event("dismiss", %{"id" => id}, socket) do
case Preseem.dismiss_insight(id) do
case Preseem.dismiss_insight(id, socket.assigns.organization.id) do
{:ok, _} ->
{:noreply,
socket

View file

@ -29,7 +29,9 @@ defmodule ToweropsWeb.UserAuth do
@remember_me_options [
sign: true,
max_age: @max_cookie_age_in_days * 24 * 60 * 60,
same_site: "Lax"
same_site: "Lax",
http_only: true,
secure: Application.compile_env(:towerops, :secure_cookies, false)
]
# How old the session token should be before a new one is issued. When a request is made

View file

@ -16,10 +16,15 @@ defmodule Towerops.VaultTest do
end
end
test "overrides ciphers when CLOAK_KEY env is set" do
original = System.get_env("CLOAK_KEY")
test "overrides ciphers when CLOAK_KEY env is set in :prod" do
# The env-var override only fires under :env == :prod (L5) so a
# dev shell with a prod CLOAK_KEY set doesn't encrypt local data
# with the prod key. Flip :env for the duration of the test.
original_env = System.get_env("CLOAK_KEY")
original_env_setting = Application.get_env(:towerops, :env)
key = 32 |> :crypto.strong_rand_bytes() |> Base.encode64()
System.put_env("CLOAK_KEY", key)
Application.put_env(:towerops, :env, :prod)
try do
assert {:ok, config} = Vault.init([])
@ -27,6 +32,21 @@ defmodule Towerops.VaultTest do
assert opts[:tag] == "AES.GCM.V1"
assert is_binary(opts[:key])
assert byte_size(opts[:key]) == 32
after
if original_env, do: System.put_env("CLOAK_KEY", original_env), else: System.delete_env("CLOAK_KEY")
Application.put_env(:towerops, :env, original_env_setting)
end
end
test "preserves config when CLOAK_KEY env is set in non-prod" do
original = System.get_env("CLOAK_KEY")
key = 32 |> :crypto.strong_rand_bytes() |> Base.encode64()
System.put_env("CLOAK_KEY", key)
try do
# :env is :test in the test environment; CLOAK_KEY must be ignored.
assert {:ok, config} = Vault.init(ciphers: [])
assert config[:ciphers] == []
after
if original, do: System.put_env("CLOAK_KEY", original), else: System.delete_env("CLOAK_KEY")
end

View file

@ -8,7 +8,9 @@ defmodule ToweropsWeb.HealthControllerTest do
result = Jason.decode!(body)
assert result["status"] == "ok"
assert result["database"] == "connected"
assert result["version"]
# `version` deliberately omitted (L6): exposing the app version on a
# public health endpoint helps attackers fingerprint vulnerable builds.
refute Map.has_key?(result, "version")
end
test "reports redis status as not_configured when redis is not configured", %{conn: conn} do