fix(config): harden provide strategy parsing with error returns

- config: ParseProvideStrategy returns error, rejects "all" mixed with
  selective strategies, removes dead strategy==0 check
- config: add MustParseProvideStrategy for pre-validated call sites
- config: ValidateProvideConfig validates strategy at startup
- config: ShouldProvideForStrategy uses bitmask check for ProvideStrategyAll
- core/node: downstream callers use MustParseProvideStrategy
- core/node: fix Pinning() nil return that caused fx.Provide panic

Author: lidel

config/provide.go

@@ -100,25 +100,55 @@ type ProvideDHT struct {
 	ResumeEnabled Flag `json:",omitempty"`
 }
 
-func ParseProvideStrategy(s string) ProvideStrategy {
+func ParseProvideStrategy(s string) (ProvideStrategy, error) {
 	var strategy ProvideStrategy
 	for part := range strings.SplitSeq(s, "+") {
 		switch part {
-		case "all", "flat", "": // special case, does not mix with others ("flat" is deprecated, maps to "all")
-			return ProvideStrategyAll
+		case "all", "flat":
+			strategy |= ProvideStrategyAll
+		case "":
+			// empty string (default config) maps to "all",
+			// but empty tokens from splitting (e.g. "pinned+") are invalid
+			if s == "" {
+				strategy |= ProvideStrategyAll
+			} else {
+				return 0, fmt.Errorf("invalid provide strategy: empty token in %q", s)
+			}
 		case "pinned":
 			strategy |= ProvideStrategyPinned
 		case "roots":
 			strategy |= ProvideStrategyRoots
 		case "mfs":
 			strategy |= ProvideStrategyMFS
+		default:
+			return 0, fmt.Errorf("unknown provide strategy token: %q in %q", part, s)
 		}
 	}
+	// "all" provides every block and cannot be combined with selective strategies
+	if strategy&ProvideStrategyAll != 0 && strategy != ProvideStrategyAll {
+		return 0, fmt.Errorf("\"all\" strategy cannot be combined with other strategies in %q", s)
+	}
+	return strategy, nil
+}
+
+// MustParseProvideStrategy is like ParseProvideStrategy but panics on error.
+// Use with strategy strings that have already been validated at startup.
+func MustParseProvideStrategy(s string) ProvideStrategy {
+	strategy, err := ParseProvideStrategy(s)
+	if err != nil {
+		panic(err)
+	}
 	return strategy
 }
 
 // ValidateProvideConfig validates the Provide configuration according to DHT requirements.
 func ValidateProvideConfig(cfg *Provide) error {
+	// Validate Provide.Strategy
+	strategy := cfg.Strategy.WithDefault(DefaultProvideStrategy)
+	if _, err := ParseProvideStrategy(strategy); err != nil {
+		return fmt.Errorf("Provide.Strategy: %w", err)
+	}
+
 	// Validate Provide.DHT.Interval
 	if !cfg.DHT.Interval.IsDefault() {
 		interval := cfg.DHT.Interval.WithDefault(DefaultProvideDHTInterval)
@@ -184,7 +214,7 @@ func ValidateProvideConfig(cfg *Provide) error {
 // ShouldProvideForStrategy determines if content should be provided based on the provide strategy
 // and content characteristics (pinned status, root status, MFS status).
 func ShouldProvideForStrategy(strategy ProvideStrategy, isPinned bool, isPinnedRoot bool, isMFS bool) bool {
-	if strategy == ProvideStrategyAll {
+	if strategy&ProvideStrategyAll != 0 {
 		// 'all' strategy: always provide
 		return true
 	}

config/provide_test.go

@@ -9,27 +9,119 @@ import (
 )
 
 func TestParseProvideStrategy(t *testing.T) {
-	tests := []struct {
-		input  string
-		expect ProvideStrategy
-	}{
-		{"all", ProvideStrategyAll},
-		{"pinned", ProvideStrategyPinned},
-		{"mfs", ProvideStrategyMFS},
-		{"pinned+mfs", ProvideStrategyPinned | ProvideStrategyMFS},
-		{"invalid", 0},
-		{"all+invalid", ProvideStrategyAll},
-		{"", ProvideStrategyAll},
-		{"flat", ProvideStrategyAll}, // deprecated, maps to "all"
-		{"flat+all", ProvideStrategyAll},
-	}
+	t.Run("valid strategies", func(t *testing.T) {
+		tests := []struct {
+			input  string
+			expect ProvideStrategy
+		}{
+			{"all", ProvideStrategyAll},
+			{"pinned", ProvideStrategyPinned},
+			{"roots", ProvideStrategyRoots},
+			{"mfs", ProvideStrategyMFS},
+			{"pinned+mfs", ProvideStrategyPinned | ProvideStrategyMFS},
+			{"pinned+roots", ProvideStrategyPinned | ProvideStrategyRoots},
+			{"pinned+mfs+roots", ProvideStrategyPinned | ProvideStrategyMFS | ProvideStrategyRoots},
+			{"", ProvideStrategyAll},                                   // empty string = default = all
+			{"flat", ProvideStrategyAll},                               // deprecated, maps to "all"
+			{"flat+all", ProvideStrategyAll},                           // redundant but valid
+			{"all+all", ProvideStrategyAll},                            // redundant but valid
+			{"mfs+pinned", ProvideStrategyMFS | ProvideStrategyPinned}, // order doesn't matter
+		}
 
-	for _, tt := range tests {
-		result := ParseProvideStrategy(tt.input)
-		if result != tt.expect {
-			t.Errorf("ParseProvideStrategy(%q) = %d, want %d", tt.input, result, tt.expect)
+		for _, tt := range tests {
+			result, err := ParseProvideStrategy(tt.input)
+			require.NoError(t, err, "ParseProvideStrategy(%q)", tt.input)
+			assert.Equal(t, tt.expect, result, "ParseProvideStrategy(%q)", tt.input)
 		}
-	}
+	})
+
+	t.Run("unknown token (including typos)", func(t *testing.T) {
+		tests := []struct {
+			input string
+			err   string
+		}{
+			{"invalid", `unknown provide strategy token: "invalid"`},
+			{"uniuqe", `unknown provide strategy token: "uniuqe"`},        // typo of "unique"
+			{"entites", `unknown provide strategy token: "entites"`},      // cspell:disable-line -- intentional typo of "entities"
+			{"pinned+uniuqe", `unknown provide strategy token: "uniuqe"`}, // typo in combo
+		}
+
+		for _, tt := range tests {
+			_, err := ParseProvideStrategy(tt.input)
+			require.Error(t, err, "ParseProvideStrategy(%q) should fail", tt.input)
+			assert.Contains(t, err.Error(), tt.err)
+		}
+	})
+
+	t.Run("empty token from delimiter", func(t *testing.T) {
+		tests := []string{
+			"pinned+",     // trailing +
+			"+pinned",     // leading +
+			"pinned++mfs", // double +
+		}
+
+		for _, input := range tests {
+			_, err := ParseProvideStrategy(input)
+			require.Error(t, err, "ParseProvideStrategy(%q) should fail", input)
+			assert.Contains(t, err.Error(), "empty token")
+		}
+	})
+
+	t.Run("all cannot be combined with other strategies", func(t *testing.T) {
+		tests := []string{
+			"all+pinned",
+			"all+mfs",
+			"all+roots",
+			"flat+pinned",
+			"all+pinned+mfs",
+		}
+
+		for _, input := range tests {
+			_, err := ParseProvideStrategy(input)
+			require.Error(t, err, "ParseProvideStrategy(%q) should fail", input)
+			assert.Contains(t, err.Error(), "cannot be combined")
+		}
+	})
+}
+
+func TestMustParseProvideStrategy(t *testing.T) {
+	t.Run("valid input returns strategy", func(t *testing.T) {
+		assert.Equal(t, ProvideStrategyAll, MustParseProvideStrategy("all"))
+		assert.Equal(t, ProvideStrategyPinned|ProvideStrategyMFS, MustParseProvideStrategy("pinned+mfs"))
+	})
+
+	t.Run("invalid input panics", func(t *testing.T) {
+		assert.Panics(t, func() { MustParseProvideStrategy("bogus") })
+		assert.Panics(t, func() { MustParseProvideStrategy("all+pinned") })
+	})
+}
+
+func TestValidateProvideConfig_Strategy(t *testing.T) {
+	t.Run("valid strategies", func(t *testing.T) {
+		for _, s := range []string{"all", "pinned", "roots", "mfs", "pinned+mfs"} {
+			cfg := &Provide{Strategy: NewOptionalString(s)}
+			require.NoError(t, ValidateProvideConfig(cfg), "strategy=%q", s)
+		}
+	})
+
+	t.Run("default (nil) strategy is valid", func(t *testing.T) {
+		cfg := &Provide{}
+		require.NoError(t, ValidateProvideConfig(cfg))
+	})
+
+	t.Run("invalid strategy", func(t *testing.T) {
+		cfg := &Provide{Strategy: NewOptionalString("bogus")}
+		err := ValidateProvideConfig(cfg)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "Provide.Strategy")
+	})
+
+	t.Run("all combined with others", func(t *testing.T) {
+		cfg := &Provide{Strategy: NewOptionalString("all+pinned")}
+		err := ValidateProvideConfig(cfg)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "cannot be combined")
+	})
 }
 
 func TestValidateProvideConfig_Interval(t *testing.T) {

core/commands/cmdenv/env.go

@@ -156,7 +156,7 @@ func ExecuteFastProvide(
 
 	// Check if strategy allows providing this content
 	strategyStr := cfg.Provide.Strategy.WithDefault(config.DefaultProvideStrategy)
-	strategy := config.ParseProvideStrategy(strategyStr)
+	strategy := config.MustParseProvideStrategy(strategyStr)
 	shouldProvide := config.ShouldProvideForStrategy(strategy, isPinned, isPinnedRoot, isMFS)
 
 	if !shouldProvide {

core/node/core.go

@@ -52,10 +52,7 @@ func BlockService(cfg *config.Config) func(lc fx.Lifecycle, bs blockstore.Blocks
 
 // Pinning creates new pinner which tells GC which blocks should be kept
 func Pinning(strategy string) func(bstore blockstore.Blockstore, ds format.DAGService, repo repo.Repo, prov DHTProvider) (pin.Pinner, error) {
-	// Parse strategy at function creation time (not inside the returned function)
-	// This happens before the provider is created, which is why we pass the strategy
-	// string and parse it here, rather than using fx-provided ProvidingStrategy.
-	strategyFlag := config.ParseProvideStrategy(strategy)
+	strategyFlag := config.MustParseProvideStrategy(strategy)
 
 	return func(bstore blockstore.Blockstore,
 		ds format.DAGService,
@@ -238,7 +235,7 @@ func Files(strategy string) func(mctx helpers.MetricsCtx, lc fx.Lifecycle, repo
 		// strategy - it ensures all MFS content gets announced as it's added or
 		// modified. For non-mfs strategies, we set provider to nil to avoid
 		// unnecessary providing.
-		strategyFlag := config.ParseProvideStrategy(strategy)
+		strategyFlag := config.MustParseProvideStrategy(strategy)
 		if strategyFlag&config.ProvideStrategyMFS == 0 {
 			prov = nil
 		}

core/node/provider.go

@@ -1094,9 +1094,8 @@ func OnlineProviders(provide bool, cfg *config.Config) fx.Option {
 
 	providerStrategy := cfg.Provide.Strategy.WithDefault(config.DefaultProvideStrategy)
 
-	strategyFlag := config.ParseProvideStrategy(providerStrategy)
-	if strategyFlag == 0 {
-		return fx.Error(fmt.Errorf("provider: unknown strategy %q", providerStrategy))
+	if _, err := config.ParseProvideStrategy(providerStrategy); err != nil {
+		return fx.Error(fmt.Errorf("provider: %w", err))
 	}
 
 	opts := []fx.Option{
@@ -1240,7 +1239,7 @@ func handleStrategyChange(strategy string, provider DHTProvider, ds datastore.Da
 }
 
 func setReproviderKeyProvider(strategy string) func(in provStrategyIn) provStrategyOut {
-	strategyFlag := config.ParseProvideStrategy(strategy)
+	strategyFlag := config.MustParseProvideStrategy(strategy)
 
 	return func(in provStrategyIn) provStrategyOut {
 		// Create the appropriate key provider based on strategy

core/node/storage.go

@@ -41,7 +41,7 @@ func BaseBlockstoreCtor(
 		// Important: Provide calls from blockstore are intentionally BLOCKING.
 		// The Provider implementation (not the blockstore) should handle concurrency/queuing.
 		// This avoids spawning unbounded goroutines for concurrent block additions.
-		strategyFlag := config.ParseProvideStrategy(providingStrategy)
+		strategyFlag := config.MustParseProvideStrategy(providingStrategy)
 		if strategyFlag&config.ProvideStrategyAll != 0 {
 			opts = append(opts, blockstore.Provider(prov))
 		}

test/cli/migrations/migration_17_to_latest_test.go

@@ -213,8 +213,8 @@ func testInvalidStrategyMigration(t *testing.T) {
 	outputStr := string(output)
 	t.Logf("Daemon output with invalid strategy: %s", outputStr)
 
-	// The error should mention unknown strategy
-	require.Contains(t, outputStr, "unknown strategy", "Should report unknown strategy error")
+	// The error should mention unknown strategy token
+	require.Contains(t, outputStr, "unknown provide strategy token", "Should report unknown strategy error")
 }
 
 func testRepoProviderReproviderMigration(t *testing.T) {