From 93b851fe81c119e5aa52aff9cd1e424bcf16fe26 Mon Sep 17 00:00:00 2001 From: Quick104 <31828688+Quick104@users.noreply.github.com> Date: Tue, 21 Jul 2026 12:25:11 -0400 Subject: [PATCH] fix(diagnostics): address round-3 review findings on PR #445 - Extend the upload write deadline alongside the read deadline so a slow upload finishing after the integrated server's 120s WriteTimeout can still return its success response instead of timing out a report that succeeded. - Reject child-profile attribution for diagnostics: wire the attribution validator through a shared profile lookup that reports IsChild and drop attribution for child profiles, which must not perform diagnostics actions. - Assert the download test captures the clicked anchor and checks its blob: href and silo-diagnostics-.tar.gz filename, not just cleanup. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012e3QjbPo96ed9Mn2qRiUkh --- internal/api/handlers/diagnostics.go | 11 ++++++ internal/api/router.go | 33 ++++++++++++---- internal/diagnostics/service.go | 20 ++++++++++ internal/diagnostics/service_test.go | 39 +++++++++++++++++++ .../hooks/queries/admin/diagnostics.test.ts | 5 +++ 5 files changed, 100 insertions(+), 8 deletions(-) diff --git a/internal/api/handlers/diagnostics.go b/internal/api/handlers/diagnostics.go index 4c19acda..ae7e2236 100644 --- a/internal/api/handlers/diagnostics.go +++ b/internal/api/handlers/diagnostics.go @@ -82,6 +82,17 @@ func (h *DiagnosticsHandler) HandleUpload(w http.ResponseWriter, r *http.Request "error", err, ) } + // Extend the write deadline the same way. cmd/silo's integrated server sets + // WriteTimeout 120s, but a slow upload can take longer than that within the + // read window above; Ingest may store and mark the report ready and then the + // final writeJSON would miss the 120s write deadline, so the client sees a + // timeout and retries a report that already succeeded. + if err := rc.SetWriteDeadline(time.Now().Add(diagnosticsUploadReadTimeout)); err != nil { + h.diagnosticsLogger().WarnContext(r.Context(), "diagnostics upload write deadline not extended", + "component", "diagnostics", + "error", err, + ) + } userID, ok := diagnosticsUserID(w, r) if !ok { diff --git a/internal/api/router.go b/internal/api/router.go index 118667a5..6e57c84f 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -255,14 +255,22 @@ func NewRouter(deps Dependencies) chi.Router { // acting-admin profile policy, degrading admin routes to the plain // role check. var checkPrimaryProfile apimw.PrimaryProfileChecker + // lookupProfile resolves a profile for the given user, returning nil when it + // does not exist. Shared by the acting-admin primary check and the + // diagnostics profile-attribution validator so both read profile state + // (IsPrimary, IsChild) the same way. + var lookupProfile func(ctx context.Context, userID int, profileID string) (*userstore.Profile, error) if deps.UserStoreProvider != nil { userStores := deps.UserStoreProvider - checkPrimaryProfile = func(ctx context.Context, userID int, profileID string) (bool, bool, error) { + lookupProfile = func(ctx context.Context, userID int, profileID string) (*userstore.Profile, error) { store, err := userStores.ForUser(ctx, userID) if err != nil { - return false, false, err + return nil, err } - profile, err := store.GetProfile(ctx, profileID) + return store.GetProfile(ctx, profileID) + } + checkPrimaryProfile = func(ctx context.Context, userID int, profileID string) (bool, bool, error) { + profile, err := lookupProfile(ctx, userID, profileID) if err != nil { return false, false, err } @@ -321,11 +329,20 @@ func NewRouter(deps Dependencies) chi.Router { diagnosticsStore, slog.Default(), ) - if checkPrimaryProfile != nil { - diagnosticsService.SetProfileAttributionValidator(diagnostics.ProfileAttributionValidatorFunc( - func(ctx context.Context, userID int, profileID string) (bool, error) { - _, found, err := checkPrimaryProfile(ctx, userID, profileID) - return found, err + if lookupProfile != nil { + // Attribute reports only to a profile that belongs to the account and + // is not a child profile; child profiles must not perform diagnostics + // actions per the design, so reject their attribution here. + diagnosticsService.SetProfileAttributionValidator(diagnostics.NewProfileAttributionValidator( + func(ctx context.Context, userID int, profileID string) (bool, bool, error) { + profile, err := lookupProfile(ctx, userID, profileID) + if err != nil { + return false, false, err + } + if profile == nil { + return false, false, nil + } + return true, profile.IsChild, nil }, )) } diff --git a/internal/diagnostics/service.go b/internal/diagnostics/service.go index 822bc46a..6f6d5dba 100644 --- a/internal/diagnostics/service.go +++ b/internal/diagnostics/service.go @@ -90,6 +90,26 @@ func (f ProfileAttributionValidatorFunc) ProfileBelongsToUser(ctx context.Contex return f(ctx, userID, profileID) } +// ProfileLookup resolves whether profileID exists for userID and whether it is +// a child profile. It backs NewProfileAttributionValidator. +type ProfileLookup func(ctx context.Context, userID int, profileID string) (found bool, isChild bool, err error) + +// NewProfileAttributionValidator builds a ProfileAttributionValidator that +// 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. +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 + }) +} + func (s *Service) SetProfileAttributionValidator(validator ProfileAttributionValidator) { s.profileValidator = validator } diff --git a/internal/diagnostics/service_test.go b/internal/diagnostics/service_test.go index 9048a5f2..d55050c3 100644 --- a/internal/diagnostics/service_test.go +++ b/internal/diagnostics/service_test.go @@ -248,6 +248,45 @@ func TestServiceIngestStoresOnlyValidatedProfileAttribution(t *testing.T) { } } +func TestNewProfileAttributionValidatorRejectsChildProfiles(t *testing.T) { + tests := []struct { + name string + found bool + isChild bool + lookErr error + want bool + wantErr 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: "lookup error propagated", lookErr: errors.New("boom"), wantErr: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + validator := NewProfileAttributionValidator(func(_ context.Context, userID int, profileID string) (bool, bool, error) { + if userID != 42 || profileID != "prof_1" { + t.Fatalf("lookup called with userID=%d profileID=%q", userID, profileID) + } + return tc.found, tc.isChild, tc.lookErr + }) + got, err := validator.ProfileBelongsToUser(context.Background(), 42, "prof_1") + if tc.wantErr { + if err == nil { + t.Fatal("ProfileBelongsToUser error = nil, want error") + } + return + } + if err != nil { + t.Fatalf("ProfileBelongsToUser: %v", err) + } + if got != tc.want { + t.Fatalf("ProfileBelongsToUser = %v, want %v", got, tc.want) + } + }) + } +} + func TestServiceIngestAcceptsManifestWithoutProfileID(t *testing.T) { bundle, manifest, _ := testDiagnosticsUpload(t, "server-1", DefaultConsentNoticeVer, "") repo := &fakeDiagnosticReportStore{} diff --git a/web/src/hooks/queries/admin/diagnostics.test.ts b/web/src/hooks/queries/admin/diagnostics.test.ts index 306dfef3..9af2572b 100644 --- a/web/src/hooks/queries/admin/diagnostics.test.ts +++ b/web/src/hooks/queries/admin/diagnostics.test.ts @@ -40,6 +40,11 @@ describe("downloadDiagnosticReport", () => { ); expect(click).toHaveBeenCalledOnce(); expect(objectURL).toHaveBeenCalledOnce(); + // The click spy records `this` as the anchor the download helper clicked; + // assert it carried the blob URL and expected filename before cleanup. + const clickedAnchor = click.mock.contexts[0] as HTMLAnchorElement; + expect(clickedAnchor.href).toBe("blob:diagnostic"); + expect(clickedAnchor.download).toBe("silo-diagnostics-ABCDEF123456.tar.gz"); expect(document.querySelector('a[download="silo-diagnostics-ABCDEF123456.tar.gz"]')).toBeNull(); await new Promise((resolve) => window.setTimeout(resolve, 0));