diff --git a/docs/design/schemas/client-diagnostics/v1/manifest.schema.json b/docs/design/schemas/client-diagnostics/v1/manifest.schema.json index 59504f0a..b0da6ff0 100644 --- a/docs/design/schemas/client-diagnostics/v1/manifest.schema.json +++ b/docs/design/schemas/client-diagnostics/v1/manifest.schema.json @@ -2,6 +2,7 @@ "$schema": "https://json-schema.org/draft/2020-12/schema", "$id": "https://silo-server.dev/schemas/client-diagnostics/v1/manifest.schema.json", "title": "Silo Client Diagnostics Manifest v1", + "description": "Canonical manifest contract for a client diagnostics bundle. STRING LENGTHS: every `maxLength` in this schema is a character-count sanity bound. The server enforces the matching limit as a UTF-8 *byte* length (Go `len(string)`), so a value near a cap that contains multi-byte characters (CJK, emoji, accented text) can satisfy `maxLength` here yet be rejected at ingest. Treat each numeric cap as a byte budget for non-ASCII text. JSON Schema cannot express byte length, so this divergence is intentional and documented rather than encoded.", "type": "object", "required": [ "schema_version", @@ -133,14 +134,17 @@ "summary": { "type": "string", "minLength": 1, + "description": "Free-text crash summary; may contain non-ASCII. maxLength counts characters — the server enforces 8192 UTF-8 bytes (see the schema-level string-length note).", "maxLength": 8192 }, "stack_excerpt": { "type": "string", + "description": "Free-text stack excerpt; may contain non-ASCII. maxLength counts characters — the server enforces 8192 UTF-8 bytes (see the schema-level string-length note).", "maxLength": 8192 }, "thread": { "type": "string", + "description": "Thread name; may contain non-ASCII. maxLength counts characters — the server enforces 128 UTF-8 bytes (see the schema-level string-length note).", "maxLength": 128 }, "foreground": { @@ -185,21 +189,25 @@ "manufacturer": { "type": "string", "minLength": 1, + "description": "Device manufacturer; may contain non-ASCII. maxLength counts characters — the server enforces 128 UTF-8 bytes (see the schema-level string-length note).", "maxLength": 128 }, "model": { "type": "string", "minLength": 1, + "description": "Device model; may contain non-ASCII (e.g. CJK). maxLength counts characters — the server enforces 128 UTF-8 bytes, so a 128-character multi-byte value can pass this schema yet be rejected at ingest (see the schema-level string-length note).", "maxLength": 128 }, "os": { "type": "string", "minLength": 1, + "description": "OS name/version; may contain non-ASCII. maxLength counts characters — the server enforces 128 UTF-8 bytes (see the schema-level string-length note).", "maxLength": 128 }, "form_factor": { "type": "string", "minLength": 1, + "description": "Device form factor; may contain non-ASCII. maxLength counts characters — the server enforces 64 UTF-8 bytes (see the schema-level string-length note).", "maxLength": 64 } }, @@ -276,9 +284,15 @@ "properties": { "entries": { "type": "array", + "description": "Tar member names in bundle order. The first entry MUST be `manifest.json`: the server reads and validates the leading member before any other and compares this order against the actual bundle. `prefixItems` pins index 0, while `contains` keeps `manifest.json` required for validators that do not implement `prefixItems`.", "uniqueItems": true, "minItems": 1, "maxItems": 8, + "prefixItems": [ + { + "const": "manifest.json" + } + ], "contains": { "const": "manifest.json" }, @@ -314,5 +328,65 @@ "additionalProperties": true } }, - "additionalProperties": true + "additionalProperties": true, + "allOf": [ + { + "$comment": "Event reports (crash/anr/native_crash/hang/abnormal_exit) must carry the crash object; mirrors ValidateManifest in internal/diagnostics/contract/contract.go.", + "if": { + "properties": { + "report": { + "properties": { + "type": { + "enum": [ + "crash", + "anr", + "native_crash", + "hang", + "abnormal_exit" + ] + } + }, + "required": [ + "type" + ] + } + }, + "required": [ + "report" + ] + }, + "then": { + "required": [ + "crash" + ] + } + }, + { + "$comment": "Manual captures must not carry a crash object.", + "if": { + "properties": { + "report": { + "properties": { + "type": { + "const": "manual" + } + }, + "required": [ + "type" + ] + } + }, + "required": [ + "report" + ] + }, + "then": { + "not": { + "required": [ + "crash" + ] + } + } + } + ] } diff --git a/internal/api/handlers/diagnostics.go b/internal/api/handlers/diagnostics.go index ae7e2236..d765f640 100644 --- a/internal/api/handlers/diagnostics.go +++ b/internal/api/handlers/diagnostics.go @@ -321,6 +321,8 @@ func writeDiagnosticsServiceError(w http.ResponseWriter, err error) { writeError(w, http.StatusBadRequest, "stale_consent", "Diagnostics consent notice is stale") case errors.Is(err, diagnostics.ErrArchiveMismatch): 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.ErrInvalidBundle): writeError(w, http.StatusBadRequest, "invalid_bundle", "Invalid diagnostics bundle") default: diff --git a/internal/diagnostics/service.go b/internal/diagnostics/service.go index 6f6d5dba..5ae38b9d 100644 --- a/internal/diagnostics/service.go +++ b/internal/diagnostics/service.go @@ -33,6 +33,7 @@ var ( ErrStaleConsent = errors.New("diagnostics consent notice is stale") ErrArchiveMismatch = errors.New("diagnostics archive metadata mismatch") ErrReportStoreUnavailable = errors.New("diagnostics report store unavailable") + ErrProfileMismatch = errors.New("diagnostics profile id mismatch") ) type AvailabilityStatus string @@ -438,22 +439,38 @@ func manifestHasUnsupportedSchema(data []byte) bool { return header.SchemaVersion != nil && *header.SchemaVersion != contract.SchemaVersion } -func normalizedProfileID(profileID *string, fallback string) *string { - value := "" +// resolveProfileID chooses the profile a report is attributed to from the +// X-Profile-Id header and the manifest's report.profile_id fallback. When both +// are present they must agree: a header captured under a different profile than +// the manifest means the client switched profiles between capture and upload, +// and silently preferring either source would attribute the DB row to one +// profile while the stored manifest/download still names the other. Reject that +// as ErrProfileMismatch. Single-source cases (only one present, or both equal) +// resolve to that value; when neither is present the report is unattributed. +func resolveProfileID(profileID *string, fallback string) (*string, error) { + header := "" if profileID != nil { - value = strings.TrimSpace(*profileID) + header = strings.TrimSpace(*profileID) } - if value == "" { - value = strings.TrimSpace(fallback) + manifestID := strings.TrimSpace(fallback) + + switch { + case header != "" && manifestID != "" && header != manifestID: + return nil, fmt.Errorf("%w: header %q, manifest %q", ErrProfileMismatch, header, manifestID) + case header != "": + return &header, nil + case manifestID != "": + return &manifestID, nil + default: + return nil, nil } - if value == "" { - return nil - } - return &value } func (s *Service) validatedProfileID(ctx context.Context, userID int, profileID *string, fallback string) (*string, error) { - candidate := normalizedProfileID(profileID, fallback) + candidate, err := resolveProfileID(profileID, fallback) + if err != nil { + return nil, err + } if candidate == nil { return nil, nil } diff --git a/internal/diagnostics/service_test.go b/internal/diagnostics/service_test.go index d55050c3..e8ae1a86 100644 --- a/internal/diagnostics/service_test.go +++ b/internal/diagnostics/service_test.go @@ -218,7 +218,9 @@ func TestServiceIngestCompensatesWithDetachedContext(t *testing.T) { } func TestServiceIngestStoresOnlyValidatedProfileAttribution(t *testing.T) { - bundle, manifest, _ := testDiagnosticsUpload(t, "server-1", DefaultConsentNoticeVer, "prof_1") + // Header-only attribution (manifest carries no profile_id): the header is + // the single source, so it is validated and stored. + bundle, manifest, _ := testDiagnosticsUpload(t, "server-1", DefaultConsentNoticeVer, "") repo := &fakeDiagnosticReportStore{} store := &fakeDiagnosticObjectStore{bucket: "private"} svc := newTestDiagnosticsService(repo, store) @@ -234,6 +236,8 @@ func TestServiceIngestStoresOnlyValidatedProfileAttribution(t *testing.T) { t.Fatalf("ProfileID = %v, want prof_header", repo.insertInput.ProfileID) } + // Manifest-only attribution that fails validation is dropped, not stored. + bundle, manifest, _ = testDiagnosticsUpload(t, "server-1", DefaultConsentNoticeVer, "prof_1") repo = &fakeDiagnosticReportStore{} store = &fakeDiagnosticObjectStore{bucket: "private"} svc = newTestDiagnosticsService(repo, store) @@ -248,6 +252,53 @@ func TestServiceIngestStoresOnlyValidatedProfileAttribution(t *testing.T) { } } +func TestServiceIngestRejectsMismatchedProfileID(t *testing.T) { + // The manifest was captured under prof_manifest but the request carries a + // different X-Profile-Id (prof_header) because the client switched profiles + // between capture and upload. The two sources must not be silently + // reconciled; the upload is rejected and no report row is created. + bundle, manifest, _ := testDiagnosticsUpload(t, "server-1", DefaultConsentNoticeVer, "prof_manifest") + repo := &fakeDiagnosticReportStore{} + store := &fakeDiagnosticObjectStore{bucket: "private"} + svc := newTestDiagnosticsService(repo, store) + // Even a validator that would accept any profile must not paper over the + // mismatch: resolution fails before attribution validation runs. + svc.SetProfileAttributionValidator(ProfileAttributionValidatorFunc(func(context.Context, int, string) (bool, error) { + return true, nil + })) + + headerProfileID := "prof_header" + _, err := svc.Ingest(context.Background(), 42, &headerProfileID, manifest, bytes.NewReader(bundle)) + if !errors.Is(err, ErrProfileMismatch) { + t.Fatalf("Ingest error = %v, want ErrProfileMismatch", err) + } + if repo.insertInput.ProfileID != nil { + t.Fatalf("ProfileID = %v, want no insert on profile mismatch", *repo.insertInput.ProfileID) + } + if len(repo.ready) != 0 || len(store.puts) != 0 { + t.Fatalf("no report should be stored on mismatch: ready=%v puts=%v", repo.ready, store.puts) + } +} + +func TestServiceIngestAcceptsMatchingProfileID(t *testing.T) { + // Header and manifest agree: attribution resolves to that single value. + bundle, manifest, _ := testDiagnosticsUpload(t, "server-1", DefaultConsentNoticeVer, "prof_1") + repo := &fakeDiagnosticReportStore{} + store := &fakeDiagnosticObjectStore{bucket: "private"} + svc := newTestDiagnosticsService(repo, store) + svc.SetProfileAttributionValidator(ProfileAttributionValidatorFunc(func(_ context.Context, userID int, profileID string) (bool, error) { + return userID == 42 && profileID == "prof_1", nil + })) + + headerProfileID := "prof_1" + if _, err := svc.Ingest(context.Background(), 42, &headerProfileID, manifest, bytes.NewReader(bundle)); err != nil { + t.Fatalf("Ingest: %v", err) + } + if repo.insertInput.ProfileID == nil || *repo.insertInput.ProfileID != "prof_1" { + t.Fatalf("ProfileID = %v, want prof_1", repo.insertInput.ProfileID) + } +} + func TestNewProfileAttributionValidatorRejectsChildProfiles(t *testing.T) { tests := []struct { name string