From 6a0b9210253f03f10c45441701e9edb508c3febf Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Wed, 22 Jul 2026 11:05:26 +0200 Subject: [PATCH] fix(controlplane): stop masking transient org-lookup errors as not found MembershipRepo.FindByOrgNameAndUser masked any non-NotFound failure of the org query as a fabricated "organization not found" error that was neither a biz.NotFound nor a wrap of the real cause. This discarded the underlying DB error and made ContextService.Current return a captured gRPC 500 for what was actually a transient failure against an org that exists. Only ent.IsNotFound now maps to biz.NewErrNotFound; every other error is returned wrapped with %w so the true root cause (SQLSTATE, statement or pool timeout, context cancellation) reaches logs and Sentry, while the client-facing masking still happens downstream in handleUseCaseErr. Assisted-by: Claude Code Signed-off-by: Javier Rodriguez Chainloop-Trace-Sessions: 421c4127-1b07-498e-aff5-0d3fef860c3c --- .../internal/service/service_test.go | 29 ++++++++++++- .../pkg/biz/membership_integration_test.go | 41 +++++++++++++++++++ app/controlplane/pkg/data/membership.go | 6 ++- 3 files changed, 73 insertions(+), 3 deletions(-) diff --git a/app/controlplane/internal/service/service_test.go b/app/controlplane/internal/service/service_test.go index 939405671..4b294aa30 100644 --- a/app/controlplane/internal/service/service_test.go +++ b/app/controlplane/internal/service/service_test.go @@ -30,6 +30,10 @@ import ( "google.golang.org/grpc/status" ) +// maskedClientMsg is the generic message servicelogger.LogAndMaskErr returns to +// the client, hiding the real (potentially sensitive) server-side error. +const maskedClientMsg = "server error" + func TestHandleUseCaseErr(t *testing.T) { t.Parallel() @@ -81,7 +85,7 @@ func TestHandleUseCaseErr(t *testing.T) { name: "server-side status error is still masked", err: status.Error(codes.Unavailable, "connection to database lost"), wantCode: codes.Internal, - wantMessage: "server error", + wantMessage: maskedClientMsg, }, { name: "validation error maps to bad request", @@ -93,7 +97,28 @@ func TestHandleUseCaseErr(t *testing.T) { name: "unknown error is masked as internal server error", err: errors.New("sensitive details"), wantCode: codes.Internal, - wantMessage: "server error", + wantMessage: maskedClientMsg, + }, + { + // PFM-6775: a transient org-lookup failure now surfaces the real DB + // cause up the stack (wrapped with %w by the data and biz layers). + // It must be masked as a generic internal error so the SQL detail + // never reaches the client, while the real error remains available + // for logging/Sentry. + name: "transient org-lookup failure is masked, SQL detail not leaked to client", + err: fmt.Errorf("failed to find membership: %w", + fmt.Errorf("querying organization %q: %w", "chainloop", + errors.New("pq: canceling statement due to statement timeout (SQLSTATE 57014)"))), + wantCode: codes.Internal, + wantMessage: maskedClientMsg, + }, + { + // PFM-6775: a genuinely missing org still maps to NotFound (never + // masked), so ContextService.Current can degrade gracefully. + name: "genuinely missing org maps to not found", + err: fmt.Errorf("failed to find membership: %w", biz.NewErrNotFound("organization chainloop")), + wantCode: codes.NotFound, + wantMessage: "failed to find membership: organization chainloop not found", }, } diff --git a/app/controlplane/pkg/biz/membership_integration_test.go b/app/controlplane/pkg/biz/membership_integration_test.go index a63c7d57e..279071c5e 100644 --- a/app/controlplane/pkg/biz/membership_integration_test.go +++ b/app/controlplane/pkg/biz/membership_integration_test.go @@ -17,6 +17,7 @@ package biz_test import ( "context" + "errors" "fmt" "testing" @@ -651,6 +652,46 @@ func TestMembershipUseCase(t *testing.T) { suite.Run(t, new(membershipFilteringPaginationTestSuite)) } +// FindByOrgNameAndUser must not mask transient (non-NotFound) failures of the +// org lookup as a fabricated "organization not found". A genuinely missing org +// still has to surface as biz.NotFound so that ContextService.Current can +// degrade gracefully instead of returning a captured gRPC 500 (see PFM-6775). +func (s *membershipIntegrationTestSuite) TestFindByOrgNameAndUserPreservesTransientError() { + ctx := context.Background() + + org, err := s.Organization.CreateWithRandomName(ctx) + s.NoError(err) + user, err := s.User.UpsertByEmail(ctx, "transient@test.com", nil) + s.NoError(err) + + userUUID, err := uuid.Parse(user.ID) + s.NoError(err) + + s.Run("transient failure surfaces the real error, not a fake not-found", func() { + // A cancelled context makes the org query fail with a non-NotFound + // error against an org that actually exists. The underlying cause must + // be preserved (wrapped with %w) rather than masked. This asserts the + // data-layer contract ("non-NotFound is preserved"); how each transient + // class then maps to a gRPC status/Sentry is covered by the + // service-layer TestHandleUseCaseErr cases. + canceledCtx, cancel := context.WithCancel(ctx) + cancel() + + _, err := s.Repos.Membership.FindByOrgNameAndUser(canceledCtx, org.Name, userUUID) + s.Error(err) + // The transient cause must remain observable... + s.True(errors.Is(err, context.Canceled), "expected the real error to be preserved, got: %v", err) + // ...and it must NOT be masked as a biz.NotFound. + s.False(biz.IsNotFound(err), "a transient failure must not be reported as not-found") + }) + + s.Run("genuinely missing org still returns biz.NotFound", func() { + _, err := s.Repos.Membership.FindByOrgNameAndUser(ctx, "non-existent-org-"+uuid.NewString(), userUUID) + s.Error(err) + s.True(biz.IsNotFound(err), "a missing org must map to biz.NotFound") + }) +} + // Utility struct to hold the test suite type membershipIntegrationTestSuite struct { testhelpers.UseCasesEachTestSuite diff --git a/app/controlplane/pkg/data/membership.go b/app/controlplane/pkg/data/membership.go index b4d82e8bf..e10b98aee 100644 --- a/app/controlplane/pkg/data/membership.go +++ b/app/controlplane/pkg/data/membership.go @@ -228,7 +228,11 @@ func (r *MembershipRepo) FindByOrgNameAndUser(ctx context.Context, orgName strin if ent.IsNotFound(err) { return nil, biz.NewErrNotFound(fmt.Sprintf("organization %s not found", orgName)) } - return nil, fmt.Errorf("organization %s not found", orgName) + // Preserve the real error (e.g. context cancellation, statement/pool + // timeout) instead of masking a transient failure as a fake "not found". + // A fabricated not-found here would surface as a captured gRPC 500 in + // ContextService.Current rather than degrading gracefully (see PFM-6775). + return nil, fmt.Errorf("querying organization %q: %w", orgName, err) } m, err := r.data.DB.Membership.Query().Where(