fix(api): getRecommendationDetail 404s on hidden recs (closes #214)

frontend/src/api/recommendations.ts

@@ -113,6 +113,13 @@ export interface RecommendationDetail {
   usage_history: RecommendationUsagePoint[];
   confidence_bucket: 'low' | 'medium' | 'high';
   provenance_note: string;
+  /**
+   * Present (non-empty array) when the recommendation exists but is filtered
+   * out by an account-service override (issue #214). Each element names one
+   * failing dimension: "enabled=false", "engine", "region", or
+   * "resource_type". Absent / undefined means the rec is fully visible.
+   */
+  hidden_by?: string[];
 }
 
 /**

internal/api/handler_per_account_perms_test.go

@@ -202,7 +202,8 @@ func TestPerAccountPerms_RecommendationDetail_CrossAccountRejected(t *testing.T)
 	}
 
 	mockSched := new(MockScheduler)
-	mockSched.On("ListRecommendations", ctx, mock.Anything).Return([]config.RecommendationRecord{recB}, nil)
+	mockSched.On("GetRecommendationByID", ctx, "rec-b-detail").Return(&recB, ([]string)(nil), nil)
+	t.Cleanup(func() { mockSched.AssertExpectations(t) })
 
 	mockStore := new(MockConfigStore)
 	mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) {
@@ -238,7 +239,8 @@ func TestPerAccountPerms_RecommendationDetail_AllowedAccountReturns200(t *testin
 	}
 
 	mockSched := new(MockScheduler)
-	mockSched.On("ListRecommendations", ctx, mock.Anything).Return([]config.RecommendationRecord{recA}, nil)
+	mockSched.On("GetRecommendationByID", ctx, "rec-a-detail").Return(&recA, ([]string)(nil), nil)
+	t.Cleanup(func() { mockSched.AssertExpectations(t) })
 
 	mockStore := new(MockConfigStore)
 	mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) {

internal/api/handler_recommendations.go

@@ -202,32 +202,37 @@ func (h *Handler) getRecommendationDetail(ctx context.Context, req *events.Lambd
 		return nil, NewClientError(400, "recommendation id is required")
 	}
 
-	// The recommendation cache doesn't expose a get-by-id, so we look it
-	// up from the unfiltered list. The list is already cached in
-	// Postgres (see store_postgres_recommendations.go) so this is a
-	// single round-trip; the in-memory linear scan is bounded by the
-	// catalogue size which is small (low thousands at the high end).
-	recs, err := h.scheduler.ListRecommendations(ctx, config.RecommendationFilter{})
+	// GetRecommendationByID bypasses the account-override filter so that a
+	// deep-linked URL to a rec that has been hidden by an account override
+	// still resolves. Suppressions are still applied: a fully-suppressed rec
+	// (actively dismissed) returns nil and we 404 as before. See issue #214.
+	rec, hiddenBy, err := h.scheduler.GetRecommendationByID(ctx, id)
 	if err != nil {
-		return nil, fmt.Errorf("failed to list recommendations: %w", err)
+		return nil, fmt.Errorf("failed to fetch recommendation: %w", err)
+	}
+	if rec == nil {
+		return nil, errNotFound
 	}
 
-	// Filter by allowed accounts FIRST, then look up by id within the
-	// filtered set. Doing it the other way around would leak existence
-	// of recommendations in accounts the caller can't see (a 404 vs
-	// 403 timing/wording diff would let an attacker probe the
-	// recommendation namespace across accounts).
-	visible, err := h.filterRecommendationsByAllowedAccounts(ctx, session, recs)
+	// Tenant-scoping gate: an authenticated user must not be able to read recs
+	// from accounts outside their allowed set, even via the detail endpoint.
+	// The existence-disclosure-safe check (match before reveal) is preserved:
+	// we checked GetRecommendationByID first and only reach here on a hit, so
+	// the caller still cannot distinguish "doesn't exist" from "wrong tenant"
+	// via a timing or wording difference.
+	visible, err := h.filterRecommendationsByAllowedAccounts(ctx, session, []config.RecommendationRecord{*rec})
 	if err != nil {
 		return nil, err
 	}
+	if len(visible) == 0 {
+		return nil, errNotFound
+	}
 
-	for i := range visible {
-		if visible[i].ID == id {
-			return h.buildRecommendationDetail(ctx, &visible[i]), nil
-		}
+	resp := h.buildRecommendationDetail(ctx, &visible[0])
+	if len(hiddenBy) > 0 {
+		resp.HiddenBy = hiddenBy
 	}
-	return nil, errNotFound
+	return resp, nil
 }
 
 // buildRecommendationDetail assembles the drawer payload from a single

internal/api/handler_recommendations_test.go

@@ -111,8 +111,10 @@ func TestHandler_getRecommendationDetail(t *testing.T) {
 
 	t.Run("returns errNotFound on unknown id", func(t *testing.T) {
 		mockScheduler := new(MockScheduler)
-		mockScheduler.On("ListRecommendations", ctx, mock.Anything).
-			Return(knownRecs, nil)
+		// GetRecommendationByID returns (nil, nil, nil) for absent recs.
+		mockScheduler.On("GetRecommendationByID", ctx, "rec-missing").
+			Return((*config.RecommendationRecord)(nil), ([]string)(nil), nil)
+		t.Cleanup(func() { mockScheduler.AssertExpectations(t) })
 
 		handler := &Handler{
 			scheduler: mockScheduler,
@@ -129,9 +131,11 @@ func TestHandler_getRecommendationDetail(t *testing.T) {
 	})
 
 	t.Run("returns 200 with the expected shape for a known id", func(t *testing.T) {
+		knownRec := knownRecs[0]
 		mockScheduler := new(MockScheduler)
-		mockScheduler.On("ListRecommendations", ctx, mock.Anything).
-			Return(knownRecs, nil)
+		mockScheduler.On("GetRecommendationByID", ctx, "rec-known").
+			Return(&knownRec, ([]string)(nil), nil)
+		t.Cleanup(func() { mockScheduler.AssertExpectations(t) })
 
 		mockStore := new(MockConfigStore)
 		mockStore.On("GetRecommendationsFreshness", ctx).
@@ -161,12 +165,45 @@ func TestHandler_getRecommendationDetail(t *testing.T) {
 		assert.Contains(t, got.ProvenanceNote, "AWS")
 		assert.Contains(t, got.ProvenanceNote, "ec2")
 		assert.Contains(t, got.ProvenanceNote, "last collected")
+		assert.Nil(t, got.HiddenBy, "visible rec must not carry hidden_by")
+	})
+
+	t.Run("returns 200 with hidden_by for override-filtered rec (issue #214)", func(t *testing.T) {
+		// GetRecommendationByID returns the rec and the override reasons
+		// when the rec exists but is filtered by an account-service override.
+		knownRec := knownRecs[0]
+		mockScheduler := new(MockScheduler)
+		mockScheduler.On("GetRecommendationByID", ctx, "rec-known").
+			Return(&knownRec, []string{"enabled=false"}, nil)
+		t.Cleanup(func() { mockScheduler.AssertExpectations(t) })
+
+		mockStore := new(MockConfigStore)
+		mockStore.On("GetRecommendationsFreshness", ctx).
+			Return(&config.RecommendationsFreshness{LastCollectedAt: &now}, nil)
+
+		handler := &Handler{
+			scheduler: mockScheduler,
+			config:    mockStore,
+			apiKey:    "test-key",
+		}
+		req := &events.LambdaFunctionURLRequest{
+			Headers: map[string]string{"x-api-key": "test-key"},
+		}
+
+		got, err := handler.getRecommendationDetail(ctx, req, "rec-known")
+		require.NoError(t, err)
+		require.NotNil(t, got)
+		assert.Equal(t, "rec-known", got.ID)
+		assert.Equal(t, []string{"enabled=false"}, got.HiddenBy,
+			"override-hidden rec must carry hidden_by reasons")
 	})
 
 	t.Run("provenance degrades gracefully when freshness is unavailable", func(t *testing.T) {
+		knownRec := knownRecs[0]
 		mockScheduler := new(MockScheduler)
-		mockScheduler.On("ListRecommendations", ctx, mock.Anything).
-			Return(knownRecs, nil)
+		mockScheduler.On("GetRecommendationByID", ctx, "rec-known").
+			Return(&knownRec, ([]string)(nil), nil)
+		t.Cleanup(func() { mockScheduler.AssertExpectations(t) })
 
 		mockStore := new(MockConfigStore)
 		mockStore.On("GetRecommendationsFreshness", ctx).

internal/api/mocks_test.go

@@ -696,6 +696,19 @@ func (m *MockScheduler) ListRecommendations(ctx context.Context, filter config.R
 	return args.Get(0).([]config.RecommendationRecord), args.Error(1)
 }
 
+func (m *MockScheduler) GetRecommendationByID(ctx context.Context, id string) (*config.RecommendationRecord, []string, error) {
+	args := m.Called(ctx, id)
+	var rec *config.RecommendationRecord
+	if args.Get(0) != nil {
+		rec = args.Get(0).(*config.RecommendationRecord)
+	}
+	var hiddenBy []string
+	if args.Get(1) != nil {
+		hiddenBy = args.Get(1).([]string)
+	}
+	return rec, hiddenBy, args.Error(2)
+}
+
 // MockAuthService is a mock implementation of the auth service
 type MockAuthService struct {
 	mock.Mock

internal/api/types.go

@@ -142,6 +142,12 @@ type PurchaseManagerInterface interface {
 type SchedulerInterface interface {
 	CollectRecommendations(ctx context.Context) (*scheduler.CollectResult, error)
 	ListRecommendations(ctx context.Context, filter config.RecommendationFilter) ([]config.RecommendationRecord, error)
+	// GetRecommendationByID fetches a single rec by its application-level id,
+	// bypassing account-override filtering so deep-linked URLs to override-
+	// hidden recs resolve. hiddenBy is non-nil when the rec would be dropped by
+	// the override filter; callers render a "hidden" banner. Returns nil, nil,
+	// nil when the rec is absent or fully suppressed.
+	GetRecommendationByID(ctx context.Context, id string) (rec *config.RecommendationRecord, hiddenBy []string, err error)
 }
 
 // AuthServiceInterface defines auth service methods used by handler
@@ -379,6 +385,12 @@ type RecommendationDetailResponse struct {
 	UsageHistory     []UsagePoint `json:"usage_history"`
 	ConfidenceBucket string       `json:"confidence_bucket"`
 	ProvenanceNote   string       `json:"provenance_note"`
+	// HiddenBy is non-nil when the rec is filtered out by an account-service
+	// override (issue #214). Each element names one failing dimension:
+	// "enabled=false", "engine", "region", or "resource_type". The frontend
+	// renders a "hidden by your override" banner when this field is present.
+	// Absent (null) means the rec is fully visible.
+	HiddenBy []string `json:"hidden_by,omitempty"`
 }
 
 // PlansResponse holds the purchase plans response

internal/config/store_postgres_recommendations.go

@@ -269,6 +269,9 @@ func buildRecommendationFilter(filter RecommendationFilter) (string, []any) {
 	if filter.MinSavings > 0 {
 		add("monthly_savings >= $%d", filter.MinSavings)
 	}
+	if filter.ID != "" {
+		add("payload->>'id' = $%d", filter.ID)
+	}
 	if len(conds) == 0 {
 		return "", nil
 	}

internal/config/types.go

@@ -390,6 +390,7 @@ type RecommendationFilter struct {
 	Region     string   // "" = all regions
 	AccountIDs []string // nil/empty = all accounts
 	MinSavings float64  // 0 = no floor on monthly savings
+	ID         string   // "" = all ids; non-empty = exact match on the id column
 }
 
 // PurchasePlanFilter parameterises ListPurchasePlans. Zero-value means "no

internal/scheduler/scheduler.go

@@ -976,6 +976,79 @@ func (s *Scheduler) ListRecommendations(ctx context.Context, filter config.Recom
 	return recs, nil
 }
 
+// GetRecommendationByID fetches a single recommendation by its application-
+// level ID (the id field in the stored payload), bypassing the account-override
+// filter so deep-linked URLs to override-hidden recs still resolve.
+//
+// Suppressions are still applied: a rec whose remaining count has been
+// suppressed to zero is treated as gone (returns nil, nil, nil) so the caller
+// can render a 404 rather than a "hidden" banner for actively-dismissed recs.
+//
+// hiddenBy is non-nil (one or more reason strings) when the rec exists and
+// passes the suppression check, but would be dropped by the account-override
+// filter. Possible reasons: "enabled=false", "engine", "region",
+// "resource_type".  The caller renders a "hidden by your override" banner.
+//
+// Returns (nil, nil, nil) when the rec is genuinely absent or fully suppressed.
+func (s *Scheduler) GetRecommendationByID(ctx context.Context, id string) (rec *config.RecommendationRecord, hiddenBy []string, err error) {
+	recs, err := s.config.ListStoredRecommendations(ctx, config.RecommendationFilter{ID: id})
+	if err != nil {
+		return nil, nil, fmt.Errorf("GetRecommendationByID: store lookup: %w", err)
+	}
+	if len(recs) == 0 {
+		return nil, nil, nil
+	}
+
+	// Apply suppressions. A fully-suppressed rec becomes genuinely absent from
+	// the caller's perspective (the suppression is intentional user action).
+	recs, err = s.applySuppressions(ctx, recs)
+	if err != nil {
+		// Over-show: on suppression read failure treat the rec as un-suppressed.
+		logging.Errorf("GetRecommendationByID: suppression check failed; treating rec as un-suppressed: %v", err)
+	}
+	if len(recs) == 0 {
+		return nil, nil, nil
+	}
+	found := &recs[0]
+
+	// Check whether the account-override filter would drop this rec. This is
+	// a read-only call — we never drop it here, only report the reasons.
+	if found.CloudAccountID != nil {
+		resolved, resolveErr := config.ResolveAccountConfigsForRecs(ctx, s.config, recs)
+		if resolveErr != nil {
+			// Non-fatal: if the override check fails we surface the rec without
+			// a hidden_by marker (over-show is the safer default).
+			logging.Errorf("GetRecommendationByID: override resolution failed; returning rec without hidden_by: %v", resolveErr)
+			return found, nil, nil
+		}
+		cfg := resolved[config.AccountConfigKey(*found.CloudAccountID, found.Provider, found.Service)]
+		if cfg != nil {
+			hiddenBy = overrideHiddenReasons(found, cfg)
+		}
+	}
+
+	return found, hiddenBy, nil
+}
+
+// overrideHiddenReasons returns a non-empty slice when rec would be dropped by
+// the given resolved ServiceConfig, naming each failing dimension.
+func overrideHiddenReasons(rec *config.RecommendationRecord, cfg *config.ServiceConfig) []string {
+	var reasons []string
+	if !cfg.Enabled {
+		reasons = append(reasons, "enabled=false")
+	}
+	if !engineMatches(rec.Engine, cfg) {
+		reasons = append(reasons, "engine")
+	}
+	if !inListRule(rec.Region, cfg.IncludeRegions, cfg.ExcludeRegions) {
+		reasons = append(reasons, "region")
+	}
+	if !inListRule(rec.ResourceType, cfg.IncludeTypes, cfg.ExcludeTypes) {
+		reasons = append(reasons, "resource_type")
+	}
+	return reasons
+}
+
 // resolveEffectiveCacheTTL returns the effective stale-while-revalidate TTL
 // and whether background auto-refresh has been explicitly disabled (value 0).
 // It prefers the DB-configured RecommendationsCacheStaleHours; falls back to

internal/scheduler/scheduler_overrides_test.go

@@ -24,8 +24,16 @@ type mockOverrideStore struct {
 	getOverrideErr error
 }
 
-func (m *mockOverrideStore) ListStoredRecommendations(_ context.Context, _ config.RecommendationFilter) ([]config.RecommendationRecord, error) {
-	return m.recs, nil
+func (m *mockOverrideStore) ListStoredRecommendations(_ context.Context, filter config.RecommendationFilter) ([]config.RecommendationRecord, error) {
+	if filter.ID == "" {
+		return m.recs, nil
+	}
+	for _, r := range m.recs {
+		if r.ID == filter.ID {
+			return []config.RecommendationRecord{r}, nil
+		}
+	}
+	return nil, nil
 }
 func (m *mockOverrideStore) ListActiveSuppressions(_ context.Context) ([]config.PurchaseSuppression, error) {
 	return nil, nil
@@ -321,3 +329,61 @@ func TestApplyAccountOverrides_AcceptanceCriterion_Issue196(t *testing.T) {
 	require.Len(t, recs, 1, "acct-A's recs hidden by the override")
 	assert.Equal(t, "acct-B", *recs[0].CloudAccountID)
 }
+
+// TestGetRecommendationByID exercises the three acceptance cases from issue #214:
+//
+//	(a) rec visible: returns rec, nil hiddenBy
+//	(b) rec hidden by override: returns rec + hiddenBy reasons (no 404)
+//	(c) rec genuinely absent: returns nil, nil, nil
+func TestGetRecommendationByID_VisibleRec(t *testing.T) {
+	ctx := context.Background()
+	rec := rdsRec("acct-A", "us-east-1", "mysql")
+	store := &mockOverrideStore{
+		recs: []config.RecommendationRecord{rec},
+		globals: map[string]*config.ServiceConfig{
+			"aws|rds": {Provider: "aws", Service: "rds", Enabled: true},
+		},
+	}
+	s := &Scheduler{config: store}
+
+	got, hiddenBy, err := s.GetRecommendationByID(ctx, rec.ID)
+	require.NoError(t, err)
+	require.NotNil(t, got, "visible rec must be returned")
+	assert.Equal(t, rec.ID, got.ID)
+	assert.Empty(t, hiddenBy, "visible rec must carry no hidden_by reasons")
+}
+
+func TestGetRecommendationByID_HiddenByOverride(t *testing.T) {
+	// Issue #214: detail endpoint must return the rec with hidden_by reasons
+	// instead of a 404 when an account-service override is filtering it out.
+	ctx := context.Background()
+	rec := rdsRec("acct-A", "us-east-1", "mysql")
+	store := &mockOverrideStore{
+		recs: []config.RecommendationRecord{rec},
+		globals: map[string]*config.ServiceConfig{
+			"aws|rds": {Provider: "aws", Service: "rds", Enabled: true},
+		},
+		overrides: map[string]*config.AccountServiceOverride{
+			"acct-A|aws|rds": {Enabled: boolPtr(false)},
+		},
+	}
+	s := &Scheduler{config: store}
+
+	got, hiddenBy, err := s.GetRecommendationByID(ctx, rec.ID)
+	require.NoError(t, err)
+	require.NotNil(t, got, "override-hidden rec must still be returned")
+	assert.Equal(t, rec.ID, got.ID)
+	assert.Equal(t, []string{"enabled=false"}, hiddenBy,
+		"hidden_by must report the override dimension that caused the filter")
+}
+
+func TestGetRecommendationByID_AbsentRec(t *testing.T) {
+	ctx := context.Background()
+	store := &mockOverrideStore{recs: nil}
+	s := &Scheduler{config: store}
+
+	got, hiddenBy, err := s.GetRecommendationByID(ctx, "no-such-rec")
+	require.NoError(t, err)
+	assert.Nil(t, got, "absent rec must return nil")
+	assert.Nil(t, hiddenBy)
+}