diff --git a/test_integrations/phoenix_app/config/dev.exs b/test_integrations/phoenix_app/config/dev.exs index 0206b416..84f59c9f 100644 --- a/test_integrations/phoenix_app/config/dev.exs +++ b/test_integrations/phoenix_app/config/dev.exs @@ -93,7 +93,8 @@ config :sentry, logs: [ level: :info, metadata: :all - ] + ], + scrubber: [param_keys: ["internal_ref"]] config :phoenix_app, Oban, repo: PhoenixApp.Repo, diff --git a/test_integrations/phoenix_app/lib/phoenix_app_web/controllers/scrubbing_demo_controller.ex b/test_integrations/phoenix_app/lib/phoenix_app_web/controllers/scrubbing_demo_controller.ex new file mode 100644 index 00000000..43813b1d --- /dev/null +++ b/test_integrations/phoenix_app/lib/phoenix_app_web/controllers/scrubbing_demo_controller.ex @@ -0,0 +1,71 @@ +defmodule PhoenixAppWeb.ScrubbingDemoController do + @moduledoc """ + Fixture for the parameter-scrubbing e2e spec. + + `index/2` renders a page to interact with: a reset form to fill in and submit, + and links carrying a secret in the path. `reset_password/2` never matches the + params those send, so Phoenix raises a `Phoenix.ActionClauseError` whose + message embeds the whole `%Plug.Conn{}`. That is what puts `request_path`, + `path_info`, `path_params` and `query_string` into the reported event, + alongside the request interface, so the spec can assert on each of them. + """ + + use PhoenixAppWeb, :controller + + plug Sentry.PlugContext, + [url_scrubber: {__MODULE__, :scrub_url}] when action == :reset_password + + plug :tag_run when action == :reset_password + + # The spec owns the exact bytes it wants on the wire, so the links echo the + # query this page was opened with rather than rebuilding it — rebuilding would + # re-encode it and hide what the scrubber does or does not rewrite. That puts + # the secrets in this page's own URL, which is the point: the browser sends it + # as the `Referer` of everything clicked from here. + def index(conn, params) do + path_secret = Map.get(params, "path_secret", "pathsecret") + reset_path = "/scrubbing-demo/reset-password/#{path_secret}" + query = drop_param(conn.query_string, "path_secret") + + render(conn, :index, + form_action: reset_path, + probe_href: "#{reset_path}?#{query}", + forwarded_href: "/scrubbing-demo/forwarded/reset-password/#{path_secret}?#{query}", + keep: Map.get(params, "keep", ""), + run: Map.get(params, "e2e_run", "") + ) + end + + defp drop_param(query_string, name) do + query_string + |> String.split("&") + |> Enum.reject(&String.starts_with?(&1, "#{name}=")) + |> Enum.join("&") + end + + def reset_password(conn, %{"confirmed" => true}), do: json(conn, %{status: "ok"}) + + @doc """ + Redacts the token segment of the demo path on top of the SDK's default URL + scrubbing, the way an application would for a URL that carries a secret. + """ + def scrub_url(conn) do + conn + |> Sentry.PlugContext.default_url_scrubber() + |> String.replace( + ~r{/reset-password/[^/?]+}, + "/reset-password/#{Sentry.Scrubber.scrubbed_value()}" + ) + end + + defp tag_run(conn, _opts) do + case conn.params do + %{"e2e_run" => run_id} when is_binary(run_id) -> + Sentry.Context.set_tags_context(%{"e2e_run" => run_id}) + conn + + _ -> + conn + end + end +end diff --git a/test_integrations/phoenix_app/lib/phoenix_app_web/controllers/scrubbing_demo_html.ex b/test_integrations/phoenix_app/lib/phoenix_app_web/controllers/scrubbing_demo_html.ex new file mode 100644 index 00000000..eabadb7f --- /dev/null +++ b/test_integrations/phoenix_app/lib/phoenix_app_web/controllers/scrubbing_demo_html.ex @@ -0,0 +1,10 @@ +defmodule PhoenixAppWeb.ScrubbingDemoHTML do + @moduledoc """ + The page the parameter-scrubbing e2e spec interacts with. + + See the `scrubbing_demo_html` directory for all templates available. + """ + use PhoenixAppWeb, :html + + embed_templates "scrubbing_demo_html/*" +end diff --git a/test_integrations/phoenix_app/lib/phoenix_app_web/controllers/scrubbing_demo_html/index.html.heex b/test_integrations/phoenix_app/lib/phoenix_app_web/controllers/scrubbing_demo_html/index.html.heex new file mode 100644 index 00000000..c10464ac --- /dev/null +++ b/test_integrations/phoenix_app/lib/phoenix_app_web/controllers/scrubbing_demo_html/index.html.heex @@ -0,0 +1,37 @@ +
+

Reset your password

+ +
+ + + + + + + + + + + +
+ + +
diff --git a/test_integrations/phoenix_app/lib/phoenix_app_web/router.ex b/test_integrations/phoenix_app/lib/phoenix_app_web/router.ex index 12bdfd46..977df461 100644 --- a/test_integrations/phoenix_app/lib/phoenix_app_web/router.ex +++ b/test_integrations/phoenix_app/lib/phoenix_app_web/router.ex @@ -55,6 +55,9 @@ defmodule PhoenixAppWeb.Router do live "/users/:id", UserLive.Show, :show live "/users/:id/show/edit", UserLive.Show, :edit + + get "/scrubbing-demo", ScrubbingDemoController, :index + get "/scrubbing-demo/reset-password/:token", ScrubbingDemoController, :reset_password end # For e2e DT tests with a front-end app @@ -74,6 +77,8 @@ defmodule PhoenixAppWeb.Router do put "/sentry-test-config", TestConfigController, :update end + forward "/scrubbing-demo/forwarded", PhoenixAppWeb.ScrubbingDemoForwardedRouter + # Other scopes may use custom stacks. # scope "/api", PhoenixAppWeb do # pipe_through :api diff --git a/test_integrations/phoenix_app/lib/phoenix_app_web/scrubbing_demo_forwarded_router.ex b/test_integrations/phoenix_app/lib/phoenix_app_web/scrubbing_demo_forwarded_router.ex new file mode 100644 index 00000000..e452bc7a --- /dev/null +++ b/test_integrations/phoenix_app/lib/phoenix_app_web/scrubbing_demo_forwarded_router.ex @@ -0,0 +1,13 @@ +defmodule PhoenixAppWeb.ScrubbingDemoForwardedRouter do + use Phoenix.Router + + pipeline :browser do + plug :accepts, ["html"] + end + + scope "/", PhoenixAppWeb do + pipe_through :browser + + get "/reset-password/:token", ScrubbingDemoController, :reset_password + end +end diff --git a/test_integrations/phoenix_app/mix.exs b/test_integrations/phoenix_app/mix.exs index d23bacbb..4b67a4a1 100644 --- a/test_integrations/phoenix_app/mix.exs +++ b/test_integrations/phoenix_app/mix.exs @@ -11,7 +11,7 @@ defmodule PhoenixApp.MixProject do start_permanent: Mix.env() == :prod, aliases: aliases(), deps: deps() - ] + ] ++ listeners(current_elixir_version()) end # Configuration for the OTP application. @@ -100,6 +100,11 @@ defmodule PhoenixApp.MixProject do defp current_elixir_version, do: Version.parse!(System.version()) + # Phoenix.CodeReloader warns on every recompile when it is not registered as a + # Mix listener. The `:listeners` option only exists from Elixir 1.18 on. + defp listeners(%Version{major: 1, minor: minor}) when minor < 18, do: [] + defp listeners(%Version{}), do: [listeners: [Phoenix.CodeReloader]] + defp lockfile(%Version{major: 1, minor: minor}) when minor < 18, do: "mix-1.15-1.17.lock" diff --git a/test_integrations/tracing/playwright.config.ts b/test_integrations/tracing/playwright.config.ts index bb1f01a7..2053e380 100644 --- a/test_integrations/tracing/playwright.config.ts +++ b/test_integrations/tracing/playwright.config.ts @@ -19,6 +19,14 @@ const SVELTE_URL = requireEnv("SENTRY_E2E_SVELTE_APP_URL"); // When servers are started externally (e.g., in CI workflow steps), skip webServer config const serversRunningExternally = process.env.SENTRY_E2E_SERVERS_RUNNING === "true"; +const realDsn = process.env.SENTRY_E2E_REAL_DSN === "true"; + +const phoenixEnv: Record = realDsn + ? { + SENTRY_DSN: requireEnv("SENTRY_DSN"), + SENTRY_ENVIRONMENT: process.env.SENTRY_ENVIRONMENT ?? "e2e-scrubbing", + } + : { SENTRY_E2E_TEST_MODE: "true", SENTRY_ORG_ID: "123" }; export default defineConfig({ testDir: "./tests", @@ -51,9 +59,10 @@ export default defineConfig({ webServer: [ { command: - 'cd ../phoenix_app && rm -f tmp/sentry_debug_events.log && SENTRY_E2E_TEST_MODE=true SENTRY_ORG_ID=123 mix phx.server', + 'cd ../phoenix_app && rm -f tmp/sentry_debug_events.log && mix phx.server', url: `${PHOENIX_URL}/health`, - reuseExistingServer: true + env: phoenixEnv, + reuseExistingServer: !realDsn }, { command: diff --git a/test_integrations/tracing/tests/param_scrubbing.spec.ts b/test_integrations/tracing/tests/param_scrubbing.spec.ts new file mode 100644 index 00000000..bf6bc44f --- /dev/null +++ b/test_integrations/tracing/tests/param_scrubbing.spec.ts @@ -0,0 +1,219 @@ +import { test, expect, type Page } from "@playwright/test"; +import { clearLoggedEvents, getLoggedEvents, waitForEvents } from "./helpers"; +import { + PLACEHOLDER, + connFields, + demoPagePath, + isScrubbingEvent, + probeValues, + scrubbedQuery, +} from "./scrubbing_fixtures"; + +const PHOENIX_URL = process.env.SENTRY_E2E_PHOENIX_APP_URL; +if (!PHOENIX_URL) { + throw new Error( + "Required environment variable SENTRY_E2E_PHOENIX_APP_URL is not set." + ); +} + +const REAL_DSN = process.env.SENTRY_E2E_REAL_DSN === "true"; +const ENVIRONMENT = process.env.SENTRY_ENVIRONMENT ?? "e2e-scrubbing"; + +if (REAL_DSN && !process.env.SENTRY_DSN) { + throw new Error( + "SENTRY_E2E_REAL_DSN=true needs SENTRY_DSN set, so the app it boots reports somewhere real." + ); +} + +const RUN_ID = process.env.SENTRY_E2E_RUN_ID ?? `run-${Date.now()}`; +const VALUES = probeValues(RUN_ID); +const EXPECTED_QUERY = scrubbedQuery(VALUES, RUN_ID); + +async function openDemoPage(page: Page): Promise { + await page.goto(`${PHOENIX_URL}${demoPagePath(VALUES, RUN_ID)}`); + await expect(page.locator("#scrubbing-demo h1")).toContainText( + "Reset your password" + ); +} + +async function capturedEvent(): Promise> { + const logged = await waitForEvents( + (events) => events.events.some(isScrubbingEvent), + { timeout: 15000 } + ); + const event = logged.events.find(isScrubbingEvent) as + | Record + | undefined; + + expect(event, "no ActionClauseError event was captured").toBeTruthy(); + + return event!; +} + +async function followLink(page: Page, id: string): Promise { + await openDemoPage(page); + await page.click(`#${id}`); +} + +async function submitResetForm(page: Page): Promise { + await openDemoPage(page); + + await page.fill("#token", VALUES.queryToken); + await page.fill("#api_key", VALUES.queryApiKey); + await page.fill("#upper_password", VALUES.upperCasePassword); + await page.fill("#user_password", VALUES.substringPassword); + await page.fill("#internal_ref", VALUES.configuredKey); + await page.fill("#password", VALUES.password); + + await page.click("#submit-reset"); +} + +test.describe("parameter scrubbing", () => { + test.skip( + REAL_DSN, + "asserts against the local envelope log, which only exists in SENTRY_E2E_TEST_MODE" + ); + + test.beforeEach(() => { + clearLoggedEvents(); + }); + + test("derives the conn's path and query from the scrubbed URL", async ({ + page, + }) => { + await followLink(page, "probe-link"); + const event = await capturedEvent(); + const conn = connFields(event); + + expect(conn.requestPath).toBe( + `/scrubbing-demo/reset-password/${PLACEHOLDER}` + ); + expect(conn.pathInfo).toEqual([ + "scrubbing-demo", + "reset-password", + PLACEHOLDER, + ]); + expect(event.request.url).toContain( + `/scrubbing-demo/reset-password/${PLACEHOLDER}` + ); + + expect(conn.queryString).not.toContain(VALUES.queryToken); + expect(conn.queryString).toContain(`keep=${VALUES.benign}`); + }); + + test("rebuilds path_info against script_name behind a forward", async ({ + page, + }) => { + await followLink(page, "forwarded-link"); + const event = await capturedEvent(); + const conn = connFields(event); + + expect(conn.scriptName).toEqual(["scrubbing-demo", "forwarded"]); + expect(conn.pathInfo).toEqual(["reset-password", PLACEHOLDER]); + expect(conn.requestPath).toBe( + `/scrubbing-demo/forwarded/reset-password/${PLACEHOLDER}` + ); + + expect(`/${[...conn.scriptName, ...conn.pathInfo].join("/")}`).toBe( + conn.requestPath + ); + }); + + test("scrubs the secret out of path params", async ({ page }) => { + await followLink(page, "probe-link"); + const event = await capturedEvent(); + const conn = connFields(event); + + expect(conn.pathParams).toBe(`%{"token" => "${PLACEHOLDER}"}`); + expect( + JSON.stringify(event), + "the path segment survived somewhere in the event" + ).not.toContain(VALUES.pathSecret); + }); + + test("redacts what a submitted form carries", async ({ page }) => { + await submitResetForm(page); + const event = await capturedEvent(); + const data = event.request.data; + + expect(data.token, "a key the spec denylist adds").toBe(PLACEHOLDER); + expect(data.api_key, "matched as a substring of `key`").toBe(PLACEHOLDER); + expect(data.PASSWORD, "matched case-insensitively").toBe(PLACEHOLDER); + expect(data.user_password, "matched as a substring").toBe(PLACEHOLDER); + expect(data.internal_ref, "sensitive only by configuration").toBe( + PLACEHOLDER + ); + expect(data.password, "the long-standing default key").toBe(PLACEHOLDER); + + expect(data.keep, "a benign field must survive").toBe(VALUES.benign); + + const serialized = JSON.stringify(event); + for (const [name, value] of Object.entries(VALUES)) { + if (name === "benign") continue; + expect(serialized, `${name} survived scrubbing`).not.toContain(value); + } + }); + + test("redacts the page URL the browser sends as the referer", async ({ + page, + }) => { + await followLink(page, "probe-link"); + const event = await capturedEvent(); + + const referer = event.request.headers.referer; + + expect(referer, "the referer header was not reported at all").toBeTruthy(); + expect(referer).toContain("/scrubbing-demo?"); + expect(referer).toContain(`token=${PLACEHOLDER}`); + expect(referer).not.toContain(VALUES.queryToken); + expect(referer).not.toContain(VALUES.pathSecret); + expect(referer, "a benign param should survive").toContain( + `keep=${VALUES.benign}` + ); + }); + + test("leaves the params it keeps exactly as they were sent", async ({ + page, + }) => { + await followLink(page, "probe-link"); + const event = await capturedEvent(); + const conn = connFields(event); + + expect(conn.queryString).toBe(EXPECTED_QUERY); + expect(conn.queryString, "the placeholder was form-encoded").not.toContain( + "%2A" + ); + expect(event.request.query_string).not.toContain("%2A"); + expect(event.request.url).not.toContain("%2A"); + + expect(conn.queryString).toContain("note=a%20b~c"); + expect(conn.queryString).toContain("&flag&"); + + expect(event.request.url.split("?")[1]).toBe(EXPECTED_QUERY); + }); +}); + +test.describe("smoke run against a real project", () => { + test.skip( + !REAL_DSN, + "set SENTRY_E2E_REAL_DSN=true and boot the app with a real SENTRY_DSN" + ); + + test("sends what the page's form and links produce", async ({ page }) => { + test.setTimeout(120000); + + clearLoggedEvents(); + + await followLink(page, "probe-link"); + await followLink(page, "forwarded-link"); + await submitResetForm(page); + + expect( + getLoggedEvents().event_count, + "the app logged envelopes locally instead of sending them — it is not booted against a real DSN" + ).toBe(0); + + console.log(`\nsearch Sentry for: e2e_run:${RUN_ID}`); + console.log(`environment: ${ENVIRONMENT}\n`); + }); +}); diff --git a/test_integrations/tracing/tests/scrubbing_fixtures.ts b/test_integrations/tracing/tests/scrubbing_fixtures.ts new file mode 100644 index 00000000..d0b60667 --- /dev/null +++ b/test_integrations/tracing/tests/scrubbing_fixtures.ts @@ -0,0 +1,114 @@ +import type { SentryEvent } from "./helpers"; + +export const PLACEHOLDER = "*********"; + +export interface ProbeValues { + pathSecret: string; + queryToken: string; + queryApiKey: string; + upperCasePassword: string; + substringPassword: string; + configuredKey: string; + password: string; + benign: string; +} + +export function probeValues(runId: string): ProbeValues { + return { + pathSecret: `pathsecret-${runId}`, + queryToken: `qtoken-${runId}`, + queryApiKey: `apikey-${runId}`, + upperCasePassword: `upper-${runId}`, + substringPassword: `substr-${runId}`, + configuredKey: `cfgref-${runId}`, + password: `pwd-${runId}`, + benign: `keepme-${runId}`, + }; +} + +function query(pairs: Array<[string, string] | [string]>): string { + return pairs.map((pair) => pair.join("=")).join("&"); +} + +export function scrubbedQuery(values: ProbeValues, runId: string): string { + return query([ + ["token", PLACEHOLDER], + ["api_key", PLACEHOLDER], + ["PASSWORD", PLACEHOLDER], + ["user_password", PLACEHOLDER], + ["internal_ref", PLACEHOLDER], + ["password", PLACEHOLDER], + ["keep", values.benign], + ["note", "a%20b~c"], + ["flag"], + ["e2e_run", runId], + ]); +} + +export function demoPagePath(values: ProbeValues, runId: string): string { + return `/scrubbing-demo?path_secret=${values.pathSecret}&${probeQuery(values, runId)}`; +} + +function probeQuery(values: ProbeValues, runId: string): string { + return query([ + ["token", values.queryToken], + ["api_key", values.queryApiKey], + ["PASSWORD", values.upperCasePassword], + ["user_password", values.substringPassword], + ["internal_ref", values.configuredKey], + ["password", values.password], + ["keep", values.benign], + ["note", "a%20b~c"], + ["flag"], + ["e2e_run", runId], + ]); +} + +const CONN_FIELD_PATTERNS = { + request_path: /request_path: "([^"]*)"/, + path_info: /path_info: (\[[^\]]*\])/, + path_params: /path_params: (%\{[^}]*\})/, + query_string: /query_string: "([^"]*)"/, + script_name: /script_name: (\[[^\]]*\])/, +} as const; + +export interface ConnFields { + requestPath: string; + pathInfo: string[]; + pathParams: string; + queryString: string; + scriptName: string[]; +} + +function capture(value: string, field: keyof typeof CONN_FIELD_PATTERNS): string { + const match = value.match(CONN_FIELD_PATTERNS[field]); + + if (!match) { + throw new Error( + `could not read ${field} out of the reported conn — has the inspect format changed?\n${value}` + ); + } + + return match[1]; +} + +function members(list: string): string[] { + return Array.from(list.matchAll(/"([^"]*)"/g), (match) => match[1]); +} + +export function connFields(event: Record): ConnFields { + const value: string = event?.exception?.[0]?.value ?? ""; + + return { + requestPath: capture(value, "request_path"), + pathInfo: members(capture(value, "path_info")), + pathParams: capture(value, "path_params"), + queryString: capture(value, "query_string"), + scriptName: members(capture(value, "script_name")), + }; +} + +export function isScrubbingEvent(event: SentryEvent): boolean { + const anyEvent = event as Record; + return anyEvent?.exception?.[0]?.type === "Phoenix.ActionClauseError"; +}