OBS-02: Error tracking and product analytics foundation#811
OBS-02: Error tracking and product analytics foundation#811Chris0Jeky merged 30 commits intomainfrom
Conversation
Adversarial Self-ReviewPrivacy & PII AssessmentPASS - PASS - Authorization and Cookie headers are stripped from Sentry breadcrumbs via PASS - The PASS - Analytics config uses Plausible/Umami which are inherently cookie-free and PII-free by design. Security AssessmentPASS - Sentry DSN is a public identifier (not a secret) per Sentry's security model. It identifies the project for event ingestion but does not grant administrative access. PASS - The PASS - No API keys, secrets, or credentials are exposed in the config response. Opt-In VerificationPASS - All three integrations (Sentry, Telemetry, Analytics) are PASS - Frontend telemetry requires BOTH user consent ( PASS - Consent UI clearly discloses what is/isn't collected and defaults to off. PASS - Revoking consent immediately clears the event buffer and stops the flush timer. Performance AssessmentLOW - Sentry SDK adds ~100KB to the backend deployment. Since it is config-gated and only initialized when enabled, there is zero runtime overhead when disabled. LOW - Frontend telemetry store uses a 200-event buffer cap and 30-second flush interval. Memory-safe and does not impact render performance. INFO - The analytics script is loaded asynchronously ( Test Coverage AssessmentPASS - 25 unit tests for TelemetryEventService covering: enabled/disabled states, event name validation (valid taxonomy names, invalid formats), empty session ID rejection, batch size limits, mixed valid/invalid batch handling. PASS - 13 API integration tests covering: DI registration for all 3 settings + service, default-disabled verification, anonymous config access, auth-required events endpoint. PASS - 22+ frontend tests covering: consent lifecycle, dual-gate activation, event buffering, flush success/failure, config loading, session ID generation. PASS - Architecture boundary tests updated to allow TelemetryController's direct ControllerBase inheritance (same pattern as HealthController). Findings Summary
No CRITICAL or HIGH findings. The implementation follows the privacy guardrails defined in |
![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: 0596bb6619
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import('./store/telemetryStore').then(({ useTelemetryStore }) => { | ||
| const telemetry = useTelemetryStore() | ||
| void telemetry.initialize() |
There was a problem hiding this comment.
The analytics integration is effectively dead code because useAnalyticsScript() is never invoked anywhere in the app lifecycle. In this commit, startup only initializes useTelemetryStore, so even when a user opts in and /api/telemetry/config enables analytics, no script is ever injected and analytics events are never collected. I verified there are no call sites for useAnalyticsScript in frontend/taskdeck-web/src.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Lays the foundation for opt-in observability in Taskdeck by introducing backend Sentry wiring, a telemetry configuration + events API, a frontend telemetry consent/store layer, and operator-facing setup documentation—all disabled by default and gated by both server config and user consent.
Changes:
- Added backend telemetry settings/service +
/api/telemetry/config(anonymous) and/api/telemetry/events(auth) endpoints, plus DI/config registration. - Added frontend telemetry store (consent persistence + buffering/flush) and analytics script injection composable, plus Settings UI consent toggle.
- Added tests (frontend Vitest + backend unit/integration/architecture) and an ops setup guide.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/taskdeck-web/src/views/ProfileSettingsView.vue | Adds consent UI for telemetry/privacy in settings. |
| frontend/taskdeck-web/src/store/telemetryStore.ts | Implements consent state, config loading, buffering, and periodic flush. |
| frontend/taskdeck-web/src/main.ts | Initializes telemetry store on app startup. |
| frontend/taskdeck-web/src/composables/useAnalyticsScript.ts | Injects/removes Plausible/Umami script based on consent + config. |
| frontend/taskdeck-web/src/api/telemetryApi.ts | Client for config fetch + event batch post endpoints. |
| frontend/taskdeck-web/src/tests/store/telemetryStore.spec.ts | Unit tests for telemetry store behavior. |
| frontend/taskdeck-web/src/tests/api/telemetryApi.spec.ts | Unit tests for telemetry API client wrapper. |
| docs/ops/OBSERVABILITY_SETUP.md | Operator documentation for Sentry/analytics/telemetry configuration. |
| backend/src/Taskdeck.Api/Controllers/TelemetryController.cs | Adds telemetry config/events endpoints. |
| backend/src/Taskdeck.Api/Extensions/SettingsRegistration.cs | Registers Sentry/Telemetry/Analytics settings + telemetry service in DI. |
| backend/src/Taskdeck.Api/Extensions/SentryRegistration.cs | Config-gated Sentry SDK integration with privacy guardrails. |
| backend/src/Taskdeck.Api/Program.cs | Wires new settings + Sentry registration into startup. |
| backend/src/Taskdeck.Api/appsettings.json | Adds default-disabled config stanzas for Sentry/Telemetry/Analytics. |
| backend/src/Taskdeck.Api/Taskdeck.Api.csproj | Adds Sentry.AspNetCore package reference. |
| backend/src/Taskdeck.Application/Services/TelemetryEventService.cs | Implements telemetry event validation + batch recording. |
| backend/src/Taskdeck.Application/Services/ITelemetryEventService.cs | Defines telemetry event service interface. |
| backend/src/Taskdeck.Application/Services/TelemetryEvent.cs | Adds telemetry event model (including properties bag). |
| backend/src/Taskdeck.Application/Services/TelemetrySettings.cs | Adds server-side telemetry settings (enabled + max batch size). |
| backend/src/Taskdeck.Application/Services/SentrySettings.cs | Adds Sentry settings model (used by registration). |
| backend/src/Taskdeck.Application/Services/AnalyticsSettings.cs | Adds analytics settings model for Plausible/Umami. |
| backend/tests/Taskdeck.Api.Tests/TelemetryApiTests.cs | Adds integration tests for telemetry endpoints (auth + defaults). |
| backend/tests/Taskdeck.Api.Tests/TelemetryConfigurationTests.cs | Adds DI/config default-disabled assertions. |
| backend/tests/Taskdeck.Application.Tests/Services/TelemetryEventServiceTests.cs | Adds unit coverage for validation/guards/batch size. |
| backend/tests/Taskdeck.Architecture.Tests/ApiControllerBoundaryTests.cs | Allows new TelemetryController base type in architecture rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var unauthClient = new HttpClient { BaseAddress = _client.BaseAddress }; | ||
| var response = await _client.PostAsJsonAsync("/api/telemetry/events", new | ||
| { |
There was a problem hiding this comment.
PostEvents_ShouldRequireAuth creates unauthClient but then posts using _client, so the test doesn't actually verify the unauthenticated case and may become flaky if another test authenticates _client (shared field). Use unauthClient.PostAsJsonAsync(...) (or create a fresh client per test) and ensure no Authorization header is present.
| var recorded = 0; | ||
| foreach (var evt in events) | ||
| { | ||
| if (RecordEvent(evt)) | ||
| { | ||
| recorded++; | ||
| } |
There was a problem hiding this comment.
RecordEvents assumes every entry in events is non-null; a request body containing null elements (e.g. [null]) would result in RecordEvent(null) and throw. Add null guards (skip null entries or treat them as invalid) to avoid turning bad input into a 500.
| /// <summary> | ||
| /// When true, PII scrubbing is enforced — Sentry SDK will not send | ||
| /// usernames, emails, IP addresses, or request bodies. | ||
| /// </summary> | ||
| public bool SendDefaultPii { get; set; } |
There was a problem hiding this comment.
The XML doc for SendDefaultPii is reversed/misleading: SendDefaultPii=true means Sentry will send default PII; setting it to false disables it. Since the integration forces options.SendDefaultPii = false, update this comment (and consider defaulting the setting to false) so operators aren't misled about privacy behavior.
| async function loadConfig() { | ||
| try { | ||
| serverConfig.value = await telemetryApi.getConfig() | ||
| configLoaded.value = true | ||
| } catch { | ||
| // Config fetch failure is non-fatal — telemetry simply stays disabled | ||
| configLoaded.value = true | ||
| } |
There was a problem hiding this comment.
On config fetch failure, serverConfig is left unchanged. If loadConfig() is ever retried after a successful load, a transient failure would leave stale (potentially enabled) config in memory. Set serverConfig.value = null in the catch/finally path to ensure failures disable telemetry/analytics until a successful refresh.
| async function flush() { | ||
| if (!isActive.value || eventBuffer.value.length === 0) { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
flush() can run concurrently (timer tick or manual call while a previous sendEvents is still in-flight) because the interval callback doesn't await. Two overlapping flushes can duplicate the same batch (both snapshot the buffer before either clears it). Add an isFlushing guard / in-flight promise to serialize flushes.
| const provider = config.provider.toLowerCase() | ||
| if (provider === 'plausible') { | ||
| script.setAttribute('data-domain', config.siteId) | ||
| } else if (provider === 'umami') { | ||
| script.setAttribute('data-website-id', config.siteId) |
There was a problem hiding this comment.
injectScript() will append the script even when provider is unknown or siteId is empty, which can lead to broken analytics (missing required attributes) and makes misconfiguration harder to detect. Consider gating injection on provider being plausible|umami and siteId being non-empty (and/or log a warning when config is invalid).
| [HttpPost("events")] | ||
| public IActionResult RecordEvents([FromBody] TelemetryBatchRequest request) | ||
| { | ||
| if (!_telemetryEventService.IsEnabled) |
There was a problem hiding this comment.
RecordEvents doesn't defensively handle a null request body (e.g. client sends JSON null), which would throw when accessing request.Events. With [ApiController] you often get an automatic 400, but it's safer to accept TelemetryBatchRequest? and explicitly return BadRequest when request is null.
| <p v-if="telemetry.consentGiven" class="td-telemetry-status td-telemetry-status--on"> | ||
| Telemetry is enabled. Anonymous usage events will be sent periodically. | ||
| </p> | ||
| <p v-else class="td-telemetry-status td-telemetry-status--off"> | ||
| Telemetry is disabled. No usage data is collected or sent. | ||
| </p> |
There was a problem hiding this comment.
The status text is driven only by telemetry.consentGiven, but telemetry sending is dual-gated (telemetry.isActive). If the user opts in while the server has telemetry disabled, the UI will incorrectly claim that events “will be sent periodically”. Consider basing the status on isActive and showing a separate “consent given, but server has telemetry disabled” state.
| if (request.Events == null || request.Events.Count == 0) | ||
| { | ||
| return BadRequest(new { error = "No events provided." }); | ||
| } |
There was a problem hiding this comment.
The endpoint currently returns 400 when events is empty. This makes the API non-idempotent for clients that simply "flush" with whatever they have (and conflicts with the frontend test that posts an empty array). Consider treating an empty batch as a no-op and returning 200 { recorded: 0 } for a simpler, more robust client contract.
| // Initialize telemetry after mount (non-blocking, opt-in). | ||
| // This restores user consent from localStorage and fetches server config. | ||
| // No events are emitted unless the user has explicitly opted in. | ||
| import('./store/telemetryStore').then(({ useTelemetryStore }) => { | ||
| const telemetry = useTelemetryStore() | ||
| void telemetry.initialize() | ||
| }) |
There was a problem hiding this comment.
telemetry.initialize() is started, but telemetry.dispose() is never hooked up to pagehide/beforeunload. As a result, any buffered events may be dropped if the user closes/navigates away before the next 30s flush interval (despite the store having a disposal path). Consider registering an unload handler here to stop the timer and best-effort flush.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive telemetry and observability system, including Sentry error tracking, web analytics (Plausible/Umami), and custom product telemetry event recording. The implementation follows an opt-in architecture, ensuring all features are disabled by default and require explicit server configuration and user consent. The changes include new API controllers, backend services, frontend stores, and associated tests. I have provided feedback on improving the robustness of PII scrubbing, standardizing API response DTOs, correcting documentation, and cleaning up unused test code.
| if (breadcrumb.Category == "http" && breadcrumb.Data != null) | ||
| { | ||
| var sensitiveKeys = new[] { "Authorization", "authorization", "Cookie", "cookie" }; | ||
| foreach (var key in sensitiveKeys) | ||
| { | ||
| if (breadcrumb.Data.ContainsKey(key)) | ||
| { | ||
| // Data contains sensitive headers — drop entire breadcrumb | ||
| // to prevent PII leakage. The breadcrumb is replaced with | ||
| // a sanitized version without data. | ||
| return new Sentry.Breadcrumb( | ||
| message: breadcrumb.Message ?? string.Empty, | ||
| type: breadcrumb.Type ?? string.Empty, | ||
| data: null, | ||
| category: breadcrumb.Category, | ||
| level: breadcrumb.Level); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation for stripping sensitive headers from Sentry breadcrumbs is case-sensitive and could miss headers like AUTHORIZATION or Cookie with different casing. HTTP headers are case-insensitive by specification.
To ensure all sensitive headers are properly stripped, I recommend using a case-insensitive check. A static readonly HashSet<string> with StringComparer.OrdinalIgnoreCase is a more robust and performant way to handle this.
if (breadcrumb.Category == "http" && breadcrumb.Data != null)
{
var sensitiveKeys = new HashSet<string>(new[] { "Authorization", "Cookie" }, StringComparer.OrdinalIgnoreCase);
if (breadcrumb.Data.Keys.Any(k => sensitiveKeys.Contains(k)))
{
// Data contains sensitive headers — drop entire breadcrumb
// to prevent PII leakage. The breadcrumb is replaced with
// a sanitized version without data.
return new Sentry.Breadcrumb(
message: breadcrumb.Message ?? string.Empty,
type: breadcrumb.Type ?? string.Empty,
data: null,
category: breadcrumb.Category,
level: breadcrumb.Level);
}
}| if (!_telemetryEventService.IsEnabled) | ||
| { | ||
| return Ok(new { recorded = 0, message = "Telemetry is disabled on this server." }); | ||
| } | ||
|
|
||
| if (request.Events == null || request.Events.Count == 0) | ||
| { | ||
| return BadRequest(new { error = "No events provided." }); | ||
| } | ||
|
|
||
| var recorded = _telemetryEventService.RecordEvents(request.Events); | ||
| return Ok(new { recorded }); | ||
| } |
There was a problem hiding this comment.
The RecordEvents action method uses anonymous types for its responses, which leads to an inconsistency between the success path and the disabled path. The success path returns { recorded }, while the disabled path returns { recorded, message }.
To improve type safety and consistency, it's better to define and use a specific response DTO for this endpoint. This aligns with the TelemetryBatchResponse interface already defined on the frontend.
if (!_telemetryEventService.IsEnabled)
{
return Ok(new TelemetryBatchResponse { Recorded = 0, Message = "Telemetry is disabled on this server." });
}
if (request.Events == null || request.Events.Count == 0)
{
return BadRequest(new { error = "No events provided." });
}
var recorded = _telemetryEventService.RecordEvents(request.Events);
return Ok(new TelemetryBatchResponse { Recorded = recorded });
}
}
public sealed class ClientTelemetryConfigResponse
{
public SentryClientConfig Sentry { get; set; } = new();
public AnalyticsClientConfig Analytics { get; set; } = new();
public TelemetryClientConfig Telemetry { get; set; } = new();
}
public sealed class SentryClientConfig
{
public bool Enabled { get; set; }
public string Dsn { get; set; } = string.Empty;
public string Environment { get; set; } = string.Empty;
public double TracesSampleRate { get; set; }
}
public sealed class AnalyticsClientConfig
{
public bool Enabled { get; set; }
public string Provider { get; set; } = string.Empty;
public string ScriptUrl { get; set; } = string.Empty;
public string SiteId { get; set; } = string.Empty;
}
public sealed class TelemetryClientConfig
{
public bool Enabled { get; set; }
}
public sealed class TelemetryBatchRequest
{
public List<TelemetryEvent> Events { get; set; } = new();
}
public sealed class TelemetryBatchResponse
{
public int Recorded { get; set; }
public string? Message { get; set; }
}| /// <summary> | ||
| /// When true, PII scrubbing is enforced — Sentry SDK will not send | ||
| /// usernames, emails, IP addresses, or request bodies. | ||
| /// </summary> | ||
| public bool SendDefaultPii { get; set; } |
There was a problem hiding this comment.
The XML comment for the SendDefaultPii property is misleading. It currently states that setting it to true enforces PII scrubbing, which is the opposite of the Sentry SDK's behavior. When SendDefaultPii is true, PII is sent to Sentry. When false, it is scrubbed.
Let's correct the comment to avoid future confusion and misconfiguration, even though the application correctly forces this value to false in code.
/// <summary>
/// When false, the Sentry SDK will not send personally identifiable information (PII)
/// such as usernames, IP addresses, or request bodies. This is the recommended setting for privacy.
/// </summary>| var unauthClient = new HttpClient { BaseAddress = _client.BaseAddress }; | ||
| var response = await _client.PostAsJsonAsync("/api/telemetry/events", new |
There was a problem hiding this comment.
The unauthClient variable is declared but never used in this test. The subsequent API call is made with _client, which is unauthenticated by default from the TestWebApplicationFactory.
While the test's logic is correct and passes as expected, the unused variable can be confusing for future readers. Removing it would make the test's intent clearer.
var response = await _client.PostAsJsonAsync("/api/telemetry/events", new
Independent Adversarial Review (Round 2) -- Privacy & Security FocusCRITICAL FindingsC1: No BeforeSend handler -- PII leaks through exception messages [CRITICAL]File:
There is no Recommendation: Add a
C2: TelemetryEvent.Properties is an unsanitized Dictionary -- PII sink [CRITICAL]File: The Properties dictionary accepts arbitrary key-value pairs from the frontend with zero validation. A frontend bug or a malicious caller could send: {"properties": {"card_title": "private medical info", "email": "user@corp.com"}}The backend logs these at LogInformation level (line 41-45 of TelemetryEventService.cs) without inspecting or sanitizing them. The doc comment says "Keys and values must not contain PII" but this is an unenforced honor-system constraint. Recommendation:
HIGH FindingsH1: Analytics ScriptUrl is injected into DOM without URL validation -- XSS via config [HIGH]File: script.src = config.scriptUrlThe scriptUrl comes from the backend config endpoint, which returns it directly from appsettings.json. If an operator misconfigures this (or if an attacker gains config write access), any arbitrary script URL -- including javascript: URIs or attacker-controlled domains -- gets injected into the DOM. There is no validation that scriptUrl is:
Recommendation: Validate scriptUrl on the backend with a URL format check (must start with https://). On the frontend, add a guard before injection: H2: Auth test is broken -- creates unauthClient but uses _client [HIGH]File: var unauthClient = new HttpClient { BaseAddress = _client.BaseAddress };
var response = await _client.PostAsJsonAsync(...) // BUG: uses _client, not unauthClientThe test creates unauthClient (line 50) but never uses it. It then posts with _client (from the factory). This test passes only because _client happens to not have auth headers set at that point -- but the test does NOT prove what it claims. The unauthClient is also not configured with the test server's HttpMessageHandler, so even if it were used, it would try to make a real HTTP call, not hit the in-memory test server. Impact: The test gives false confidence that auth is enforced. If someone changes the factory to auto-authenticate, this test would still pass while no longer testing the unauthenticated path. Recommendation: Remove the unused unauthClient. The test currently works by accident because _client has no auth headers, but add a comment explaining this. H3: No DNT (Do Not Track) / GPC (Global Privacy Control) respect [HIGH]File: Neither the telemetry store nor the analytics script checks navigator.doNotTrack or navigator.globalPrivacyControl. While telemetry defaults to off and requires opt-in, the consent UI does not inform users about DNT/GPC, and the system will happily accept opt-in from a user whose browser signals DNT=1. For a product that values privacy as a core principle, respecting these signals is both an ethical and potential legal requirement (GPC has legal force under CCPA). Recommendation:
MEDIUM FindingsM1: SentrySettings.SendDefaultPii property is misleading -- creates false sense of control [MEDIUM]Files: The config model exposes SendDefaultPii as a settable property and appsettings.json includes it. The docs say "Always forced to false in code" but having the property in config invites someone to set it to true expecting it to work. The code silently ignores it -- a classic pit of confusion. Recommendation: Remove SendDefaultPii from SentrySettings and from appsettings.json. If it must exist for documentation purposes, add a startup warning if config sets it to true. M2: No rate limiting on POST /api/telemetry/events [MEDIUM]File: The events endpoint has auth but no rate limiting. An authenticated user could flood the server with telemetry batches (100 events each, unlimited frequency). This is a DoS vector that would fill application logs (since events are logged at Information level). Recommendation: Apply the existing rate limiting infrastructure to the telemetry endpoint, or add a dedicated per-user throttle. M3: Sentry breadcrumb scrubbing is case-incomplete and misses other sensitive headers [MEDIUM]File: var sensitiveKeys = new[] { "Authorization", "authorization", "Cookie", "cookie" };HTTP headers are case-insensitive (RFC 7230). This only handles two casings. Headers like AUTHORIZATION, COOKIE, Set-Cookie, or X-Api-Key would pass through. The comparison should be case-insensitive. Recommendation: Use case-insensitive comparison for header key matching. M4: Frontend telemetry emit() does not validate event names [MEDIUM]File: The backend validates event names against noun.verb regex, but the frontend emit() accepts any string. Invalid events are buffered, sent to the server, and rejected -- wasting bandwidth and buffer space. Frontend validation would catch typos early and avoid unnecessary network traffic. Recommendation: Add the same noun.verb regex validation in emit() before buffering. M5: dispose() calls flush() as fire-and-forget -- events silently lost on page unload [MEDIUM]File:
Additionally, dispose() is never actually called -- it is exported but there is no beforeunload listener or app-level teardown that invokes it. Recommendation: Wire up LOW FindingsL1: Consent disclosure says "Error codes (no error messages or stack traces)" but Sentry sends full stack traces [LOW]File: The "What data is collected?" section tells users "Error codes (no error messages or stack traces)" -- but when Sentry is enabled, full exception stack traces ARE sent to Sentry. The consent toggle controls both product telemetry events AND Sentry, but the disclosure only describes the telemetry events. Recommendation: Either separate Sentry consent from telemetry consent, or update the disclosure to mention that error tracking (when enabled by the server) includes stack traces. L2: TelemetryEventService is Singleton with no thread safety concerns documented [LOW]File: The service is registered as Singleton and is stateless (just logs), so this is technically fine. But if a future iteration adds event persistence (as suggested in the doc comment), the singleton lifetime will create concurrency issues. Recommendation: Consider Scoped lifetime, or add a comment warning future contributors about the singleton constraint. L3: Config endpoint leaks environment and tracesSampleRate even when Sentry is disabled [LOW]File: When Sentry is disabled, Dsn is correctly returned as empty, but Environment (e.g., "production") and TracesSampleRate are still returned. This leaks infrastructure information to unauthenticated callers. Recommendation: Return empty/default values for all Sentry fields when disabled, not just the DSN. INFOI1: No ADR for telemetry architecture decisions. This PR introduces a cross-cutting concern (telemetry) with several design decisions (dual-gate model, event taxonomy, consent persistence strategy). Per CLAUDE.md, decisions with cross-cutting impact should have ADRs. I2: Good: sessionId is per-app-load, not persistent. The session ID is regenerated on each app load and stored only in memory (not localStorage), preventing cross-session tracking. I3: Good: dual-gate design is sound. Requiring both server config AND user consent is the right architecture. Summary
The CRITICAL findings are PR-blocking. The Properties dictionary being an open PII sink and the lack of Sentry BeforeSend scrubbing must be addressed before merge. |
- Add SetBeforeSend callback that scrubs email patterns and JWT tokens from SentryEvent.Message and SentryException.Value before transmission - Set ServerName to empty string to prevent hostname leakage - Fix breadcrumb header matching to use case-insensitive comparison (HTTP headers are case-insensitive per RFC 7230) - Add Set-Cookie and X-Api-Key to sensitive header list
Properties dictionary previously accepted arbitrary key-value pairs, creating a PII sink. Now validates against an allowlist of known safe keys, caps at 10 properties, and truncates string values to 200 chars. Disallowed keys are stripped silently (logged at Debug level).
Prevents XSS via javascript:, data:, or blob: URIs in the ScriptUrl config. Uses URL constructor for proper protocol validation.
The test created unauthClient but used _client. Removed the dead code and added a comment explaining that _client has no auth headers by default, which is why the test correctly verifies 401 behavior.
When navigator.globalPrivacyControl or navigator.doNotTrack is active, consent is not auto-restored from localStorage on page load. Users must explicitly opt in each session. The consent UI shows a notice when DNT/GPC is detected.
Fixes Applied for CRITICAL and HIGH FindingsPushed 5 commits addressing all CRITICAL and HIGH issues from the adversarial review: CRITICAL fixesC1 - Sentry PII scrubbing (
C2 - Properties allowlist (
HIGH fixesH1 - ScriptUrl HTTPS validation (
H2 - Broken auth test (
H3 - DNT/GPC respect (
Verification
Remaining MEDIUM/LOW items (not fixed in this push)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c52d71ad9
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import('./store/telemetryStore').then(({ useTelemetryStore }) => { | ||
| const telemetry = useTelemetryStore() | ||
| void telemetry.initialize() |
There was a problem hiding this comment.
App startup only calls telemetry.initialize(), but there is no Sentry browser SDK initialization anywhere in frontend/taskdeck-web/src. That means when the server returns sentry.enabled=true and a user has opted in, client-side exceptions are still never reported, so the frontend error-tracking path described by this telemetry config is effectively inert.
Useful? React with 👍 / 👎.
| var value = kvp.Value; | ||
| if (value is string strValue && strValue.Length > MaxPropertyValueLength) | ||
| { |
There was a problem hiding this comment.
This truncation logic only handles runtime string values, but telemetry properties arrive through JSON and Dictionary<string, object> values are typically JsonElement. As a result, large string property values in API requests bypass MaxPropertyValueLength and are kept intact, which undermines the payload-size guardrail for allowed keys.
Useful? React with 👍 / 👎.
- main.ts: Initialize analytics script watcher during app bootstrap - TelemetryApiTests.cs: Use unauthClient instead of _client for auth test - TelemetryEventService.cs: Guard against null elements in RecordEvents batch - SentrySettings.cs: Fix XML doc for SendDefaultPii (was reversed) - telemetryStore.ts: Set serverConfig to null on config fetch failure - telemetryStore.ts: Add isFlushing guard to prevent concurrent flush duplicates - useAnalyticsScript.ts: Validate provider and siteId before script injection - TelemetryController.cs: Handle null request body in RecordEvents
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Tests cover: - Script injection for Plausible and Umami providers - HTTPS URL validation (rejects http:, javascript:, data: protocols) - Provider validation (only plausible and umami supported) - SiteId format validation (prevents injection attacks) - Script deduplication (no duplicate script elements) - Script removal on unmount and consent revocation - Case-insensitive provider matching - initAnalyticsScriptWatcher function for main.ts bootstrap
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Resolve ProfileSettingsView.vue conflict by keeping both telemetry styles (from this branch) and GitHub OAuth linking styles (from main).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf60d0f529
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const eventsToSend = [...eventBuffer.value] | ||
| eventBuffer.value = [] | ||
|
|
||
| try { | ||
| await telemetryApi.sendEvents(eventsToSend) |
There was a problem hiding this comment.
The flush path posts the entire in-memory buffer in one request, but the backend rejects any batch larger than Telemetry.MaxBatchSize (default 100 in backend/src/Taskdeck.Api/appsettings.json, enforced in TelemetryEventService.RecordEvents). Because this call clears the buffer before sending and treats HTTP 200 as success regardless of recorded, any flush with 101+ events is silently dropped, which can corrupt telemetry counts during bursty usage.
Useful? React with 👍 / 👎.
| // Re-buffer events on failure (up to max size) | ||
| eventBuffer.value = [...eventsToSend, ...eventBuffer.value].slice( | ||
| -MAX_BUFFER_SIZE, | ||
| ) |
There was a problem hiding this comment.
If a flush is in flight and the user revokes consent, setConsent(false) clears the buffer, but a subsequent send failure re-adds the previously captured events here. That leaves telemetry data resident in memory after explicit opt-out, contradicting the store’s own revocation behavior and making consent enforcement race-dependent.
Useful? React with 👍 / 👎.
Update STATUS.md with post-merge housekeeping entry, recertified test counts (4279 backend + 2245 frontend = ~6500+), and delivered status for distributed caching, SSO/OIDC/MFA, and staged rollout. Update TESTING_GUIDE.md with current test counts and new test categories (resilience, MFA/OIDC, telemetry, cache). Update IMPLEMENTATION_MASTERPLAN.md marking all expansion wave items as delivered. Extend AUTHENTICATION.md with OIDC/SSO login flow, MFA setup/verify/ recovery, API key management, and account linking endpoints. Update MANUAL_TEST_CHECKLIST.md: mark all PRs as merged, add testing tasks for error tracking (#811), MCP HTTP transport (#819), distributed caching (#805), and resilience tests (#820).
Summary
Implements issue #549 — error tracking, web analytics, and product telemetry foundation for Taskdeck.
Sentry.AspNetCore) with hard privacy guardrails (SendDefaultPiialwaysfalse, auth headers stripped from breadcrumbs). Disabled by default.ITelemetryEventServicewith event name validation against the taxonomy (noun.verbformat fromdocs/product/TELEMETRY_TAXONOMY.md), opt-in guard, and batch size limits.GET /api/telemetry/config(anonymous, returns client config) andPOST /api/telemetry/events(authenticated, records batched events).docs/ops/OBSERVABILITY_SETUP.mdcovering all configuration options.All integrations are disabled by default and require explicit configuration (server) + opt-in (user).
Closes #549
Key design decisions
SendDefaultPiiforced tofalsein code (not just config), auth headers stripped from Sentry breadcrumbsnoun.verbtaxonomy before recordingTest plan