diff --git a/internal/api/handlers/admin.go b/internal/api/handlers/admin.go index b55f30f0..979df400 100644 --- a/internal/api/handlers/admin.go +++ b/internal/api/handlers/admin.go @@ -2472,6 +2472,11 @@ func (h *AdminHandler) normalizeDiagnosticsNumericSetting(ctx context.Context, k if value > settings.MaxUncompressedBytes { return "", fmt.Errorf("%s must not exceed %s (%d bytes)", key, diagnostics.KeyMaxUncompressedBytes, settings.MaxUncompressedBytes) } + // A single bundle can never exceed the per-user byte cap, or every upload + // at this size would fail quota; keep the two bounds consistent. + if value > settings.MaxBytesPerUser { + return "", fmt.Errorf("%s must not exceed %s (%d bytes)", key, diagnostics.KeyMaxBytesPerUser, settings.MaxBytesPerUser) + } case diagnostics.KeyMaxUncompressedBytes: if value < settings.MaxBundleBytes || value > gib { return "", fmt.Errorf("%s must be between %s (%d bytes) and 1 GiB (%d bytes)", key, diagnostics.KeyMaxBundleBytes, settings.MaxBundleBytes, gib) @@ -2488,6 +2493,12 @@ func (h *AdminHandler) normalizeDiagnosticsNumericSetting(ctx context.Context, k if value < 10*mib || value > 10*gib { return "", fmt.Errorf("%s must be between 10 MiB (%d bytes) and 10 GiB (%d bytes)", key, 10*mib, 10*gib) } + // The per-user cap must leave room for at least one max-size bundle, or + // /diagnostics/status would advertise a bundle size InsertReceiving always + // rejects as quota_exceeded. + if value < settings.MaxBundleBytes { + return "", fmt.Errorf("%s must be at least %s (%d bytes)", key, diagnostics.KeyMaxBundleBytes, settings.MaxBundleBytes) + } default: return "", fmt.Errorf("unsupported diagnostics numeric setting %s", key) } diff --git a/internal/api/handlers/diagnostics.go b/internal/api/handlers/diagnostics.go index d765f640..d42188b6 100644 --- a/internal/api/handlers/diagnostics.go +++ b/internal/api/handlers/diagnostics.go @@ -323,6 +323,8 @@ func writeDiagnosticsServiceError(w http.ResponseWriter, err error) { writeError(w, http.StatusBadRequest, "archive_mismatch", "Diagnostics archive metadata does not match") case errors.Is(err, diagnostics.ErrProfileMismatch): writeError(w, http.StatusBadRequest, "profile_mismatch", "Diagnostics profile does not match the captured report") + case errors.Is(err, diagnostics.ErrChildProfileForbidden): + writeError(w, http.StatusForbidden, "child_profile_forbidden", "Diagnostics cannot be attributed to a child profile") case errors.Is(err, diagnostics.ErrInvalidBundle): writeError(w, http.StatusBadRequest, "invalid_bundle", "Invalid diagnostics bundle") default: diff --git a/internal/api/handlers/diagnostics_test.go b/internal/api/handlers/diagnostics_test.go index 0bedc264..82af48f9 100644 --- a/internal/api/handlers/diagnostics_test.go +++ b/internal/api/handlers/diagnostics_test.go @@ -447,7 +447,7 @@ func TestDiagnosticsNumericSettingsAcceptBoundaries(t *testing.T) { initial map[string]string }{ {name: "bundle minimum", key: diagnostics.KeyMaxBundleBytes, value: "1048576", initial: map[string]string{diagnostics.KeyMaxUncompressedBytes: "1073741824"}}, - {name: "bundle maximum", key: diagnostics.KeyMaxBundleBytes, value: "268435456", initial: map[string]string{diagnostics.KeyMaxUncompressedBytes: "1073741824"}}, + {name: "bundle maximum", key: diagnostics.KeyMaxBundleBytes, value: "268435456", initial: map[string]string{diagnostics.KeyMaxUncompressedBytes: "1073741824", diagnostics.KeyMaxBytesPerUser: "10737418240"}}, {name: "uncompressed bundle floor", key: diagnostics.KeyMaxUncompressedBytes, value: "1048576", initial: map[string]string{diagnostics.KeyMaxBundleBytes: "1048576"}}, {name: "uncompressed maximum", key: diagnostics.KeyMaxUncompressedBytes, value: "1073741824", initial: map[string]string{diagnostics.KeyMaxBundleBytes: "1048576"}}, {name: "reports minimum", key: diagnostics.KeyMaxReportsPerUserDay, value: "1"}, @@ -482,6 +482,8 @@ func TestDiagnosticsNumericSettingsRejectOutOfRangeValues(t *testing.T) { {name: "bundle above maximum", key: diagnostics.KeyMaxBundleBytes, value: "268435457", initial: map[string]string{diagnostics.KeyMaxUncompressedBytes: "1073741824"}}, {name: "bundle above uncompressed", key: diagnostics.KeyMaxBundleBytes, value: "67108865", initial: map[string]string{diagnostics.KeyMaxUncompressedBytes: "67108864"}}, {name: "uncompressed below bundle", key: diagnostics.KeyMaxUncompressedBytes, value: "10485759", initial: map[string]string{diagnostics.KeyMaxBundleBytes: "10485760"}}, + {name: "bundle above per-user cap", key: diagnostics.KeyMaxBundleBytes, value: "104857600", initial: map[string]string{diagnostics.KeyMaxUncompressedBytes: "1073741824", diagnostics.KeyMaxBytesPerUser: "10485760"}}, + {name: "per-user below bundle", key: diagnostics.KeyMaxBytesPerUser, value: "10485760", initial: map[string]string{diagnostics.KeyMaxBundleBytes: "104857600", diagnostics.KeyMaxUncompressedBytes: "1073741824"}}, {name: "uncompressed above maximum", key: diagnostics.KeyMaxUncompressedBytes, value: "1073741825"}, {name: "reports below minimum", key: diagnostics.KeyMaxReportsPerUserDay, value: "0"}, {name: "reports above maximum", key: diagnostics.KeyMaxReportsPerUserDay, value: "1001"}, diff --git a/internal/api/middleware/demo_guard.go b/internal/api/middleware/demo_guard.go index 9c2725f7..ba0400bf 100644 --- a/internal/api/middleware/demo_guard.go +++ b/internal/api/middleware/demo_guard.go @@ -17,7 +17,8 @@ type DemoSettingsReader interface { // Allowed: browsing, playback, favorites, watchlist, ratings, collections, // profiles, playback progress, watched state. // -// Blocked: API key management, downloads, history imports, subtitle downloads. +// Blocked: API key management, downloads, history imports, subtitle downloads, +// diagnostics report uploads/deletes. type DemoGuard struct { settings DemoSettingsReader } @@ -41,6 +42,9 @@ var demoBlockedRoutes = []blockedRoute{ {methods: []string{"POST"}, prefix: "/api/v1/subtitles/download"}, {methods: []string{"POST"}, prefix: "/api/v1/subtitles/upload"}, {methods: []string{"DELETE"}, prefix: "/api/v1/subtitles/"}, + // Diagnostics report uploads/deletes write DB rows and private-bucket blobs; + // GET /api/v1/diagnostics/status is unaffected (GETs always pass). + {methods: []string{"POST", "DELETE"}, prefix: "/api/v1/diagnostics/reports"}, } // Guard is an HTTP middleware that enforces demo mode restrictions. diff --git a/internal/api/router.go b/internal/api/router.go index 6e57c84f..7165bec4 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -1816,6 +1816,12 @@ func NewRouter(deps Dependencies) chi.Router { if diagnosticsHandler != nil && authMiddleware != nil { r.Group(func(r chi.Router) { r.Use(authMiddleware.RequireAuth) + // Demo mode blocks non-admin report uploads (a write to the + // private bucket and DB); the read-only status endpoint stays + // available because DemoGuard always lets GETs through. + if demoGuard != nil { + r.Use(demoGuard.Guard) + } if deps.RateLimitMW != nil { r.Use(deps.RateLimitMW.Handler) } diff --git a/internal/diagnostics/admin.go b/internal/diagnostics/admin.go index 9bac839c..957c32de 100644 --- a/internal/diagnostics/admin.go +++ b/internal/diagnostics/admin.go @@ -78,13 +78,7 @@ func (s *Service) DeleteReport(ctx context.Context, id string) (*Report, error) return nil, err } if err := deleteReportObjects(ctx, s.store, deleted, s.logger); err != nil { - s.logger.ErrorContext(ctx, "diagnostic report blob deletion failed after row deletion", - "component", "diagnostics", - "report_id", deleted.ID, - "bucket", reportBlobBucket(deleted, s.store), - "keys", reportObjectKeys(*deleted), - "error", err, - ) + logDeferredBlobDeletion(ctx, s.logger, s.store, deleted, err) } return deleted, nil } diff --git a/internal/diagnostics/cleanup.go b/internal/diagnostics/cleanup.go index 55f32bd3..ff591366 100644 --- a/internal/diagnostics/cleanup.go +++ b/internal/diagnostics/cleanup.go @@ -94,14 +94,17 @@ func CleanupReports( return result, errors.Join(errs...) } for _, report := range candidates { - if err := deleteReportObjects(ctx, store, &report, logger); err != nil { - errs = append(errs, fmt.Errorf("delete diagnostic retention blob %s: %w", report.ID, err)) - continue - } + // Row first: a DB failure after the object is gone would leave a visible + // ready report whose bundle 404s on download. If the blob delete then + // fails the row is already gone, so log it for the orphan pass below to + // reap rather than aborting the run (see the admin DeleteReport path). if _, err := repo.DeleteByID(ctx, report.ID); err != nil && !IsReportNotFound(err) { errs = append(errs, fmt.Errorf("delete diagnostic retention row %s: %w", report.ID, err)) continue } + if err := deleteReportObjects(ctx, store, &report, logger); err != nil { + logDeferredBlobDeletion(ctx, logger, store, &report, err) + } result.RetentionReportsDeleted++ } @@ -111,10 +114,9 @@ func CleanupReports( return result, errors.Join(errs...) } for _, report := range stale { - if err := deleteReportObjects(ctx, store, &report, logger); err != nil { - errs = append(errs, fmt.Errorf("delete stale diagnostic blob %s: %w", report.ID, err)) - continue - } + // Same rows-first ordering as retention: mark/delete the row before the + // blob so a mid-cleanup DB failure never leaves a row pointing at a + // missing object, and let orphan cleanup reap a blob that fails to delete. if report.State == StateReceiving { if err := repo.MarkFailed(ctx, report.ID); err != nil && !IsReportNotFound(err) { errs = append(errs, fmt.Errorf("mark stale diagnostic report failed %s: %w", report.ID, err)) @@ -125,6 +127,9 @@ func CleanupReports( errs = append(errs, fmt.Errorf("delete stale diagnostic row %s: %w", report.ID, err)) continue } + if err := deleteReportObjects(ctx, store, &report, logger); err != nil { + logDeferredBlobDeletion(ctx, logger, store, &report, err) + } result.StaleReportsDeleted++ } @@ -196,6 +201,25 @@ func logSkippedObjectDeletion(ctx context.Context, logger *slog.Logger, reportID ) } +// logDeferredBlobDeletion records a report blob that could not be deleted after +// its DB row was already removed. The row is the source of truth, so the object +// is now an orphan for the orphan-cleanup pass (this run or a later one) to +// reap; we log the bucket and keys rather than fail so one unreachable object +// can't block row deletion. Shared by the retention/stale loops and the admin +// DeleteReport path. +func logDeferredBlobDeletion(ctx context.Context, logger *slog.Logger, store ObjectStore, report *Report, err error) { + if logger == nil { + logger = slog.Default() + } + logger.ErrorContext(ctx, "diagnostic report blob deletion failed after row deletion", + "component", "diagnostics", + "report_id", report.ID, + "bucket", reportBlobBucket(report, store), + "keys", reportObjectKeys(*report), + "error", err, + ) +} + func deleteObjectIfPresent(ctx context.Context, store ObjectStore, bucket, key string) error { key = strings.TrimSpace(key) if key == "" { diff --git a/internal/diagnostics/cleanup_test.go b/internal/diagnostics/cleanup_test.go index 2b99794a..c42a441e 100644 --- a/internal/diagnostics/cleanup_test.go +++ b/internal/diagnostics/cleanup_test.go @@ -9,7 +9,7 @@ import ( "time" ) -func TestCleanupReportsDeletesBlobBeforeRowAndToleratesMissingObject(t *testing.T) { +func TestCleanupReportsDeletesRowBeforeBlobAndToleratesMissingObject(t *testing.T) { ops := []string{} repo := &fakeCleanupRepo{ retention: []Report{ @@ -38,15 +38,15 @@ func TestCleanupReportsDeletesBlobBeforeRowAndToleratesMissingObject(t *testing. t.Fatalf("RetentionReportsDeleted = %d, want 2", result.RetentionReportsDeleted) } wantOps := []string{ - "delete-object:diagnostics/7/r1.tar.gz", "delete-row:r1", - "delete-object:diagnostics/7/r2.tar.gz", + "delete-object:diagnostics/7/r1.tar.gz", "delete-row:r2", + "delete-object:diagnostics/7/r2.tar.gz", } assertStrings(t, ops, wantOps) } -func TestCleanupReportsContinuesPastPerReportFailure(t *testing.T) { +func TestCleanupReportsTreatsBlobFailureAsNonFatal(t *testing.T) { ops := []string{} repo := &fakeCleanupRepo{ retention: []Report{ @@ -68,17 +68,20 @@ func TestCleanupReportsContinuesPastPerReportFailure(t *testing.T) { Now: func() time.Time { return time.Date(2026, 7, 20, 12, 0, 0, 0, time.UTC) }, Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), }) - if err == nil { - t.Fatal("CleanupReports error = nil, want aggregated per-report failure") + // r1's row is deleted first, so its blob delete failing does not abort the + // run or roll back the row: it is logged for orphan cleanup to reap, both + // rows count as deleted, and no error is surfaced. + if err != nil { + t.Fatalf("CleanupReports: %v", err) } - // r1's blob delete failed, so its row is left intact; r2 is still processed. - if result.RetentionReportsDeleted != 1 { - t.Fatalf("RetentionReportsDeleted = %d, want 1", result.RetentionReportsDeleted) + if result.RetentionReportsDeleted != 2 { + t.Fatalf("RetentionReportsDeleted = %d, want 2", result.RetentionReportsDeleted) } assertStrings(t, ops, []string{ + "delete-row:r1", "delete-object:diagnostics/7/r1.tar.gz", - "delete-object:diagnostics/7/r2.tar.gz", "delete-row:r2", + "delete-object:diagnostics/7/r2.tar.gz", }) } @@ -103,9 +106,9 @@ func TestCleanupReportsCleansStaleReceiving(t *testing.T) { t.Fatalf("StaleReportsDeleted = %d, want 1", result.StaleReportsDeleted) } assertStrings(t, ops, []string{ - "delete-object:diagnostics/7/r1.tar.gz", "mark-failed:r1", "delete-row:r1", + "delete-object:diagnostics/7/r1.tar.gz", }) } diff --git a/internal/diagnostics/repo.go b/internal/diagnostics/repo.go index d4378fc8..2286454a 100644 --- a/internal/diagnostics/repo.go +++ b/internal/diagnostics/repo.go @@ -260,7 +260,7 @@ func (r *PostgresRepository) ListForAdmin(ctx context.Context, filters ListFilte } query := fmt.Sprintf("%s\n\t\tWHERE %s\n\t\tORDER BY received_at DESC, id DESC\n\t\tLIMIT $%d", - reportSelectSQL(), strings.Join(conditions, " AND "), argIdx) + reportListSelectSQL(), strings.Join(conditions, " AND "), argIdx) args = append(args, limit+1) rows, err := r.pool.Query(ctx, query, args...) @@ -271,7 +271,7 @@ func (r *PostgresRepository) ListForAdmin(ctx context.Context, filters ListFilte reports := make([]Report, 0, limit+1) for rows.Next() { - report, err := scanReport(rows) + report, err := scanReportSummary(rows) if err != nil { return ListResult{}, err } @@ -314,13 +314,13 @@ func (r *PostgresRepository) RetentionCandidates(ctx context.Context, olderThan return nil, nil } - query := reportSelectSQL() + ` + query := reportListSelectSQL() + ` WHERE received_at < $1 ORDER BY received_at ASC, id ASC ` args := []any{olderThan} if olderThan.IsZero() { - query = reportSelectSQL() + ` + query = reportListSelectSQL() + ` WHERE false ` args = nil @@ -341,12 +341,12 @@ func (r *PostgresRepository) RetentionCandidates(ctx context.Context, olderThan ) ranked WHERE retained_bytes > $1 ) - ` + reportSelectSQL() + ` + ` + reportListSelectSQL() + ` WHERE id IN (SELECT id FROM quota_candidates) ORDER BY received_at ASC, id ASC ` if olderThan.IsZero() { - return r.queryReports(ctx, byteCapQuery, perUserByteCap) + return r.queryReportSummaries(ctx, byteCapQuery, perUserByteCap) } query = ` @@ -363,14 +363,14 @@ func (r *PostgresRepository) RetentionCandidates(ctx context.Context, olderThan ) ranked WHERE retained_bytes > $2 ) - ` + reportSelectSQL() + ` + ` + reportListSelectSQL() + ` WHERE received_at < $1 OR id IN (SELECT id FROM quota_candidates) ORDER BY received_at ASC, id ASC ` args = []any{olderThan, perUserByteCap} } - return r.queryReports(ctx, query, args...) + return r.queryReportSummaries(ctx, query, args...) } func (r *PostgresRepository) StaleReceiving(ctx context.Context, grace time.Duration) ([]Report, error) { @@ -378,7 +378,7 @@ func (r *PostgresRepository) StaleReceiving(ctx context.Context, grace time.Dura grace = time.Hour } cutoff := time.Now().UTC().Add(-grace) - return r.queryReports(ctx, reportSelectSQL()+` + return r.queryReportSummaries(ctx, reportListSelectSQL()+` WHERE state IN ('receiving', 'failed') AND received_at < $1 ORDER BY received_at ASC, id ASC @@ -417,7 +417,9 @@ func (r *PostgresRepository) LiveBlobKeys(ctx context.Context, keys []string) (m return live, nil } -func (r *PostgresRepository) queryReports(ctx context.Context, query string, args ...any) ([]Report, error) { +// queryReportSummaries runs a reportListSelectSQL-shaped query; the returned +// reports carry no manifest. Only list/cleanup paths use it. +func (r *PostgresRepository) queryReportSummaries(ctx context.Context, query string, args ...any) ([]Report, error) { rows, err := r.pool.Query(ctx, query, args...) if err != nil { return nil, fmt.Errorf("query diagnostic reports: %w", err) @@ -426,7 +428,7 @@ func (r *PostgresRepository) queryReports(ctx context.Context, query string, arg reports := []Report{} for rows.Next() { - report, err := scanReport(rows) + report, err := scanReportSummary(rows) if err != nil { return nil, err } @@ -463,6 +465,10 @@ func validateInsertReceivingInput(input InsertReceivingInput) error { return nil } +// reportSelectSQL is the full projection including the manifest JSONB (up to +// MaxManifestBytes per row). Reserve it for single-report detail/download paths; +// list and cleanup queries use reportListSelectSQL so browsing a page or running +// a retention/stale batch doesn't drag every report's manifest through the DB. func reportSelectSQL() string { return `SELECT id::text, short_id, user_id, profile_id, state, captured_at, received_at, report_type, platform, app_version, crash_summary, manifest, playback_session_ids, @@ -470,64 +476,120 @@ func reportSelectSQL() string { FROM client_diagnostic_reports` } +// reportListSelectSQL mirrors reportSelectSQL but omits the manifest column. +// Reports scanned with scanReportSummary therefore have a nil Manifest; the +// list and cleanup callers only need summary and blob fields. +func reportListSelectSQL() string { + return `SELECT id::text, short_id, user_id, profile_id, state, captured_at, received_at, + report_type, platform, app_version, crash_summary, playback_session_ids, + blob_bucket, blob_key, blob_bytes, uncompressed_bytes, blob_sha256 + FROM client_diagnostic_reports` +} + type reportScanner interface { Scan(dest ...any) error } +// reportNulls holds the nullable columns shared by the full and summary +// projections so both scanners apply them the same way. +type reportNulls struct { + profileID sql.NullString + crashSummary sql.NullString + blobBucket sql.NullString + blobKey sql.NullString + blobSHA256 sql.NullString + blobBytes sql.NullInt64 + uncompressedBytes sql.NullInt64 +} + +func (n *reportNulls) apply(report *Report) { + if n.profileID.Valid { + report.ProfileID = &n.profileID.String + } + if n.crashSummary.Valid { + report.CrashSummary = &n.crashSummary.String + } + if report.PlaybackSessionIDs == nil { + report.PlaybackSessionIDs = []string{} + } + if n.blobBucket.Valid { + report.BlobBucket = &n.blobBucket.String + } + if n.blobKey.Valid { + report.BlobKey = &n.blobKey.String + } + if n.blobBytes.Valid { + report.BlobBytes = &n.blobBytes.Int64 + } + if n.uncompressedBytes.Valid { + report.UncompressedBytes = &n.uncompressedBytes.Int64 + } + if n.blobSHA256.Valid { + report.BlobSHA256 = &n.blobSHA256.String + } +} + +// scanReport scans the full reportSelectSQL projection, including the manifest. func scanReport(row reportScanner) (*Report, error) { var report Report - var profileID, crashSummary, blobBucket, blobKey, blobSHA256 sql.NullString - var blobBytes, uncompressedBytes sql.NullInt64 + var nulls reportNulls var manifest []byte if err := row.Scan( &report.ID, &report.ShortID, &report.UserID, - &profileID, + &nulls.profileID, &report.State, &report.CapturedAt, &report.ReceivedAt, &report.ReportType, &report.Platform, &report.AppVersion, - &crashSummary, + &nulls.crashSummary, &manifest, &report.PlaybackSessionIDs, - &blobBucket, - &blobKey, - &blobBytes, - &uncompressedBytes, - &blobSHA256, + &nulls.blobBucket, + &nulls.blobKey, + &nulls.blobBytes, + &nulls.uncompressedBytes, + &nulls.blobSHA256, ); err != nil { return nil, fmt.Errorf("scan diagnostic report: %w", err) } - if profileID.Valid { - report.ProfileID = &profileID.String - } - if crashSummary.Valid { - report.CrashSummary = &crashSummary.String - } if len(manifest) > 0 { report.Manifest = append(json.RawMessage(nil), manifest...) } - if report.PlaybackSessionIDs == nil { - report.PlaybackSessionIDs = []string{} - } - if blobBucket.Valid { - report.BlobBucket = &blobBucket.String - } - if blobKey.Valid { - report.BlobKey = &blobKey.String - } - if blobBytes.Valid { - report.BlobBytes = &blobBytes.Int64 - } - if uncompressedBytes.Valid { - report.UncompressedBytes = &uncompressedBytes.Int64 - } - if blobSHA256.Valid { - report.BlobSHA256 = &blobSHA256.String + nulls.apply(&report) + return &report, nil +} + +// scanReportSummary scans the reportListSelectSQL projection, which omits the +// manifest; the returned report's Manifest is left nil. +func scanReportSummary(row reportScanner) (*Report, error) { + var report Report + var nulls reportNulls + if err := row.Scan( + &report.ID, + &report.ShortID, + &report.UserID, + &nulls.profileID, + &report.State, + &report.CapturedAt, + &report.ReceivedAt, + &report.ReportType, + &report.Platform, + &report.AppVersion, + &nulls.crashSummary, + &report.PlaybackSessionIDs, + &nulls.blobBucket, + &nulls.blobKey, + &nulls.blobBytes, + &nulls.uncompressedBytes, + &nulls.blobSHA256, + ); err != nil { + return nil, fmt.Errorf("scan diagnostic report: %w", err) } + nulls.apply(&report) return &report, nil } diff --git a/internal/diagnostics/service.go b/internal/diagnostics/service.go index 5ae38b9d..8c1731dd 100644 --- a/internal/diagnostics/service.go +++ b/internal/diagnostics/service.go @@ -34,6 +34,7 @@ var ( ErrArchiveMismatch = errors.New("diagnostics archive metadata mismatch") ErrReportStoreUnavailable = errors.New("diagnostics report store unavailable") ErrProfileMismatch = errors.New("diagnostics profile id mismatch") + ErrChildProfileForbidden = errors.New("diagnostics child profile attribution forbidden") ) type AvailabilityStatus string @@ -99,15 +100,20 @@ type ProfileLookup func(ctx context.Context, userID int, profileID string) (foun // attributes a report to a profile only when it belongs to the user and is not // a child profile. The client diagnostics design forbids child profiles from // performing diagnostics actions, so a nonconforming client that sends a child -// profile's ID (via X-Profile-Id or a manifest profile_id) has that attribution -// rejected rather than recorded against the child. +// profile's ID (via X-Profile-Id or a manifest profile_id) has the upload +// rejected with ErrChildProfileForbidden rather than silently losing the +// attribution. A profile that is simply not the user's (found == false) is not +// an error: attribution is dropped and the upload proceeds unattributed. func NewProfileAttributionValidator(lookup ProfileLookup) ProfileAttributionValidator { return ProfileAttributionValidatorFunc(func(ctx context.Context, userID int, profileID string) (bool, error) { found, isChild, err := lookup(ctx, userID, profileID) if err != nil { return false, err } - return found && !isChild, nil + if found && isChild { + return false, ErrChildProfileForbidden + } + return found, nil }) } @@ -479,6 +485,12 @@ func (s *Service) validatedProfileID(ctx context.Context, userID int, profileID } found, err := s.profileValidator.ProfileBelongsToUser(ctx, userID, *candidate) if err != nil { + // A child-profile attribution is a client-facing rejection, not an + // internal failure: surface it verbatim so the handler can return a + // clear error instead of a generic 500. + if errors.Is(err, ErrChildProfileForbidden) { + return nil, err + } return nil, fmt.Errorf("validate diagnostic profile attribution: %w", err) } if !found { diff --git a/internal/diagnostics/service_test.go b/internal/diagnostics/service_test.go index e8ae1a86..1726ee4c 100644 --- a/internal/diagnostics/service_test.go +++ b/internal/diagnostics/service_test.go @@ -301,16 +301,17 @@ func TestServiceIngestAcceptsMatchingProfileID(t *testing.T) { func TestNewProfileAttributionValidatorRejectsChildProfiles(t *testing.T) { tests := []struct { - name string - found bool - isChild bool - lookErr error - want bool - wantErr bool + name string + found bool + isChild bool + lookErr error + want bool + wantErr bool + wantChild bool }{ {name: "non-child profile attributed", found: true, isChild: false, want: true}, - {name: "child profile rejected", found: true, isChild: true, want: false}, - {name: "missing profile rejected", found: false, isChild: false, want: false}, + {name: "child profile forbidden", found: true, isChild: true, wantErr: true, wantChild: true}, + {name: "missing profile dropped", found: false, isChild: false, want: false}, {name: "lookup error propagated", lookErr: errors.New("boom"), wantErr: true}, } for _, tc := range tests { @@ -326,6 +327,9 @@ func TestNewProfileAttributionValidatorRejectsChildProfiles(t *testing.T) { if err == nil { t.Fatal("ProfileBelongsToUser error = nil, want error") } + if tc.wantChild && !errors.Is(err, ErrChildProfileForbidden) { + t.Fatalf("ProfileBelongsToUser error = %v, want ErrChildProfileForbidden", err) + } return } if err != nil { @@ -338,6 +342,30 @@ func TestNewProfileAttributionValidatorRejectsChildProfiles(t *testing.T) { } } +func TestServiceIngestRejectsChildProfileAttribution(t *testing.T) { + // A nonconforming client attributes the report to a child profile. The + // validator rejects it with ErrChildProfileForbidden, so the upload is + // refused outright rather than silently stored without attribution. + bundle, manifest, _ := testDiagnosticsUpload(t, "server-1", DefaultConsentNoticeVer, "prof_child") + repo := &fakeDiagnosticReportStore{} + store := &fakeDiagnosticObjectStore{bucket: "private"} + svc := newTestDiagnosticsService(repo, store) + svc.SetProfileAttributionValidator(ProfileAttributionValidatorFunc(func(context.Context, int, string) (bool, error) { + return false, ErrChildProfileForbidden + })) + + _, err := svc.Ingest(context.Background(), 42, nil, manifest, bytes.NewReader(bundle)) + if !errors.Is(err, ErrChildProfileForbidden) { + t.Fatalf("Ingest error = %v, want ErrChildProfileForbidden", err) + } + if repo.insertInput.ProfileID != nil { + t.Fatalf("ProfileID = %v, want no insert on child profile", *repo.insertInput.ProfileID) + } + if len(repo.ready) != 0 || len(store.puts) != 0 { + t.Fatalf("no report should be stored on child rejection: ready=%v puts=%v", repo.ready, store.puts) + } +} + func TestServiceIngestAcceptsManifestWithoutProfileID(t *testing.T) { bundle, manifest, _ := testDiagnosticsUpload(t, "server-1", DefaultConsentNoticeVer, "") repo := &fakeDiagnosticReportStore{} diff --git a/internal/diagnostics/types.go b/internal/diagnostics/types.go index b697b926..925d1686 100644 --- a/internal/diagnostics/types.go +++ b/internal/diagnostics/types.go @@ -82,7 +82,7 @@ type Report struct { Platform string `json:"platform"` AppVersion string `json:"app_version"` CrashSummary *string `json:"crash_summary,omitempty"` - Manifest json.RawMessage `json:"manifest"` + Manifest json.RawMessage `json:"manifest,omitempty"` PlaybackSessionIDs []string `json:"playback_session_ids"` BlobBucket *string `json:"blob_bucket,omitempty"` BlobKey *string `json:"blob_key,omitempty"`