fix(storage): allow digits in SQL table names and return errors instead of panicking

escapeForTableName's allow-list omitted digits, so a network, channel or
namespace containing one — "channel1" being about as common as it gets — was
rejected. Worse, the failure arrived as a panic: the only caller was the
Must* formatter, and nothing on the path from store construction recovers, so
a routine configuration value crashed the node.

Allow digits in the table name parameters and validate the composed
identifier instead of the fragment, so a leading digit is accepted behind a
prefix and rejected only when there is none. Return errors along the whole
chain rather than panicking, and drop the Must* calls from it.

Report every invalid short-code override rather than only the first. The
prefix and the params are shared by all seventeen names, so a problem there is
validated once up front and reported once; overrides apply to a single key
each, so those are collected and joined — otherwise an operator with two bad
entries under token.storage.tableNames fixes one, restarts, and hits the next.

Add FuzzGetTableNamesNoPanic, covering both properties (never panics; either
errors or emits a legal SQL identifier) and wired into nightly-fuzz.yml. The
table prefix is deliberately not fuzzed: every accepted prefix is memoised
forever in the package-level formatter cache, which has no eviction, so
fuzzing it would grow the worker heap for the whole four-hour nightly run.
The prefix edge cases are table-driven tests instead.

Fixes #2034

Signed-off-by: AkramBitar <akram@il.ibm.com>

Author: AkramBitar

.github/workflows/nightly-fuzz.yml

@@ -105,6 +105,9 @@ jobs:
           - name: fabtoken-owner-verifier
             pkg: ./token/core/fabtoken/v1/driver
             func: FuzzOwnerVerifierNoPanic
+          - name: storage-sql-table-names
+            pkg: ./token/services/storage/db/sql/common
+            func: FuzzGetTableNamesNoPanic
 
     steps:
       - name: Checkout code

docs/configuration.md

@@ -693,6 +693,14 @@ to `<params>_<short_code>` (params still apply when provided; short-code overrid
 > will cause the node to look for tables under different names. Make sure the underlying
 > tables already exist under the unprefixed names before enabling this flag.
 
+> **Note:** `<params>` is the TMS identity (network, channel, namespace). `_`, `-` and `.`
+> in those values are escaped to `__`, `_d` and `_f`; letters, digits and underscores pass
+> through unchanged, so a channel named `channel1` is fine. The composed name must still be
+> a valid SQL identifier — it cannot start with a digit, which is only reachable with
+> `skipPrefix: true` and a network name starting with a digit. Any other character is a
+> configuration error and is reported as such, not a crash. See
+> [Table Name Customisation](services/storage.md#table-name-customisation).
+
 ---
 
 ### Optional: token.tms.<name>.services.storage.cleanup

docs/services/storage.md

@@ -74,6 +74,35 @@ Two independent options let you control how the final name is composed:
 | Override individual short codes | `token.storage.tableNames` | _(none)_ |
 | Skip the FSC-generated prefix entirely | `token.storage.skipPrefix` | `false` |
 
+### Allowed characters
+
+The `TableNameParams` are the TMS identity — **network**, **channel** and **namespace** —
+and they become part of every table name, so they must be reducible to a legal SQL
+identifier. The characters that are common in Fabric names but illegal in an identifier
+are escaped rather than rejected:
+
+| Character in a param | Becomes |
+|---|---|
+| `_` | `__` |
+| `-` | `_d` |
+| `.` | `_f` |
+
+Letters, digits and underscores pass through as-is, so a channel called `channel1` or
+`mychannel01` is fine. The **final composed name** — prefix included — must still be a
+valid identifier: only letters, digits and underscores, and it cannot **start** with a
+digit (unquoted identifiers in both SQLite and PostgreSQL must start with a letter or an
+underscore). Since the prefix comes first and can never start with a digit, this only
+bites when `skipPrefix` is enabled *and* the network name starts with a digit.
+
+Anything else — a space, `!`, `/`, `;`, … — is a **configuration error**: store
+construction returns an error naming the offending value, it does not crash the node.
+A bad prefix or param breaks every table name in the same way and is reported once;
+invalid [short-code overrides](#overriding-short-codes-tablenames) are per-key, so **all**
+of them are reported together and you do not have to fix them one restart at a time.
+
+The `TablePrefix` is stricter: only letters and underscores, at most 100 characters. It is
+lower-cased before use.
+
 ### Overriding short codes (`tableNames`)
 
 You can replace any short code globally (for all TMS instances on the node) via the

token/services/storage/db/sql/common/init.go

@@ -8,10 +8,19 @@ package common
 
 import (
 	"github.com/LFDT-Panurus/panurus/token/services/logging"
+	"github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors"
 	"github.com/hyperledger-labs/fabric-smart-client/platform/view/services/storage/driver/common"
 )
 
-const defaultPrefix = "fsc"
+const (
+	defaultPrefix = "fsc"
+	// sharedNameProbe is the short code used to validate the part of a table name
+	// that every short code shares: the prefix and the params. It is a real
+	// canonical short code rather than a synthetic one, so a failure reports a name
+	// the operator actually recognises, and it is never read from the overrides, so
+	// the check does not depend on them.
+	sharedNameProbe = "movements"
+)
 
 var (
 	logger     = logging.MustGetLogger()
@@ -92,7 +101,7 @@ func GetTableNamesWithOverrides(prefix string, overrides TableNamesConfig, param
 		return TableNames{}, err
 	}
 
-	return buildTableNames(prefix, params, overrides, nc.MustFormat)
+	return buildTableNames(prefix, params, overrides, nc.Format)
 }
 
 // GetTableNamesWithOverridesSkipPrefix is like GetTableNamesWithOverrides but
@@ -103,48 +112,76 @@ func GetTableNamesWithOverridesSkipPrefix(prefix string, overrides TableNamesCon
 		return TableNames{}, err
 	}
 
-	return buildTableNames(prefix, params, overrides, nc.MustFormatWithoutPrefix)
+	return buildTableNames(prefix, params, overrides, nc.FormatWithoutPrefix)
 }
 
 // buildTableNames constructs a TableNames value by applying format to each
 // canonical short code (after resolving any override), forwarding params.
-func buildTableNames(prefix string, params []string, overrides TableNamesConfig, format func(string, ...string) string) (TableNames, error) {
+//
+// format is the error-returning formatter on purpose: an illegal character in
+// the prefix, in a short-code override or in one of the params (network,
+// channel, namespace) must surface as a configuration error to the caller, not
+// as a panic at store-construction time.
+func buildTableNames(prefix string, params []string, overrides TableNamesConfig, format func(string, ...string) (string, error)) (TableNames, error) {
 	// Warn on unknown override keys before applying any overrides.
 	for k := range overrides {
 		if _, ok := knownShortCodes[k]; !ok {
 			logger.Warnf("unknown table name override key %q — ignored", k)
 		}
 	}
 
-	// resolve returns the effective short code: the override value if present,
-	// otherwise the canonical default.
-	resolve := func(defaultCode string) string {
+	// The prefix and the params are shared by every short code, so a problem there
+	// breaks all of the names in the same way. Validate that shared part once, with
+	// a short code that is always a legal identifier fragment, so it is reported
+	// once instead of repeated for every table.
+	if _, err := format(sharedNameProbe, params...); err != nil {
+		return TableNames{}, errors.WithMessage(err, "invalid table name prefix or parameters")
+	}
+
+	// Past that point a failure can only come from the short code itself, and an
+	// override is specific to its own key, so collect them all: returning just the
+	// first would make an operator fix one key, restart, and hit the next.
+	var errs []error
+
+	// name formats the effective short code for defaultCode: the override value
+	// if present, otherwise the canonical default.
+	name := func(defaultCode string) string {
+		code := defaultCode
 		if v, ok := overrides[defaultCode]; ok {
-			return v
+			code = v
+		}
+		tableName, err := format(code, params...)
+		if err != nil {
+			errs = append(errs, errors.Wrapf(err, "failed to build table name for short code [%s]", code))
 		}
 
-		return defaultCode
+		return tableName
 	}
 
-	return TableNames{
+	tableNames := TableNames{
 		Prefix:                 prefix,
 		Params:                 params,
-		Movements:              format(resolve("movements"), params...),
-		Transactions:           format(resolve("txs"), params...),
-		TransactionEndorseAck:  format(resolve("tx_ends"), params...),
-		Requests:               format(resolve("requests"), params...),
-		Validations:            format(resolve("req_vals"), params...),
-		Tokens:                 format(resolve("tokens"), params...),
-		Ownership:              format(resolve("tkn_own"), params...),
-		Certifications:         format(resolve("tkn_crts"), params...),
-		TokenLocks:             format(resolve("tkn_locks"), params...),
-		PublicParams:           format(resolve("public_params"), params...),
-		Wallets:                format(resolve("wallets"), params...),
-		IdentityConfigurations: format(resolve("id_cfgs"), params...),
-		IdentityInfo:           format(resolve("id_info"), params...),
-		Signers:                format(resolve("id_signers"), params...),
-		KeyStore:               format(resolve("key_store"), params...),
-		EIDLeases:              format(resolve("eid_leases"), params...),
-		TokenSKICleanups:       format(resolve("tkn_ski_cleanups"), params...),
-	}, nil
+		Movements:              name("movements"),
+		Transactions:           name("txs"),
+		TransactionEndorseAck:  name("tx_ends"),
+		Requests:               name("requests"),
+		Validations:            name("req_vals"),
+		Tokens:                 name("tokens"),
+		Ownership:              name("tkn_own"),
+		Certifications:         name("tkn_crts"),
+		TokenLocks:             name("tkn_locks"),
+		PublicParams:           name("public_params"),
+		Wallets:                name("wallets"),
+		IdentityConfigurations: name("id_cfgs"),
+		IdentityInfo:           name("id_info"),
+		Signers:                name("id_signers"),
+		KeyStore:               name("key_store"),
+		EIDLeases:              name("eid_leases"),
+		TokenSKICleanups:       name("tkn_ski_cleanups"),
+	}
+	if err := errors.Join(errs...); err != nil {
+		return TableNames{}, err
+	}
+
+	return tableNames, nil
 }

token/services/storage/db/sql/common/init_test.go

@@ -7,6 +7,7 @@ SPDX-License-Identifier: Apache-2.0
 package common_test
 
 import (
+	"strings"
 	"testing"
 
 	common "github.com/LFDT-Panurus/panurus/token/services/storage/db/sql/common"
@@ -243,6 +244,118 @@ func TestGetTableNamesWithOverridesSkipPrefix(t *testing.T) {
 	assert.Contains(t, got.Transactions, "txs")
 }
 
+// TestGetTableNamesWithDigitsInParams is a regression test for
+// https://github.com/LFDT-Panurus/panurus/issues/2034: a network, channel or
+// namespace name containing a digit — e.g. the very common "channel1" — used to
+// panic while building the table names instead of producing a valid name.
+func TestGetTableNamesWithDigitsInParams(t *testing.T) {
+	for _, params := range [][]string{
+		{"testnetwork", "channel1", "ns"},
+		{"testnetwork", "testchannel1", "ns"},
+		{"testnetwork", "mychannel01", "ns"},
+		{"network1", "channel1", "namespace1"},
+		{"net0", "ch-1.2", "ns_3"},
+	} {
+		t.Run(strings.Join(params, "/"), func(t *testing.T) {
+			var got common.TableNames
+			var err error
+			require.NotPanics(t, func() {
+				got, err = common.GetTableNames("pfx", params...)
+			})
+			require.NoError(t, err)
+
+			for _, name := range allTableNames(got) {
+				assert.Regexp(t, `^[a-zA-Z_][a-zA-Z0-9_]*$`, name)
+			}
+		})
+	}
+
+	// Spot-check the exact name produced for the canonical case.
+	names, err := common.GetTableNames("pfx", "testnetwork", "channel1", "ns")
+	require.NoError(t, err)
+	assert.Equal(t, "pfx_testnetwork__channel1__ns_txs", names.Transactions)
+
+	// The same must hold when the prefix is skipped.
+	got, err := common.GetTableNamesWithOverridesSkipPrefix("pfx", nil, "testnetwork", "channel1", "ns")
+	require.NoError(t, err)
+	assert.Equal(t, "testnetwork__channel1__ns_txs", got.Transactions)
+}
+
+// TestGetTableNamesInvalidParamsReturnError checks that a param that cannot be
+// turned into a legal SQL identifier is reported as an error rather than
+// crashing the process: this call chain is reachable from ordinary store
+// construction and nothing along it recovers.
+func TestGetTableNamesInvalidParamsReturnError(t *testing.T) {
+	for _, params := range [][]string{
+		{"testnetwork", "channel!"},
+		{"testnetwork", "channel 1"},
+		{"testnetwork", "channel;drop table x"},
+	} {
+		t.Run(strings.Join(params, "/"), func(t *testing.T) {
+			require.NotPanics(t, func() {
+				names, err := common.GetTableNames("pfx", params...)
+				require.Error(t, err)
+				assert.Equal(t, common.TableNames{}, names)
+			})
+		})
+	}
+
+	// A param starting with a digit is fine behind a prefix, but not when the
+	// prefix is skipped: an unquoted identifier cannot start with a digit in
+	// either SQLite or PostgreSQL.
+	withPrefix, err := common.GetTableNames("pfx", "1network", "channel1")
+	require.NoError(t, err)
+	assert.Equal(t, "pfx_1network__channel1_txs", withPrefix.Transactions)
+
+	require.NotPanics(t, func() {
+		names, err := common.GetTableNamesWithOverridesSkipPrefix("pfx", nil, "1network", "channel1")
+		require.Error(t, err)
+		assert.Equal(t, common.TableNames{}, names)
+	})
+}
+
+// TestGetTableNamesInvalidOverridesReportEveryKey checks that every invalid
+// short-code override is reported, not just the first one. An override applies to
+// a single key, so reporting only one would make an operator fix that key, restart
+// the node, and hit the next one — once per bad key.
+func TestGetTableNamesInvalidOverridesReportEveryKey(t *testing.T) {
+	overrides := common.TableNamesConfig{
+		"txs":     "bad!name",
+		"tokens":  "worse name",
+		"wallets": "wrong;name",
+	}
+
+	names, err := common.GetTableNamesWithOverrides("pfx", overrides, "net", "ch", "ns")
+	require.Error(t, err)
+	assert.Equal(t, common.TableNames{}, names)
+	for _, badName := range overrides {
+		assert.Contains(t, err.Error(), badName,
+			"every invalid override must be reported, got: %s", err)
+	}
+}
+
+// TestGetTableNamesInvalidParamsReportedOnce checks that a problem in the part
+// every table name shares — the prefix and the params — is reported once instead
+// of repeated for each of the seventeen names it breaks.
+func TestGetTableNamesInvalidParamsReportedOnce(t *testing.T) {
+	names, err := common.GetTableNames("pfx", "net", "ch!")
+	require.Error(t, err)
+	assert.Equal(t, common.TableNames{}, names)
+	assert.Equal(t, 1, strings.Count(err.Error(), "unsupported chars"),
+		"a shared failure must be reported once, got: %s", err)
+}
+
+// allTableNames returns every generated table name in tn.
+func allTableNames(tn common.TableNames) []string {
+	return []string{
+		tn.Movements, tn.Transactions, tn.TransactionEndorseAck,
+		tn.Requests, tn.Validations, tn.Tokens, tn.Ownership,
+		tn.Certifications, tn.TokenLocks, tn.PublicParams,
+		tn.Wallets, tn.IdentityConfigurations, tn.IdentityInfo,
+		tn.Signers, tn.KeyStore, tn.EIDLeases, tn.TokenSKICleanups,
+	}
+}
+
 // TestGetTableNamesWithConfig_SkipPrefixFalse checks that GetTableNamesWithConfig
 // behaves identically to GetTableNamesWithOverrides when SkipPrefix is false.
 func TestGetTableNamesWithConfig_SkipPrefixFalse(t *testing.T) {