M-L03: limit number of related ids per session key state

[philanton]

Description

In channel_v1, SubmitSessionKeyState accepts a ChannelSessionKeyStateV1 payload containing an unbounded assets array but never validates its length against the configured maxSessionKeyIDs limit. In contrast, the equivalent app_session_v1.SubmitSessionKeyState handler correctly enforces this limit. As a result, a user request can supply an arbitrarily large asset list, which is then packed into the session key metadata hash and persisted as one database row per asset.

Summary by CodeRabbit

• Configuration Changes

  • Default maximum session key IDs reduced from 256 to 10 (configurable via environment variable).

• Bug Fixes

  • Improved validation for session key state submissions with more specific error messaging.

[coderabbitai]

📝 Walkthrough

Walkthrough

Session key state submission validation was refactored to check length limits after unmarshalling in the app session API, asset validation was added to the channel session API, comprehensive test suites were introduced for both handlers, and the default maximum session key ID limit was reduced from 256 to 10.

Changes

Cohort / File(s)	Summary
App Session V1 Handler & Tests clearnode/api/app_session_v1/submit_session_key_state.go, clearnode/api/app_session_v1/submit_session_key_state_test.go	Refactored validation to check AppSessionIDs and ApplicationIDs array lengths after unmarshalling and core validation instead of before mapping. Added comprehensive test suite covering successful requests, boundary conditions, validation failures for oversized arrays, invalid addresses, expired times, missing signatures, version mismatches, and signature verification.
Channel V1 Handler & Tests clearnode/api/channel_v1/submit_session_key_state.go, clearnode/api/channel_v1/submit_session_key_state_test.go	Added validation to reject requests when Assets array length exceeds maxSessionKeyIDs. Introduced extensive test suite with helper function and test cases covering successful submissions, array boundary conditions, malformed addresses, past expiration times, missing signatures, version mismatches, and signature verification failures.
Configuration clearnode/runtime.go	Reduced ValidationLimits.MaxSessionKeyIDs environment default value from 256 to 10 via CLEARNODE_MAX_SESSION_KEY_IDS env tag.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

• dimast-x

Poem

🐰 Hoppy validation times!

With tests that spring and bounds that bind,
Our session keys are well-designed!
From channels deep to sessions vast,
The verification holds steadfast.
Ten's the limit, truth be told—
A rabbit's audit, brave and bold! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

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

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'M-L03: limit number of related ids per session key state' directly and accurately describes the main change: enforcing maximum length limits on ID arrays in session key state handlers across both channel_v1 and app_session_v1, including the reduced default limit in runtime configuration.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/clearnode-l-03

---

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.}

[nksazonov]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[nksazonov]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

clearnode/api/channel_v1/submit_session_key_state_test.go (1)

90-126: Strengthen failure-path assertions by checking store is never touched.

For over-limit (and similar validation-failure tests), add explicit assertions that DB access methods were not called. That locks in the early-return guarantee.

♻️ Suggested test hardening

  handler.SubmitSessionKeyState(ctx)

  require.NotNil(t, ctx.Response)
  respErr := ctx.Response.Error()
  require.NotNil(t, respErr)
  assert.Contains(t, respErr.Error(), "assets array exceeds maximum length of 2")
+ mockStore.AssertNotCalled(t, "GetLastChannelSessionKeyVersion", mock.Anything, mock.Anything)
+ mockStore.AssertNotCalled(t, "StoreChannelSessionKeyState", mock.Anything)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@clearnode/api/channel_v1/submit_session_key_state_test.go` around lines 90 -
126, The test TestChannelSubmitSessionKeyState_AssetsExceedsMax should also
assert that the store was never accessed to enforce the early-return validation:
after calling handler.SubmitSessionKeyState(ctx) and verifying the validation
error, add assertions on the MockStore (mockStore) to ensure its DB methods (the
methods used by Handler via useStoreInTx/StoreTxHandler) were not invoked —
e.g., verify no calls/expectations on mockStore occurred or that specific
methods like Create/Update/Find were not called — so the test guarantees the
handler returned before touching the store.

clearnode/api/app_session_v1/submit_session_key_state_test.go (1)

104-151: Consider asserting no store interaction on validation failures.

In max-length rejection tests, add AssertNotCalled checks for store methods to ensure invalid payloads never proceed into version lookup/persistence.

♻️ Suggested test hardening

  handler.SubmitSessionKeyState(ctx)

  require.NotNil(t, ctx.Response)
  respErr := ctx.Response.Error()
  require.NotNil(t, respErr)
  assert.Contains(t, respErr.Error(), "application_ids array exceeds maximum length of 2")
+ mockStore.AssertNotCalled(t, "GetLastAppSessionKeyVersion", mock.Anything, mock.Anything)
+ mockStore.AssertNotCalled(t, "StoreAppSessionKeyState", mock.Anything)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@clearnode/api/app_session_v1/submit_session_key_state_test.go` around lines
104 - 151, The test TestSubmitSessionKeyState_ApplicationIDsExceedsMax should
also assert that the store was never called when validation fails: after calling
handler.SubmitSessionKeyState(ctx) and checking ctx.Response/Error, add
AssertNotCalled (or equivalent) on the mockStore for any persistence/lookup
methods used by the handler (e.g., the methods implemented on MockStore such as
GetSessionKeyVersion, SaveSessionKeyState or other Store interface methods) to
ensure Handler.useStoreInTx was not invoked and no version lookup or persistence
occurred.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@clearnode/runtime.go`:
- Line 84: The change to MaxSessionKeyIDs (struct field MaxSessionKeyIDs with
env var CLEARNODE_MAX_SESSION_KEY_IDS) lowered the implicit default from 256 to
10 which can silently reject existing clients; either revert the env-default
back to "256" to preserve previous behavior and rely on new validation to bound
inputs, or keep "10" but add an explicit migration/rollout note in the
PR/CHANGELOG and documentation calling out the breaking default change and
required operator action to set CLEARNODE_MAX_SESSION_KEY_IDS if they need the
previous capacity.

---

Nitpick comments:
In `@clearnode/api/app_session_v1/submit_session_key_state_test.go`:
- Around line 104-151: The test
TestSubmitSessionKeyState_ApplicationIDsExceedsMax should also assert that the
store was never called when validation fails: after calling
handler.SubmitSessionKeyState(ctx) and checking ctx.Response/Error, add
AssertNotCalled (or equivalent) on the mockStore for any persistence/lookup
methods used by the handler (e.g., the methods implemented on MockStore such as
GetSessionKeyVersion, SaveSessionKeyState or other Store interface methods) to
ensure Handler.useStoreInTx was not invoked and no version lookup or persistence
occurred.

In `@clearnode/api/channel_v1/submit_session_key_state_test.go`:
- Around line 90-126: The test TestChannelSubmitSessionKeyState_AssetsExceedsMax
should also assert that the store was never accessed to enforce the early-return
validation: after calling handler.SubmitSessionKeyState(ctx) and verifying the
validation error, add assertions on the MockStore (mockStore) to ensure its DB
methods (the methods used by Handler via useStoreInTx/StoreTxHandler) were not
invoked — e.g., verify no calls/expectations on mockStore occurred or that
specific methods like Create/Update/Find were not called — so the test
guarantees the handler returned before touching the store.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 412e6ce8-0c32-4d22-a3dc-13fc64b03345

📥 Commits

Reviewing files that changed from the base of the PR and between 38d2ad1 and 6fba88b.

📒 Files selected for processing (5)

• clearnode/api/app_session_v1/submit_session_key_state.go
• clearnode/api/app_session_v1/submit_session_key_state_test.go
• clearnode/api/channel_v1/submit_session_key_state.go
• clearnode/api/channel_v1/submit_session_key_state_test.go
• clearnode/runtime.go