fix(nitronode): remediate MF3-I* audit findings

[philanton]

Remediates the MF3-I* batch of audit findings (informational severity). Each finding is its own commit so they can be reviewed independently.

Findings

MF3-I01 — MAX_SESSION_KEYS_PER_USER=0 silently disables the per-user cap
The cap is skipped whenever maxSessionKeysPerUser <= 0, but that wasn't documented and 0 could be read as "invalid" or "zero keys allowed". Documented <= 0 as the unlimited mode on the config field and in the app-session submit README. Kept it as a documented behavior rather than a startup-reject, since that matches the existing WsBytesPerSec "<0 to disable" / MaxSignedUpdates 0 conventions and avoids breaking anyone already running 0.

MF3-I02 — duplicate asset symbols accepted case-insensitively
verifyAssetsConfig() accepted both usdt and USDT, which is ambiguous because the in-memory registry matches symbols case-sensitively while the persistence layer lowercases them. Now tracks enabled symbols by their lowercased form and fails startup on a case-insensitive (or exact) duplicate. Disabled assets are skipped; request-time exact-symbol semantics are unchanged.

MF3-I03 — stale err in missing-user-signature error
The missing-user_sig guard formatted an err that is always nil at that point, polluting the message with <nil>. Dropped the format arg.

MF3-I04 — nil-deref in NewTransactionFromTransition release branch
The release case dereferenced receiverState.UserWallet before the nil check, so a nil receiver state panicked instead of returning an error. Moved the nil check ahead of any field access. No RPC path passes nil today, but the helper is now safe regardless of caller. Added a regression test.

MF3-I05 — pagination offset can wrap negative on 32-bit
GetOffsetAndLimit returned a caller-controlled uint32 offset that every store query converts via int(offset) before GORM's Offset(); on a 32-bit target a large offset wraps negative and GORM treats it as "no offset", silently returning the first page. Clamped offset to MaxInt32 in GetOffsetAndLimit, then routed the handlers that still computed offset/limit by hand through the same helper so the clamp is enforced everywhere (channels.v1.get_channels, channels.v1.get_last_key_states, app_sessions.v1.get_last_key_states). An explicit limit: 0 still falls back to the endpoint default so page-count math never divides by zero.

MF3-I07 — session-key revocation mislogged
Both SubmitSessionKeyState handlers logged a revocation whenever the submitted expires_at was not in the future, ignoring whether the previous state was active — so inactive-to-inactive updates (and already-expired new versions) were logged as revocations. Now gated on the real transition: revoked := prevActive && !submittedActive.

MF3-I08 — unvalidated channel / app-session IDs
GetAppSessions accepted a non-nil but empty app_session_id, which the store treated as "no filter", silently returning an unfiltered paginated list instead of honoring the documented either-id-or-participant contract. Added core.HashRegex + core.IsValidHash (a 0x-prefixed 32-byte hash — the canonical form of channel and app-session IDs), mirroring app.IsValidApplicationID, and reject malformed IDs at the RPC boundary in GetAppSessions and GetEscrowChannel. HomeChannelID is left as-is — it has no bare-id read endpoint and is already verified by exact derivation match or existence lookup.

MF3-I09 — UpdateChannel rewrites immutable config
UpdateChannel rewrote immutable config (blockchain_id, token, nonce) on every lifecycle update, widening the blast radius if a future caller passes a partial/stale core.Channel. Restricted it to mutable lifecycle fields (status, state_version, challenge_expires_at, updated_at); immutable config is set once in CreateChannel.

Test plan

• go build ./...
• go test ./pkg/core/... ./nitronode/store/database/... ./nitronode/api/channel_v1/... ./nitronode/api/app_session_v1/... ./nitronode/event_handlers/...
• go vet ./... on touched packages

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Session-key revocation logging now only fires when a key actually transitions from active to inactive
  • Error message for missing state signature corrected
  • Prevented a potential crash when handling a specific transaction case

• New Features

  • Validation added for session and channel identifiers to reject malformed IDs

• Improvements

  • Centralized, more consistent pagination and safer offset handling across endpoints
  • Asset symbol validation now treats duplicates as case-insensitive
  • Channel updates no longer modify immutable configuration fields

• Documentation

  • Clarified per-user active session-key cap semantics and disabling behavior

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ecce22ee-0a39-4f20-bfac-6ce6649d6db0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds canonical hash validation and tests, centralizes pagination via PaginationParams and SafeOffset to avoid offset wraparound, refines session-key revocation logging to actual active→inactive transitions, restricts UpdateChannel to mutable fields, enforces case-insensitive enabled-asset symbol uniqueness, and includes small bugfixes and doc updates.

Changes

Input validation, pagination, and state transitions

Layer / File(s)	Summary
Core utils & pagination clamping pkg/core/utils.go, pkg/core/utils_test.go, pkg/core/types.go, pkg/core/types_test.go	Adds HashRegex and IsValidHash(), clamps pagination offsets to math.MaxInt32, adjusts TransitionTypeRelease nil-check ordering, and adds tests for SafeOffset, IsValidHash, and pagination boundary cases.
Handler ID validation guards nitronode/api/app_session_v1/get_app_sessions.go, nitronode/api/app_session_v1/get_app_sessions_test.go, nitronode/api/channel_v1/get_escrow_channel.go, nitronode/api/channel_v1/get_escrow_channel_test.go	Validate provided hash IDs early with IsValidHash; return rpc.Errorf("invalid ...") for malformed IDs; update/add tests to assert invalid inputs are rejected and store methods are not invoked.
API pagination centralization nitronode/api/app_session_v1/get_last_key_states.go, nitronode/api/channel_v1/get_channels.go, nitronode/api/channel_v1/get_last_key_states.go	Use core.PaginationParams and GetOffsetAndLimit to derive/clamp offset/limit consistently across handlers while preserving response metadata.
Store SafeOffset usage nitronode/store/database/*.go (apps, app_session, app_session_key_state, channel_session_key_state, transaction, etc.)	Replace int(offset) casts with core.SafeOffset(offset) for GORM Offset(...) calls to avoid negative/overflow offsets.
Session-key revocation transition logic nitronode/api/app_session_v1/submit_session_key_state.go, nitronode/api/channel_v1/submit_session_key_state.go	Introduce revoked flag computed from prior stored active state and submitted activeness; emit "revoked" logs only for active→inactive transitions.
Channel update immutability enforcement nitronode/store/database/channel.go, nitronode/store/database/channel_test.go	UpdateChannel now persists only lifecycle fields (status, state_version, challenge_expires_at, updated_at); tests assert immutable config fields (blockchain_id, token, nonce) remain unchanged.
Case-insensitive asset symbol validation nitronode/store/memory/asset_config.go, nitronode/store/memory/asset_config_test.go	Enforce case-insensitive uniqueness for enabled asset Symbol values via lowercased map lookup; add tests for conflicts and disabled-asset exceptions.
Bug fixes and docs nitronode/api/channel_v1/submit_state.go, nitronode/runtime.go, nitronode/api/app_session_v1/README.md, tests	Fix undefined err formatting in SubmitState, add doc comment for ValidationLimits.MaxSessionKeysPerUser, update README clarifying cap semantics, and small test formatting cleanup.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• layer-3/nitrolite#749: Overlaps on get_last_key_states pagination and active-only/latest semantics.
• layer-3/nitrolite#724: Related pagination/offset normalization changes in GetLastKeyStates handlers.

Suggested reviewers

• dimast-x
• ihsraham
• nksazonov

Poem

🐰 I hopped through hashes, tidy and neat,
Clamped the offsets so pages won't cheat,
Revoked when it truly fell night to day,
Immutable fields stood firm in their way,
Symbols sang lowercased — orderly and sweet.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 27.78% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title clearly and concisely identifies the main objective: remediating a set of audit findings (MF3-I*) in the nitronode component. While it doesn't enumerate all nine individual findings, it appropriately summarizes the primary change at a high level.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/mf3-i

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

nitronode/store/memory/asset_config_test.go (1)

171-181: ⚡ Quick win

Add reverse-order disabled duplicate test case.

Please add one more subtest where the disabled duplicate appears first and the enabled asset appears second, to lock in the “disabled assets are skipped regardless of ordering” contract.

Proposed test addition

+	t.Run("duplicate symbol with first asset disabled", func(t *testing.T) {
+		cfg := AssetsConfig{
+			Assets: []AssetConfig{
+				{Symbol: "USDT", Disabled: true},
+				{Symbol: "usdt", SuggestedBlockchainID: 1},
+			},
+		}
+		err := verifyAssetsConfig(&cfg)
+		require.NoError(t, err)
+	})

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nitronode/store/memory/asset_config_test.go` around lines 171 - 181, Add a
new subtest that mirrors the existing "duplicate symbol with disabled asset"
case but with the disabled AssetConfig listed first to ensure ordering doesn't
matter: create an AssetsConfig with AssetConfig{Symbol: "USDT", Disabled: true}
followed by AssetConfig{Symbol: "usdt", SuggestedBlockchainID: 1}, call
verifyAssetsConfig(&cfg) and assert require.NoError(t, err). Use a distinct
t.Run name like "duplicate symbol with disabled asset reversed" and reference
the same types (AssetsConfig, AssetConfig) and function verifyAssetsConfig to
locate where to add the test.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@nitronode/store/memory/asset_config_test.go`:
- Around line 171-181: Add a new subtest that mirrors the existing "duplicate
symbol with disabled asset" case but with the disabled AssetConfig listed first
to ensure ordering doesn't matter: create an AssetsConfig with
AssetConfig{Symbol: "USDT", Disabled: true} followed by AssetConfig{Symbol:
"usdt", SuggestedBlockchainID: 1}, call verifyAssetsConfig(&cfg) and assert
require.NoError(t, err). Use a distinct t.Run name like "duplicate symbol with
disabled asset reversed" and reference the same types (AssetsConfig,
AssetConfig) and function verifyAssetsConfig to locate where to add the test.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a7239c22-065f-486d-806a-edb310727feb

📥 Commits

Reviewing files that changed from the base of the PR and between 0c46c9e and dd855d2.

📒 Files selected for processing (20)

• nitronode/api/app_session_v1/README.md
• nitronode/api/app_session_v1/get_app_sessions.go
• nitronode/api/app_session_v1/get_app_sessions_test.go
• nitronode/api/app_session_v1/get_last_key_states.go
• nitronode/api/app_session_v1/submit_session_key_state.go
• nitronode/api/channel_v1/get_channels.go
• nitronode/api/channel_v1/get_escrow_channel.go
• nitronode/api/channel_v1/get_escrow_channel_test.go
• nitronode/api/channel_v1/get_last_key_states.go
• nitronode/api/channel_v1/submit_session_key_state.go
• nitronode/api/channel_v1/submit_state.go
• nitronode/runtime.go
• nitronode/store/database/channel.go
• nitronode/store/database/channel_test.go
• nitronode/store/memory/asset_config.go
• nitronode/store/memory/asset_config_test.go
• pkg/core/types.go
• pkg/core/types_test.go
• pkg/core/utils.go
• pkg/core/utils_test.go

[ihsraham]

approving for the MF3-I batch. the fixes cover the audit asks: the session-key cap behavior is documented, asset symbols are checked case-insensitively at config load, the stale signature error is cleaned up, the release transaction nil check is before the dereference, ID filters are rejected before store reads, revocation logging now keys off an active-to-inactive transition, and UpdateChannel no longer rewrites immutable config fields.

one non-blocking follow-up: the I-05 offset clamp is enforced at the current RPC callers before they reach the store, which closes the runtime path I reviewed. the lower-level store methods still accept raw uint32 offset values and cast them with int(offset), so if we want the store API itself to carry the invariant, it would be worth adding the same clamp there too.

also worth running gofmt on nitronode/runtime.go, nitronode/store/database/channel_test.go, pkg/core/types.go, and pkg/core/utils_test.go before merge. my local checks otherwise passed, and the scanner output I saw was existing/toolchain noise rather than PR-introduced risk.

[nksazonov]

Good work — all eight addressed findings are correctly implemented and backed by targeted regression tests.

[nksazonov]

The clamp enforces the invariant at the RPC handler layer. However, six store methods still do int(offset) directly without a guard: app.go:79, app_session.go:119, channel.go:192, transaction.go:109, app_session_key_state.go:142, channel_session_key_state.go:123. Any caller that reaches the store without routing through GetOffsetAndLimit — admin tooling, future internal endpoints, tests passing raw values — can still trigger the silent first-page bug. Consider either adding the clamp in the store query helpers, or adding a doc comment on each store method requiring callers to pre-clamp.