From e85d42b8dfe9e43a8f40b27e7c5480e4edf02f45 Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Sun, 4 Oct 2026 03:30:13 +0530 Subject: [PATCH 1/2] Redact authentication material from application telemetry --- CHANGELOG.md | 6 + apps/web/tests/telemetry-browser-faro.test.ts | 9 +- apps/web/tests/telemetry.test.ts | 121 ++++++++++++++++-- packages/core/OBSERVABILITY.md | 24 ++-- packages/core/src/http_logging.py | 6 +- packages/core/src/log_privacy.py | 99 ++++++++++++++ packages/core/src/logging.py | 11 +- packages/telemetry/src/auth-privacy.ts | 88 +++++++++++++ packages/telemetry/src/privacy.ts | 8 +- packages/telemetry/src/server.ts | 4 +- tests/fixtures/auth_redaction_cases.json | 52 ++++++++ tests/test_cli.py | 14 +- tests/test_http_logging.py | 71 ++++++++-- tests/test_job_logs.py | 12 +- tests/test_log_privacy.py | 43 +++++++ tests/test_logging.py | 12 +- tests/test_queued_logging.py | 2 +- 17 files changed, 529 insertions(+), 53 deletions(-) create mode 100644 packages/core/src/log_privacy.py create mode 100644 packages/telemetry/src/auth-privacy.ts create mode 100644 tests/fixtures/auth_redaction_cases.json create mode 100644 tests/test_log_privacy.py diff --git a/CHANGELOG.md b/CHANGELOG.md index af88dcc49..030000212 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ Application releases and Alembic schema revisions are separate identifiers. +## Unreleased + +### Fixes + +- Keep sign-in codes, state values, and authentication credentials out of request logs and browser diagnostics. + ## 0.0.50 — 2026-10-03 ### Enhancements diff --git a/apps/web/tests/telemetry-browser-faro.test.ts b/apps/web/tests/telemetry-browser-faro.test.ts index 439dcf902..4aa6e0879 100644 --- a/apps/web/tests/telemetry-browser-faro.test.ts +++ b/apps/web/tests/telemetry-browser-faro.test.ts @@ -8,7 +8,7 @@ import { } from "@grafana/faro-web-sdk"; import { normalizeMeta, normalizePayload } from "@devfeed/telemetry/privacy"; -it("telemetry preserves Faro sampling and original event and error content", () => { +it("telemetry preserves Faro sampling and diagnostics while removing authentication values", () => { const delivered: TransportItem[] = []; class Capture extends BaseTransport { name = "test-capture"; @@ -44,6 +44,10 @@ it("telemetry preserves Faro sampling and original event and error content", () faro.api.setUser({ email: "private-email", id: "private-user" }); faro.api.pushEvent("telemetry_ready", { url: "/search?q=original-query" }); faro.api.pushError(new Error("original-error-message")); + faro.api.pushEvent("route_change", { + url: "/api/v1/user/auth/callback?code=private-code&state=private-state&limit=25", + }); + faro.api.pushError(new Error("GET /auth/callback?access_token=private-token failed")); expect( delivered.some( (item) => @@ -53,7 +57,8 @@ it("telemetry preserves Faro sampling and original event and error content", () expect(JSON.stringify(delivered)).toContain("private-email"); expect(JSON.stringify(delivered)).toContain("original-error-message"); expect(JSON.stringify(delivered)).toContain("/search?q=original-query"); - expect(JSON.stringify(delivered)).not.toContain("redacted"); + expect(JSON.stringify(delivered)).not.toMatch(/private-code|private-state|private-token/); + expect(JSON.stringify(delivered)).toContain("code=[REDACTED]&state=[REDACTED]&limit=25"); expect(JSON.stringify(delivered)).not.toContain("isSampled"); faro.instrumentations.remove(...faro.instrumentations.instrumentations); }); diff --git a/apps/web/tests/telemetry.test.ts b/apps/web/tests/telemetry.test.ts index 453179cc3..f70e10af1 100644 --- a/apps/web/tests/telemetry.test.ts +++ b/apps/web/tests/telemetry.test.ts @@ -3,12 +3,96 @@ import { traceHeaders } from "@devfeed/telemetry/propagation"; import { describe, expect, it, vi } from "vitest"; import { createServer } from "node:http"; import { once } from "node:events"; -import { routeName, normalizeBody, normalizePayload } from "@devfeed/telemetry/privacy"; +import { + routeName, + normalizeBody, + normalizePayload, + redactAuthText, +} from "@devfeed/telemetry/privacy"; +import redactionCases from "../../../tests/fixtures/auth_redaction_cases.json"; import { receiveTelemetry } from "@devfeed/telemetry/receiver"; import { registerTelemetry } from "@devfeed/telemetry/server"; const settings = { enabled: true, app: "web" as const, version: "test", environment: "test" }; describe("operational telemetry", () => { + it.each(redactionCases)("redacts authentication in $name", ({ input, expected }) => { + expect(redactAuthText(input)).toBe(expected); + expect(redactAuthText(expected)).toBe(expected); + }); + it("redacts nested Faro URLs, credentials and OTLP attributes while preserving correlation", () => { + const callback = + "https://devfeed.tech/auth/callback?code=private-code&state=private-state&q=react"; + const input = { + meta: { page: { url: callback }, session: { id: "session-123" } }, + exceptions: [ + { value: `GET ${callback} failed`, stacktrace: { frames: [{ filename: callback }] } }, + ], + events: [{ name: "route_change", attributes: { url: callback, token: "private-token" } }], + logs: [{ message: callback }], + measurements: [{ type: "web-vitals", values: { lcp: 123 } }], + traces: { + resourceSpans: [ + { + scopeSpans: [ + { + spans: [ + { + traceId: "a".repeat(32), + spanId: "b".repeat(16), + status: { code: 2 }, + attributes: [ + { key: "http.url", value: { stringValue: callback } }, + { + key: "url.query", + value: { stringValue: "code=private-code&state=private-state" }, + }, + { + key: "http.request.header.authorization", + value: { stringValue: "Bearer private-token" }, + }, + ], + }, + ], + }, + ], + }, + ], + }, + }; + const result = normalizeBody(input, settings); + expect(JSON.stringify(result)).not.toMatch(/private-code|private-state|private-token/); + expect(result.meta).toMatchObject({ + page: { url: callback.replaceAll(/private-(?:code|state)/g, "[REDACTED]") }, + session: { id: "session-123" }, + }); + expect(result.measurements[0]).toMatchObject({ values: { lcp: 123 } }); + expect(result.traces).toMatchObject({ + resourceSpans: [ + { + scopeSpans: [ + { + spans: [ + { + traceId: "a".repeat(32), + spanId: "b".repeat(16), + status: { code: 2 }, + attributes: [ + { key: "http.url" }, + { key: "url.query" }, + { + key: "http.request.header.authorization", + value: { stringValue: "[REDACTED]" }, + }, + ], + }, + ], + }, + ], + }, + ], + }); + expect(JSON.stringify(input)).toContain("private-code"); + }); it("bounds unknown routes, slugs, query strings and dynamic API paths", () => { const routes = new Set( Array.from({ length: 1000 }, (_, i) => routeName(`/articles/private-${i}?token=secret`)), @@ -32,7 +116,7 @@ describe("operational telemetry", () => { attributes: { url: "/content/articles/private-id" }, }); }); - it("preserves exception messages, frames, console logs and metadata", () => { + it("preserves diagnostics and metadata except authentication fields", () => { const result = normalizeBody( { meta: { @@ -77,7 +161,7 @@ describe("operational telemetry", () => { expect(result.exceptions[0]).toMatchObject({ type: "secret", value: "secret", - context: { secret: "secret" }, + context: { secret: "[REDACTED]" }, stacktrace: { frames: [ { @@ -89,7 +173,7 @@ describe("operational telemetry", () => { ], }, }); - expect(JSON.stringify(result)).not.toMatch(/redacted|omitted/); + expect(JSON.stringify(result)).not.toContain("omitted"); }); it("preserves trace correlation, timing and original OTLP fields", () => { const result = normalizePayload("trace", { @@ -154,8 +238,14 @@ describe("operational telemetry", () => { await receiveTelemetry( request( JSON.stringify({ - meta: { user: { email: "secret" } }, - logs: [{ message: "secret" }], + meta: { + user: { email: "secret" }, + page: { url: "/auth/callback?code=private-code&state=private-state" }, + }, + logs: [ + { message: "secret" }, + { message: "/auth/callback?access_token=private-token" }, + ], }), ), app, @@ -164,7 +254,11 @@ describe("operational telemetry", () => { ).toBe(202); const options = upstream.mock.calls[0][1]; expect(options.headers["X-API-Key"]).toBe("server-key"); - expect(JSON.parse(options.body).logs).toEqual([{ message: "secret" }]); + expect(JSON.parse(options.body).logs).toEqual([ + { message: "secret" }, + { message: "/auth/callback?access_token=[REDACTED]" }, + ]); + expect(options.body).not.toMatch(/private-code|private-state|private-token/); expect(JSON.stringify(options.headers)).not.toContain("secret-cookie"); upstream.mockRejectedValueOnce(new Error("private-collector-error")); expect((await receiveTelemetry(request("{}"), app)).status).toBe(503); @@ -201,7 +295,18 @@ describe("operational telemetry", () => { expect((await fetch(`http://127.0.0.1:${appPort}/`)).status).toBe(200); await fetch(`http://127.0.0.1:${appPort}/articles/item-123?token=original-value`); expect(log.mock.calls.map(([line]) => JSON.parse(line))).toContainEqual( - expect.objectContaining({ route: "/articles/item-123?token=original-value" }), + expect.objectContaining({ route: "/articles/item-123?token=[REDACTED]" }), + ); + await fetch( + `http://127.0.0.1:${appPort}/api/v1/user/auth/callback?code=private-code&state=private-state&limit=25`, + ); + expect(log.mock.calls.map(([line]) => JSON.parse(line))).toContainEqual( + expect.objectContaining({ + route: "/api/v1/user/auth/callback?code=[REDACTED]&state=[REDACTED]&limit=25", + }), + ); + expect(JSON.stringify(log.mock.calls)).not.toMatch( + /original-value|private-code|private-state/, ); expect((await fetch(`http://127.0.0.1:${port}/`)).status).toBe(404); const response = await fetch(`http://127.0.0.1:${port}/metrics`); diff --git a/packages/core/OBSERVABILITY.md b/packages/core/OBSERVABILITY.md index fa635b0ff..452daad30 100644 --- a/packages/core/OBSERVABILITY.md +++ b/packages/core/OBSERVABILITY.md @@ -125,21 +125,29 @@ this change covers the Python services. ## Application log content -Text, JSON and stored job logs no longer redact supplied values. Request URLs keep -query values; structured fields, nested values, library messages and exception -messages are preserved. Text summaries remain concise, with full context at DEBUG. +Text and JSON logs remove authentication material at their output boundary. +Request URLs retain concrete paths, IDs and ordinary query values, while codes, +state, tokens, client secrets and other authentication parameters become +`[REDACTED]`. Matching is case-insensitive and covers encoded parameter names, +repeated parameters, fragments and encoded nested URLs. URL credentials and +credential fields in structured context, library messages and exception messages +are also removed, including Bearer and Basic authorization values. Ordinary +search/filter values, status codes, request IDs and trace correlation remain +available. Text summaries remain concise, with full context at DEBUG. Request URLs and log messages have no string-length cap. Control characters are escaped; job-stream event size, queue and retention limits remain. The logger does not automatically capture request bodies, headers or local variables. -Browser telemetry retains original error messages, stack filenames/functions, -console log payloads, event attributes and metadata. Browser and Node trace exports -retain supplied span attributes and events. The browser receiver still validates +Browser telemetry removes the same authentication material from error messages, +stack filenames, log payloads, event attributes, metadata and OTLP attributes. +Sanitization runs before browser transport and again at the receiver, including +when a client skips browser normalization. Node request logs use the same URL policy. +Other diagnostic content is preserved. The browser receiver still validates origin, content type and size, enforces rate/time limits, and assigns service identity. Metric routes remain grouped to limit cardinality; Node request logs use concrete URLs. -Restart running services to load the updated logging code. Previously masked values -in retained records cannot be reconstructed. This change needs no database migration. +Restart running services to load the updated logging code. Redaction applies to new +records; it does not remove authentication material from existing retained logs. ## Migration diff --git a/packages/core/src/http_logging.py b/packages/core/src/http_logging.py index b7e8660ca..05e8877f3 100644 --- a/packages/core/src/http_logging.py +++ b/packages/core/src/http_logging.py @@ -1,10 +1,12 @@ -"""Concrete request URLs for logs, with all supplied query values retained.""" +"""Concrete request URLs for logs, excluding authentication material.""" import re from collections.abc import Mapping from typing import Any from urllib.parse import quote +from devfeed_core.log_privacy import redact_auth_text + _HOST = re.compile(r"(?:[a-z0-9.-]+|\[[a-f0-9:.]+\])(?::[0-9]+)?\Z", re.IGNORECASE) @@ -38,5 +40,5 @@ def request_log_fields(scope: Mapping[str, Any]) -> dict[str, str]: return { "method": scope["method"], "route": path, - "request_url": url + ("?" + query if query else ""), + "request_url": redact_auth_text(url + ("?" + query if query else "")), } diff --git a/packages/core/src/log_privacy.py b/packages/core/src/log_privacy.py new file mode 100644 index 000000000..1050e7596 --- /dev/null +++ b/packages/core/src/log_privacy.py @@ -0,0 +1,99 @@ +"""Remove authentication material without hiding ordinary operational context.""" + +import re +from urllib.parse import quote, unquote + +REDACTED = "[REDACTED]" +# These names are credentials even outside a URL. Avoid ambiguous fields such as +# status.code and application state; code/state are sensitive query parameters. +_CREDENTIAL_FIELDS = frozenset( + { + "accesstoken", + "refreshtoken", + "idtoken", + "token", + "clientsecret", + "codeverifier", + "assertion", + "clientassertion", + "password", + "passwd", + "secret", + "apikey", + "authorization", + "cookie", + "setcookie", + "samlresponse", + "oauthtoken", + "oauthverifier", + "devicecode", + "usercode", + } +) +_QUERY_FIELDS = _CREDENTIAL_FIELDS | {"code", "state", "sessionstate", "relaystate"} +_PARAMETER = re.compile(r"(^|[?&#;\s])([\w%+.-]+)=([^&#;\s\"'<>]*)") +_USERINFO = re.compile(r"(\b[a-z][a-z0-9+.-]*://)[^/?#\s@]+@", re.IGNORECASE) +_AUTHORIZATION = re.compile(r"(\b(?:bearer|basic)\s+)[a-z0-9._~+/-]+=*", re.IGNORECASE) + + +def _name(value: str) -> str: + return re.sub(r"[_.-]", "", unquote(value).casefold()) + + +def _credential_field(value: str) -> bool: + return _name(value.rsplit(".", 1)[-1]) in _CREDENTIAL_FIELDS + + +def _encoded_value(value: str, depth: int) -> str: + decoded = unquote(value) + if decoded == value: + return value + if depth >= 5: + return REDACTED + sanitized = _encoded_value(decoded, depth + 1) + if "?" in sanitized or "#" in sanitized or "://" in sanitized: + sanitized = redact_auth_text(sanitized, depth + 1) + return quote(sanitized, safe="") if sanitized != decoded else value + + +def redact_auth_text(value: str, depth: int = 0) -> str: + """Keep URL spelling/ordering intact except credentials, including nested URLs.""" + value = _AUTHORIZATION.sub(lambda match: match[1] + REDACTED, value) + if "=" not in value and "@" not in value: + return value + value = _USERINFO.sub(lambda match: match[1] + REDACTED + "@", value) + + def parameter(match: re.Match[str]) -> str: + prefix, key, original = match.groups() + if _name(key) in _QUERY_FIELDS: + return prefix + key + "=" + REDACTED + if "%" in original: + return prefix + key + "=" + _encoded_value(original, depth) + return match[0] + + return _PARAMETER.sub(parameter, value) + + +def redact_authentication(value, depth: int = 0): + """Protect structured fields and URL-bearing messages at the output boundary.""" + if isinstance(value, str): + return redact_auth_text(value) + if isinstance(value, (dict, list)) and depth >= 20: + return "[truncated]" + if isinstance(value, dict): + output = { + key: REDACTED if _credential_field(str(key)) else redact_authentication(item, depth + 1) + for key, item in value.items() + } + if ( + isinstance(value.get("key"), str) + and "value" in value + and _credential_field(value["key"]) + ): + output["value"] = ( + {"stringValue": REDACTED} if isinstance(value["value"], dict) else REDACTED + ) + return output + if isinstance(value, list): + return [redact_authentication(item, depth + 1) for item in value] + return value diff --git a/packages/core/src/logging.py b/packages/core/src/logging.py index ec8f9410e..a9ffc79cf 100644 --- a/packages/core/src/logging.py +++ b/packages/core/src/logging.py @@ -1,4 +1,4 @@ -"""Shared, bounded text/JSON logging. Preserve supplied messages and fields.""" +"""Shared, bounded text/JSON logging with authentication material removed.""" import json import logging @@ -16,6 +16,7 @@ from pathlib import Path from uuid import UUID +from devfeed_core.log_privacy import redact_auth_text, redact_authentication from devfeed_core.log_text import context_text, error_text, event_text, inline, local_time _context: ContextVar[dict | None] = ContextVar("devfeed_log_context", default=None) @@ -107,7 +108,7 @@ def payload(self, record: logging.LogRecord) -> dict: if record.exc_info and record.exc_info[1] is not None: payload["error_type"] = type(record.exc_info[1]).__name__ payload["exception"] = exception_details(record.exc_info[1]) - return payload + return redact_authentication(payload) class TextFormatter(JsonFormatter): @@ -177,7 +178,11 @@ def exception_details(exc: BaseException | None) -> list[dict]: "function": frame.f_code.co_name, } ) - detail = {"type": type(exc).__name__, "message": str(exc), "frames": frames[-30:]} + detail = { + "type": type(exc).__name__, + "message": redact_auth_text(str(exc)), + "frames": frames[-30:], + } # Preserve interpreter-supplied identifiers without inspecting the object. if ( isinstance(exc, AttributeError) diff --git a/packages/telemetry/src/auth-privacy.ts b/packages/telemetry/src/auth-privacy.ts new file mode 100644 index 000000000..8483b8f94 --- /dev/null +++ b/packages/telemetry/src/auth-privacy.ts @@ -0,0 +1,88 @@ +// Keep this policy aligned with devfeed_core.log_privacy and the shared fixtures. +const redacted = "[REDACTED]"; +const credentialFields = new Set([ + "accesstoken", + "refreshtoken", + "idtoken", + "token", + "clientsecret", + "codeverifier", + "assertion", + "clientassertion", + "password", + "passwd", + "secret", + "apikey", + "authorization", + "cookie", + "setcookie", + "samlresponse", + "oauthtoken", + "oauthverifier", + "devicecode", + "usercode", +]); +const queryFields = new Set([...credentialFields, "code", "state", "sessionstate", "relaystate"]); +function decoded(value: string) { + // Decode valid runs independently: malformed input must not hide an encoded key. + return value.replace(/(?:%[a-f\d]{2})+/gi, (run) => { + try { + return decodeURIComponent(run); + } catch { + return run; + } + }); +} +function name(value: string) { + return decoded(value).toLowerCase().replace(/[_.-]/g, ""); +} +function credentialField(value: string) { + return credentialFields.has(name(value.split(".").at(-1) || value)); +} +function encodedValue(value: string, depth: number): string { + const plain = decoded(value); + if (plain === value) return value; + if (depth >= 5) return redacted; + let sanitized = encodedValue(plain, depth + 1); + if (sanitized.includes("?") || sanitized.includes("#") || sanitized.includes("://")) + sanitized = redactAuthText(sanitized, depth + 1); + return sanitized !== plain + ? encodeURIComponent(sanitized).replace( + /[!'()*]/g, + (char) => `%${char.charCodeAt(0).toString(16).toUpperCase()}`, + ) + : value; +} +export function redactAuthText(value: string, depth = 0): string { + value = value.replace(/(\b(?:bearer|basic)\s+)[a-z\d._~+/-]+=*/gi, `$1${redacted}`); + if (!value.includes("=") && !value.includes("@")) return value; + value = value.replace(/(\b[a-z][a-z\d+.-]*:\/\/)[^/?#\s@]+@/gi, `$1${redacted}@`); + return value.replace( + /(^|[?&#;\s])([\w%+.-]+)=([^&#;\s"'<>]*)/g, + (original, prefix: string, key: string, parameter: string) => { + if (queryFields.has(name(key))) return `${prefix}${key}=${redacted}`; + if (parameter.includes("%")) return `${prefix}${key}=${encodedValue(parameter, depth)}`; + return original; + }, + ); +} +export function redactAuthentication(value: unknown, depth = 0): unknown { + if (typeof value === "string") return redactAuthText(value); + if (value === null || typeof value !== "object") return value; + if (depth >= 20) return "[truncated]"; + if (Array.isArray(value)) return value.map((item) => redactAuthentication(item, depth + 1)); + const input = value as Record; + const output = Object.fromEntries( + Object.entries(input).map(([key, item]) => [ + key, + credentialField(key) ? redacted : redactAuthentication(item, depth + 1), + ]), + ); + // OTLP attributes store the semantic field name separately from its value. + if (typeof input.key === "string" && "value" in input && credentialField(input.key)) + output.value = + input.value !== null && typeof input.value === "object" + ? { stringValue: redacted } + : redacted; + return output; +} diff --git a/packages/telemetry/src/privacy.ts b/packages/telemetry/src/privacy.ts index b873681b2..272dc6549 100644 --- a/packages/telemetry/src/privacy.ts +++ b/packages/telemetry/src/privacy.ts @@ -1,4 +1,6 @@ // Shared by the browser and the receiver. Never trust browser-supplied telemetry. +import { redactAuthentication } from "./auth-privacy"; +export { redactAuthText } from "./auth-privacy"; type ObjectValue = Record; const object = (value: unknown): ObjectValue => value !== null && typeof value === "object" && !Array.isArray(value) @@ -129,7 +131,7 @@ export function normalizePayload( settings?: BrowserSettings, ): ObjectValue | null { if (value === null || typeof value !== "object" || Array.isArray(value)) return null; - const input = object(value); + const input = object(redactAuthentication(value)); if (type === "trace" && settings) { return { ...input, @@ -167,7 +169,7 @@ export interface BrowserSettings { environment: string; } export function normalizeMeta(value: unknown, settings: BrowserSettings, preserveSampling = false) { - const meta = object(value); + const meta = object(redactAuthentication(value)); const session = object(meta.session); const attributes = { ...object(session.attributes) }; if (!preserveSampling) delete attributes.isSampled; @@ -183,7 +185,7 @@ export function normalizeMeta(value: unknown, settings: BrowserSettings, preserv }; } export function normalizeBody(value: unknown, settings: BrowserSettings) { - const body = object(value); + const body = object(redactAuthentication(value)); return { ...body, meta: normalizeMeta(body.meta, settings), diff --git a/packages/telemetry/src/server.ts b/packages/telemetry/src/server.ts index adb9621df..86f397724 100644 --- a/packages/telemetry/src/server.ts +++ b/packages/telemetry/src/server.ts @@ -6,7 +6,7 @@ import { stat } from "node:fs/promises"; import path from "node:path"; import { Registry, Counter, Gauge, Histogram, collectDefaultMetrics } from "@prometheus-io/client"; import { observeDeliveries } from "./delivery"; -import { routeName, methodName } from "./privacy"; +import { routeName, methodName, redactAuthText } from "./privacy"; let registered = false; export async function registerTelemetry(app: "web" | "admin") { if (registered || process.env.DEVFEED_METRICS_ENABLED !== "true") return; @@ -96,7 +96,7 @@ export async function registerTelemetry(app: "web" | "admin") { level: status >= 500 ? "error" : "info", service, method: state.method, - route: request.url, + route: request.url && redactAuthText(request.url), status_code: status, duration_ms: Math.round(performance.now() - state.time), ...(span ? { trace_id: span.traceId, span_id: span.spanId } : {}), diff --git a/tests/fixtures/auth_redaction_cases.json b/tests/fixtures/auth_redaction_cases.json new file mode 100644 index 000000000..019248bb6 --- /dev/null +++ b/tests/fixtures/auth_redaction_cases.json @@ -0,0 +1,52 @@ +[ + { + "name": "reader callback", + "input": "https://devfeed.tech/api/v1/user/auth/callback?code=private-code&state=private-state&iss=https%3A%2F%2Fidentity.example&limit=25", + "expected": "https://devfeed.tech/api/v1/user/auth/callback?code=[REDACTED]&state=[REDACTED]&iss=https%3A%2F%2Fidentity.example&limit=25" + }, + { + "name": "encoded and repeated case-insensitive names", + "input": "/v1/admin/auth/callback?%63ode=private-code&STATE=private-state&code=second-code&session_state=session-value&Access-Token=private-token", + "expected": "/v1/admin/auth/callback?%63ode=[REDACTED]&STATE=[REDACTED]&code=[REDACTED]&session_state=[REDACTED]&Access-Token=[REDACTED]" + }, + { + "name": "token fragment and ordinary query", + "input": "https://devfeed.tech/mcp/authorize?q=original-query#access_token=private-token&token_type=Bearer&expires_in=300", + "expected": "https://devfeed.tech/mcp/authorize?q=original-query#access_token=[REDACTED]&token_type=Bearer&expires_in=300" + }, + { + "name": "encoded nested callback", + "input": "/login?return_to=https%3A%2F%2Fdevfeed.tech%2Fcallback%3Fcode%3Dprivate-code%26state%3Dprivate-state%26limit%3D25&languages=en&languages=fr", + "expected": "/login?return_to=https%3A%2F%2Fdevfeed.tech%2Fcallback%3Fcode%3D%5BREDACTED%5D%26state%3D%5BREDACTED%5D%26limit%3D25&languages=en&languages=fr" + }, + { + "name": "URL in dependency error and URL credentials", + "input": "GET https://operator:private-password@admin.example/auth/callback?client_secret=private-secret&code_verifier=private-verifier failed", + "expected": "GET https://[REDACTED]@admin.example/auth/callback?client_secret=[REDACTED]&code_verifier=[REDACTED] failed" + }, + { + "name": "raw OTLP query and alternate separators", + "input": "code=private-code;state=private-state&refresh_token=private-refresh&id_token=private-id&api_key=private-key", + "expected": "code=[REDACTED];state=[REDACTED]&refresh_token=[REDACTED]&id_token=[REDACTED]&api_key=[REDACTED]" + }, + { + "name": "double-encoded nested callback", + "input": "/login?return_to=%252Fcallback%253Fcode%253Dprivate-code%2526state%253Dprivate-state", + "expected": "/login?return_to=%252Fcallback%253Fcode%253D%255BREDACTED%255D%2526state%253D%255BREDACTED%255D" + }, + { + "name": "encoded search expressions remain ordinary context", + "input": "/search?q=code%3Dpython%26state%3Dready&languages=en", + "expected": "/search?q=code%3Dpython%26state%3Dready&languages=en" + }, + { + "name": "authorization in dependency messages", + "input": "Request Authorization: Bearer private-token failed; Basic dXNlcjpwYXNzd29yZA== denied", + "expected": "Request Authorization: Bearer [REDACTED] failed; Basic [REDACTED] denied" + }, + { + "name": "ordinary operational context remains exact", + "input": "https://devfeed.tech/v1/topics/%E6%97%A5%E6%9C%AC%E8%AA%9E/with%2Fslash?tag=react&tag=typescript&diverse=true&languages=en&languages=fr&limit=25&q=original-query&unknown=original-value&return_to=%2Flatest%3Ftopic%3Dreact#section", + "expected": "https://devfeed.tech/v1/topics/%E6%97%A5%E6%9C%AC%E8%AA%9E/with%2Fslash?tag=react&tag=typescript&diverse=true&languages=en&languages=fr&limit=25&q=original-query&unknown=original-value&return_to=%2Flatest%3Ftopic%3Dreact#section" + } +] diff --git a/tests/test_cli.py b/tests/test_cli.py index 35fbc40f0..bde69e0ca 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -152,7 +152,7 @@ def test_worker_and_scheduler_commands_delegate_to_runtime(monkeypatch, capsys): @pytest.mark.parametrize("duplicate_name", [False, True]) -def test_worker_startup_errors_keep_actionable_messages_and_original_details( +def test_worker_startup_errors_keep_actionable_messages_without_credentials( monkeypatch, capsys, duplicate_name ): closed = [] @@ -178,7 +178,8 @@ def fail(**kwargs): ) assert expected in output.err if not duplicate_name: - assert "redis://user:secret@host" in output.err + assert "redis://[REDACTED]@host" in output.err + assert "user:secret" not in output.err assert output.out == "" assert closed == [True] * 5 # Includes the fresh article extraction lane. @@ -190,7 +191,7 @@ def fail(**kwargs): OperationalError("secret", {}, Exception("secret")), ], ) -def test_connection_errors_have_nonzero_exit_and_original_details(error, monkeypatch, capsys): +def test_connection_errors_have_nonzero_exit_and_safe_details(error, monkeypatch, capsys): def failure(): raise error @@ -198,7 +199,12 @@ def failure(): assert run(["scheduler", "--once"]) == 1 output = capsys.readouterr() assert output.out == "" - assert "secret" in output.err and "Traceback" not in output.err + assert "Traceback" not in output.err + if isinstance(error, RedisConnectionError): + assert "redis://[REDACTED]@host" in output.err + assert "secret" not in output.err + else: + assert "secret" in output.err # Ordinary error text remains actionable. def test_empty_or_oversized_import_is_rejected_before_database(monkeypatch, capsys): diff --git a/tests/test_http_logging.py b/tests/test_http_logging.py index bd9bcb672..a73b5bacd 100644 --- a/tests/test_http_logging.py +++ b/tests/test_http_logging.py @@ -29,12 +29,12 @@ def test_request_url_preserves_ids_query_filters_and_encoded_path(): "https://admin.example:8443" + fields["route"] + "?tag=react&tag=typescript&diverse=true&languages=en&languages=fr&limit=25" - + "&q=private-query&token=private-token" + + "&q=private-query&token=[REDACTED]" ) @pytest.mark.parametrize("formatter", [JsonFormatter, TextFormatter]) -def test_both_formats_preserve_complete_request_url(formatter): +def test_both_formats_redact_authentication_in_request_urls(formatter): url = ( "https://operator:private-password@admin.example/v1/admin/auth/callback" "?code=private-code&state=private-state&access_token=private-token" @@ -52,9 +52,14 @@ def test_both_formats_preserve_complete_request_url(formatter): } ) output = formatter("admin-api").format(record) - assert url in output + assert "private-password" not in output + assert "private-code" not in output + assert "private-state" not in output + assert "private-token" not in output + assert "private-secret" not in output + assert "unknown=private-value" in output assert "limit=30" in output - assert "[redacted" not in output + assert "[REDACTED]" in output @pytest.mark.parametrize("formatter", [JsonFormatter, TextFormatter]) @@ -69,22 +74,60 @@ def test_full_urls_survive_request_fields_messages_and_exceptions(formatter): "query_string": query.encode(), } ) - url = "https://example.test/search?" + query + original_url = "https://example.test/search?" + query + url = original_url.replace("last-value", "[REDACTED]") assert fields["request_url"] == url record = logging.makeLogRecord( {"name": "devfeed_api.logging", "msg": "request_completed", "levelname": "INFO", **fields} ) assert url in formatter("api").format(record) record = logging.makeLogRecord( - {"name": "httpx", "msg": "GET %s", "args": (url,), "levelname": "WARNING"} + {"name": "httpx", "msg": "GET %s", "args": (original_url,), "levelname": "WARNING"} ) assert url in formatter("api").format(record) - error = RuntimeError(url) + error = RuntimeError(original_url) record.exc_info = (RuntimeError, error, None) assert url in formatter("api").format(record) assert json.loads(JsonFormatter("api").format(record))["exception"][0]["message"] == url +@pytest.mark.parametrize("service", ["api", "user-api", "admin-api"]) +@pytest.mark.parametrize("log_format", ["text", "json"]) +def test_callback_logs_are_redacted_without_changing_authentication_inputs(service, log_format): + app = FastAPI() + logger = logging.getLogger(f"devfeed_{service.replace('-', '_')}.logging") + app.add_middleware(RequestLoggingMiddleware, service=service, logger=logger) + formatter = JsonFormatter(service) if log_format == "json" else TextFormatter(service) + events = [] + + class Capture(logging.Handler): + def emit(self, record): + events.append(formatter.format(record)) + + handler = Capture() + previous_level = logger.level + logger.setLevel(logging.INFO) + logger.addHandler(handler) + + @app.get("/auth/callback") + def callback(code: str, state: str): + assert code == "private-code" and state == "private-state" + logger.info("callback_checked") + return {"accepted": True} + + try: + with TestClient(app) as client: + response = client.get("/auth/callback?code=private-code&state=private-state&limit=25") + assert response.json() == {"accepted": True} + assert events and all( + "private-code" not in item and "private-state" not in item for item in events + ) + assert all("code=[REDACTED]&state=[REDACTED]&limit=25" in item for item in events) + finally: + logger.removeHandler(handler) + logger.setLevel(previous_level) + + @pytest.mark.parametrize("formatter", [JsonFormatter, TextFormatter]) def test_request_url_controls_are_escaped_without_removing_values(formatter): url = "https://example.test/a\x00b?limit=1\nFORGED&token=private" @@ -98,8 +141,10 @@ def test_request_url_controls_are_escaped_without_removing_values(formatter): ) output = formatter("api").format(record) assert len(output.splitlines()) == 1 - assert "FORGED&token=private" in output - assert json.loads(JsonFormatter("api").format(record))["request_url"] == url + assert "FORGED&token=[REDACTED]" in output + assert json.loads(JsonFormatter("api").format(record))["request_url"] == url.replace( + "token=private", "token=[REDACTED]" + ) def test_origin_falls_back_to_server_and_ignores_forwarded_headers(): @@ -119,7 +164,7 @@ def test_origin_falls_back_to_server_and_ignores_forwarded_headers(): assert fields["request_url"] == "http://[::1]:8001/missing?unknown=\ufffd" -@pytest.mark.parametrize("package", ["devfeed_api", "devfeed_admin_api"]) +@pytest.mark.parametrize("package", ["devfeed_api", "devfeed_admin_api", "devfeed_user_api"]) @pytest.mark.parametrize("log_format", ["text", "json"]) def test_both_middlewares_log_concrete_locations_for_all_responses(package, log_format): middleware = RequestLoggingMiddleware @@ -156,9 +201,11 @@ def source(source_id: int): events.clear() response = client.get(path) assert response.status_code == status - expected = "http://testserver" + path + expected = "http://testserver" + path.replace("private-token", "[REDACTED]") assert events and all(expected in event for event in events) - assert not any("[redacted" in event or "{source_id}" in event for event in events) + assert not any( + "private-token" in event or "{source_id}" in event for event in events + ) if log_format == "json": payloads = [json.loads(event) for event in events] assert all( diff --git a/tests/test_job_logs.py b/tests/test_job_logs.py index 75e9a4940..98aa65d14 100644 --- a/tests/test_job_logs.py +++ b/tests/test_job_logs.py @@ -121,9 +121,12 @@ def test_capture_preserves_content_without_changing_console_level(stream_store, assert error.fields["error_type"] == "ValueError" assert error.fields["exception"][0]["frames"][-1]["file"] == "test_job_logs.py" assert page.items[3].fields["event"] == "dependency_log" - for sensitive in ("secret content", "secret-token", "secret-body", "https://secret"): - assert sensitive in page.model_dump_json() - assert "private-token" in page.model_dump_json() + for context in ("secret content", "secret-body", "https://secret"): + assert context in page.model_dump_json() + assert "password=[REDACTED]" in page.model_dump_json() + assert "Bearer [REDACTED]" in page.model_dump_json() + assert "private-token" not in page.model_dump_json() + assert "secret-token" not in page.model_dump_json() assert "outside_job" not in page.model_dump_json() assert "feed_fetch_started" not in capsys.readouterr().err assert logging.getLogger("devfeed_core").level == logging.INFO @@ -215,7 +218,8 @@ def test_storage_failure_is_nonfatal_rate_limited_and_recovers(stream_store, cap assert stream_store.executions == 2 output = capsys.readouterr().err assert output.count("Job log storage unavailable") == 1 - assert "redis://user:secret@redis.invalid" in output + assert "redis://[REDACTED]@redis.invalid" in output + assert "user:secret" not in output def test_handler_lazily_creates_new_connection_after_fork(stream_store, monkeypatch): diff --git a/tests/test_log_privacy.py b/tests/test_log_privacy.py new file mode 100644 index 000000000..ad5b804b1 --- /dev/null +++ b/tests/test_log_privacy.py @@ -0,0 +1,43 @@ +import json +from pathlib import Path + +import pytest +from devfeed_core.log_privacy import redact_auth_text, redact_authentication + +CASES = json.loads((Path(__file__).parent / "fixtures/auth_redaction_cases.json").read_text()) + + +@pytest.mark.parametrize("case", CASES, ids=lambda case: case["name"]) +def test_authentication_redaction_matches_shared_url_contract(case): + assert redact_auth_text(case["input"]) == case["expected"] + assert redact_auth_text(case["expected"]) == case["expected"] + + +def test_structured_credentials_are_removed_without_losing_status_and_correlation(): + payload = { + "trace_id": "a" * 32, + "request_id": "request-123", + "status": {"code": 2}, + "state": "ready", + "nested": [{"access_token": "private-token", "password": "private-password"}], + "attributes": [ + { + "key": "http.request.header.authorization", + "value": {"stringValue": "Bearer private-token"}, + }, + {"key": "http.response.status_code", "value": {"intValue": 302}}, + ], + "message": "Callback /auth/callback?code=private-code&state=private-state", + } + result = redact_authentication(payload) + assert result["trace_id"] == payload["trace_id"] + assert result["request_id"] == "request-123" + assert result["status"] == {"code": 2} + assert result["state"] == "ready" + assert result["nested"] == [{"access_token": "[REDACTED]", "password": "[REDACTED]"}] + assert result["message"] == "Callback /auth/callback?code=[REDACTED]&state=[REDACTED]" + assert result["attributes"] == [ + {"key": "http.request.header.authorization", "value": {"stringValue": "[REDACTED]"}}, + {"key": "http.response.status_code", "value": {"intValue": 302}}, + ] + assert payload["nested"][0]["access_token"] == "private-token" diff --git a/tests/test_logging.py b/tests/test_logging.py index 81adbb3ca..8ddbc6784 100644 --- a/tests/test_logging.py +++ b/tests/test_logging.py @@ -100,6 +100,8 @@ def test_formats_keep_metadata_payloads_and_exception_messages(formatter): assert "RuntimeError" in output and "ValueError" in output assert "test_formats_keep_metadata" in output assert "private-" in output and "SELECT" in output + assert "private-password" not in output and "private-token" not in output + assert "private-article-body" in output and "[REDACTED]" in output assert len(output.splitlines()) == 1 @@ -155,8 +157,9 @@ def test_fields_are_bounded_escaped_and_preserved(formatter): formatter("worker", verbose=True) if formatter is TextFormatter else formatter("worker") ) output = renderer.format(record) - assert len(output.splitlines()) == 1 and "[redacted" not in output - assert "private-password" in output and "secret=token" in output + assert len(output.splitlines()) == 1 + assert "private-password" not in output and "secret=token" not in output + assert "[REDACTED]" in output assert "[truncated]" in output assert "custom\\nfield" in output assert "https://example.test/?q=original-value" in output @@ -195,7 +198,8 @@ def test_structured_fields_preserve_nested_values_without_overriding_metadata(): ) payload = json.loads(JsonFormatter("api").format(record)) assert payload["event"] == "event" - assert payload["details"] == record.details + assert payload["details"] == {"token": "[REDACTED]", "url": "redis://[REDACTED]@cache/0"} + assert record.details["token"] == "example-token" assert "args" not in payload and "exc_info" not in payload @@ -579,7 +583,7 @@ def test_discovery_counts_survive_safe_log_formatting(formatter): ) output = formatter("scheduler").format(record) if formatter is JsonFormatter: - assert json.loads(output)["secret"] == "supplied-value" + assert json.loads(output)["secret"] == "[REDACTED]" assert json.loads(output)["tags_linked"] == 2 else: assert "tags: 4 checked, 2 linked, 1 unlinked, 1 ambiguous" in output diff --git a/tests/test_queued_logging.py b/tests/test_queued_logging.py index 6bec5f4d8..9b9508c51 100644 --- a/tests/test_queued_logging.py +++ b/tests/test_queued_logging.py @@ -47,7 +47,7 @@ def flush(self): first, second, _ = [json.loads(line) for line in lines] assert first["event"] == "request_completed" assert first["request_id"] == "first" and second["request_id"] == "second" - assert first["request_url"] == "https://app.test/?token=private" + assert first["request_url"] == "https://app.test/?token=[REDACTED]" try: asyncio.run(scenario()) From aaa294e3e6dec9522885895c58f44ec39de8c90a Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Sun, 4 Oct 2026 03:39:27 +0530 Subject: [PATCH 2/2] Keep shared redaction fixtures in the Docker build context --- apps/web/tests/telemetry.test.ts | 2 +- .../telemetry/fixtures/auth-redaction-cases.json | 0 tests/test_log_privacy.py | 3 ++- 3 files changed, 3 insertions(+), 2 deletions(-) rename tests/fixtures/auth_redaction_cases.json => packages/telemetry/fixtures/auth-redaction-cases.json (100%) diff --git a/apps/web/tests/telemetry.test.ts b/apps/web/tests/telemetry.test.ts index f70e10af1..7d68ae9e2 100644 --- a/apps/web/tests/telemetry.test.ts +++ b/apps/web/tests/telemetry.test.ts @@ -9,7 +9,7 @@ import { normalizePayload, redactAuthText, } from "@devfeed/telemetry/privacy"; -import redactionCases from "../../../tests/fixtures/auth_redaction_cases.json"; +import redactionCases from "../../../packages/telemetry/fixtures/auth-redaction-cases.json"; import { receiveTelemetry } from "@devfeed/telemetry/receiver"; import { registerTelemetry } from "@devfeed/telemetry/server"; const settings = { enabled: true, app: "web" as const, version: "test", environment: "test" }; diff --git a/tests/fixtures/auth_redaction_cases.json b/packages/telemetry/fixtures/auth-redaction-cases.json similarity index 100% rename from tests/fixtures/auth_redaction_cases.json rename to packages/telemetry/fixtures/auth-redaction-cases.json diff --git a/tests/test_log_privacy.py b/tests/test_log_privacy.py index ad5b804b1..84102b581 100644 --- a/tests/test_log_privacy.py +++ b/tests/test_log_privacy.py @@ -4,7 +4,8 @@ import pytest from devfeed_core.log_privacy import redact_auth_text, redact_authentication -CASES = json.loads((Path(__file__).parent / "fixtures/auth_redaction_cases.json").read_text()) +FIXTURES = Path(__file__).parents[1] / "packages/telemetry/fixtures" +CASES = json.loads((FIXTURES / "auth-redaction-cases.json").read_text()) @pytest.mark.parametrize("case", CASES, ids=lambda case: case["name"])