haeaeae/silo-server
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(diagnostics): address round-4 review findings on PR #445

Browse Source 
- schema: add crash/report.type conditionals (allOf if/then) so a
  crash/anr/native_crash/hang/abnormal_exit manifest requires `crash`
  and a `manual` manifest forbids it, matching ValidateManifest.
- service: reject uploads where X-Profile-Id and manifest.report.profile_id
  are both present but differ (new ErrProfileMismatch, mapped to 400
  profile_mismatch) instead of silently preferring the header; single-source
  and matching cases unchanged. Adds service tests for mismatch, match, and
  header-only attribution.
- schema: require manifest.json as the first archive.entries element via
  prefixItems (contains retained for validators without prefixItems support).
- schema: document that maxLength is a character-count bound while the server
  enforces UTF-8 byte length, via a top-level note and per-field notes on the
  free-text device_summary and crash fields.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012e3QjbPo96ed9Mn2qRiUkh
This commit is contained in:
Quick104
2026-07-21 12:58:28 -04:00
co-authored by Claude Fable 5
parent 93b851fe81
commit 1f9bd99990
4 changed files with 156 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/design/schemas/client-diagnostics/v1/manifest.schema.json
+75 -1
View File
@@ -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"
]
}
}
}
]
}
internal/api/handlers/diagnostics.go
+2
View File
@@ -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:
internal/diagnostics/service.go
+27 -10
View File
@@ -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
}
internal/diagnostics/service_test.go
+52 -1
View File
@@ -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