Skip to content

Validate module names in the memiavl snapshot importer - #4469

Merged
masih merged 2 commits into
mainfrom
masih/1791282035-memiavl-import-module-name
Oct 6, 2026
Merged

masih merged 2 commits into
mainfrom
masih/1791282035-memiavl-import-module-name

Conversation

@masih

@masih masih commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

MultiTreeImporter.AddModule used the module name from the snapshot stream as a path component without validating it. Real module names are store keys, so anything other than a plain directory name means the snapshot is malformed.

AddModule now rejects empty, ., .., absolute and multi-component names before it creates any files, so such a snapshot fails early with a clear error. Valid snapshots import exactly as before and the AppHash is unchanged.

@devin-ai-integration

devin-ai-integration Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

I'll fix CI failures and address comments from users with write access that start with 'Devin'.

  • Disable automatic comment, CI, and merge conflict monitoring

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedOct 6, 2026, 10:46 AM

@masih
masih marked this pull request as ready for review October 6, 2026 10:26
@codecov

codecov Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.80%. Comparing base (7a2ec6e) to head (1c71b99).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4469   +/-   ##
=======================================
  Coverage   56.80%   56.80%           
=======================================
  Files        2127     2127           
  Lines      167022   167027    +5     
=======================================
+ Hits        94879    94888    +9     
+ Misses      72138    72134    -4     
  Partials        5        5           
Flag Coverage Δ
sei-chain 54.98% <ø> (+<0.01%) ⬆️
sei-db 75.17% <ø> (ø)
sei-db-state-db 78.88% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
sei-db/state_db/sc/memiavl/import.go 77.12% <100.00%> (+1.17%) ⬆️

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cursor

cursor Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches snapshot restore path handling; low blast radius for valid snapshots but blocks previously accepted malformed names and prevents path-component abuse during import.

Overview
Snapshot restore now rejects unsafe or malformed module names before any import directories are created.

MultiTreeImporter.AddModule validates names with validateModuleName (plain local segment only: no empty, ., .., slashes, or absolute paths) and returns invalid snapshot module name on failure. Root-multi restore tests expect that error for an unnamed store item; memiavl adds TestMultiTreeImporterValidatesModuleName to cover bad names and confirm valid modules (e.g. bank) still import under the snapshot temp dir without path traversal.

Reviewed by Cursor Bugbot for commit 1c71b99. Bugbot is set up for automated code reviews on this repo. Configure here.

@masih masih added the backport release/v6.7 Backport to release v6.7 label Oct 6, 2026
Only accept plain directory names from the snapshot stream, so a malformed snapshot fails early with a clear error.
@devin-ai-integration
devin-ai-integration Bot force-pushed the masih/1791282035-memiavl-import-module-name branch from 5d53880 to aea3209 Compare October 6, 2026 10:28
@devin-ai-integration devin-ai-integration Bot changed the title Reject snapshot module names that escape the memiavl import dir Validate module names in the memiavl snapshot importer Oct 6, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review found nothing blocking in the commit it read. It does not approve: this pull request was pushed to while the review ran, so the approval would stand against code the review never saw. Ask for another review to have the new commit read.

@seidroid

seidroid Bot commented Oct 6, 2026

Copy link
Copy Markdown

This change makes MultiTreeImporter.AddModule reject any snapshot module name that is not a plain single directory name (empty, ., .., absolute, or containing a separator) before it joins the name onto the import temp dir. That closes the path-traversal hole for module names that come from peers during state sync, restore in rootmulti already aborts on the returned error, and I found nothing blocking. The one other reading (codex) found nothing, which matches my reading; the tests could not be run because this sandbox has no Go toolchain.

seidroid review · decision approve · session 91cb333bade445ca9b0b1c5e206cc88f · turn resp_claude_a78a94e578b3b2ff9357bfd8d3a17796 · item ccf47ec6cbce5e769f2e36e93b76dfbd

Findings: 0 blocking | 0 non-blocking | 0 posted inline

@seidroid seidroid Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since my last review only the doc comment, the test name and fixture wording and the PR description have changed; validateModuleName and the AddModule guard are identical, so names that would leave the memiavl import temp dir are still rejected, and I raised nothing before that needed addressing. Nothing blocks; the one other reading (codex) found nothing, which matches mine, and the tests were not run because this sandbox has no Go toolchain.

seidroid review · decision approve · session 91cb333bade445ca9b0b1c5e206cc88f · turn resp_claude_95e4a8270bb4cb4f5a1f0a8b3b85b60a · item 8806d07bebab51b7942be1f02a178662

Findings: 0 blocking | 0 non-blocking | 0 posted inline

@masih
masih added this pull request to the merge queue Oct 6, 2026
Merged via the queue into main with commit 96e0483 Oct 6, 2026
63 checks passed
@masih
masih deleted the masih/1791282035-memiavl-import-module-name branch October 6, 2026 13:59
@seidroid

seidroid Bot commented Oct 6, 2026

Copy link
Copy Markdown

Created backport PR for release/v6.7:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-4469-to-release/v6.7
git worktree add --checkout .worktree/backport-4469-to-release/v6.7 backport-4469-to-release/v6.7
cd .worktree/backport-4469-to-release/v6.7
git reset --hard HEAD^
git cherry-pick -x 96e048374c34995998a4a15ab12dced82c4201b3
git push --force-with-lease

devin-ai-integration Bot pushed a commit that referenced this pull request Oct 6, 2026
`MultiTreeImporter.AddModule` used the module name from the snapshot
stream as a path component without validating it. Real module names are
store keys, so anything other than a plain directory name means the
snapshot is malformed.

`AddModule` now rejects empty, `.`, `..`, absolute and multi-component
names before it creates any files, so such a snapshot fails early with a
clear error. Valid snapshots import exactly as before and the AppHash is
unchanged.

(cherry picked from commit 96e0483)
masih added a commit that referenced this pull request Oct 7, 2026
…t importer (#4474)

Backport of #4469 to `release/v6.7`.

Co-authored-by: Masih H. Derkani <m@derkani.org>
masih pushed a commit that referenced this pull request Oct 8, 2026
Systematic changelog re-generation for the v6.7.1 patch release.

Adds the PRs merged to `release/v6.7` since the v6.7.0 changelog
(#4443), as reported by `scripts/generate-changelog.sh release/v6.6
release/v6.7`:
- #4473 Backport `release/v6.7`: Make the composite store router an
atomic pointer
- #4465 Backport `release/v6.7`: fix(seidb): keep the memIAVL nonce when
state sync restores a mid-mig…
- #4445 Backport `release/v6.7`: Update v6.7 changelog in prep to cut
v6.7.0
- #4444 Bump version to v6.7.0 in prep for release

Only the `## v6.7` PR list changes, so the `backport release/v6.7`
cherry-pick applies cleanly (simulated with `git merge-tree` against
`origin/release/v6.7`).

Not included: #4474 (the `release/v6.7` backport of #4469), which is
still open. If it lands before the cut, this list needs regenerating.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants